freeaddrinfo(NULL) segfaults in v1.1.21

freeaddrinfo(NULL) segfaults in v1.1.21

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

Hi,

previous to v1.1.21 it was fine to call `freeaddrinfo(NULL)`, as
the implementation simply called `free(NULL)` in that case. Since
commit d1395c43 (allow freeaddrinfo of arbitrary sublists of
addrinfo list, 2018-10-04), this is no longer the case, as musl
now tries always de-references the passed pointer to free
potential sublists.

As long as I didn't miss it, freeaddrinfo(3P) doesn't explicitly
say whether it needs to be called with a valid pointer, and sure
enough there's applications out there which aren't careful here.
One example I found is e.g. nfs-utils, where I hit segfaults in
different places after upgrading to musl v1.1.21.

So was this change in behavior intended or is it an unwanted
side-effect of the commit in question?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: freeaddrinfo(NULL) segfaults in v1.1.21

[Bobby Bingham]

On Sun, Feb 03, 2019 at 07:02:38PM +0100, Patrick Steinhardt wrote:
> Hi,
> 
> previous to v1.1.21 it was fine to call `freeaddrinfo(NULL)`, as
> the implementation simply called `free(NULL)` in that case. Since
> commit d1395c43 (allow freeaddrinfo of arbitrary sublists of
> addrinfo list, 2018-10-04), this is no longer the case, as musl
> now tries always de-references the passed pointer to free
> potential sublists.
> 
> As long as I didn't miss it, freeaddrinfo(3P) doesn't explicitly
> say whether it needs to be called with a valid pointer, and sure
> enough there's applications out there which aren't careful here.
> One example I found is e.g. nfs-utils, where I hit segfaults in
> different places after upgrading to musl v1.1.21.

In general, unless it is specified otherwise, it is undefined behavior
to call functions accepting a pointer with an invalid or NULL pointer.

In this particular case, the spec[1] says:

  The freeaddrinfo() function shall free one or more addrinfo
  structures returned by getaddrinfo(), along with any additional
  storage associated with those structures.

A NULL pointer does not point to "one or more addrinfo structures".

  1. http://pubs.opengroup.org/onlinepubs/9699919799/functions/freeaddrinfo.html

> 
> So was this change in behavior intended or is it an unwanted
> side-effect of the commit in question?

I would guess it's an unintended, but not necessarily unwanted, side
effect.  When it's easy to detect such application bugs, musl generally
opts to fail loudly at the point of error, so these bugs can be noticed
and fixed by application developers, rather than silently hiding them.

Bobby

Re: freeaddrinfo(NULL) segfaults in v1.1.21

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

On Sun, Feb 03, 2019 at 01:13:35PM -0600, Bobby Bingham wrote:
> On Sun, Feb 03, 2019 at 07:02:38PM +0100, Patrick Steinhardt wrote:
> > As long as I didn't miss it, freeaddrinfo(3P) doesn't explicitly
> > say whether it needs to be called with a valid pointer, and sure
> > enough there's applications out there which aren't careful here.
> > One example I found is e.g. nfs-utils, where I hit segfaults in
> > different places after upgrading to musl v1.1.21.
> 
> In general, unless it is specified otherwise, it is undefined behavior
> to call functions accepting a pointer with an invalid or NULL pointer.
> 
> In this particular case, the spec[1] says:
> 
>   The freeaddrinfo() function shall free one or more addrinfo
>   structures returned by getaddrinfo(), along with any additional
>   storage associated with those structures.
> 
> A NULL pointer does not point to "one or more addrinfo structures".
> 
>   1. http://pubs.opengroup.org/onlinepubs/9699919799/functions/freeaddrinfo.html

Yeah, I thought that this section might indeed imply that `NULL`
is not considered valid input for this function. Thanks for
confirming!

> > So was this change in behavior intended or is it an unwanted
> > side-effect of the commit in question?
> 
> I would guess it's an unintended, but not necessarily unwanted, side
> effect.  When it's easy to detect such application bugs, musl generally
> opts to fail loudly at the point of error, so these bugs can be noticed
> and fixed by application developers, rather than silently hiding them.

I assumed as much, especially considering that a similar stance
was adopted with regards to closedir(3P). It's annoying that one
frequently has to deal with such issues, but I'd rather blame it
at glibc and dependents who aren't paying attention to standards
instead of blaming it on musl.

I guess I'll try and upstream some patches for inclusion in
nfs-utils to fix the issue.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]