[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[Libguestfs] [PATCH nbdkit v2] server: public: Add nbdkit_parse_* functions for safely parsing integers.



On Mon, Sep 23, 2019 at 01:23:28PM -0500, Eric Blake wrote:
> > Hopefully it will warn us if uid_t stops being int.  (Also
> > we're assuming pid_t == int).
> 
> 64-bit mingw has had a long-standing bug that pid_t was declared as
> 64-bit, even though getpid() returns a 32-bit 'int'; I wish they'd fix
> their headers, but it violates the assumption of pid_t == int.
> 
> Also, POSIX is considering standardizing fcntl(F_SETOWN_EX) in order to
> allow pid_t > int: http://austingroupbugs.net/view.php?id=1274
> 
> So while your assumption may hold for now on Linux and BSD, I wouldn't
> hold my breath on it lasting forever.

I should say our existing code already has this bug, this
patch doesn't change it :-)

Below is V2 which fixes everything you mentioned.  In this version I
have got rid of long, unsigned_long, size_t and ssize_t, but added
int8_t and uint8_t.

Rich.

>From 375e286be27f563a9f1a886e29bdcfcebebfa81c Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones redhat com>
Date: Sat, 21 Sep 2019 07:30:40 +0100
Subject: [PATCH] server: public: Add nbdkit_parse_* functions for safely
 parsing integers.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

sscanf is sadly not safe (because it doesn't handle integer overflow
correctly), and strto*l functions are a pain to use correctly.
Therefore add some functions to hide the pain of parsing integers from
the command line.

The simpler uses of sscanf and strto*l are replaced.  There are still
a few where we are using advanced features of sscanf.

This may change command line parsing in a few corner cases:

* For some parameters you might have written (eg)
  ‘cache-high-threshold=08’ to mean decimal 8, but now it would be a
  parse error.  ‘cache-high-threshold=010’ would previously have
  parsed as decimal 10, but would now parse as octal 10 (decimal 8).

* Some parameters previously accepted a wider range of values and will
  now give an error (but it should be that the narrower range is more
  correct).
---
 docs/nbdkit-plugin.pod              |  46 +++++++
 filters/cache/cache.c               |  16 ++-
 filters/cache/cache.h               |   2 +-
 filters/partition/partition.c       |   8 +-
 filters/partition/partition.h       |   2 +-
 filters/retry/retry.c               |  14 +--
 filters/xz/xz.c                     |  12 +-
 include/nbdkit-common.h             |  20 +++
 plugins/curl/curl.c                 |   8 +-
 plugins/nbd/nbd-standalone.c        |   9 +-
 plugins/nbd/nbd.c                   |   9 +-
 plugins/partitioning/partitioning.c |   6 +-
 plugins/partitioning/virtual-disk.h |   4 +-
 plugins/random/random.c             |   4 +-
 plugins/ssh/ssh.c                   |  15 ++-
 plugins/vddk/vddk.c                 |  12 +-
 server/internal.h                   |   2 +-
 server/main.c                       |  21 +---
 server/nbdkit.syms                  |  10 ++
 server/public.c                     | 186 ++++++++++++++++++++++++++++
 server/socket-activation.c          |  10 +-
 server/test-public.c                | 180 ++++++++++++++++++++++++++-
 server/usergroup.c                  |   4 +-
 23 files changed, 504 insertions(+), 96 deletions(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 2a84444..e34ffd1 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -1013,6 +1013,52 @@ might be killed by C<SIGKILL> or segfault).
 
 =head1 PARSING COMMAND LINE PARAMETERS
 
+=head2 Parsing numbers
+
+There are several functions for parsing numbers.  These all deal
+correctly with overflow, out of range and parse errors, and you should
+use them instead of unsafe functions like L<sscanf(3)>, L<atoi(3)> and
+similar.
+
+ int nbdkit_parse_int (const char *what, const char *str, int *r);
+ int nbdkit_parse_unsigned (const char *what,
+                            const char *str, unsigned *r);
+ int nbdkit_parse_int8_t (const char *what,
+                          const char *str, int8_t *r);
+ int nbdkit_parse_uint8_t (const char *what,
+                           const char *str, uint8_t *r);
+ int nbdkit_parse_int16_t (const char *what,
+                           const char *str, int16_t *r);
+ int nbdkit_parse_uint16_t (const char *what,
+                            const char *str, uint16_t *r);
+ int nbdkit_parse_int32_t (const char *what,
+                           const char *str, int32_t *r);
+ int nbdkit_parse_uint32_t (const char *what,
+                            const char *str, uint32_t *r);
+ int nbdkit_parse_int64_t (const char *what,
+                           const char *str, int64_t *r);
+ int nbdkit_parse_uint64_t (const char *what,
+                            const char *str, uint64_t *r);
+
+Parse string C<str> into an integer of various types.  These functions
+parse a decimal, hexadecimal (C<"0x...">) or octal (C<"0...">) number.
+
+On success the functions return C<0> and set C<*r> to the parsed value
+(unless C<*r == NULL> in which case the result is discarded).  On
+error, C<nbdkit_error> is called and the functions return C<-1>.  On
+error C<*r> is always unchanged.
+
+The C<what> parameter is printed in error messages to provide context.
+It should usually be a short descriptive string of what you are trying
+to parse, eg:
+
+ if (nbdkit_parse_int ("random seed", argv[1], &seed) == -1)
+   return -1;
+
+might print an error:
+
+ random seed: could not parse number: "lalala"
+
 =head2 Parsing sizes
 
 Use the C<nbdkit_parse_size> utility function to parse human-readable
diff --git a/filters/cache/cache.c b/filters/cache/cache.c
index 14a3c0a..faf6023 100644
--- a/filters/cache/cache.c
+++ b/filters/cache/cache.c
@@ -70,7 +70,7 @@ static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
 unsigned blksize;
 enum cache_mode cache_mode = CACHE_MODE_WRITEBACK;
 int64_t max_size = -1;
-int hi_thresh = 95, lo_thresh = 80;
+unsigned hi_thresh = 95, lo_thresh = 80;
 bool cache_on_read = false;
 
 static int cache_flush (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle, uint32_t flags, int *err);
@@ -129,22 +129,20 @@ cache_config (nbdkit_next_config *next, void *nxdata,
     return 0;
   }
   else if (strcmp (key, "cache-high-threshold") == 0) {
-    if (sscanf (value, "%d", &hi_thresh) != 1) {
-      nbdkit_error ("invalid cache-high-threshold parameter: %s", value);
+    if (nbdkit_parse_unsigned ("cache-high-threshold",
+                               value, &hi_thresh) == -1)
       return -1;
-    }
-    if (hi_thresh <= 0) {
+    if (hi_thresh == 0) {
       nbdkit_error ("cache-high-threshold must be greater than zero");
       return -1;
     }
     return 0;
   }
   else if (strcmp (key, "cache-low-threshold") == 0) {
-    if (sscanf (value, "%d", &lo_thresh) != 1) {
-      nbdkit_error ("invalid cache-low-threshold parameter: %s", value);
+    if (nbdkit_parse_unsigned ("cache-low-threshold",
+                               value, &lo_thresh) == -1)
       return -1;
-    }
-    if (lo_thresh <= 0) {
+    if (lo_thresh == 0) {
       nbdkit_error ("cache-low-threshold must be greater than zero");
       return -1;
     }
diff --git a/filters/cache/cache.h b/filters/cache/cache.h
index c67e173..2b72221 100644
--- a/filters/cache/cache.h
+++ b/filters/cache/cache.h
@@ -47,7 +47,7 @@ extern unsigned blksize;
 
 /* Maximum size of the cache and high/low thresholds. */
 extern int64_t max_size;
-extern int hi_thresh, lo_thresh;
+extern unsigned hi_thresh, lo_thresh;
 
 /* Cache read requests. */
 extern bool cache_on_read;
diff --git a/filters/partition/partition.c b/filters/partition/partition.c
index 15f785e..17dd33a 100644
--- a/filters/partition/partition.c
+++ b/filters/partition/partition.c
@@ -45,7 +45,7 @@
 
 #include "partition.h"
 
-int partnum = -1;
+unsigned partnum = 0;
 
 /* Called for each key=value passed on the command line. */
 static int
@@ -53,7 +53,9 @@ partition_config (nbdkit_next_config *next, void *nxdata,
                   const char *key, const char *value)
 {
   if (strcmp (key, "partition") == 0) {
-    if (sscanf (value, "%d", &partnum) != 1 || partnum <= 0) {
+    if (nbdkit_parse_unsigned ("partition", value, &partnum) == -1)
+      return -1;
+    if (partnum == 0) {
       nbdkit_error ("invalid partition number");
       return -1;
     }
@@ -67,7 +69,7 @@ partition_config (nbdkit_next_config *next, void *nxdata,
 static int
 partition_config_complete (nbdkit_next_config_complete *next, void *nxdata)
 {
-  if (partnum == -1) {
+  if (partnum == 0) {
     nbdkit_error ("you must supply the partition parameter on the command line");
     return -1;
   }
diff --git a/filters/partition/partition.h b/filters/partition/partition.h
index e25a695..b20d39f 100644
--- a/filters/partition/partition.h
+++ b/filters/partition/partition.h
@@ -37,7 +37,7 @@
 
 #define SECTOR_SIZE 512
 
-extern int partnum;
+extern unsigned partnum;
 
 extern int find_mbr_partition (struct nbdkit_next_ops *next_ops, void *nxdata,
                                int64_t size, uint8_t *mbr,
diff --git a/filters/retry/retry.c b/filters/retry/retry.c
index b1864fa..4d73f17 100644
--- a/filters/retry/retry.c
+++ b/filters/retry/retry.c
@@ -44,8 +44,8 @@
 
 #include "cleanup.h"
 
-static int retries = 5;         /* 0 = filter is disabled */
-static int initial_delay = 2;
+static unsigned retries = 5;    /* 0 = filter is disabled */
+static unsigned initial_delay = 2;
 static bool exponential_backoff = true;
 static bool force_readonly = false;
 
@@ -67,15 +67,15 @@ retry_config (nbdkit_next_config *next, void *nxdata,
   int r;
 
   if (strcmp (key, "retries") == 0) {
-    if (sscanf (value, "%d", &retries) != 1 || retries < 0) {
-      nbdkit_error ("cannot parse retries: %s", value);
+    if (nbdkit_parse_unsigned ("retries", value, &retries) == -1)
       return -1;
-    }
     return 0;
   }
   else if (strcmp (key, "retry-delay") == 0) {
-    if (sscanf (value, "%d", &initial_delay) != 1 || initial_delay <= 0) {
-      nbdkit_error ("cannot parse retry-delay: %s", value);
+    if (nbdkit_parse_unsigned ("retry-delay", value, &initial_delay) == -1)
+      return -1;
+    if (initial_delay == 0) {
+      nbdkit_error ("retry-delay cannot be 0");
       return -1;
     }
     return 0;
diff --git a/filters/xz/xz.c b/filters/xz/xz.c
index a420d38..4445ce0 100644
--- a/filters/xz/xz.c
+++ b/filters/xz/xz.c
@@ -49,7 +49,7 @@
 #include "blkcache.h"
 
 static uint64_t maxblock = 512 * 1024 * 1024;
-static size_t maxdepth = 8;
+static uint32_t maxdepth = 8;
 
 static int
 xz_config (nbdkit_next_config *next, void *nxdata,
@@ -63,18 +63,12 @@ xz_config (nbdkit_next_config *next, void *nxdata,
     return 0;
   }
   else if (strcmp (key, "xz-max-depth") == 0) {
-    size_t r;
-
-    if (sscanf (value, "%zu", &r) != 1) {
-      nbdkit_error ("could not parse 'xz-max-depth' parameter");
+    if (nbdkit_parse_uint32_t ("xz-max-depth", value, &maxdepth) == -1)
       return -1;
-    }
-    if (r == 0) {
+    if (maxdepth == 0) {
       nbdkit_error ("'xz-max-depth' parameter must be >= 1");
       return -1;
     }
-
-    maxdepth = r;
     return 0;
   }
   else
diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index aac63fb..e2a8b88 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -84,6 +84,26 @@ extern void nbdkit_vdebug (const char *msg, va_list args);
 extern char *nbdkit_absolute_path (const char *path);
 extern int64_t nbdkit_parse_size (const char *str);
 extern int nbdkit_parse_bool (const char *str);
+extern int nbdkit_parse_int (const char *what, const char *str,
+                             int *r);
+extern int nbdkit_parse_unsigned (const char *what, const char *str,
+                                  unsigned *r);
+extern int nbdkit_parse_int8_t (const char *what, const char *str,
+                                int8_t *r);
+extern int nbdkit_parse_uint8_t (const char *what, const char *str,
+                                 uint8_t *r);
+extern int nbdkit_parse_int16_t (const char *what, const char *str,
+                                 int16_t *r);
+extern int nbdkit_parse_uint16_t (const char *what, const char *str,
+                                  uint16_t *r);
+extern int nbdkit_parse_int32_t (const char *what, const char *str,
+                                 int32_t *r);
+extern int nbdkit_parse_uint32_t (const char *what, const char *str,
+                                  uint32_t *r);
+extern int nbdkit_parse_int64_t (const char *what, const char *str,
+                                 int64_t *r);
+extern int nbdkit_parse_uint64_t (const char *what, const char *str,
+                                  uint64_t *r);
 extern int nbdkit_read_password (const char *value, char **password);
 extern char *nbdkit_realpath (const char *path);
 extern int nbdkit_nanosleep (unsigned sec, unsigned nsec);
diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c
index 0ed3984..cf01aae 100644
--- a/plugins/curl/curl.c
+++ b/plugins/curl/curl.c
@@ -62,7 +62,7 @@ static const char *proxy_user = NULL;
 static char *proxy_password = NULL;
 static char *cookie = NULL;
 static bool sslverify = true;
-static long timeout = 0;
+static uint32_t timeout = 0;
 static const char *unix_socket_path = NULL;
 static long protocols = CURLPROTO_ALL;
 
@@ -204,7 +204,9 @@ curl_config (const char *key, const char *value)
   }
 
   else if (strcmp (key, "timeout") == 0) {
-    if (sscanf (value, "%ld", &timeout) != 1 || timeout < 0) {
+    if (nbdkit_parse_uint32_t ("timeout", value, &timeout) == -1)
+      return -1;
+    if (timeout < 0) {
       nbdkit_error ("'timeout' must be 0 or a positive timeout in seconds");
       return -1;
     }
@@ -334,7 +336,7 @@ curl_open (int readonly)
     curl_easy_setopt (h->c, CURLOPT_REDIR_PROTOCOLS, protocols);
   }
   if (timeout > 0)
-    curl_easy_setopt (h->c, CURLOPT_TIMEOUT, timeout);
+    curl_easy_setopt (h->c, CURLOPT_TIMEOUT, (long) timeout);
   if (!sslverify) {
     curl_easy_setopt (h->c, CURLOPT_SSL_VERIFYPEER, 0L);
     curl_easy_setopt (h->c, CURLOPT_SSL_VERIFYHOST, 0L);
diff --git a/plugins/nbd/nbd-standalone.c b/plugins/nbd/nbd-standalone.c
index 1468098..0fabbfe 100644
--- a/plugins/nbd/nbd-standalone.c
+++ b/plugins/nbd/nbd-standalone.c
@@ -102,7 +102,7 @@ static char *servname;
 static const char *export;
 
 /* Number of retries */
-static unsigned long retry;
+static unsigned retry;
 
 /* True to share single server connection among all clients */
 static bool shared;
@@ -128,7 +128,6 @@ nbd_unload (void)
 static int
 nbd_config (const char *key, const char *value)
 {
-  char *end;
   int r;
 
   if (strcmp (key, "socket") == 0) {
@@ -145,12 +144,8 @@ nbd_config (const char *key, const char *value)
   else if (strcmp (key, "export") == 0)
     export = value;
   else if (strcmp (key, "retry") == 0) {
-    errno = 0;
-    retry = strtoul (value, &end, 0);
-    if (value == end || errno) {
-      nbdkit_error ("could not parse retry as integer (%s)", value);
+    if (nbdkit_parse_unsigned ("retry", value, &retry) == -1)
       return -1;
-    }
   }
   else if (strcmp (key, "shared") == 0) {
     r = nbdkit_parse_bool (value);
diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index cddcfde..3215636 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -89,7 +89,7 @@ static const char *uri;
 static const char *export;
 
 /* Number of retries */
-static unsigned long retry;
+static unsigned retry;
 
 /* True to share single server connection among all clients */
 static bool shared;
@@ -124,7 +124,6 @@ nbdplug_unload (void)
 static int
 nbdplug_config (const char *key, const char *value)
 {
-  char *end;
   int r;
 
   if (strcmp (key, "socket") == 0) {
@@ -143,12 +142,8 @@ nbdplug_config (const char *key, const char *value)
   else if (strcmp (key, "export") == 0)
     export = value;
   else if (strcmp (key, "retry") == 0) {
-    errno = 0;
-    retry = strtoul (value, &end, 0);
-    if (value == end || errno) {
-      nbdkit_error ("could not parse retry as integer (%s)", value);
+    if (nbdkit_parse_unsigned ("retry", value, &retry) == -1)
       return -1;
-    }
   }
   else if (strcmp (key, "shared") == 0) {
     r = nbdkit_parse_bool (value);
diff --git a/plugins/partitioning/partitioning.c b/plugins/partitioning/partitioning.c
index 0b4e66b..6e426b9 100644
--- a/plugins/partitioning/partitioning.c
+++ b/plugins/partitioning/partitioning.c
@@ -68,7 +68,7 @@ int partitioning_debug_regions;
  * following partitions.
  */
 unsigned long alignment = DEFAULT_ALIGNMENT;
-int mbr_id = DEFAULT_MBR_ID;
+uint8_t mbr_id = DEFAULT_MBR_ID;
 char type_guid[16]; /* initialized by partitioning_load function below */
 
 /* partition-type parameter. */
@@ -211,10 +211,8 @@ partitioning_config (const char *key, const char *value)
   else if (strcmp (key, "mbr-id") == 0) {
     if (strcasecmp (value, "default") == 0)
       mbr_id = DEFAULT_MBR_ID;
-    else if (sscanf (value, "%i", &mbr_id) != 1) {
-      nbdkit_error ("could not parse mbr-id: %s", value);
+    else if (nbdkit_parse_uint8_t ("mbr-id", value, &mbr_id) == -1)
       return -1;
-    }
   }
   else if (strcmp (key, "type-guid") == 0) {
     if (strcasecmp (value, "default") == 0)
diff --git a/plugins/partitioning/virtual-disk.h b/plugins/partitioning/virtual-disk.h
index 512756d..e173978 100644
--- a/plugins/partitioning/virtual-disk.h
+++ b/plugins/partitioning/virtual-disk.h
@@ -69,7 +69,7 @@
 extern int partitioning_debug_regions;
 
 extern unsigned long alignment;
-extern int mbr_id;
+extern uint8_t mbr_id;
 extern char type_guid[16];
 
 #define PARTTYPE_UNSET 0
@@ -84,7 +84,7 @@ struct file {
   struct stat statbuf;
   char guid[16];                /* random GUID used for GPT */
   unsigned long alignment;      /* alignment of this partition */
-  int mbr_id;                   /* MBR ID of this partition */
+  uint8_t mbr_id;               /* MBR ID of this partition */
   char type_guid[16];           /* partition type GUID of this partition */
 };
 
diff --git a/plugins/random/random.c b/plugins/random/random.c
index 6377310..cdebdf9 100644
--- a/plugins/random/random.c
+++ b/plugins/random/random.c
@@ -67,10 +67,8 @@ random_config (const char *key, const char *value)
   int64_t r;
 
   if (strcmp (key, "seed") == 0) {
-    if (sscanf (value, "%" SCNu32, &seed) != 1) {
-      nbdkit_error ("could not parse seed parameter");
+    if (nbdkit_parse_uint32_t ("seed", value, &seed) == -1)
       return -1;
-    }
   }
   else if (strcmp (key, "size") == 0) {
     r = nbdkit_parse_size (value);
diff --git a/plugins/ssh/ssh.c b/plugins/ssh/ssh.c
index ee42ee1..d163294 100644
--- a/plugins/ssh/ssh.c
+++ b/plugins/ssh/ssh.c
@@ -38,6 +38,7 @@
 #include <stdarg.h>
 #include <stdint.h>
 #include <inttypes.h>
+#include <limits.h>
 #include <string.h>
 #include <unistd.h>
 #include <fcntl.h>
@@ -59,7 +60,7 @@ static bool verify_remote_host = true;
 static const char *known_hosts = NULL;
 static const char **identity = NULL;
 static size_t nr_identities = 0;
-static long timeout = 0;
+static uint32_t timeout = 0;
 
 /* config can be:
  * NULL => parse options from default file
@@ -151,8 +152,11 @@ ssh_config (const char *key, const char *value)
   }
 
   else if (strcmp (key, "timeout") == 0) {
-    if (sscanf (value, "%ld", &timeout) != 1) {
-      nbdkit_error ("cannot parse timeout: %s", value);
+    if (nbdkit_parse_uint32_t ("timeout", value, &timeout) == -1)
+      return -1;
+    /* Because we have to cast it to long before calling the libssh API. */
+    if (timeout > LONG_MAX) {
+      nbdkit_error ("timeout too large");
       return -1;
     }
   }
@@ -403,9 +407,10 @@ ssh_open (int readonly)
     }
   }
   if (timeout > 0) {
-    r = ssh_options_set (h->session, SSH_OPTIONS_TIMEOUT, &timeout);
+    long arg = timeout;
+    r = ssh_options_set (h->session, SSH_OPTIONS_TIMEOUT, &arg);
     if (r != SSH_OK) {
-      nbdkit_error ("failed to set timeout in libssh session: %ld: %s",
+      nbdkit_error ("failed to set timeout in libssh session: %" PRIu32 ": %s",
                     timeout, ssh_get_error (h->session));
       goto err;
     }
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 8ea05b1..db12a85 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -91,9 +91,9 @@ static char *config = NULL;                /* config */
 static const char *cookie = NULL;          /* cookie */
 static const char *filename = NULL;        /* file */
 static char *libdir = NULL;                /* libdir */
-static int nfc_host_port = 0;              /* nfchostport */
+static uint16_t nfc_host_port = 0;         /* nfchostport */
 static char *password = NULL;              /* password */
-static int port = 0;                       /* port */
+static uint16_t port = 0;                  /* port */
 static const char *server_name = NULL;     /* server */
 static bool single_link = false;           /* single-link */
 static const char *snapshot_moref = NULL;  /* snapshot */
@@ -271,10 +271,8 @@ vddk_config (const char *key, const char *value)
       return -1;
   }
   else if (strcmp (key, "nfchostport") == 0) {
-    if (sscanf (value, "%d", &nfc_host_port) != 1) {
-      nbdkit_error ("cannot parse nfchostport: %s", value);
+    if (nbdkit_parse_uint16_t ("nfchostport", value, &nfc_host_port) == -1)
       return -1;
-    }
   }
   else if (strcmp (key, "password") == 0) {
     free (password);
@@ -282,10 +280,8 @@ vddk_config (const char *key, const char *value)
       return -1;
   }
   else if (strcmp (key, "port") == 0) {
-    if (sscanf (value, "%d", &port) != 1) {
-      nbdkit_error ("cannot parse port: %s", value);
+    if (nbdkit_parse_uint16_t ("port", value, &port) == -1)
       return -1;
-    }
   }
   else if (strcmp (key, "server") == 0) {
     server_name = value;
diff --git a/server/internal.h b/server/internal.h
index 043a13d..fbabce6 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -98,7 +98,7 @@ extern bool read_only;
 extern const char *run;
 extern bool listen_stdin;
 extern const char *selinux_label;
-extern int threads;
+extern unsigned threads;
 extern int tls;
 extern const char *tls_certificates_dir;
 extern const char *tls_psk;
diff --git a/server/main.c b/server/main.c
index d433c1f..975716d 100644
--- a/server/main.c
+++ b/server/main.c
@@ -76,7 +76,7 @@ bool read_only;                 /* -r */
 const char *run;                /* --run */
 bool listen_stdin;              /* -s */
 const char *selinux_label;      /* --selinux-label */
-int threads;                    /* -t */
+unsigned threads;               /* -t */
 int tls;                        /* --tls : 0=off 1=on 2=require */
 const char *tls_certificates_dir; /* --tls-certificates */
 const char *tls_psk;            /* --tls-psk */
@@ -149,7 +149,6 @@ main (int argc, char *argv[])
   } *filter_filenames = NULL;
   size_t i;
   const char *magic_config_key;
-  char *end;
 
   /* Refuse to run if stdin/out/err are closed, whether or not -s is used. */
   if (fcntl (STDERR_FILENO, F_GETFL) == -1) {
@@ -217,7 +216,8 @@ main (int argc, char *argv[])
         if (!flag->name) goto debug_flag_perror;
         flag->flag = strndup (p, q-p-1);
         if (!flag->flag) goto debug_flag_perror;
-        if (sscanf (q, "%d", &flag->value) != 1) goto bad_debug_flag;
+        if (nbdkit_parse_int ("flag", q, &flag->value) == -1)
+          goto bad_debug_flag;
         flag->used = false;
 
         /* Add flag to the linked list. */
@@ -351,13 +351,9 @@ main (int argc, char *argv[])
       break;
 
     case MASK_HANDSHAKE_OPTION:
-      errno = 0;
-      mask_handshake = strtoul (optarg, &end, 0);
-      if (errno || *end) {
-        fprintf (stderr, "%s: cannot parse '%s' into mask-handshake\n",
-                 program_name, optarg);
+      if (nbdkit_parse_unsigned ("mask-handshake",
+                                 optarg, &mask_handshake) == -1)
         exit (EXIT_FAILURE);
-      }
       break;
 
     case 'n':
@@ -401,13 +397,8 @@ main (int argc, char *argv[])
       break;
 
     case 't':
-      errno = 0;
-      threads = strtoul (optarg, &end, 0);
-      if (errno || *end) {
-        fprintf (stderr, "%s: cannot parse '%s' into threads\n",
-                 program_name, optarg);
+      if (nbdkit_parse_unsigned ("threads", optarg, &threads) == -1)
         exit (EXIT_FAILURE);
-      }
       /* XXX Worth a maximimum limit on threads? */
       break;
 
diff --git a/server/nbdkit.syms b/server/nbdkit.syms
index d792a5f..390972e 100644
--- a/server/nbdkit.syms
+++ b/server/nbdkit.syms
@@ -49,7 +49,17 @@
     nbdkit_get_extent;
     nbdkit_nanosleep;
     nbdkit_parse_bool;
+    nbdkit_parse_int8_t;
+    nbdkit_parse_int16_t;
+    nbdkit_parse_int32_t;
+    nbdkit_parse_int64_t;
+    nbdkit_parse_int;
     nbdkit_parse_size;
+    nbdkit_parse_uint8_t;
+    nbdkit_parse_uint16_t;
+    nbdkit_parse_uint32_t;
+    nbdkit_parse_uint64_t;
+    nbdkit_parse_unsigned;
     nbdkit_peer_name;
     nbdkit_read_password;
     nbdkit_realpath;
diff --git a/server/public.c b/server/public.c
index d0d9ff4..9a3aa31 100644
--- a/server/public.c
+++ b/server/public.c
@@ -45,6 +45,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <limits.h>
+#include <ctype.h>
 #include <termios.h>
 #include <errno.h>
 #include <poll.h>
@@ -108,6 +109,191 @@ nbdkit_realpath (const char *path)
   return ret;
 }
 
+/* Common code for parsing integers. */
+#define PARSE_COMMON_TAIL                                               \
+  if (errno != 0) {                                                     \
+    nbdkit_error ("%s: could not parse number: \"%s\": %m",             \
+                  what, str);                                           \
+    return -1;                                                          \
+  }                                                                     \
+  if (end == str) {                                                     \
+    nbdkit_error ("%s: empty string where we expected a number",        \
+                  what);                                                \
+    return -1;                                                          \
+  }                                                                     \
+  if (*end) {                                                           \
+    nbdkit_error ("%s: could not parse number: \"%s\": trailing garbage", \
+                  what, str);                                           \
+    return -1;                                                          \
+  }                                                                     \
+                                                                        \
+  if (rp)                                                               \
+    *rp = r;                                                            \
+  return 0
+
+/* Functions for parsing signed integers. */
+int
+nbdkit_parse_int (const char *what, const char *str, int *rp)
+{
+  long r;
+  char *end;
+
+  errno = 0;
+  r = strtol (str, &end, 0);
+#if INT_MAX != LONG_MAX
+  if (r < INT_MIN || r > INT_MAX)
+    errno = ERANGE;
+#endif
+  PARSE_COMMON_TAIL;
+}
+
+int
+nbdkit_parse_int8_t (const char *what, const char *str, int8_t *rp)
+{
+  long r;
+  char *end;
+
+  errno = 0;
+  r = strtol (str, &end, 0);
+  if (r < INT8_MIN || r > INT8_MAX)
+    errno = ERANGE;
+  PARSE_COMMON_TAIL;
+}
+
+int
+nbdkit_parse_int16_t (const char *what, const char *str, int16_t *rp)
+{
+  long r;
+  char *end;
+
+  errno = 0;
+  r = strtol (str, &end, 0);
+  if (r < INT16_MIN || r > INT16_MAX)
+    errno = ERANGE;
+  PARSE_COMMON_TAIL;
+}
+
+int
+nbdkit_parse_int32_t (const char *what, const char *str, int32_t *rp)
+{
+  long r;
+  char *end;
+
+  errno = 0;
+  r = strtol (str, &end, 0);
+#if INT32_MAX != LONG_MAX
+  if (r < INT32_MIN || r > INT32_MAX)
+    errno = ERANGE;
+#endif
+  PARSE_COMMON_TAIL;
+}
+
+int
+nbdkit_parse_int64_t (const char *what, const char *str, int64_t *rp)
+{
+  long long r;
+  char *end;
+
+  errno = 0;
+  r = strtoll (str, &end, 0);
+#if INT64_MAX != LONGLONG_MAX
+  if (r < INT64_MIN || r > INT64_MAX)
+    errno = ERANGE;
+#endif
+  PARSE_COMMON_TAIL;
+}
+
+/* Functions for parsing unsigned integers. */
+
+/* strtou* functions have surprising behaviour if the first character
+ * (after whitespace) is '-', so reject this early.
+ */
+#define PARSE_ERROR_IF_NEGATIVE                                         \
+  do {                                                                  \
+    while (isspace (*str))                                              \
+      str++;                                                            \
+    if (*str == '-') {                                                  \
+      nbdkit_error ("%s: negative numbers are not allowed", what);      \
+      return -1;                                                        \
+    }                                                                   \
+  } while (0)
+
+int
+nbdkit_parse_unsigned (const char *what, const char *str, unsigned *rp)
+{
+  unsigned long r;
+  char *end;
+
+  PARSE_ERROR_IF_NEGATIVE;
+  errno = 0;
+  r = strtoul (str, &end, 0);
+#if UINT_MAX != ULONG_MAX
+  if (r > UINT_MAX)
+    errno = ERANGE;
+#endif
+  PARSE_COMMON_TAIL;
+}
+
+int
+nbdkit_parse_uint8_t (const char *what, const char *str, uint8_t *rp)
+{
+  unsigned long r;
+  char *end;
+
+  PARSE_ERROR_IF_NEGATIVE;
+  errno = 0;
+  r = strtoul (str, &end, 0);
+  if (r > UINT8_MAX)
+    errno = ERANGE;
+  PARSE_COMMON_TAIL;
+}
+
+int
+nbdkit_parse_uint16_t (const char *what, const char *str, uint16_t *rp)
+{
+  unsigned long r;
+  char *end;
+
+  PARSE_ERROR_IF_NEGATIVE;
+  errno = 0;
+  r = strtoul (str, &end, 0);
+  if (r > UINT16_MAX)
+    errno = ERANGE;
+  PARSE_COMMON_TAIL;
+}
+
+int
+nbdkit_parse_uint32_t (const char *what, const char *str, uint32_t *rp)
+{
+  unsigned long r;
+  char *end;
+
+  PARSE_ERROR_IF_NEGATIVE;
+  errno = 0;
+  r = strtoul (str, &end, 0);
+#if UINT32_MAX != ULONG_MAX
+  if (r > UINT32_MAX)
+    errno = ERANGE;
+#endif
+  PARSE_COMMON_TAIL;
+}
+
+int
+nbdkit_parse_uint64_t (const char *what, const char *str, uint64_t *rp)
+{
+  unsigned long long r;
+  char *end;
+
+  PARSE_ERROR_IF_NEGATIVE;
+  errno = 0;
+  r = strtoull (str, &end, 0);
+#if UINT64_MAX != ULONGLONG_MAX
+  if (r > UINT64_MAX)
+    errno = ERANGE;
+#endif
+  PARSE_COMMON_TAIL;
+}
+
 /* Parse a string as a size with possible scaling suffix, or return -1
  * after reporting the error.
  */
diff --git a/server/socket-activation.c b/server/socket-activation.c
index b9db67c..50df4ca 100644
--- a/server/socket-activation.c
+++ b/server/socket-activation.c
@@ -59,11 +59,8 @@ get_socket_activation (void)
   s = getenv ("LISTEN_PID");
   if (s == NULL)
     return 0;
-  if (sscanf (s, "%u", &pid) != 1) {
-    fprintf (stderr, "%s: malformed %s environment variable (ignored)\n",
-             program_name, "LISTEN_PID");
+  if (nbdkit_parse_unsigned ("LISTEN_PID", s, &pid) == -1)
     return 0;
-  }
   if (pid != getpid ()) {
     fprintf (stderr, "%s: %s was not for us (ignored)\n",
              program_name, "LISTEN_PID");
@@ -73,11 +70,8 @@ get_socket_activation (void)
   s = getenv ("LISTEN_FDS");
   if (s == NULL)
     return 0;
-  if (sscanf (s, "%u", &nr_fds) != 1) {
-    fprintf (stderr, "%s: malformed %s environment variable (ignored)\n",
-             program_name, "LISTEN_FDS");
+  if (nbdkit_parse_unsigned ("LISTEN_FDS", s, &nr_fds) == -1)
     return 0;
-  }
 
   /* So these are not passed to any child processes we might start. */
   unsetenv ("LISTEN_FDS");
diff --git a/server/test-public.c b/server/test-public.c
index 4a01936..7b64288 100644
--- a/server/test-public.c
+++ b/server/test-public.c
@@ -33,8 +33,11 @@
 #include <config.h>
 
 #include <stdio.h>
-#include <inttypes.h>
+#include <stdlib.h>
 #include <stdbool.h>
+#include <stdint.h>
+#include <inttypes.h>
+#include <limits.h>
 #include <string.h>
 #include <unistd.h>
 
@@ -153,6 +156,180 @@ test_nbdkit_parse_size (void)
   return pass;
 }
 
+static bool
+test_nbdkit_parse_ints (void)
+{
+  bool pass = true;
+
+#define PARSE(...) PARSE_(__VA_ARGS__)
+#define PARSE_(TYPE, FORMAT, TEST, RET, EXPECTED)                        \
+  do {                                                                  \
+    error_flagged = false;                                              \
+    TYPE i = 0;                                                         \
+    int r = nbdkit_parse_##TYPE ("test", TEST, &i);                     \
+    if (r != RET || i != EXPECTED) {                                    \
+      fprintf (stderr,                                                  \
+               "%s: %d: wrong parse for %s: r=%d i=" FORMAT "\n",       \
+               __FILE__, __LINE__, TEST, r, i);                         \
+      pass = false;                                                     \
+    }                                                                   \
+    if ((r == -1) != error_flagged) {                                   \
+      fprintf (stderr,                                                  \
+               "%s: %d: wrong error message handling for %s\n",         \
+               __FILE__, __LINE__, TEST);                               \
+      pass = false;                                                     \
+    }                                                                   \
+  } while (0)
+#define OK 0
+#define BAD -1, 0
+
+  /* Test the basic parsing of decimals, hexadecimal, octal and
+   * negative numbers.
+   */
+  PARSE (int, "%d", "0",    OK, 0);
+  PARSE (int, "%d", " 0",   OK, 0);
+  PARSE (int, "%d", "  0",  OK, 0);
+  PARSE (int, "%d", "   0", OK, 0);
+  PARSE (int, "%d", "1",    OK, 1);
+  PARSE (int, "%d", " 1",   OK, 1);
+  PARSE (int, "%d", "  1",  OK, 1);
+  PARSE (int, "%d", "   1", OK, 1);
+  PARSE (int, "%d", "99",   OK, 99);
+  PARSE (int, "%d", "0x1",  OK, 1);
+  PARSE (int, "%d", "0xf",  OK, 15);
+  PARSE (int, "%d", "0x10", OK, 16);
+  PARSE (int, "%d", "0xff", OK, 255);
+  PARSE (int, "%d", "0Xff", OK, 255);
+  PARSE (int, "%d", "01",   OK, 1);
+  PARSE (int, "%d", "07",   OK, 7);
+  PARSE (int, "%d", "010",  OK, 8);
+  PARSE (int, "%d", "+0",   OK, 0);
+  PARSE (int, "%d", " +0",  OK, 0);
+  PARSE (int, "%d", "+99",  OK, 99);
+  PARSE (int, "%d", "+0xf", OK, 15);
+  PARSE (int, "%d", "+010", OK, 8);
+  PARSE (int, "%d", "-0",   OK, 0);
+  PARSE (int, "%d", " -0",  OK, 0);
+  PARSE (int, "%d", "  -0", OK, 0);
+  PARSE (int, "%d", "-99",  OK, -99);
+  PARSE (int, "%d", "-0xf", OK, -15);
+  PARSE (int, "%d", "-0XF", OK, -15);
+  PARSE (int, "%d", "-010", OK, -8);
+  PARSE (int, "%d", "2147483647", OK, 2147483647); /* INT_MAX */
+  PARSE (int, "%d", "-2147483648", OK, -2147483648); /* INT_MIN */
+  PARSE (int, "%d", "0x7fffffff", OK, 0x7fffffff);
+  PARSE (int, "%d", "-0x80000000", OK, -0x80000000);
+
+  /* Test basic error handling. */
+  PARSE (int, "%d", "",        BAD);
+  PARSE (int, "%d", "-",       BAD);
+  PARSE (int, "%d", "- 0",     BAD);
+  PARSE (int, "%d", "+",       BAD);
+  PARSE (int, "%d", "++",      BAD);
+  PARSE (int, "%d", "++0",     BAD);
+  PARSE (int, "%d", "--0",     BAD);
+  PARSE (int, "%d", "0x",      BAD);
+  PARSE (int, "%d", "0xg",     BAD);
+  PARSE (int, "%d", "08",      BAD);
+  PARSE (int, "%d", "0x1p1",   BAD);
+  PARSE (int, "%d", "42x",     BAD);
+  PARSE (int, "%d", "42e42",   BAD);
+  PARSE (int, "%d", "42-",     BAD);
+  PARSE (int, "%d", "garbage", BAD);
+  PARSE (int, "%d", "inf",     BAD);
+  PARSE (int, "%d", "nan",     BAD);
+  PARSE (int, "%d", "0.0",     BAD);
+  PARSE (int, "%d", "1,000",   BAD);
+  PARSE (int, "%d", "2147483648", BAD); /* INT_MAX + 1 */
+  PARSE (int, "%d", "-2147483649", BAD); /* INT_MIN - 1 */
+  PARSE (int, "%d", "999999999999999999999999", BAD);
+  PARSE (int, "%d", "-999999999999999999999999", BAD);
+
+  /* Test nbdkit_parse_unsigned. */
+  PARSE (unsigned, "%u", "0",    OK, 0);
+  PARSE (unsigned, "%u", " 0",   OK, 0);
+  PARSE (unsigned, "%u", "1",    OK, 1);
+  PARSE (unsigned, "%u", "99",   OK, 99);
+  PARSE (unsigned, "%u", "0x1",  OK, 1);
+  PARSE (unsigned, "%u", "0xf",  OK, 15);
+  PARSE (unsigned, "%u", "0x10", OK, 16);
+  PARSE (unsigned, "%u", "0xff", OK, 255);
+  PARSE (unsigned, "%u", "01",   OK, 1);
+  PARSE (unsigned, "%u", "07",   OK, 7);
+  PARSE (unsigned, "%u", "010",  OK, 8);
+  PARSE (unsigned, "%u", "+0",   OK, 0);
+  PARSE (unsigned, "%u", "+99",  OK, 99);
+  PARSE (unsigned, "%u", "+0xf", OK, 15);
+  PARSE (unsigned, "%u", "+010", OK, 8);
+  PARSE (unsigned, "%u", "-0",   BAD); /* this is by choice */
+  PARSE (unsigned, "%u", " -0",  BAD);
+  PARSE (unsigned, "%u", "-99",  BAD);
+  PARSE (unsigned, "%u", "-0xf", BAD);
+  PARSE (unsigned, "%u", "-010", BAD);
+  PARSE (unsigned, "%u", "2147483647", OK, 2147483647); /* INT_MAX */
+  PARSE (unsigned, "%u", "-2147483648", BAD); /* INT_MIN */
+  PARSE (unsigned, "%u", "0x7fffffff", OK, 0x7fffffff);
+  PARSE (unsigned, "%u", "-0x80000000", BAD);
+
+  /* Test nbdkit_parse_int8_t. */
+  PARSE (int8_t, "%" PRIi8, "0",     OK, 0);
+  PARSE (int8_t, "%" PRIi8, "0x7f",  OK, 0x7f);
+  PARSE (int8_t, "%" PRIi8, "-0x80", OK, -0x80);
+  PARSE (int8_t, "%" PRIi8, "0x80",  BAD);
+  PARSE (int8_t, "%" PRIi8, "-0x81", BAD);
+
+  /* Test nbdkit_parse_uint8_t. */
+  PARSE (uint8_t, "%" PRIu8, "0",     OK, 0);
+  PARSE (uint8_t, "%" PRIu8, "0xff",  OK, 0xff);
+  PARSE (uint8_t, "%" PRIu8, "0x100", BAD);
+  PARSE (uint8_t, "%" PRIu8, "-1",    BAD);
+
+  /* Test nbdkit_parse_int16_t. */
+  PARSE (int16_t, "%" PRIi16, "0",       OK, 0);
+  PARSE (int16_t, "%" PRIi16, "0x7fff",  OK, 0x7fff);
+  PARSE (int16_t, "%" PRIi16, "-0x8000", OK, -0x8000);
+  PARSE (int16_t, "%" PRIi16, "0x8000",  BAD);
+  PARSE (int16_t, "%" PRIi16, "-0x8001", BAD);
+
+  /* Test nbdkit_parse_uint16_t. */
+  PARSE (uint16_t, "%" PRIu16, "0",       OK, 0);
+  PARSE (uint16_t, "%" PRIu16, "0xffff",  OK, 0xffff);
+  PARSE (uint16_t, "%" PRIu16, "0x10000", BAD);
+  PARSE (uint16_t, "%" PRIu16, "-1",      BAD);
+
+  /* Test nbdkit_parse_int32_t. */
+  PARSE (int32_t, "%" PRIi32, "0",           OK, 0);
+  PARSE (int32_t, "%" PRIi32, "0x7fffffff",  OK, 0x7fffffff);
+  PARSE (int32_t, "%" PRIi32, "-0x80000000", OK, -0x80000000);
+  PARSE (int32_t, "%" PRIi32, "0x80000000",  BAD);
+  PARSE (int32_t, "%" PRIi32, "-0x80000001", BAD);
+
+  /* Test nbdkit_parse_uint32_t. */
+  PARSE (uint32_t, "%" PRIu32, "0",           OK, 0);
+  PARSE (uint32_t, "%" PRIu32, "0xffffffff",  OK, 0xffffffff);
+  PARSE (uint32_t, "%" PRIu32, "0x100000000", BAD);
+  PARSE (uint32_t, "%" PRIu32, "-1",          BAD);
+
+  /* Test nbdkit_parse_int64_t. */
+  PARSE (int64_t, "%" PRIi64, "0",                   OK, 0);
+  PARSE (int64_t, "%" PRIi64, "0x7fffffffffffffff",  OK, 0x7fffffffffffffff);
+  PARSE (int64_t, "%" PRIi64, "-0x8000000000000000", OK, -0x8000000000000000);
+  PARSE (int64_t, "%" PRIi64, "0x8000000000000000",  BAD);
+  PARSE (int64_t, "%" PRIi64, "-0x8000000000000001", BAD);
+
+  /* Test nbdkit_parse_uint64_t. */
+  PARSE (uint64_t, "%" PRIu64, "0",                   OK, 0);
+  PARSE (uint64_t, "%" PRIu64, "0xffffffffffffffff",  OK, 0xffffffffffffffff);
+  PARSE (uint64_t, "%" PRIu64, "0x10000000000000000", BAD);
+  PARSE (uint64_t, "%" PRIu64, "-1",                  BAD);
+
+#undef PARSE
+#undef PARSE_
+#undef OK
+#undef BAD
+  return pass;
+}
+
 static bool
 test_nbdkit_read_password (void)
 {
@@ -228,6 +405,7 @@ main (int argc, char *argv[])
 {
   bool pass = true;
   pass &= test_nbdkit_parse_size ();
+  pass &= test_nbdkit_parse_ints ();
   pass &= test_nbdkit_read_password ();
   /* nbdkit_absolute_path and nbdkit_nanosleep not unit-tested here, but
    * get plenty of coverage in the main testsuite.
diff --git a/server/usergroup.c b/server/usergroup.c
index 719c816..11bafce 100644
--- a/server/usergroup.c
+++ b/server/usergroup.c
@@ -97,7 +97,7 @@ parseuser (const char *id)
 
     saved_errno = errno;
 
-    if (sscanf (id, "%d", &val) == 1)
+    if (nbdkit_parse_int ("parseuser", id, &val) == 0)
       return val;
 
     fprintf (stderr, "%s: -u option: %s is not a valid user name or uid",
@@ -125,7 +125,7 @@ parsegroup (const char *id)
 
     saved_errno = errno;
 
-    if (sscanf (id, "%d", &val) == 1)
+    if (nbdkit_parse_int ("parsegroup", id, &val) == 0)
       return val;
 
     fprintf (stderr, "%s: -g option: %s is not a valid group name or gid",
-- 
2.23.0


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]