Re: [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 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs