Re: [PATCH v2] Remove abuse of ai_canonname

[Peter Wagner <tripolar@gmx.at>]

On Tue, 19 Feb 2019 10:52:36 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> Peter Wagner <tripolar@gmx.at> 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.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Signed-off-by: Peter Wagner <tripolar@gmx.at>

> ---
>  support/export/hostname.c |   58
> +++++++++------------------------------------
> utils/mountd/auth.c       |   16 ++++++------ 2 files changed, 20
> insertions(+), 54 deletions(-)
> 
> 
> Compile-tested only.
> 
> 
> diff --git a/support/export/hostname.c b/support/export/hostname.c
> index 5c4c824..96c5449 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;
>  }
>  
>  /**
> @@ -323,7 +318,6 @@ host_numeric_addrinfo(const struct sockaddr *sap)
>  {
>  	socklen_t salen = nfs_sockaddr_length(sap);
>  	char buf[INET6_ADDRSTRLEN];
> -	struct addrinfo *ai;
>  	int error;
>  
>  	if (salen == 0) {
> @@ -348,21 +342,7 @@ host_numeric_addrinfo(const struct sockaddr *sap)
>  		return NULL;
>  	}
>  
> -	ai = host_pton(buf);
> -
> -	/*
> -	 * getaddrinfo(AI_NUMERICHOST) never fills in ai_canonname
> -	 */
> -	if (ai != NULL) {
> -		free(ai->ai_canonname);		/* just in
> case */
> -		ai->ai_canonname = strdup(buf);
> -		if (ai->ai_canonname == NULL) {
> -			freeaddrinfo(ai);
> -			ai = NULL;
> -		}
> -	}
> -
> -	return ai;
> +	return host_pton(buf);
>  }
>  #else	/* !HAVE_GETNAMEINFO */
>  __attribute__((__malloc__))
> @@ -372,7 +352,6 @@ host_numeric_addrinfo(const struct sockaddr *sap)
>  	const struct sockaddr_in *sin = (const struct sockaddr_in
> *)sap; const struct in_addr *addr = &sin->sin_addr;
>  	char buf[INET_ADDRSTRLEN];
> -	struct addrinfo *ai;
>  
>  	if (sap->sa_family != AF_INET)
>  		return NULL;
> @@ -382,19 +361,6 @@ host_numeric_addrinfo(const struct sockaddr *sap)
>  					(socklen_t)sizeof(buf)) ==
> NULL) return NULL;
>  
> -	ai = host_pton(buf);
> -
> -	/*
> -	 * getaddrinfo(AI_NUMERICHOST) never fills in ai_canonname
> -	 */
> -	if (ai != NULL) {
> -		ai->ai_canonname = strdup(buf);
> -		if (ai->ai_canonname == NULL) {
> -			freeaddrinfo(ai);
> -			ai = NULL;
> -		}
> -	}
> -
> -	return ai;
> +	return host_pton(buf);
>  }
>  #endif	/* !HAVE_GETNAMEINFO */
> 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);
>