[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: 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: "Richard W.M. Jones" <rjones(a)redhat.com&gt; 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