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

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

[Michael Kerrisk (man-pages)]

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.

Thanks,

Michael


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

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

[Michael Kerrisk (man-pages)]

[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/