[PATCH RFC] Remove one usage of ai_canonname

* [PATCH RFC] Remove one usage of ai_canonname
@ 2019-02-19 15:31 Chuck Lever
  2019-02-19 15:40 ` Chuck Lever
  0 siblings, 1 reply; 2+ messages in thread
From: Chuck Lever @ 2019-02-19 15:31 UTC (permalink / raw)
  To: linux-nfs; +Cc: SteveD

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 |   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);


^ permalink raw reply related	[flat|nested] 2+ messages in thread