Re: [patch] freeaddrinfo.3: memory leaks in freeaddrinfo examples

["Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>]

[CC += beej, to alert the author about the memory leaks 
in the network programming guide]

Hello Marko,

> On Thu, Sep 17, 2020 at 7:42 AM Michael Kerrisk (man-pages) <
> mtk.manpages@gmail.com> wrote:
> 
>> Hi Marko,
>>
>> On Thu, 17 Sep 2020 at 07:34, Marko Hrastovec <marko.hrastovec@gmail.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> examples in freeaddrinfo.3 have a memory leak, which is replicated in
>> many real world programs copying an example from manual pages. The two
>> examples should have different order of lines, which is done in the
>> following patch.
>>>
>>> diff --git a/man3/getaddrinfo.3 b/man3/getaddrinfo.3
>>> index c9a4b3e43..4d383bea0 100644
>>> --- a/man3/getaddrinfo.3
>>> +++ b/man3/getaddrinfo.3
>>> @@ -711,13 +711,13 @@ main(int argc, char *argv[])
>>>          close(sfd);
>>>      }
>>>
>>> +    freeaddrinfo(result);           /* No longer needed */
>>> +
>>>      if (rp == NULL) {               /* No address succeeded */
>>>          fprintf(stderr, "Could not bind\en");
>>>          exit(EXIT_FAILURE);
>>>      }
>>>
>>> -    freeaddrinfo(result);           /* No longer needed */
>>> -
>>>      /* Read datagrams and echo them back to sender */
>>>
>>>      for (;;) {
>>> @@ -804,13 +804,13 @@ main(int argc, char *argv[])
>>>          close(sfd);
>>>      }
>>>
>>> +    freeaddrinfo(result);           /* No longer needed */
>>> +
>>>      if (rp == NULL) {               /* No address succeeded */
>>>          fprintf(stderr, "Could not connect\en");
>>>          exit(EXIT_FAILURE);
>>>      }
>>>
>>> -    freeaddrinfo(result);           /* No longer needed */
>>> -
>>>      /* Send remaining command\-line arguments as separate
>>>         datagrams, and read responses from server */
>>>
>>
>> When you say "memory leak", do you mean that something like valgrind
>> complains? I mean, strictly speaking, there is no memory leak that I
>> can see that is fixed by that patch, since the if-branches that the
>> freeaddrinfo() calls are shifted above terminates the process in each
>> case.
>
> you are right about terminating the process. However, people copy that
> example and put the code in function changing "exit" to "return". There are
> a bunch of examples like that here https://beej.us/guide/bgnet/html/#poll,
> for instance.

Oh -- I see what you mean.

> That error bothered me when reading the network programming
> guide https://beej.us/guide/bgnet/html/. Than I looked for information
> elsewhere:
> -
> https://stackoverflow.com/questions/6712740/valgrind-reporting-that-getaddrinfo-is-leaking-memory
> -
> https://stackoverflow.com/questions/15690303/server-client-sockets-freeaddrinfo3-placement
> And finally, I checked manual pages and saw where these errors come from.
> 
> When you change that to a function and return without doing freeaddrinfo,
> that is a memory leak. I believe an example should show good programming
> practices. Relying on exiting and clearing the memory in that case is not
> such a case. In my opinion, these examples lead people to make mistakes in
> their programs.

Yes, I can buy that argument. I've applied your patch.

Thanks,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/