5:05 p.m.
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