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.
>
>
>> 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....
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
>
>
>
