| Message ID | 1423478877-12053-1-git-send-email-mrezanin@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/02/2015 11:47, mrezanin@redhat.com wrote: > From: Miroslav Rezanina <mrezanin@redhat.com> > > 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 <mrezanin@redhat.com> > --- > 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
On Mon, Feb 09, 2015 at 12:31:18PM +0100, Paolo Bonzini wrote: > > > On 09/02/2015 11:47, mrezanin@redhat.com wrote: > > From: Miroslav Rezanina <mrezanin@redhat.com> > > > > 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 <mrezanin@redhat.com> > > --- > > 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 Hi Paolo, I miss the patches as they are not committed yet. The patch do almost the same think as my v1 (with one more check) that was rejected by Stefan because "adding more checks only". In addition to preventing null compare, this patch removes the dead code in the function. I'll revisit it after Marcus fix is committed. Mirek
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;