Message ID | 1410248706-9769-1-git-send-email-mrezanin@redhat.com |
---|---|
State | New |
Headers | show |
On Tue, Sep 09, 2014 at 09:45:06AM +0200, 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> > --- > util/uri.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/util/uri.c b/util/uri.c > index e348c17..16c01d0 100644 > --- a/util/uri.c > +++ b/util/uri.c > @@ -1985,7 +1985,8 @@ uri_resolve_relative (const char *uri, const char * base) > val = g_strdup (uri); > goto done; > } > - if (!strcmp(bas->path, ref->path)) { > + if (bas->path != NULL && ref->path != NULL && > + !strcmp(bas->path, ref->path)) { > val = g_strdup(""); > goto done; > } This patch adds more conditions, what's needed is to audit and clean up this function. There is dead code and things could be simplified instead: if (!strcmp(bas->path, ref->path)) { val = g_strdup(""); goto done; } if (bas->path == NULL) { val = g_strdup(ref->path); goto done; } if (ref->path == NULL) { ref->path = (char *) "/"; remove_path = 1; } <---- bas->path cannot be NULL because we took goto done <---- ref->path cannot be NULL because we assigned "/" /* * 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) { <--- dead code! if (ref->path != NULL) { uptr = ref->path; if (*uptr == '/') uptr++; /* exception characters from uri_to_string */ val = uri_string_escape(uptr, "/;&=+$,"); } goto done; } bptr = bas->path; if (ref->path == NULL) { <--- dead code! Also, perhaps the strcmp() you touched should really be moved below the NULL checks.
Stefan Hajnoczi <stefanha@gmail.com> writes: > On Tue, Sep 09, 2014 at 09:45:06AM +0200, 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> >> --- >> util/uri.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/util/uri.c b/util/uri.c >> index e348c17..16c01d0 100644 >> --- a/util/uri.c >> +++ b/util/uri.c >> @@ -1985,7 +1985,8 @@ uri_resolve_relative (const char *uri, const char * base) >> val = g_strdup (uri); >> goto done; >> } >> - if (!strcmp(bas->path, ref->path)) { >> + if (bas->path != NULL && ref->path != NULL && >> + !strcmp(bas->path, ref->path)) { >> val = g_strdup(""); >> goto done; >> } > > This patch adds more conditions, what's needed is to audit and clean up this > function. There is dead code and things could be simplified instead: > > if (!strcmp(bas->path, ref->path)) { > val = g_strdup(""); > goto done; > } > if (bas->path == NULL) { > val = g_strdup(ref->path); > goto done; > } > if (ref->path == NULL) { > ref->path = (char *) "/"; > remove_path = 1; > } > <---- bas->path cannot be NULL because we took goto done > <---- ref->path cannot be NULL because we assigned "/" > > /* > * 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) { <--- dead code! > if (ref->path != NULL) { > uptr = ref->path; > if (*uptr == '/') > uptr++; > /* exception characters from uri_to_string */ > val = uri_string_escape(uptr, "/;&=+$,"); > } > goto done; > } > bptr = bas->path; > if (ref->path == NULL) { <--- dead code! > > Also, perhaps the strcmp() you touched should really be moved below the NULL > checks. If you simplify this function, please get rid of the silly conditionals around g_strdup(). See commits 80e0090 c14e984 c64f50d for examples.
Markus Armbruster <armbru@redhat.com> writes: > Stefan Hajnoczi <stefanha@gmail.com> writes: > >> On Tue, Sep 09, 2014 at 09:45:06AM +0200, 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> >>> --- >>> util/uri.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/util/uri.c b/util/uri.c >>> index e348c17..16c01d0 100644 >>> --- a/util/uri.c >>> +++ b/util/uri.c >>> @@ -1985,7 +1985,8 @@ uri_resolve_relative (const char *uri, const char * base) >>> val = g_strdup (uri); >>> goto done; >>> } >>> - if (!strcmp(bas->path, ref->path)) { >>> + if (bas->path != NULL && ref->path != NULL && >>> + !strcmp(bas->path, ref->path)) { >>> val = g_strdup(""); >>> goto done; >>> } >> >> This patch adds more conditions, what's needed is to audit and clean up this >> function. There is dead code and things could be simplified instead: >> >> if (!strcmp(bas->path, ref->path)) { >> val = g_strdup(""); >> goto done; >> } >> if (bas->path == NULL) { >> val = g_strdup(ref->path); >> goto done; >> } >> if (ref->path == NULL) { >> ref->path = (char *) "/"; >> remove_path = 1; >> } >> <---- bas->path cannot be NULL because we took goto done >> <---- ref->path cannot be NULL because we assigned "/" >> >> /* >> * 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) { <--- dead code! >> if (ref->path != NULL) { >> uptr = ref->path; >> if (*uptr == '/') >> uptr++; >> /* exception characters from uri_to_string */ >> val = uri_string_escape(uptr, "/;&=+$,"); >> } >> goto done; >> } >> bptr = bas->path; >> if (ref->path == NULL) { <--- dead code! >> >> Also, perhaps the strcmp() you touched should really be moved below the NULL >> checks. > > If you simplify this function, please get rid of the silly conditionals > around g_strdup(). See commits 80e0090 c14e984 c64f50d for examples. Likewise: g_free(). See commits 9e28840 64641d8 f7047c2 fb7da62 fec0da9 ec15993.
diff --git a/util/uri.c b/util/uri.c index e348c17..16c01d0 100644 --- a/util/uri.c +++ b/util/uri.c @@ -1985,7 +1985,8 @@ uri_resolve_relative (const char *uri, const char * base) val = g_strdup (uri); goto done; } - if (!strcmp(bas->path, ref->path)) { + if (bas->path != NULL && ref->path != NULL && + !strcmp(bas->path, ref->path)) { val = g_strdup(""); goto done; }