From patchwork Mon Feb 8 18:28:38 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jim Meyering X-Patchwork-Id: 44809 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 471E4B7D16 for ; Tue, 9 Feb 2010 05:37:22 +1100 (EST) Received: from localhost ([127.0.0.1]:34835 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NeYOi-0005n7-8I for incoming@patchwork.ozlabs.org; Mon, 08 Feb 2010 13:31:48 -0500 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NeYLj-000571-Pz for qemu-devel@nongnu.org; Mon, 08 Feb 2010 13:28:43 -0500 Received: from [199.232.76.173] (port=45668 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NeYLj-00056r-Fn for qemu-devel@nongnu.org; Mon, 08 Feb 2010 13:28:43 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NeYLg-0006z0-AI for qemu-devel@nongnu.org; Mon, 08 Feb 2010 13:28:43 -0500 Received: from mx.meyering.net ([82.230.74.64]:52908) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NeYLf-0006yo-Pq for qemu-devel@nongnu.org; Mon, 08 Feb 2010 13:28:40 -0500 Received: by rho.meyering.net (Acme Bit-Twister, from userid 1000) id F0A612125B; Mon, 8 Feb 2010 19:28:38 +0100 (CET) From: Jim Meyering To: qemu-devel@nongnu.org Date: Mon, 08 Feb 2010 19:28:38 +0100 Message-ID: <87fx5br38p.fsf@meyering.net> Lines: 137 MIME-Version: 1.0 X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 3) Subject: [Qemu-devel] [PATCH] don't dereference NULL after failed strdup X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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. * 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(-) -- 1.7.0.rc2.156.g2ac04 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";