* Re: [PATCH v2] Remove abuse of ai_canonname
2019-02-19 15:52 [PATCH v2] Remove abuse of ai_canonname Chuck Lever
@ 2019-02-19 18:57 ` Peter Wagner
2019-02-19 19:40 ` Chuck Lever
2019-02-21 11:25 ` Peter Wagner
2019-02-27 16:53 ` Steve Dickson
2 siblings, 1 reply; 6+ messages in thread
From: Peter Wagner @ 2019-02-19 18:57 UTC (permalink / raw)
To: Chuck Lever; +Cc: linux-nfs, SteveD
Looks good to me.
I run tested it on musl and in combination with me nfs_freeaddrinfo
patch it works. I will post my patch in a few minutes
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>
> ---
> 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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Remove abuse of ai_canonname
2019-02-19 18:57 ` Peter Wagner
@ 2019-02-19 19:40 ` Chuck Lever
2019-02-20 18:12 ` Steve Dickson
0 siblings, 1 reply; 6+ messages in thread
From: Chuck Lever @ 2019-02-19 19:40 UTC (permalink / raw)
To: Peter Wagner; +Cc: Linux NFS Mailing List, Steve Dickson
> On Feb 19, 2019, at 1:57 PM, Peter Wagner <tripolar@gmx.at> wrote:
>
> Looks good to me.
> I run tested it on musl and in combination with me nfs_freeaddrinfo
> patch it works.
Very good. Let's see what Steve's testing shows up, if anything.
> I will post my patch in a few minutes
>
> 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>
>> ---
>> 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);
>>
>
--
Chuck Lever
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Remove abuse of ai_canonname
2019-02-19 19:40 ` Chuck Lever
@ 2019-02-20 18:12 ` Steve Dickson
0 siblings, 0 replies; 6+ messages in thread
From: Steve Dickson @ 2019-02-20 18:12 UTC (permalink / raw)
To: Chuck Lever, Peter Wagner; +Cc: Linux NFS Mailing List
On 2/19/19 2:40 PM, Chuck Lever wrote:
>> On Feb 19, 2019, at 1:57 PM, Peter Wagner <tripolar@gmx.at> wrote:
>>
>> Looks good to me.
>> I run tested it on musl and in combination with me nfs_freeaddrinfo
>> patch it works.
> Very good. Let's see what Steve's testing shows up, if anything.
I've rolled them up and smoking them now!! :-) I'll let you know...
steved.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Remove abuse of ai_canonname
2019-02-19 15:52 [PATCH v2] Remove abuse of ai_canonname Chuck Lever
2019-02-19 18:57 ` Peter Wagner
@ 2019-02-21 11:25 ` Peter Wagner
2019-02-27 16:53 ` Steve Dickson
2 siblings, 0 replies; 6+ messages in thread
From: Peter Wagner @ 2019-02-21 11:25 UTC (permalink / raw)
To: Chuck Lever; +Cc: linux-nfs, SteveD
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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Remove abuse of ai_canonname
2019-02-19 15:52 [PATCH v2] Remove abuse of ai_canonname Chuck Lever
2019-02-19 18:57 ` Peter Wagner
2019-02-21 11:25 ` Peter Wagner
@ 2019-02-27 16:53 ` Steve Dickson
2 siblings, 0 replies; 6+ messages in thread
From: Steve Dickson @ 2019-02-27 16:53 UTC (permalink / raw)
To: Chuck Lever, linux-nfs; +Cc: tripolar
On 2/19/19 10:52 AM, Chuck Lever 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>
Committed....
steved.
> ---
> 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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread