[Libguestfs] [PATCH nbdkit v2] server: public: Add nbdkit_parse_* functions for safely parsing integers.
Eric Blake
eblake at redhat.com
Mon Sep 23 21:21:01 UTC 2019
On 9/23/19 3:06 PM, Richard W.M. Jones wrote:
> 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 :-)
True.
>
> 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.
>
Looks reasonable.
> 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).
Thanks for that update. I don't know if it will ever bite someone in
practice, but it's always nicer to be able to point a bug report back to
a commit message stating that the change was intentional if it does come up.
> +++ 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;
> @@ -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);
I might have left a comment here (the cast is necessary because
curl_easy_setopt() is varargs), so no one accidentally deletes the cast
thinking it is pointless.
> +++ b/plugins/ssh/ssh.c
> @@ -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");
C17 5.2.4.2.1 requires 'long' to be at least 32 bits. Ergo, (uint32_t)
timeout > LONG_MAX is always false. You could assert() rather than
trying to use nbdkit_error().
> 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);
This conversion is correct.
> +++ 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 { \
Alignment is off.
> + error_flagged = false; \
> + TYPE i = 0; \
Ooh, we could also test that *r is unchanged on failure. Here, assign
TYPE i = 123; (or something unlikely and unused elsewhere, remembering
that we have to fit in [u]int8_t)...
> + int r = nbdkit_parse_##TYPE ("test", TEST, &i); \
> + if (r != RET || i != EXPECTED) { \
then here, if (r != RET || i != (r ? 123 : EXPECTED)) {
We didn't properly parenthesize uses of (RET) or (EXPECTED), but it's
also easy to see that all uses are local to this function and don't need
extra grouping, plus the fact that this macro is not all expressions,
but also includes type names, so it's already atypical from the usual
patterns of needing to protect arguments.
> + 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
> +
At any rate, I think you're good to go.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190923/c85f75c3/attachment.sig>
More information about the Libguestfs
mailing list