[Libvir] [PATCH] Handle failed strdup and malloc.
I noticed a bunch of unchecked strdup's in a row, and audited the rest of the file: Handle failed strdup and malloc. * src/remote_internal.c: Don't dereference NULL after failed strdup or malloc in doRemoteOpen. --- src/remote_internal.c | 22 ++++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-) diff --git a/src/remote_internal.c b/src/remote_internal.c index 1420a88..bd8b5a7 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -522,6 +522,10 @@ doRemoteOpen (virConnectPtr conn, struct private_data *priv, const char *uri_str sockname = strdup (LIBVIRTD_PRIV_UNIX_SOCKET_RO); else sockname = strdup (LIBVIRTD_PRIV_UNIX_SOCKET); + if (sockname == NULL) { + error (NULL, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + goto failed; + } } } @@ -576,10 +580,19 @@ doRemoteOpen (virConnectPtr conn, struct private_data *priv, const char *uri_str if (no_tty) nr_args += 5; /* For -T -o BatchMode=yes -e none */ command = command ? : strdup ("ssh"); + if (command == NULL) { + error (NULL, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + goto failed; + } // Generate the final command argv[] array. // ssh -p $port [-l $username] $hostname $netcat -U $sockname [NULL] cmd_argv = malloc (nr_args * sizeof (char *)); + if (cmd_argv == NULL) { + error (NULL, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + goto failed; + } + j = 0; cmd_argv[j++] = strdup (command); cmd_argv[j++] = strdup ("-p"); @@ -601,6 +614,11 @@ doRemoteOpen (virConnectPtr conn, struct private_data *priv, const char *uri_str cmd_argv[j++] = strdup (sockname ? sockname : LIBVIRTD_PRIV_UNIX_SOCKET); cmd_argv[j++] = 0; assert (j == nr_args); + for (j = 0; j < nr_args; j++) + if (cmd_argv[j] == NULL) { + error (NULL, VIR_ERR_SYSTEM_ERROR, strerror (ENOMEM)); + goto failed; + } } /*FALLTHROUGH*/ @@ -633,6 +651,10 @@ doRemoteOpen (virConnectPtr conn, struct private_data *priv, const char *uri_str // Run the external process. if (!cmd_argv) { cmd_argv = malloc (2 * sizeof (char *)); + if (cmd_argv == NULL) { + error (NULL, VIR_ERR_SYSTEM_ERROR, strerror (errno)); + goto failed; + } cmd_argv[0] = command; cmd_argv[1] = 0; } -- 1.5.3.5.666.gfb5f
Jim Meyering wrote:
I noticed a bunch of unchecked strdup's in a row, and audited the rest of the file:
Handle failed strdup and malloc.
* src/remote_internal.c: Don't dereference NULL after failed strdup or malloc in doRemoteOpen.
This is all good stuff, except that the calls to error () should take the conn (virConnectPtr) as first argument if conn is available, which it is here. So +1 if that change is made. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
"Richard W.M. Jones" <rjones@redhat.com> wrote:
Jim Meyering wrote:
I noticed a bunch of unchecked strdup's in a row, and audited the rest of the file:
Handle failed strdup and malloc.
* src/remote_internal.c: Don't dereference NULL after failed strdup or malloc in doRemoteOpen.
This is all good stuff, except that the calls to error () should take the conn (virConnectPtr) as first argument if conn is available, which it is here. So +1 if that change is made.
Hi Rich, Thanks for the quick review. I'll be happy to fix all of the uses of error -- and similar wrapper functions -- but in a separate patch. However, note that this is a general problem: the vast majority of uses of error (at least in that file) currently use NULL as the first parameter, even when there's a usable "conn" in scope.
Jim Meyering wrote:
"Richard W.M. Jones" <rjones@redhat.com> wrote:
I noticed a bunch of unchecked strdup's in a row, and audited the rest of the file:
Handle failed strdup and malloc.
* src/remote_internal.c: Don't dereference NULL after failed strdup or malloc in doRemoteOpen. This is all good stuff, except that the calls to error () should take
Jim Meyering wrote: the conn (virConnectPtr) as first argument if conn is available, which it is here. So +1 if that change is made.
Hi Rich,
Thanks for the quick review.
I'll be happy to fix all of the uses of error -- and similar wrapper functions -- but in a separate patch. However, note that this is a general problem: the vast majority of uses of error (at least in that file) currently use NULL as the first parameter, even when there's a usable "conn" in scope.
Yup, those are all bugs. Even worse in xen_internal.c where we'd need some major restructuring to pass conn to all the places where it's needed. Oh well. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
participants (2)
-
Jim Meyering -
Richard W.M. Jones