Libguestfs - Sep 2019 - [PATCH nbdkit v2] 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

On 9/23/19 12:52 PM, Richard W.M. Jones wrote:
> 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)
Eww - that one takes a vararg, but integers are indeed passed as 'long'
and there may be ABIs where passing 'int' causes wrong behavior.

Still, we could parse into an 'int' or 'unsigned', then assign
that to
yet another variable of type 'long', before passing the long to cur..
> and libssh (ssh_options_set).
That one takes a void* which will be interpreted as 'long*', so again,
we have to have the correct type in place.  But again, we could parse
into 'int' or 'unsigned', then copy to a 'long' prior to
calling the API.
>  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.
(Potentially) widening casts aren't a problem, although I do share your
concern that 32-bit platforms are not tested as frequently.

>>> +++ 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.
Yeah, now I remember.

The mere act of marking something __attribute__((nonnull)) can make
(older?) gcc miscompile code into completely eliminating NULL checks.
But since the attribute in the public header is advisory only (and not
compulsory), we probably WANT explicit NULL checks in our function
bodies (unless we think SEGV'ing a poorly-written plugin is reasonable).
 It may turn into something where when compiling the REST of the code
base, we want the attributes, but when compiling server/public.c, we
intentionally avoid the attributes.  Separate patch, if at all.

>>> +
>>>  /* 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 also an array length where a misconfigured input can cause the
filter to be non-functional; see commit fd2deeb1 where I abused it to
intentionally cause calloc() failure to prove what happens when a filter
.open() fails.  Do we really need that much flexibility, or can we get
away with stating that the max depth has to fit within 32 bits, even if
it does populate an array length later?  (Here, I'm not as sure of the
answer, as I didn't actually research where it is used, only that I
could overflow the calloc).
> 
> It's not worth dying on this particular hill, but I think there is
> some use for all of these functions.
True, and so I'm not going to object if we expose it after all; it's
just harder to remove something we don't like than to add something we
found to be missing.

>>> +#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 :-(
Easy fix:

#define PARSE(...) PARSE(__VA_ARGS__)
#define PARSE_(TYPE, FORMAT, TEST, RET, EXPECTED)

now you can #define BAD -1, 0, and the expansion of PARSE() causes the
expansion of BAD prior to the expansion of PARSE_().

>>> +++ 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).
64-bit mingw has had a long-standing bug that pid_t was declared as
64-bit, even though getpid() returns a 32-bit 'int'; I wish they'd
fix
their headers, but it violates the assumption of pid_t == int.

Also, POSIX is considering standardizing fcntl(F_SETOWN_EX) in order to
allow pid_t > int: http://austingroupbugs.net/view.php?id=1274

So while your assumption may hold for now on Linux and BSD, I wouldn't
hold my breath on it lasting forever.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

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

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

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

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

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

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

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

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