From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60306) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YKmYr-0007M5-Ek for qemu-devel@nongnu.org; Mon, 09 Feb 2015 06:31:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YKmYn-0005Lc-Em for qemu-devel@nongnu.org; Mon, 09 Feb 2015 06:31:29 -0500 Received: from mail-we0-x233.google.com ([2a00:1450:400c:c03::233]:40505) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YKmYm-0005LU-TM for qemu-devel@nongnu.org; Mon, 09 Feb 2015 06:31:25 -0500 Received: by mail-we0-f179.google.com with SMTP id u56so20721003wes.10 for ; Mon, 09 Feb 2015 03:31:22 -0800 (PST) Sender: Paolo Bonzini Message-ID: <54D89A86.9050008@redhat.com> Date: Mon, 09 Feb 2015 12:31:18 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1423478877-12053-1-git-send-email-mrezanin@redhat.com> In-Reply-To: <1423478877-12053-1-git-send-email-mrezanin@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv2] Prevent segmentation fault in case of relative resolve of uri List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mrezanin@redhat.com, qemu-devel@nongnu.org Cc: armbru@redhat.com, stefanha@redhat.com On 09/02/2015 11:47, mrezanin@redhat.com wrote: > From: Miroslav Rezanina > > It was possible to call strcmp with NULL argument, that can cause > segmentation fault. Properly checking parameters to prevent this > situation. > > Signed-off-by: Miroslav Rezanina > --- > v2: > - instead of adding NULL checks to strcmp call refactor whole > NULL checking path. This will remove dead code and make whole checking > easier to understand. > > Relative path generation part is not touched as I'm not fully sure > of correct behavior and purpose of this patch is to prevent segmentation > fault. > --- > util/uri.c | 55 +++++++++++++++++++++++++------------------------------ > 1 file changed, 25 insertions(+), 30 deletions(-) > > diff --git a/util/uri.c b/util/uri.c > index 918d235..23dbaca 100644 > --- a/util/uri.c > +++ b/util/uri.c > @@ -1964,44 +1964,39 @@ uri_resolve_relative (const char *uri, const char * base) > * If the scheme / server on the URI differs from the base, > * just return the URI > */ > - if ((ref->scheme != NULL) && > - ((bas->scheme == NULL) || > - (strcmp (bas->scheme, ref->scheme)) || > - (strcmp (bas->server, ref->server)))) { > - val = g_strdup (uri); > - goto done; > + > + if ((ref->scheme != NULL) && > + ((bas->scheme == NULL) || (strcmp (bas->scheme, ref->scheme)))) { > + val = g_strdup(uri); > + goto done; > } > - if (!strcmp(bas->path, ref->path)) { > - val = g_strdup(""); > - goto done; > - } > - if (bas->path == NULL) { > - val = g_strdup(ref->path); > - goto done; > + if ((ref->server != NULL) && > + ((bas->server == NULL) || (strcmp (bas->server, ref->server)))) { > + val = g_strdup(uri); > + goto done; > } > + > if (ref->path == NULL) { > ref->path = (char *) "/"; > - remove_path = 1; > + remove_path = 1; > } > > - /* > - * At this point (at last!) we can compare the two paths > - * > - * First we take care of the special case where either of the > - * two path components may be missing (bug 316224) > - */ > if (bas->path == NULL) { > - if (ref->path != NULL) { > - uptr = ref->path; > - if (*uptr == '/') > - uptr++; > - /* exception characters from uri_to_string */ > - val = uri_string_escape(uptr, "/;&=+$,"); > - } > - goto done; > + uptr = ref->path; > + if (*uptr == '/') > + uptr++; > + /* exception characters from uri_to_string */ > + val = uri_string_escape(uptr, "/;&=+$,"); > + goto done; > } > + > + if (!strcmp(bas->path, ref->path)) { > + val = g_strdup(""); > + goto done; > + } > + > bptr = bas->path; > - if (ref->path == NULL) { > + if (remove_path == 1) { > for (ix = 0; bptr[ix] != 0; ix++) { > if (bptr[ix] == '/') > nbslash++; > @@ -2010,7 +2005,7 @@ uri_resolve_relative (const char *uri, const char * base) > len = 1; /* this is for a string terminator only */ > } else { > /* > - * Next we compare the two strings and find where they first differ > + * We compare the two strings and find where they first differ > */ > if ((ref->path[pos] == '.') && (ref->path[pos+1] == '/')) > pos += 2; > It's the third time a fix for this is submitted. :) [PATCH 3/3] util/uri: URI member path can be null, compare more carfully (by Markus) [PATCH 3/7] uri: avoid NULL arguments to strcmp (by me). Markus's patch was accepted. Further cleanups on the code on top of his patch are welcome, though. The logic for "compare two possibly-NULL strings" could be replaced by g_strcmp0, but that's only provided by glib versions 2.16 or newer, while we support 2.12. Paolo