* [PATCH v2] Remove abuse of ai_canonname
@ 2019-02-19 15:52 Chuck Lever
2019-02-19 18:57 ` Peter Wagner
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Chuck Lever @ 2019-02-19 15:52 UTC (permalink / raw)
To: linux-nfs; +Cc: SteveD, tripolar
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 related [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-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
end of thread, other threads:[~2019-02-27 16:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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-20 18:12 ` Steve Dickson
2019-02-21 11:25 ` Peter Wagner
2019-02-27 16:53 ` Steve Dickson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).