[PATCH 0/3] nfs-utils patches

[PATCH 0/3] nfs-utils patches

[Chuck Lever]

Hi Steve-

Three more patches for nfs-utils in the IPv6 series.

The first is a minor bug fix for the sm-notify command, since we've
been focusing on that area recently.  It fixes a use-after-free.

The next two don't add new IPv6 functionality, but are prerequisites.
The first adds a new string parsing API that specifically handles
mount options that take a numeric value.  The second changes the
handler for one such option (retry= in this case) to use the new API.

Subsequent patches will make use of this new API to handle mount
options that are used to fill in pmap structs.

---

Chuck Lever (3):
      text-based mount command: use po_get_numeric() for handling retry=
      text-based mount command: add function to parse numeric mount options
      sm-notify command: fix a use-after-free bug


 utils/mount/parse_opt.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++
 utils/mount/parse_opt.h |    3 +++
 utils/mount/stropts.c   |   20 +++++++++--------
 utils/statd/sm-notify.c |   25 ++++++++++++---------
 4 files changed, 83 insertions(+), 20 deletions(-)

-- 
Chuck Lever

[PATCH 1/3] sm-notify command: fix a use-after-free bug

[Chuck Lever]

The recv_reply() function was referencing host->ai in a freeaddrinfo(3)
call after it had freed @host.

This is not likely to be harmful in a single-threaded user context,
but it's still bad form, and it will get called out if testing
sm-notify with poisoned free memory.  The less noise, the better we
are able to see real problems.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/statd/sm-notify.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index 9342bad..943caf8 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -133,6 +133,17 @@ static struct addrinfo *smn_lookup(const sa_family_t family, const char *name)
 	return ai;
 }
 
+static void smn_forget_host(struct nsm_host *host)
+{
+	unlink(host->path);
+	free(host->path);
+	free(host->name);
+	if (host->ai)
+		freeaddrinfo(host->ai);
+
+	free(host);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -342,13 +353,8 @@ notify(void)
 			hp = hosts;
 			hosts = hp->next;
 
-			if (notify_host(sock, hp)){
-				unlink(hp->path);
-				free(hp->name);
-				free(hp->path);
-				free(hp);
+			if (notify_host(sock, hp))
 				continue;
-			}
 
 			/* Set the timeout for this call, using an
 			   exponential timeout strategy */
@@ -403,6 +409,7 @@ notify_host(int sock, struct nsm_host *host)
 			nsm_log(LOG_WARNING,
 				"%s doesn't seem to be a valid address,"
 				" skipped", host->name);
+			smn_forget_host(host);
 			return 1;
 		}
 	}
@@ -547,11 +554,7 @@ recv_reply(int sock)
 		if (p <= end) {
 			nsm_log(LOG_DEBUG, "Host %s notified successfully",
 					hp->name);
-			unlink(hp->path);
-			free(hp->name);
-			free(hp->path);
-			free(hp);
-			freeaddrinfo(hp->ai);
+			smn_forget_host(hp);
 			return;
 		}
 	}

[PATCH 2/3] text-based mount command: add function to parse numeric mount options

[Chuck Lever]

Introduce a function that is especially for parsing keyword mount options
that take a numeric value.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/mount/parse_opt.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++
 utils/mount/parse_opt.h |    3 +++
 2 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/utils/mount/parse_opt.c b/utils/mount/parse_opt.c
index cb398bd..f61d0dd 100644
--- a/utils/mount/parse_opt.c
+++ b/utils/mount/parse_opt.c
@@ -36,6 +36,10 @@
  */
 
 
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
 #include <ctype.h>
 #include <unistd.h>
 #include <stdio.h>
@@ -366,6 +370,57 @@ char *po_get(struct mount_options *options, char *keyword)
 }
 
 /**
+ * po_get_numeric - return numeric value of rightmost instance of keyword option
+ * @options: pointer to mount options
+ * @keyword: pointer to a C string containing option keyword for which to search
+ * @value: OUT: set to the value of the keyword
+ *
+ * This is specifically for parsing keyword options that take only a numeric
+ * value.  If multiple instances of the same option are present in a mount
+ * option list, the rightmost instance is always the effective one.
+ *
+ * Returns:
+ *	* PO_FOUND if the keyword was found and the value is numeric; @value is
+ *	  set to the keyword's value
+ *	* PO_NOT_FOUND if the keyword was not found
+ *	* PO_BAD_VALUE if the keyword was found, but the value is not numeric
+ *
+ * These last two are separate in case the caller wants to warn about bad mount
+ * options instead of silently using a default.
+ */
+#ifdef HAVE_STRTOL
+po_found_t po_get_numeric(struct mount_options *options, char *keyword, long *value)
+{
+	char *option, *endptr;
+	long tmp;
+
+	option = po_get(options, keyword);
+	if (option == NULL)
+		return PO_NOT_FOUND;
+
+	errno = 0;
+	tmp = strtol(option, &endptr, 10);
+	if (errno == 0 && endptr != option) {
+		*value = tmp;
+		return PO_FOUND;
+	}
+	return PO_BAD_VALUE;
+}
+#else	/* HAVE_STRTOL */
+po_found_t po_get_numeric(struct mount_options *options, char *keyword, long *value)
+{
+	char *option;
+
+	option = po_get(options, keyword);
+	if (option == NULL)
+		return PO_NOT_FOUND;
+
+	*value = atoi(option);
+	return PO_FOUND;
+}
+#endif	/* HAVE_STRTOL */
+
+/**
  * po_rightmost - determine the relative position of two options
  * @options: pointer to mount options
  * @key1: pointer to a C string containing an option keyword
diff --git a/utils/mount/parse_opt.h b/utils/mount/parse_opt.h
index fb003c3..199630f 100644
--- a/utils/mount/parse_opt.h
+++ b/utils/mount/parse_opt.h
@@ -32,6 +32,7 @@ typedef enum {
 typedef enum {
 	PO_NOT_FOUND = 0,
 	PO_FOUND = 1,
+	PO_BAD_VALUE = 2,
 } po_found_t;
 
 typedef enum {
@@ -50,6 +51,8 @@ po_return_t		po_join(struct mount_options *, char **);
 po_return_t		po_append(struct mount_options *, char *);
 po_found_t		po_contains(struct mount_options *, char *);
 char *			po_get(struct mount_options *, char *);
+po_found_t		po_get_numeric(struct mount_options *,
+					char *, long *);
 po_rightmost_t		po_rightmost(struct mount_options *, char *, char *);
 po_found_t		po_remove_all(struct mount_options *, char *);
 void			po_destroy(struct mount_options *);

[PATCH 3/3] text-based mount command: use po_get_numeric() for handling retry=

[Chuck Lever]

Replace the logic in nfs_parse_retry_option() with a call to the new
po_get_numeric() function.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/mount/stropts.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 09fca86..43791e6 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -99,19 +99,21 @@ struct nfsmount_info {
 static time_t nfs_parse_retry_option(struct mount_options *options,
 				     unsigned int timeout_minutes)
 {
-	char *retry_option, *endptr;
+	long tmp;
 
-	retry_option = po_get(options, "retry");
-	if (retry_option) {
-		long tmp;
-
-		errno = 0;
-		tmp = strtol(retry_option, &endptr, 10);
-		if (errno == 0 && endptr != retry_option && tmp >= 0)
+	switch (po_get_numeric(options, "retry", &tmp)) {
+	case PO_NOT_FOUND:
+		break;
+	case PO_FOUND:
+		if (tmp >= 0) {
 			timeout_minutes = tmp;
-		else if (verbose)
+			break;
+		}
+	case PO_BAD_VALUE:
+		if (verbose)
 			nfs_error(_("%s: invalid retry timeout was specified; "
 					"using default timeout"), progname);
+		break;
 	}
 
 	return time(NULL) + (time_t)(timeout_minutes * 60);

Re: [PATCH 0/3] nfs-utils patches

[Steve Dickson]

Chuck Lever wrote:
> Hi Steve-
> 
> Three more patches for nfs-utils in the IPv6 series.
> 
> The first is a minor bug fix for the sm-notify command, since we've
> been focusing on that area recently.  It fixes a use-after-free.
> 
> The next two don't add new IPv6 functionality, but are prerequisites.
> The first adds a new string parsing API that specifically handles
> mount options that take a numeric value.  The second changes the
> handler for one such option (retry= in this case) to use the new API.
> 
> Subsequent patches will make use of this new API to handle mount
> options that are used to fill in pmap structs.
> 
> ---
> 
> Chuck Lever (3):
>       text-based mount command: use po_get_numeric() for handling retry=
>       text-based mount command: add function to parse numeric mount options
>       sm-notify command: fix a use-after-free bug
Committed.... 

steved.