From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NfKJZ-0001b6-4T for qemu-devel@nongnu.org; Wed, 10 Feb 2010 16:41:41 -0500 Received: from [199.232.76.173] (port=37824 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NfKJY-0001ah-Ij for qemu-devel@nongnu.org; Wed, 10 Feb 2010 16:41:40 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NfKJW-0001ua-9k for qemu-devel@nongnu.org; Wed, 10 Feb 2010 16:41:40 -0500 Received: from mail-iw0-f194.google.com ([209.85.223.194]:60256) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NfKJV-0001uW-VE for qemu-devel@nongnu.org; Wed, 10 Feb 2010 16:41:38 -0500 Received: by iwn32 with SMTP id 32so907977iwn.14 for ; Wed, 10 Feb 2010 13:41:37 -0800 (PST) Message-ID: <4B73280E.808@codemonkey.ws> Date: Wed, 10 Feb 2010 15:41:34 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] don't dereference NULL after failed strdup References: <87fx5br38p.fsf@meyering.net> In-Reply-To: <87fx5br38p.fsf@meyering.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jim Meyering Cc: qemu-devel@nongnu.org On 02/08/2010 12:28 PM, Jim Meyering wrote: > Most of these are obvious NULL-deref bug fixes, for example, > the ones in these files: > > block/curl.c > net.c > slirp/misc.c > > and the first one in block/vvfat.c. > The others in block/vvfat.c may not lead to an immediate segfault, but I > traced the two schedule_rename(..., strdup(path)) uses, and a failed > strdup would appear to trigger this assertion in handle_renames_and_mkdirs: > > assert(commit->path); > > The conversion to use qemu_strdup in envlist_to_environ is not technically > needed, but does avoid a theoretical leak in the caller when strdup fails > for one value, but later succeeds in allocating another buffer(plausible, > if one string length is much larger than the others). The caller does > not know the length of the returned list, and as such can only free > pointers until it hits the first NULL. If there are non-NULL pointers > beyond the first, their buffers would be leaked. This one is admittedly > far-fetched. > > The two in linux-user/main.c are worth fixing to ensure that an > OOM error is diagnosed up front, rather than letting it provoke some > harder-to-diagnose secondary error, in case of exec failure, or worse, in > case the exec succeeds but with an invalid list of command line options. > However, considering how unlikely it is to encounter a failed strdup early > in main, this isn't a big deal. Note that adding the required uses of > qemu_strdup here and in envlist.c induce link failures because qemu_strdup > is not currently in any library they're linked with. So for now, I've > omitted those changes, as well as the fixes in target-i386/helper.c > and target-sparc/helper.c. > > If you'd like to see the above discussion (or anything else) > in the commit log, just let me know and I'll be happy to adjust. > > > > From 9af42864fd1ea666bd25e2cecfdfae74c20aa8c7 Mon Sep 17 00:00:00 2001 > From: Jim Meyering > Date: Mon, 8 Feb 2010 18:29:29 +0100 > Subject: [PATCH] don't dereference NULL after failed strdup > > Handle failing strdup by replacing each use with qemu_strdup, > so as not to dereference NULL or trigger a failing assertion. > * block/curl.c (curl_open): s/\bstrdup\b/qemu_strdup/ > * block/vvfat.c (init_directories): Likewise. > (get_cluster_count_for_direntry, check_directory_consistency): Likewise. > * net.c (parse_host_src_port): Likewise. > Applied, but this got ugly in git am. FYI, if you want to include comments with a patch that aren't in the git commit message, you can use a '---' separator just like with diffstat below. Regards, Anthony Liguori > * slirp/misc.c (fork_exec): Likewise. > --- > block/curl.c | 2 +- > block/vvfat.c | 10 +++++----- > net.c | 2 +- > slirp/misc.c | 2 +- > 4 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/block/curl.c b/block/curl.c > index fe08f7b..2cf72cb 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -309,7 +309,7 @@ static int curl_open(BlockDriverState *bs, const char *filename, int flags) > > static int inited = 0; > > - file = strdup(filename); > + file = qemu_strdup(filename); > s->readahead_size = READ_AHEAD_SIZE; > > /* Parse a trailing ":readahead=#:" param, if present. */ > diff --git a/block/vvfat.c b/block/vvfat.c > index d2787b9..bb707c0 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -883,7 +883,7 @@ static int init_directories(BDRVVVFATState* s, > mapping->dir_index = 0; > mapping->info.dir.parent_mapping_index = -1; > mapping->first_mapping_index = -1; > - mapping->path = strdup(dirname); > + mapping->path = qemu_strdup(dirname); > i = strlen(mapping->path); > if (i> 0&& mapping->path[i - 1] == '/') > mapping->path[i - 1] = '\0'; > @@ -1633,10 +1633,10 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s, > > /* rename */ > if (strcmp(basename, basename2)) > - schedule_rename(s, cluster_num, strdup(path)); > + schedule_rename(s, cluster_num, qemu_strdup(path)); > } else if (is_file(direntry)) > /* new file */ > - schedule_new_file(s, strdup(path), cluster_num); > + schedule_new_file(s, qemu_strdup(path), cluster_num); > else { > assert(0); > return 0; > @@ -1753,10 +1753,10 @@ static int check_directory_consistency(BDRVVVFATState *s, > mapping->mode&= ~MODE_DELETED; > > if (strcmp(basename, basename2)) > - schedule_rename(s, cluster_num, strdup(path)); > + schedule_rename(s, cluster_num, qemu_strdup(path)); > } else > /* new directory */ > - schedule_mkdir(s, cluster_num, strdup(path)); > + schedule_mkdir(s, cluster_num, qemu_strdup(path)); > > lfn_init(&lfn); > do { > diff --git a/net.c b/net.c > index 6ef93e6..8e951ca 100644 > --- a/net.c > +++ b/net.c > @@ -96,7 +96,7 @@ int parse_host_src_port(struct sockaddr_in *haddr, > struct sockaddr_in *saddr, > const char *input_str) > { > - char *str = strdup(input_str); > + char *str = qemu_strdup(input_str); > char *host_str = str; > char *src_str; > const char *src_str2; > diff --git a/slirp/misc.c b/slirp/misc.c > index 05f4fb3..dcb1dc1 100644 > --- a/slirp/misc.c > +++ b/slirp/misc.c > @@ -179,7 +179,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty) > close(s); > > i = 0; > - bptr = strdup(ex); /* No need to free() this */ > + bptr = qemu_strdup(ex); /* No need to free() this */ > if (do_pty == 1) { > /* Setup "slirp.telnetd -x" */ > argv[i++] = "slirp.telnetd"; > -- > 1.7.0.rc2.156.g2ac04 > > > >