> On Feb 19, 2019, at 10:31 AM, Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>
> Peter Wagner <tripolar@xxxxxx> reports a portability issue with
> freeing ai_canonname (and subsequently replacing that pointer via
> strdup(3)). The relevant standards text is:
>
>> If nodename is not null, and if requested by the AI_CANONNAME
>> flag, the ai_canonname field of the first returned addrinfo
>> structure shall point to a null-terminated string containing the
>> canonical name corresponding to the input nodename; if the
>> canonical name is not available, then ai_canonname shall refer to
>> the nodename argument or a string with the same contents.
>
> There is no indication that this string may be freed using free(3).
> Eg, the library could have allocated it as part of the addrinfo
> struct itself, or it could point to static memory. The Linux man
> page is equally silent on this issue.
>
> There is only one caller to host_reliable_addrinfo() that actually
> uses the string in ai->ai_canonname, and then only for debugging
> messages. Change those to display the IP address instead.
Actually, now that I look at callers to host_numeric_addrinfo,
it appears that client_resolve is the only one. So this might fix
the entire issue (with the addition of hunks to remove the faulty
free/strdup logic from host_numeric_addrinfo).
Steve, Peter, is that your reading as well?
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> support/export/hostname.c | 25 ++++++++++---------------
> utils/mountd/auth.c | 16 ++++++++--------
> 2 files changed, 18 insertions(+), 23 deletions(-)
>
> This patch is compile-tested only. Steve, does this patch pass your
> internal tests? Are the new debugging messages sufficient IYO ?
>
> diff --git a/support/export/hostname.c b/support/export/hostname.c
> index 5c4c824..9914e0d 100644
> --- a/support/export/hostname.c
> +++ b/support/export/hostname.c
> @@ -264,9 +264,9 @@ host_canonname(const struct sockaddr *sap)
> * Reverse and forward lookups are performed to ensure the address has
> * matching forward and reverse mappings.
> *
> - * Returns addrinfo structure with just the provided address with
> - * ai_canonname filled in. If there is a problem with resolution or
> - * the resolved records don't match up properly then it returns NULL
> + * Returns addrinfo structure with just the provided address. If there
> + * is a problem with resolution or the resolved records don't match up
> + * properly then returns NULL.
> *
> * Caller must free the returned structure with freeaddrinfo(3).
> */
> @@ -277,13 +277,15 @@ host_reliable_addrinfo(const struct sockaddr *sap)
> struct addrinfo *ai, *a;
> char *hostname;
>
> + ai = NULL;
> hostname = host_canonname(sap);
> if (hostname == NULL)
> - return NULL;
> + goto out;
>
> ai = host_addrinfo(hostname);
> + free(hostname);
> if (!ai)
> - goto out_free_hostname;
> + goto out;
>
> /* make sure there's a matching address in the list */
> for (a = ai; a; a = a->ai_next)
> @@ -291,22 +293,15 @@ host_reliable_addrinfo(const struct sockaddr *sap)
> break;
>
> freeaddrinfo(ai);
> + ai = NULL;
> if (!a)
> - goto out_free_hostname;
> + goto out;
>
> /* get addrinfo with just the original address */
> ai = host_numeric_addrinfo(sap);
> - if (!ai)
> - goto out_free_hostname;
>
> - /* and populate its ai_canonname field */
> - free(ai->ai_canonname);
> - ai->ai_canonname = hostname;
> +out:
> return ai;
> -
> -out_free_hostname:
> - free(hostname);
> - return NULL;
> }
>
> /**
> diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
> index 8299256..cb4848c 100644
> --- a/utils/mountd/auth.c
> +++ b/utils/mountd/auth.c
> @@ -261,40 +261,40 @@ auth_authenticate(const char *what, const struct sockaddr *caller,
> *p = '\0';
> }
>
> + host_ntop(caller, buf, sizeof(buf));
> switch (error) {
> case bad_path:
> xlog(L_WARNING, "bad path in %s request from %s: \"%s\"",
> - what, host_ntop(caller, buf, sizeof(buf)), path);
> + what, buf, path);
> break;
>
> case unknown_host:
> xlog(L_WARNING, "refused %s request from %s for %s (%s): unmatched host",
> - what, host_ntop(caller, buf, sizeof(buf)), path, epath);
> + what, buf, path, epath);
> break;
>
> case no_entry:
> xlog(L_WARNING, "refused %s request from %s for %s (%s): no export entry",
> - what, ai->ai_canonname, path, epath);
> + what, buf, path, epath);
> break;
>
> case not_exported:
> xlog(L_WARNING, "refused %s request from %s for %s (%s): not exported",
> - what, ai->ai_canonname, path, epath);
> + what, buf, path, epath);
> break;
>
> case illegal_port:
> xlog(L_WARNING, "refused %s request from %s for %s (%s): illegal port %u",
> - what, ai->ai_canonname, path, epath, nfs_get_port(caller));
> + what, buf, path, epath, nfs_get_port(caller));
> break;
>
> case success:
> xlog(L_NOTICE, "authenticated %s request from %s:%u for %s (%s)",
> - what, ai->ai_canonname, nfs_get_port(caller), path, epath);
> + what, buf, nfs_get_port(caller), path, epath);
> break;
> default:
> xlog(L_NOTICE, "%s request from %s:%u for %s (%s) gave %d",
> - what, ai->ai_canonname, nfs_get_port(caller),
> - path, epath, error);
> + what, buf, nfs_get_port(caller), path, epath, error);
> }
>
> freeaddrinfo(ai);
>
--
Chuck Lever
