> On Feb 18, 2019, at 11:34 AM, Steve Dickson <SteveD@xxxxxxxxxx> wrote:
>
>
>
> On 2/17/19 9:21 PM, Chuck Lever wrote:
>>
>>
>>> On Feb 17, 2019, at 6:11 PM, Peter Wagner <tripolar@xxxxxx> wrote:
>>>
>>> Hi Chuck,
>>>
>>>> It might be cleaner to define a local version of freeaddrinfo in
>>>> nfs-utils to reduce duplication of code and provide a place
>>>> in the code itself to document this issue with a comment.
>>>
>>> But why use a local freeaddrinfo function? The code is checking at
>>> multiple places if an addrinfo* == NULL and if it's NULL it just returns
>>> without freeing it and at some other places it just frees it. For
>>> censistence reasions i would propose to check if it's != NULL before
>>> freeing it.
>>
>> I didn't think this would be a controversial suggestion.
>> Here is what I meant:
>>
>> Find a common location, say support/include/exportfs.h, and
>> add this:
>>
>> /*
>> * Some versions of freeaddrinfo(3) do not tolerate being
>> * passed a NULL pointer.
>> */
>> static inline void nfs_freeaddrinfo(struct addrinfo *ai)
>> {
>> if (ai)
>> freeaddrinfo(ai);
>> }
>>
>> Then replace the freeaddrinfo() call sites within nfs-utils
>> with calls to nfs_freeaddrinfo().
> +1
>
>>
>>
>>
>> The logic in host_numeric_addrinfo() will need a closer
>> look. If what Rich says is true then both freeing the
>> ai_canonname string _and_ replacing it with strdup(3)
>> will be problematic.
>>
>> I see that host_reliable_addrinfo() has the same issue.
> I just ran Rich's program that is suppose to
> crash... It does not crash on Fedora 29.
It probably does crash with musl 1.1.21. :-)
>>> The spec also just defines the != NULL case and we shouldn't depend on
>>> undefined behavior imho.
>>
>> Some counterpoints:
>>
>> - It is good defensive coding that a library function should
>> perform basic sanity checks on its argument(s) to prevent
>> crashes.
>>
>> - It's always possible for a spec to be wrong.
>>
>> - If a spec says the behavior is "undefined" that generally
>> means the spec writer is giving the implementer permission
>> to choose a convenient and possibly useful behavior. Crashing
>> is not especially useful. A better approach would be to ignore
>> NULL, and add hooks to a tool like valgrind to report this
>> kind of issue.
>>
>> - nfs-utils is not careless about this. The design of the code
>> clearly assumes that freeaddrinfo(3) will not crash in this
>> case. It's not an unreasonable assumption. The goal here is
>> to construct simple-to-read code.
>>
>> - Strictly speaking, d1395c43 introduced a regression in
>> freeaddrinfo(3) that should be fixed, because existing
>> applications have depended on the old behavior for a long
>> while. IMO a better fix here would be to make musl's
>> freeaddrinfo(3) deal properly with a NULL argument instead
>> of changing all of its callers, but that's not my call.
> I also took a look at how freeaddrinfo(3) in glibc works:
>
> void
> freeaddrinfo (struct addrinfo *ai)
> {
> struct addrinfo *p;
>
> while (ai != NULL)
> {
> p = ai;
> ai = ai->ai_next;
> free (p->ai_canonname);
> free (p);
> }
> }
> It makes the assumption that ai_canonname is
> free-able memory....
Right, I seem to recall looking at this implementation when
I wrote the IPv6 support for nfs-utils. That seems sensible
to me.
However Peter's point is not all C libraries have to work
that way, and I can see that argument too.
I have no objection to taking his fix as long as the hunk
in host_numeric_addrinfo() is dropped while we figure out
a more complete fix for that issue. I would also like to
see his fix use a helper like nfs_freeaddrinfo(), but that
is only a suggestion.
> steved.
>>
>> - The equivalent to free(3) in the Linux kernel, kfree(),
>> explicitly allows callers to pass NULL because that eliminates
>> the need to add "if (arg != NULL)" at every call site.
>>
>> I'm not arguing that we should do nothing, but rather that
>> there is a matter of subjectivity and taste here about what
>> is correct behavior. The OG spec is just one opinion.
>>
>>
>>> Regards,
>>> Peter
>>>
>>>
>>>
>>> On Sun, 17 Feb 2019 12:40:10 -0500
>>> Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>>>
>>>> Hi Peter-
>>>>
>>>>> On Feb 17, 2019, at 11:39 AM, Peter Wagner <tripolar@xxxxxx> wrote:
>>>>>
>>>>> Afer the update to musl 1.1.21 freeaddrinfo is broken in some
>>>>> places in the nfs-utils code because glibc seems to ignore when
>>>>> freeaddrinfo is called with a NULL pointer which seems to be not
>>>>> defined in the spec.
>>>>>
>>>>> See: https://www.openwall.com/lists/musl/2019/02/03/4
>>>>
>>>> It might be cleaner to define a local version of freeaddrinfo in
>>>> nfs-utils to reduce duplication of code and provide a place
>>>> in the code itself to document this issue with a comment.
>>>>
>>>>
>>>>> The free in support/export/hostname.c is removed too
>>>>>
>>>>> See: https://www.openwall.com/lists/musl/2019/02/17/2
>>>>
>>>> Actually this seems like a separate issue because a distribution
>>>> that uses another C library might decide it is pertinent to apply
>>>> separately from the other changes here.
>>>>
>>>> Please create a separate patch. It should remove the free(3)
>>>> call instead of commenting it out, and the patch description
>>>> should have its own copy of Rich’s email comments.
>>>>
>>>> Thanks!
>>>>
>>>>
>>>>> From 43e27735553b4c1e75964f32b2f887e84398055f Mon Sep 17 00:00:00
>>>>> 2001 From: Peter Wagner <tripolar@xxxxxx>
>>>>> Date: Sun, 17 Feb 2019 17:32:08 +0100
>>>>> Subject: [PATCH] fix addrinfo usage
>>>>>
>>>>> Signed-off-by: Peter Wagner <tripolar@xxxxxx>
>>>>> ---
>>>>> support/export/client.c | 3 ++-
>>>>> support/export/hostname.c | 2 +-
>>>>> utils/exportfs/exportfs.c | 12 ++++++++----
>>>>> utils/mount/stropts.c | 3 ++-
>>>>> utils/mountd/cache.c | 6 ++++--
>>>>> utils/statd/hostname.c | 6 ++++--
>>>>> 6 files changed, 21 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/support/export/client.c b/support/export/client.c
>>>>> index baf59c8..750eb7d 100644
>>>>> --- a/support/export/client.c
>>>>> +++ b/support/export/client.c
>>>>> @@ -309,7 +309,8 @@ client_lookup(char *hname, int canonical)
>>>>> init_addrlist(clp, ai);
>>>>>
>>>>> out:
>>>>> - freeaddrinfo(ai);
>>>>> + if (ai)
>>>>> + freeaddrinfo(ai);
>>>>> return clp;
>>>>> }
>>>>>
>>>>> diff --git a/support/export/hostname.c b/support/export/hostname.c
>>>>> index 5c4c824..710bf61 100644
>>>>> --- a/support/export/hostname.c
>>>>> +++ b/support/export/hostname.c
>>>>> @@ -354,7 +354,7 @@ host_numeric_addrinfo(const struct sockaddr
>>>>> *sap)
>>>>> * getaddrinfo(AI_NUMERICHOST) never fills in ai_canonname
>>>>> */
>>>>> if (ai != NULL) {
>>>>> - free(ai->ai_canonname); /* just in case */
>>>>> + //free(ai->ai_canonname); /* just in case */
>>>>> ai->ai_canonname = strdup(buf);
>>>>> if (ai->ai_canonname == NULL) {
>>>>> freeaddrinfo(ai);
>>>>> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
>>>>> index cd3c979..2f8d59a 100644
>>>>> --- a/utils/exportfs/exportfs.c
>>>>> +++ b/utils/exportfs/exportfs.c
>>>>> @@ -282,7 +282,8 @@ exportfs_parsed(char *hname, char *path, char
>>>>> *options, int verbose) validate_export(exp);
>>>>>
>>>>> out:
>>>>> - freeaddrinfo(ai);
>>>>> + if (ai)
>>>>> + freeaddrinfo(ai);
>>>>> }
>>>>>
>>>>> static int exportfs_generic(char *arg, char *options, int verbose)
>>>>> @@ -395,7 +396,8 @@ unexportfs_parsed(char *hname, char *path, int
>>>>> verbose) if (!success)
>>>>> xlog(L_ERROR, "Could not find '%s:%s' to unexport.", hname,
>>>>> path);
>>>>>
>>>>> - freeaddrinfo(ai);
>>>>> + if (ai)
>>>>> + freeaddrinfo(ai);
>>>>> }
>>>>>
>>>>> static int unexportfs_generic(char *arg, int verbose)
>>>>> @@ -639,8 +641,10 @@ matchhostname(const char *hostname1, const
>>>>> char *hostname2) }
>>>>>
>>>>> out:
>>>>> - freeaddrinfo(results1);
>>>>> - freeaddrinfo(results2);
>>>>> + if (results1)
>>>>> + freeaddrinfo(results1);
>>>>> + if (results2)
>>>>> + freeaddrinfo(results2);
>>>>> return result;
>>>>> }
>>>>>
>>>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>>>> index 0a25b1f..8b7a0a8 100644
>>>>> --- a/utils/mount/stropts.c
>>>>> +++ b/utils/mount/stropts.c
>>>>> @@ -1268,7 +1268,8 @@ int nfsmount_string(const char *spec, const
>>>>> char *node, char *type, } else
>>>>> nfs_error(_("%s: internal option parsing error"), progname);
>>>>>
>>>>> - freeaddrinfo(mi.address);
>>>>> + if (mi.address)
>>>>> + freeaddrinfo(mi.address);
>>>>> free(mi.hostname);
>>>>> return retval;
>>>>> }
>>>>> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
>>>>> index 7e8d403..8cee1c8 100644
>>>>> --- a/utils/mountd/cache.c
>>>>> +++ b/utils/mountd/cache.c
>>>>> @@ -834,7 +834,8 @@ static void nfsd_fh(int f)
>>>>> out:
>>>>> if (found_path)
>>>>> free(found_path);
>>>>> - freeaddrinfo(ai);
>>>>> + if(ai)
>>>>> + freeaddrinfo(ai);
>>>>> free(dom);
>>>>> xlog(D_CALL, "nfsd_fh: found %p path %s", found, found ?
>>>>> found->e_path : NULL); }
>>>>> @@ -1355,7 +1356,8 @@ static void nfsd_export(int f)
>>>>> xlog(D_CALL, "nfsd_export: found %p path %s", found, path ?
>>>>> path : NULL); if (dom) free(dom);
>>>>> if (path) free(path);
>>>>> - freeaddrinfo(ai);
>>>>> + if (ai)
>>>>> + freeaddrinfo(ai);
>>>>> }
>>>>>
>>>>>
>>>>> diff --git a/utils/statd/hostname.c b/utils/statd/hostname.c
>>>>> index 8cccdb8..6556ab1 100644
>>>>> --- a/utils/statd/hostname.c
>>>>> +++ b/utils/statd/hostname.c
>>>>> @@ -308,8 +308,10 @@ statd_matchhostname(const char *hostname1,
>>>>> const char *hostname2) }
>>>>>
>>>>> out:
>>>>> - freeaddrinfo(results2);
>>>>> - freeaddrinfo(results1);
>>>>> + if (results2)
>>>>> + freeaddrinfo(results2);
>>>>> + if (results1)
>>>>> + freeaddrinfo(results1);
>>>>>
>>>>> xlog(D_CALL, "%s: hostnames %s and %s %s", __func__,
>>>>> hostname1, hostname2,
>>>>> --
>>>>> 2.20.1
>>>>>
>>>>
>>>
>>
>> --
>> Chuck Lever
--
Chuck Lever
