Eric Blake
2022-Sep-26 22:05 UTC
[Libguestfs] [libnbd PATCH v3 01/18] api: Fix crashes on nbd_connect_command with bad argv
nbd_connect_command (h, (char **) { NULL }) triggers SIGABRT when
preparing to exec a NULL command name (during
enter_STATE_CONNECT_COMMAND_START in v1.0).
nbd_connect_command (h, NULL) in newer releases triggers SIGSEGV by
trying to dereference NULL (with LIBNBD_DEBUG=1, during
nbd_internal_printable_string_list in v1.4; otherwise, during
nbd_internal_set_argv in v1.6). In older releases, it behaves the
same as (char **) { NULL } and triggers SIGABRT.
Both crashes are corner cases that have existed for a long time, and
unlikely to hit in real code. Still, we shouldn't crash in a library.
Forbid a NULL StringList in general (similar to how we already forbid
a NULL String); which also means a StringList parameter implies
may_set_error=true. Refactor nbd_internal_set_argv() to split out the
vector population into a new helper function that can only fail with
ENOMEM (this function will also be useful in later patches that want
to copy a StringList, but can tolerate 0 elements), as well as to set
errors itself (all 2 callers updated) since detecting NULL for argv[0]
is unique to argv.
Tests are added in the next patch, to make it easier to review by
temporarily swapping patch order.
Changes from:
$ nbdsh -c 'h.connect_command([])'
nbdsh: generator/states-connect.c:247: enter_STATE_CONNECT_COMMAND_START:
Assertion `h->argv.ptr[0]' failed.
Aborted (core dumped)
to:
nbdsh: command line script failed: nbd_connect_command: missing command name in
argv list: Invalid argument
Fixes: 0b7bdf62 ("generator: Improve trace messages.", v1.3.2)
Fixes: 6f3eee2e ("lib: Replace a few uses of realloc with nbdkit vector
library.", v1.5.5)
Fixes: 8b164376 ("api: Get rid of nbd_connection.", v0.1)
---
lib/internal.h | 3 ++-
generator/API.ml | 3 ++-
generator/C.ml | 2 +-
lib/connect.c | 10 +++-------
lib/utils.c | 45 ++++++++++++++++++++++++++++++++++-----------
5 files changed, 42 insertions(+), 21 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index e4c08ca3..04bd8134 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -476,7 +476,8 @@ extern int nbd_internal_aio_get_direction (enum state
state);
/* utils.c */
extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp);
-extern int nbd_internal_set_argv (string_vector *v, char **argv);
+extern int nbd_internal_copy_string_list (string_vector *v, char **in);
+extern int nbd_internal_set_argv (struct nbd_handle *h, char **argv);
extern const char *nbd_internal_fork_safe_itoa (long v, char *buf, size_t len);
extern void nbd_internal_fork_safe_perror (const char *s);
extern char *nbd_internal_printable_buffer (const void *buf, size_t count);
diff --git a/generator/API.ml b/generator/API.ml
index abf77972..7be870a4 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -3458,7 +3458,8 @@ let () | name, { args; may_set_error = false }
when List.exists
(function
- | Closure _ | Enum _ | Flags _ | String _ -> true
+ | Closure _ | Enum _ | Flags _
+ | String _ | StringList _ -> true
| _ -> false) args ->
failwithf "%s: if args contains Closure/Enum/Flags/String
parameter, may_set_error must be false" name
diff --git a/generator/C.ml b/generator/C.ml
index b2d46f98..73ecffa0 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -570,7 +570,7 @@ let
need_out_label := true
| Flags (n, flags) ->
print_flags_check n flags None
- | String n ->
+ | String n | StringList n ->
let value = match errcode with
| Some value -> value
| None -> assert false in
diff --git a/lib/connect.c b/lib/connect.c
index 50080630..ffa50d5b 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -251,10 +251,8 @@ nbd_unlocked_aio_connect_socket (struct nbd_handle *h, int
sock)
int
nbd_unlocked_aio_connect_command (struct nbd_handle *h, char **argv)
{
- if (nbd_internal_set_argv (&h->argv, argv) == -1) {
- set_error (errno, "realloc");
+ if (nbd_internal_set_argv (h, argv) == -1)
return -1;
- }
return nbd_internal_run (h, cmd_connect_command);
}
@@ -263,10 +261,8 @@ int
nbd_unlocked_aio_connect_systemd_socket_activation (struct nbd_handle *h,
char **argv)
{
- if (nbd_internal_set_argv (&h->argv, argv) == -1) {
- set_error (errno, "realloc");
+ if (nbd_internal_set_argv (h, argv) == -1)
return -1;
- }
return nbd_internal_run (h, cmd_connect_sa);
}
diff --git a/lib/utils.c b/lib/utils.c
index 3d3b7f45..da3cee72 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -53,16 +53,17 @@ nbd_internal_hexdump (const void *data, size_t len, FILE
*fp)
}
}
-/* Replace the string_vector with a deep copy of argv (including final NULL).
*/
+/* Replace a string_vector with a deep copy of in (including final NULL). */
int
-nbd_internal_set_argv (string_vector *v, char **argv)
+nbd_internal_copy_string_list (string_vector *v, char **in)
{
size_t i;
+ assert (in);
string_vector_reset (v);
- for (i = 0; argv[i] != NULL; ++i) {
- char *copy = strdup (argv[i]);
+ for (i = 0; in[i] != NULL; ++i) {
+ char *copy = strdup (in[i]);
if (copy == NULL)
return -1;
if (string_vector_append (v, copy) == -1) {
@@ -74,6 +75,24 @@ nbd_internal_set_argv (string_vector *v, char **argv)
return string_vector_append (v, NULL);
}
+/* Store argv into h, or diagnose an error on failure. */
+int
+nbd_internal_set_argv (struct nbd_handle *h, char **argv)
+{
+ assert (argv);
+
+ if (argv[0] == NULL) {
+ set_error (EINVAL, "missing command name in argv list");
+ return -1;
+ }
+ if (nbd_internal_copy_string_list (&h->argv, argv) == -1) {
+ set_error (errno, "realloc");
+ return -1;
+ }
+
+ return 0;
+}
+
/* Like sprintf ("%ld", v), but safe to use between fork and exec.
Do
* not use this function in any other context.
*
@@ -247,13 +266,17 @@ nbd_internal_printable_string_list (char **list)
if (fp == NULL)
return NULL;
- fprintf (fp, "[");
- for (i = 0; list[i] != NULL; ++i) {
- if (i > 0)
- fprintf (fp, ", ");
- printable_string (list[i], fp);
+ if (list == NULL)
+ fprintf (fp, "NULL");
+ else {
+ fprintf (fp, "[");
+ for (i = 0; list[i] != NULL; ++i) {
+ if (i > 0)
+ fprintf (fp, ", ");
+ printable_string (list[i], fp);
+ }
+ fprintf (fp, "]");
}
- fprintf (fp, "]");
fclose (fp);
return s;
--
2.37.3
Richard W.M. Jones
2022-Sep-27 11:14 UTC
[Libguestfs] [libnbd PATCH v3 01/18] api: Fix crashes on nbd_connect_command with bad argv
On Mon, Sep 26, 2022 at 05:05:43PM -0500, Eric Blake wrote:> nbd_connect_command (h, (char **) { NULL }) triggers SIGABRT when > preparing to exec a NULL command name (during > enter_STATE_CONNECT_COMMAND_START in v1.0). > > nbd_connect_command (h, NULL) in newer releases triggers SIGSEGV by > trying to dereference NULL (with LIBNBD_DEBUG=1, during > nbd_internal_printable_string_list in v1.4; otherwise, during > nbd_internal_set_argv in v1.6). In older releases, it behaves the > same as (char **) { NULL } and triggers SIGABRT. > > Both crashes are corner cases that have existed for a long time, and > unlikely to hit in real code. Still, we shouldn't crash in a library.Is this an actual rule? I checked some libc functions and it seems like printf(NULL) => errno EINVAL puts(NULL) => segfault Both functions _do_ have nonnull attributes on the parameters, so GCC will warn (if it can statically analyze the situation).> Forbid a NULL StringList in general (similar to how we already forbid > a NULL String); which also means a StringList parameter implies > may_set_error=true. Refactor nbd_internal_set_argv() to split out the > vector population into a new helper function that can only fail with > ENOMEM (this function will also be useful in later patches that want > to copy a StringList, but can tolerate 0 elements), as well as to set > errors itself (all 2 callers updated) since detecting NULL for argv[0] > is unique to argv. > > Tests are added in the next patch, to make it easier to review by > temporarily swapping patch order. > > Changes from: > > $ nbdsh -c 'h.connect_command([])' > nbdsh: generator/states-connect.c:247: enter_STATE_CONNECT_COMMAND_START: Assertion `h->argv.ptr[0]' failed. > Aborted (core dumped)I do agree that this is wrong. If I'm following the Python bindings correctly what is happening is that we're calling nbd_connect_command (h, { NULL }); which is causing the first case above.> to: > > nbdsh: command line script failed: nbd_connect_command: missing command name in argv list: Invalid argumentThis is definitely an improvement for Python code (which really should never crash). So I'm not sure about the total fix here. Definitely we should be returning an error if a zero length list is passed to nbd_connect_* functions. But one thing I especially don't like about libvirt is that it turns various virFoo (NULL) calls into errors instead of segfaults, which in turn makes it much easier to ignore serious errors in the calling code. While I won't necessarily push too hard against this patch I don't feel it's the right direction unless someone can persuade me otherwise. Rich.> Fixes: 0b7bdf62 ("generator: Improve trace messages.", v1.3.2) > Fixes: 6f3eee2e ("lib: Replace a few uses of realloc with nbdkit vector library.", v1.5.5) > Fixes: 8b164376 ("api: Get rid of nbd_connection.", v0.1) > --- > lib/internal.h | 3 ++- > generator/API.ml | 3 ++- > generator/C.ml | 2 +- > lib/connect.c | 10 +++------- > lib/utils.c | 45 ++++++++++++++++++++++++++++++++++----------- > 5 files changed, 42 insertions(+), 21 deletions(-) > > diff --git a/lib/internal.h b/lib/internal.h > index e4c08ca3..04bd8134 100644 > --- a/lib/internal.h > +++ b/lib/internal.h > @@ -476,7 +476,8 @@ extern int nbd_internal_aio_get_direction (enum state state); > > /* utils.c */ > extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp); > -extern int nbd_internal_set_argv (string_vector *v, char **argv); > +extern int nbd_internal_copy_string_list (string_vector *v, char **in); > +extern int nbd_internal_set_argv (struct nbd_handle *h, char **argv); > extern const char *nbd_internal_fork_safe_itoa (long v, char *buf, size_t len); > extern void nbd_internal_fork_safe_perror (const char *s); > extern char *nbd_internal_printable_buffer (const void *buf, size_t count); > diff --git a/generator/API.ml b/generator/API.ml > index abf77972..7be870a4 100644 > --- a/generator/API.ml > +++ b/generator/API.ml > @@ -3458,7 +3458,8 @@ let () > | name, { args; may_set_error = false } > when List.exists > (function > - | Closure _ | Enum _ | Flags _ | String _ -> true > + | Closure _ | Enum _ | Flags _ > + | String _ | StringList _ -> true > | _ -> false) args -> > failwithf "%s: if args contains Closure/Enum/Flags/String parameter, may_set_error must be false" name > > diff --git a/generator/C.ml b/generator/C.ml > index b2d46f98..73ecffa0 100644 > --- a/generator/C.ml > +++ b/generator/C.ml > @@ -570,7 +570,7 @@ let > need_out_label := true > | Flags (n, flags) -> > print_flags_check n flags None > - | String n -> > + | String n | StringList n -> > let value = match errcode with > | Some value -> value > | None -> assert false in > diff --git a/lib/connect.c b/lib/connect.c > index 50080630..ffa50d5b 100644 > --- a/lib/connect.c > +++ b/lib/connect.c > @@ -1,5 +1,5 @@ > /* NBD client library in userspace > - * Copyright (C) 2013-2020 Red Hat Inc. > + * Copyright (C) 2013-2022 Red Hat Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -251,10 +251,8 @@ nbd_unlocked_aio_connect_socket (struct nbd_handle *h, int sock) > int > nbd_unlocked_aio_connect_command (struct nbd_handle *h, char **argv) > { > - if (nbd_internal_set_argv (&h->argv, argv) == -1) { > - set_error (errno, "realloc"); > + if (nbd_internal_set_argv (h, argv) == -1) > return -1; > - } > > return nbd_internal_run (h, cmd_connect_command); > } > @@ -263,10 +261,8 @@ int > nbd_unlocked_aio_connect_systemd_socket_activation (struct nbd_handle *h, > char **argv) > { > - if (nbd_internal_set_argv (&h->argv, argv) == -1) { > - set_error (errno, "realloc"); > + if (nbd_internal_set_argv (h, argv) == -1) > return -1; > - } > > return nbd_internal_run (h, cmd_connect_sa); > } > diff --git a/lib/utils.c b/lib/utils.c > index 3d3b7f45..da3cee72 100644 > --- a/lib/utils.c > +++ b/lib/utils.c > @@ -1,5 +1,5 @@ > /* NBD client library in userspace > - * Copyright (C) 2013-2020 Red Hat Inc. > + * Copyright (C) 2013-2022 Red Hat Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -53,16 +53,17 @@ nbd_internal_hexdump (const void *data, size_t len, FILE *fp) > } > } > > -/* Replace the string_vector with a deep copy of argv (including final NULL). */ > +/* Replace a string_vector with a deep copy of in (including final NULL). */ > int > -nbd_internal_set_argv (string_vector *v, char **argv) > +nbd_internal_copy_string_list (string_vector *v, char **in) > { > size_t i; > > + assert (in); > string_vector_reset (v); > > - for (i = 0; argv[i] != NULL; ++i) { > - char *copy = strdup (argv[i]); > + for (i = 0; in[i] != NULL; ++i) { > + char *copy = strdup (in[i]); > if (copy == NULL) > return -1; > if (string_vector_append (v, copy) == -1) { > @@ -74,6 +75,24 @@ nbd_internal_set_argv (string_vector *v, char **argv) > return string_vector_append (v, NULL); > } > > +/* Store argv into h, or diagnose an error on failure. */ > +int > +nbd_internal_set_argv (struct nbd_handle *h, char **argv) > +{ > + assert (argv); > + > + if (argv[0] == NULL) { > + set_error (EINVAL, "missing command name in argv list"); > + return -1; > + } > + if (nbd_internal_copy_string_list (&h->argv, argv) == -1) { > + set_error (errno, "realloc"); > + return -1; > + } > + > + return 0; > +} > + > /* Like sprintf ("%ld", v), but safe to use between fork and exec. Do > * not use this function in any other context. > * > @@ -247,13 +266,17 @@ nbd_internal_printable_string_list (char **list) > if (fp == NULL) > return NULL; > > - fprintf (fp, "["); > - for (i = 0; list[i] != NULL; ++i) { > - if (i > 0) > - fprintf (fp, ", "); > - printable_string (list[i], fp); > + if (list == NULL) > + fprintf (fp, "NULL"); > + else { > + fprintf (fp, "["); > + for (i = 0; list[i] != NULL; ++i) { > + if (i > 0) > + fprintf (fp, ", "); > + printable_string (list[i], fp); > + } > + fprintf (fp, "]"); > } > - fprintf (fp, "]"); > fclose (fp); > > return s; > -- > 2.37.3 > > _______________________________________________ > Libguestfs mailing list > Libguestfs at redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org