Richard W.M. Jones
2019-Sep-21 11:56 UTC
[Libguestfs] [PATCH nbdkit] server: public: Add nbdkit_parse_* functions for safely parsing integers.
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.
---
docs/nbdkit-plugin.pod | 48 ++++++
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 | 10 +-
include/nbdkit-common.h | 22 +++
plugins/curl/curl.c | 4 +-
plugins/nbd/nbd-standalone.c | 9 +-
plugins/nbd/nbd.c | 9 +-
plugins/partitioning/partitioning.c | 4 +-
plugins/random/random.c | 4 +-
plugins/ssh/ssh.c | 6 +-
plugins/vddk/vddk.c | 8 +-
server/internal.h | 2 +-
server/main.c | 21 +--
server/nbdkit.syms | 12 ++
server/public.c | 218 ++++++++++++++++++++++++++++
server/socket-activation.c | 10 +-
server/test-public.c | 181 ++++++++++++++++++++++-
server/usergroup.c | 4 +-
22 files changed, 527 insertions(+), 87 deletions(-)
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 2a84444..2e1913f 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -1013,6 +1013,54 @@ 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_long (const char *what, const char *str, long *r);
+ int nbdkit_parse_unsigned_long (const char *what,
+ const char *str, unsigned long *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);
+ int nbdkit_parse_ssize_t (const char *what,
+ const char *str, ssize_t *r);
+ int nbdkit_parse_size_t (const char *what,
+ const char *str, size_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>.
+
+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..3a030c6 100644
--- a/filters/xz/xz.c
+++ b/filters/xz/xz.c
@@ -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_size_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..274da8c 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -84,6 +84,28 @@ 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_long (const char *what, const char *str,
+ long *r);
+extern int nbdkit_parse_unsigned_long (const char *what, const char *str,
+ unsigned long *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_parse_ssize_t (const char *what, const char *str, ssize_t
*r);
+extern int nbdkit_parse_size_t (const char *what, const char *str, size_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..be087ca 100644
--- a/plugins/curl/curl.c
+++ b/plugins/curl/curl.c
@@ -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_long ("timeout", value, &timeout) == -1)
+ return -1;
+ if (timeout < 0) {
nbdkit_error ("'timeout' must be 0 or a positive timeout in
seconds");
return -1;
}
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..05f9ad3 100644
--- a/plugins/partitioning/partitioning.c
+++ b/plugins/partitioning/partitioning.c
@@ -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_int ("mbr-id", value, &mbr_id) == -1)
return -1;
- }
}
else if (strcmp (key, "type-guid") == 0) {
if (strcasecmp (value, "default") == 0)
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..6fc6ed0 100644
--- a/plugins/ssh/ssh.c
+++ b/plugins/ssh/ssh.c
@@ -59,7 +59,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 unsigned long timeout = 0;
/* config can be:
* NULL => parse options from default file
@@ -151,10 +151,8 @@ 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_unsigned_long ("timeout", value, &timeout)
== -1)
return -1;
- }
}
else {
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 8ea05b1..2e56b4a 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -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_int ("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_int ("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..3bf62e8 100644
--- a/server/nbdkit.syms
+++ b/server/nbdkit.syms
@@ -49,7 +49,19 @@
nbdkit_get_extent;
nbdkit_nanosleep;
nbdkit_parse_bool;
+ nbdkit_parse_int16_t;
+ nbdkit_parse_int32_t;
+ nbdkit_parse_int64_t;
+ nbdkit_parse_int;
+ nbdkit_parse_long;
nbdkit_parse_size;
+ nbdkit_parse_size_t;
+ nbdkit_parse_ssize_t;
+ nbdkit_parse_uint16_t;
+ nbdkit_parse_uint32_t;
+ nbdkit_parse_uint64_t;
+ nbdkit_parse_unsigned;
+ nbdkit_parse_unsigned_long;
nbdkit_peer_name;
nbdkit_read_password;
nbdkit_realpath;
diff --git a/server/public.c b/server/public.c
index d0d9ff4..fc097d2 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,223 @@ 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_long (const char *what, const char *str, long *rp)
+{
+ long r;
+ char *end;
+
+ errno = 0;
+ r = strtol (str, &end, 0);
+ 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;
+}
+
+int
+nbdkit_parse_ssize_t (const char *what, const char *str, ssize_t *rp)
+{
+ long long r;
+ char *end;
+
+ errno = 0;
+ r = strtoll (str, &end, 0);
+#if SSIZE_MAX != LONGLONG_MAX
+#ifndef SSIZE_MIN
+#define SSIZE_MIN (-SSIZE_MAX-1)
+#endif
+ if (r < SSIZE_MIN || r > SSIZE_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 deny this.
+ */
+#define PARSE_ERROR_IF_NEGATIVE \
+ do { \
+ const char *p = str; \
+ while (isspace (*p)) \
+ p++; \
+ if (*p == '-') {
\
+ 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_unsigned_long (const char *what, const char *str,
+ unsigned long *rp)
+{
+ unsigned long r;
+ char *end;
+
+ PARSE_ERROR_IF_NEGATIVE;
+ errno = 0;
+ r = strtoul (str, &end, 0);
+ 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;
+}
+
+int
+nbdkit_parse_size_t (const char *what, const char *str, size_t *rp)
+{
+ unsigned long long r;
+ char *end;
+
+ PARSE_ERROR_IF_NEGATIVE;
+ errno = 0;
+ r = strtoull (str, &end, 0);
+#if SIZE_MAX != ULONGLONG_MAX
+ if (r > SIZE_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..cd35434 100644
--- a/server/test-public.c
+++ b/server/test-public.c
@@ -33,10 +33,14 @@
#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>
+#include <assert.h>
#include "internal.h"
@@ -153,6 +157,180 @@ test_nbdkit_parse_size (void)
return pass;
}
+static bool
+test_nbdkit_parse_ints (void)
+{
+ bool pass = true;
+ char s[64];
+
+#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
+
+ /* Test the basic parsing of decimals, hexadecimal, octal and
+ * negative numbers.
+ */
+ PARSE (int, "%d", "0", OK, 0);
+ 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", "01", OK, 1);
+ PARSE (int, "%d", "07", OK, 7);
+ PARSE (int, "%d", "010", OK, 8);
+ 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", "-99", OK, -99);
+ 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, 0);
+ PARSE (int, "%d", "-", BAD, 0);
+ PARSE (int, "%d", "+", BAD, 0);
+ PARSE (int, "%d", "++", BAD, 0);
+ PARSE (int, "%d", "++0", BAD, 0);
+ PARSE (int, "%d", "--0", BAD, 0);
+ PARSE (int, "%d", "0x", BAD, 0);
+ PARSE (int, "%d", "42x", BAD, 0);
+ PARSE (int, "%d", "42-", BAD, 0);
+ PARSE (int, "%d", "garbage", BAD, 0);
+ PARSE (int, "%d", "2147483648", BAD, 0); /* INT_MAX + 1
*/
+ PARSE (int, "%d", "-2147483649", BAD, 0); /* INT_MIN - 1
*/
+ PARSE (int, "%d", "999999999999999999999999", BAD, 0);
+ PARSE (int, "%d", "-999999999999999999999999", BAD, 0);
+
+ /* Test nbdkit_parse_unsigned. */
+ 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, 0);
+ PARSE (unsigned, "%u", "-99", BAD, 0);
+ PARSE (unsigned, "%u", "-0xf", BAD, 0);
+ PARSE (unsigned, "%u", "-010", BAD, 0);
+ PARSE (unsigned, "%u", "2147483647", OK, 2147483647); /*
INT_MAX */
+ PARSE (unsigned, "%u", "-2147483648", BAD, 0); /* INT_MIN
*/
+ PARSE (unsigned, "%u", "0x7fffffff", OK, 0x7fffffff);
+ PARSE (unsigned, "%u", "-0x80000000", BAD, 0);
+
+ /* Test nbdkit_parse_long. */
+ PARSE (long, "%ld", "0", OK, 0);
+ assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) LONG_MAX) !=
-1);
+ PARSE (long, "%ld", s, OK, LONG_MAX);
+ assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) LONG_MIN) !=
-1);
+ PARSE (long, "%ld", s, OK, LONG_MIN);
+ PARSE (long, "%ld", "999999999999999999999999", BAD, 0);
+ PARSE (long, "%ld", "-999999999999999999999999", BAD, 0);
+
+ /* Test nbdkit_parse_unsigned_long. */
+ typedef unsigned long unsigned_long;
+ PARSE (unsigned_long, "%lu", "0", OK, 0);
+ assert (snprintf (s, sizeof s, "%" PRIu64, (uint64_t) ULONG_MAX) !=
-1);
+ PARSE (unsigned_long, "%lu", s, OK, ULONG_MAX);
+ PARSE (unsigned_long, "%lu", "999999999999999999999999",
BAD, 0);
+ PARSE (unsigned_long, "%lu", "-1", BAD, 0);
+
+ /* 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, 0);
+ PARSE (int16_t, "%" PRIi16, "-0x8001", BAD, 0);
+
+ /* 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, 0);
+ PARSE (uint16_t, "%" PRIu16, "-1", BAD, 0);
+
+ /* 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, 0);
+ PARSE (int32_t, "%" PRIi32, "-0x80000001", BAD, 0);
+
+ /* 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, 0);
+ PARSE (uint32_t, "%" PRIu32, "-1", BAD, 0);
+
+ /* 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,
0);
+ PARSE (int64_t, "%" PRIi64, "-0x8000000000000001", BAD,
0);
+
+ /* 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,
0);
+ PARSE (uint64_t, "%" PRIu64, "-1", BAD,
0);
+
+ /* Test nbdkit_parse_ssize_t. */
+ PARSE (ssize_t, "%zd", "0", OK, 0);
+ assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) SSIZE_MAX) !=
-1);
+ PARSE (ssize_t, "%zd", s, OK, SSIZE_MAX);
+#ifndef SSIZE_MIN
+#define SSIZE_MIN (-SSIZE_MAX-1)
+#endif
+ assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) SSIZE_MIN) !=
-1);
+ PARSE (ssize_t, "%zd", s, OK, SSIZE_MIN);
+ PARSE (ssize_t, "%zd", "999999999999999999999999", BAD,
0);
+ PARSE (ssize_t, "%zd", "-999999999999999999999999", BAD,
0);
+
+ /* Test nbdkit_parse_size_t. */
+ PARSE (size_t, "%zu", "0", OK, 0);
+ assert (snprintf (s, sizeof s, "%" PRIu64, (uint64_t) SIZE_MAX) !=
-1);
+ PARSE (size_t, "%zu", s, OK, SIZE_MAX);
+ PARSE (size_t, "%zu", "999999999999999999999999", BAD,
0);
+ PARSE (size_t, "%zu", "-1", BAD, 0);
+
+#undef PARSE
+#undef OK
+#undef BAD
+ return pass;
+}
+
static bool
test_nbdkit_read_password (void)
{
@@ -228,6 +406,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
Eric Blake
2019-Sep-23 17:05 UTC
Re: [Libguestfs] [PATCH nbdkit] server: public: Add nbdkit_parse_* functions for safely parsing integers.
On 9/21/19 6:56 AM, Richard W.M. Jones wrote:> 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.I note you are not permitting specification of a base (that is, hard-coding base=0 in the underlying strto*l); that appears to be fine for all users thus converted.> =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_long (const char *what, const char *str, long *r); > + int nbdkit_parse_unsigned_long (const char *what, > + const char *str, unsigned long *r);Do we really want to encourage the use of parse_long and parse_unsigned_long? Those differ between 32- and 64-bit platforms, at which point, it's often better to explicitly call out the intended size. It would also make a bit more sense that you don't offer parse_long_long, if the explicit-sized versions are good enough when 64-bit values are needed. It's also a bit odd to not offer a parse_long_long for strtoll, altagain arguing that the explicit parse_int64_t is probably better to encourage.> + 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);I guess we can add [u]int8_t variants later if a need arises? Or should we do it now?> + 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); > + int nbdkit_parse_ssize_t (const char *what, > + const char *str, ssize_t *r); > + int nbdkit_parse_size_t (const char *what, > + const char *str, size_t *r);[s]size_t is another one that can differ between 32- and 64-bit platforms; but I'm fine with having these function, especially since there are still efforts being considered about an alternative kernel ABI with 64-bit integers but 32-bit size_t.> + > +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>. > + > +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" > +Do we want to make it part of our documented contract that *r is unchanged if we return -1?> +++ 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;So you're fixing typing errors at the same time as using the new helper functions. Makes the patch a bit bigger to review, but it's not worth the effort to split it now.> 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)Semantic change; previously "010" was '10' and "08" was '8', now "010" is '8' and "08" is a failure (because we used "%d" instead of "%i"). (Multiple spots in this patch). Not a big deal to me, but we may want to call it out in the commit message as intentional.> +++ b/include/nbdkit-common.h > @@ -84,6 +84,28 @@ 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);Should we mark 'what' and 'str' as being non-null parameters? But you definitely document 'r' as valid when NULL.> +++ b/plugins/curl/curl.c > @@ -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_long ("timeout", value, &timeout) == -1) > + return -1; > + if (timeout < 0) { > nbdkit_error ("'timeout' must be 0 or a positive timeout in seconds"); > return -1;Why not change to 'unsigned timeout'? 'long' is rather oversized on 64-bit platforms, and keeping things signed doesn't buy much compared to just going unsigned. (Yep, that's me trying to get rid of a need for parse_long).> +++ b/plugins/partitioning/partitioning.c > @@ -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_int ("mbr-id", value, &mbr_id) == -1) > return -1;Is 'int' the best type for this?> +++ b/plugins/ssh/ssh.c > @@ -59,7 +59,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 unsigned long timeout = 0;Again, a 32-bit timeout regardless of 32- or 64-bit platform is probably saner.> +++ b/plugins/vddk/vddk.c > @@ -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_int ("nfchostport", value, &nfc_host_port) == -1) > return -1; > - }Is 'int' the right type here?> +++ 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,223 @@ 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 > +A bit more painful for gdb debugging to have this much in a macro, but not too bad given our unit tests exercise it. Leaves *rp untouched if we return -1; I don't mind that, but think we should document it as intentional.> +/* 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; > +}Looks correct.> + > +int > +nbdkit_parse_long (const char *what, const char *str, long *rp) > +{ > + long r; > + char *end; > + > + errno = 0; > + r = strtol (str, &end, 0); > + PARSE_COMMON_TAIL; > +}I argue we don't want this one.> + > +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; > +}No int8_t counterpart? But looks correct.> + > +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; > +}Probably a dead #if, but doesn't hurt to leave it in.> + > +int > +nbdkit_parse_ssize_t (const char *what, const char *str, ssize_t *rp) > +{ > + long long r; > + char *end; > + > + errno = 0; > + r = strtoll (str, &end, 0); > +#if SSIZE_MAX != LONGLONG_MAX > +#ifndef SSIZE_MIN > +#define SSIZE_MIN (-SSIZE_MAX-1)Assumes twos-complement, but that's sane enough for the platforms we target.> +#endif > + if (r < SSIZE_MIN || r > SSIZE_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 deny this. > + */ > +#define PARSE_ERROR_IF_NEGATIVE \ > + do { \ > + const char *p = str; \ > + while (isspace (*p)) \ > + p++; \ > + if (*p == '-') { \ > + nbdkit_error ("%s: negative numbers are not allowed", what); \ > + return -1; \ > + } \ > + } while (0) > +Agree. However, why not modify str in-place, so that the subsequent strtoul() does not have to repeat our prefix scan?> +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; > +}Much easier to write for both 32- and 64-bit platforms when you pre-filter out the surprising '-' behavior. Looks correct.> + > +int > +nbdkit_parse_unsigned_long (const char *what, const char *str, > + unsigned long *rp) > +{ > + unsigned long r; > + char *end; > + > + PARSE_ERROR_IF_NEGATIVE; > + errno = 0; > + r = strtoul (str, &end, 0); > + PARSE_COMMON_TAIL; > +}Again, I argue we don't need this one.> + > +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; > +#endifAnother probably dead #if, but I'm fine with leaving it.> + PARSE_COMMON_TAIL; > +} > + > +int > +nbdkit_parse_size_t (const char *what, const char *str, size_t *rp) > +{ > + unsigned long long r; > + char *end; > + > + PARSE_ERROR_IF_NEGATIVE; > + errno = 0; > + r = strtoull (str, &end, 0); > +#if SIZE_MAX != ULONGLONG_MAX > + if (r > SIZE_MAX) > + errno = ERANGE; > +#endif > + PARSE_COMMON_TAIL; > +} > + > /* Parse a string as a size with possible scaling suffix, or return -1 > * after reporting the error. > */Hmm, seeing this comment, do we really need raw [s]size_t parsing, or can we get away with scaled parsing anywhere that sizes are intended? But the code looks correct if you want to keep it.> 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; > - }Possible semantic change: on parse failure, are we still printing something to stderr, or is nbdkit_error() not effective this early?> 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; > - }and again> +++ b/server/test-public.c> +static bool > +test_nbdkit_parse_ints (void) > +{ > + bool pass = true; > + char s[64]; > + > +#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 -1Nice.> + > + /* Test the basic parsing of decimals, hexadecimal, octal and > + * negative numbers. > + */ > + PARSE (int, "%d", "0", OK, 0); > + 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", "01", OK, 1); > + PARSE (int, "%d", "07", OK, 7); > + PARSE (int, "%d", "010", OK, 8); > + 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", "-99", OK, -99); > + PARSE (int, "%d", "-0xf", OK, -15);I'd test upper case at least once, such as -0XF here. Testing leading whitespace would also be nice.> + 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);Matches our earlier assumptions of twos-complement targets.> + > + /* Test basic error handling. */ > + PARSE (int, "%d", "", BAD, 0); > + PARSE (int, "%d", "-", BAD, 0);I might also test "- 0" as BAD.> + PARSE (int, "%d", "+", BAD, 0); > + PARSE (int, "%d", "++", BAD, 0); > + PARSE (int, "%d", "++0", BAD, 0); > + PARSE (int, "%d", "--0", BAD, 0); > + PARSE (int, "%d", "0x", BAD, 0);I'd also test "08" as BAD.> + PARSE (int, "%d", "42x", BAD, 0); > + PARSE (int, "%d", "42-", BAD, 0); > + PARSE (int, "%d", "garbage", BAD, 0); > + PARSE (int, "%d", "2147483648", BAD, 0); /* INT_MAX + 1 */ > + PARSE (int, "%d", "-2147483649", BAD, 0); /* INT_MIN - 1 */ > + PARSE (int, "%d", "999999999999999999999999", BAD, 0); > + PARSE (int, "%d", "-999999999999999999999999", BAD, 0);It might also be worth testing "0x1p1" as invalid (and not an accidental hex-float). Other possible non-integer strings to be sure we don't accidentally parse include "inf", "nan", "0.0".> + > + /* Test nbdkit_parse_unsigned. */ > + 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, 0); > + PARSE (unsigned, "%u", "-99", BAD, 0); > + PARSE (unsigned, "%u", "-0xf", BAD, 0); > + PARSE (unsigned, "%u", "-010", BAD, 0);I'd also test at least " 0" and " -0" (or similar), to prove we handled leading spaces correctly.> + PARSE (unsigned, "%u", "2147483647", OK, 2147483647); /* INT_MAX */ > + PARSE (unsigned, "%u", "-2147483648", BAD, 0); /* INT_MIN */ > + PARSE (unsigned, "%u", "0x7fffffff", OK, 0x7fffffff); > + PARSE (unsigned, "%u", "-0x80000000", BAD, 0); > + > + /* Test nbdkit_parse_long. */ > + PARSE (long, "%ld", "0", OK, 0); > + assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) LONG_MAX) != -1); > + PARSE (long, "%ld", s, OK, LONG_MAX); > + assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) LONG_MIN) != -1); > + PARSE (long, "%ld", s, OK, LONG_MIN); > + PARSE (long, "%ld", "999999999999999999999999", BAD, 0); > + PARSE (long, "%ld", "-999999999999999999999999", BAD, 0);snprintf() side effects inside assert(). Tsk tsk - the test will now fail under -DNDEBUG. And that's part of why I don't think we want to expose parse_long.> + > + /* Test nbdkit_parse_ssize_t. */ > + PARSE (ssize_t, "%zd", "0", OK, 0); > + assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) SSIZE_MAX) != -1);Again, bad side effects. But I'm less sure whether to avoid exposing parse_ssize_t.> + PARSE (ssize_t, "%zd", s, OK, SSIZE_MAX); > +#ifndef SSIZE_MIN > +#define SSIZE_MIN (-SSIZE_MAX-1) > +#endif > + assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) SSIZE_MIN) != -1); > + PARSE (ssize_t, "%zd", s, OK, SSIZE_MIN); > + PARSE (ssize_t, "%zd", "999999999999999999999999", BAD, 0); > + PARSE (ssize_t, "%zd", "-999999999999999999999999", BAD, 0); > + > + /* Test nbdkit_parse_size_t. */ > + PARSE (size_t, "%zu", "0", OK, 0); > + assert (snprintf (s, sizeof s, "%" PRIu64, (uint64_t) SIZE_MAX) != -1); > + PARSE (size_t, "%zu", s, OK, SIZE_MAX); > + PARSE (size_t, "%zu", "999999999999999999999999", BAD, 0); > + PARSE (size_t, "%zu", "-1", BAD, 0); > + > +#undef PARSE > +#undef OK > +#undef BAD > + return pass; > +}A few tweaks still needed, but overall looking good.> +++ 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)Is 'int' still the right type for this?> 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", >-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2019-Sep-23 17:52 UTC
Re: [Libguestfs] [PATCH nbdkit] server: public: Add nbdkit_parse_* functions for safely parsing integers.
On Mon, Sep 23, 2019 at 12:05:11PM -0500, Eric Blake wrote:> > + int nbdkit_parse_long (const char *what, const char *str, long *r); > > + int nbdkit_parse_unsigned_long (const char *what, > > + const char *str, unsigned long *r); > > Do we really want to encourage the use of parse_long and > parse_unsigned_long? Those differ between 32- and 64-bit platforms, at > which point, it's often better to explicitly call out the intended size.So I'll say why I added these. It may not be a good reason, but here goes :-) There are two external APIs we use which need long types, libcurl (curl_easy_setopt) and libssh (ssh_options_set). The existing parameters had type long (I don't think unsigned long is actually used) which we passed directly to one of these APIs. To use another type would involve a slightly awkward check + cast, with the danger that the code would break on the rarely tested 32 bit platform. ...> > + 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); > > I guess we can add [u]int8_t variants later if a need arises? Or should > we do it now?We certainly could. I couldn't see an immediate reason, except maybe for the ’mbr-id’ parameter.> > + 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); > > + int nbdkit_parse_ssize_t (const char *what, > > + const char *str, ssize_t *r); > > + int nbdkit_parse_size_t (const char *what, > > + const char *str, size_t *r); > > [s]size_t is another one that can differ between 32- and 64-bit > platforms; but I'm fine with having these function, especially since > there are still efforts being considered about an alternative kernel ABI > with 64-bit integers but 32-bit size_t.Yes I think if we had to drop long or size_t, I'd rather keep the size_t ones. Those are actually meaningful -- eg. if used for sizing an internal array.> > +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>. > > + > > +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" > > + > > Do we want to make it part of our documented contract that *r is > unchanged if we return -1?Yes I think we can document this. It's useful to have that behaviour defined because it makes error recovery simpler. I will add it to my copy.> > @@ -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) > > Semantic change; previously "010" was '10' and "08" was '8', now "010" > is '8' and "08" is a failure (because we used "%d" instead of "%i"). > (Multiple spots in this patch). Not a big deal to me, but we may want > to call it out in the commit message as intentional.I guess %i (not %d) is the one which parses octal and hex. I think this change is for the better however since it improves consistency, so I'll mention it in the commit message.> > +++ b/include/nbdkit-common.h > > @@ -84,6 +84,28 @@ 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); > > Should we mark 'what' and 'str' as being non-null parameters? But you > definitely document 'r' as valid when NULL.I think we decided to avoid using __attribute__((nonnull)) on the external API, because it was ... unsafe? I don't recall now if we avoided it deliberately or not, but we definitely don't use it except on internal functions! Yes it would make sense as long as it's not unsafe.> > +++ b/plugins/curl/curl.c > > @@ -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_long ("timeout", value, &timeout) == -1) > > + return -1; > > + if (timeout < 0) { > > nbdkit_error ("'timeout' must be 0 or a positive timeout in seconds"); > > return -1; > > Why not change to 'unsigned timeout'? 'long' is rather oversized on > 64-bit platforms, and keeping things signed doesn't buy much compared to > just going unsigned. (Yep, that's me trying to get rid of a need for > parse_long).See above about curl_easy_setopt.> > +++ b/plugins/partitioning/partitioning.c > > @@ -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_int ("mbr-id", value, &mbr_id) == -1) > > return -1; > > Is 'int' the best type for this?Probably not, see above.> > +++ b/plugins/ssh/ssh.c > > @@ -59,7 +59,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 unsigned long timeout = 0; > > Again, a 32-bit timeout regardless of 32- or 64-bit platform is probably > saner.See above about ssh_options_set.> > +++ b/plugins/vddk/vddk.c > > @@ -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_int ("nfchostport", value, &nfc_host_port) == -1) > > return -1; > > - } > > Is 'int' the right type here?Actually this is a bug twice over. The VDDK struct wants uint32_t for both of the port fields. Obviously since they are port numbers something like uint16_t would be more suitable. (0 has the special meaning of "default port"). I think I'll change this to uint16_t because the implicit cast up to uint32_t when we assign it to the VDDK struct shouldn't be a problem, and it's useful to have bounds checking.> > +/* 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; > > +} > > Looks correct.I actually compiled this and ran the tests with -m32 and -m64. It took several rounds to get it right.> > +#endif > > + if (r < SSIZE_MIN || r > SSIZE_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 deny this. > > + */ > > +#define PARSE_ERROR_IF_NEGATIVE \ > > + do { \ > > + const char *p = str; \ > > + while (isspace (*p)) \ > > + p++; \ > > + if (*p == '-') { \ > > + nbdkit_error ("%s: negative numbers are not allowed", what); \ > > + return -1; \ > > + } \ > > + } while (0) > > + > > Agree. However, why not modify str in-place, so that the subsequent > strtoul() does not have to repeat our prefix scan?Yes, that's a good idea, I didn't think it through.> > + PARSE_COMMON_TAIL; > > +} > > + > > +int > > +nbdkit_parse_size_t (const char *what, const char *str, size_t *rp) > > +{ > > + unsigned long long r; > > + char *end; > > + > > + PARSE_ERROR_IF_NEGATIVE; > > + errno = 0; > > + r = strtoull (str, &end, 0); > > +#if SIZE_MAX != ULONGLONG_MAX > > + if (r > SIZE_MAX) > > + errno = ERANGE; > > +#endif > > + PARSE_COMMON_TAIL; > > +} > > + > > /* Parse a string as a size with possible scaling suffix, or return -1 > > * after reporting the error. > > */ > > Hmm, seeing this comment, do we really need raw [s]size_t parsing, or > can we get away with scaled parsing anywhere that sizes are intended? > But the code looks correct if you want to keep it.nbdkit_parse_ssize_t is not used. nbdkit_parse_size_t is used for a single case, ‘xz-max-depth’ which is genuinely an array length, so size_t is (albeit marginally) useful here. It's not worth dying on this particular hill, but I think there is some use for all of these functions.> > 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; > > - } > > Possible semantic change: on parse failure, are we still printing > something to stderr, or is nbdkit_error() not effective this early?Yes nbdkit_error works right from the start. We have already used it for parsing server argv[].> > +++ b/server/test-public.c > > > +static bool > > +test_nbdkit_parse_ints (void) > > +{ > > + bool pass = true; > > + char s[64]; > > + > > +#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 > > Nice.But I discovered you cannot do: #define BAD -1, 0 which is a shame :-(> > + > > + /* Test the basic parsing of decimals, hexadecimal, octal and > > + * negative numbers. > > + */ > > + PARSE (int, "%d", "0", OK, 0); > > + 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", "01", OK, 1); > > + PARSE (int, "%d", "07", OK, 7); > > + PARSE (int, "%d", "010", OK, 8); > > + 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", "-99", OK, -99); > > + PARSE (int, "%d", "-0xf", OK, -15); > > I'd test upper case at least once, such as -0XF here. Testing leading > whitespace would also be nice.Ah yes, good idea.> > + 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); > > Matches our earlier assumptions of twos-complement targets. > > > + > > + /* Test basic error handling. */ > > + PARSE (int, "%d", "", BAD, 0); > > + PARSE (int, "%d", "-", BAD, 0); > > I might also test "- 0" as BAD.OK.> > + PARSE (int, "%d", "+", BAD, 0); > > + PARSE (int, "%d", "++", BAD, 0); > > + PARSE (int, "%d", "++0", BAD, 0); > > + PARSE (int, "%d", "--0", BAD, 0); > > + PARSE (int, "%d", "0x", BAD, 0); > > I'd also test "08" as BAD.OK.> > + PARSE (int, "%d", "42x", BAD, 0); > > + PARSE (int, "%d", "42-", BAD, 0); > > + PARSE (int, "%d", "garbage", BAD, 0); > > + PARSE (int, "%d", "2147483648", BAD, 0); /* INT_MAX + 1 */ > > + PARSE (int, "%d", "-2147483649", BAD, 0); /* INT_MIN - 1 */ > > + PARSE (int, "%d", "999999999999999999999999", BAD, 0); > > + PARSE (int, "%d", "-999999999999999999999999", BAD, 0); > > It might also be worth testing "0x1p1" as invalid (and not an accidental > hex-float). Other possible non-integer strings to be sure we don't > accidentally parse include "inf", "nan", "0.0".OK.> > + > > + /* Test nbdkit_parse_unsigned. */ > > + 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, 0); > > + PARSE (unsigned, "%u", "-99", BAD, 0); > > + PARSE (unsigned, "%u", "-0xf", BAD, 0); > > + PARSE (unsigned, "%u", "-010", BAD, 0); > > I'd also test at least " 0" and " -0" (or similar), to prove we handled > leading spaces correctly.OK.> > + PARSE (unsigned, "%u", "2147483647", OK, 2147483647); /* INT_MAX */ > > + PARSE (unsigned, "%u", "-2147483648", BAD, 0); /* INT_MIN */ > > + PARSE (unsigned, "%u", "0x7fffffff", OK, 0x7fffffff); > > + PARSE (unsigned, "%u", "-0x80000000", BAD, 0); > > + > > + /* Test nbdkit_parse_long. */ > > + PARSE (long, "%ld", "0", OK, 0); > > + assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) LONG_MAX) != -1); > > + PARSE (long, "%ld", s, OK, LONG_MAX); > > + assert (snprintf (s, sizeof s, "%" PRIi64, (int64_t) LONG_MIN) != -1); > > + PARSE (long, "%ld", s, OK, LONG_MIN); > > + PARSE (long, "%ld", "999999999999999999999999", BAD, 0); > > + PARSE (long, "%ld", "-999999999999999999999999", BAD, 0); > > snprintf() side effects inside assert(). Tsk tsk - the test will now > fail under -DNDEBUG. And that's part of why I don't think we want to > expose parse_long.We should probably loudly break or skip the tests if they are compiled with -DNDEBUG. We're using assert for testing in a couple of places.> > +++ 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) > > Is 'int' still the right type for this?Hopefully it will warn us if uid_t stops being int. (Also we're assuming pid_t == int). Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Possibly Parallel Threads
- [PATCH nbdkit] server: public: Add nbdkit_parse_* functions for safely parsing integers.
- [PATCH nbdkit v2] server: public: Add nbdkit_parse_* functions for safely parsing integers.
- Re: [PATCH nbdkit] server: public: Add nbdkit_parse_* functions for safely parsing integers.
- [PATCH nbdkit 5/9 patch split 2/5] lib: Move code for parsing, passwords and paths into libnbdkit.so.
- Re: [PATCH nbdkit] server: public: Add nbdkit_parse_* functions for safely parsing integers.