NULL dereference in ab/wildmatch

NULL dereference in ab/wildmatch

[Jeff King]

On Sun, Jun 25, 2017 at 02:50:10AM -0700, scan-admin@coverity.com wrote:

> 
> ** CID 1413016:  Null pointer dereferences  (FORWARD_NULL)
> /config.c: 213 in include_by_gitdir()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1413016:  Null pointer dereferences  (FORWARD_NULL)
> /config.c: 213 in include_by_gitdir()
> 207     {
> 208     	struct strbuf text = STRBUF_INIT;
> 209     	struct strbuf pattern = STRBUF_INIT;
> 210     	int ret = 0, prefix;
> 211     	const char *git_dir;
> 212     	int already_tried_absolute = 0;
> >>>     CID 1413016:  Null pointer dereferences  (FORWARD_NULL)
> >>>     Assigning: "code" = "NULL".
> 213     	struct wildmatch_compiled *code = NULL;
> 214     
> 215     	if (opts->git_dir)
> 216     		git_dir = opts->git_dir;
> 217     	else
> 218     		goto done;

I think this comes the "goto done" at the bottom of the context. After
that label, we call wildmatch_free() unconditionally. Probably it just
needs:

diff --git a/config.c b/config.c
index 515f8518e2..cfee92ebbf 100644
--- a/config.c
+++ b/config.c
@@ -260,7 +260,8 @@ static int include_by_gitdir(const struct config_options *opts,
 done:
 	strbuf_release(&pattern);
 	strbuf_release(&text);
-	wildmatch_free(code);
+	if (code)
+		wildmatch_free(code);
 	return ret;
 }
 

though arguably wildmatch_free() should be taught to handle NULL.

-Peff

Re: NULL dereference in ab/wildmatch

[Ævar Arnfjörð Bjarmason]

On Sun, Jun 25, 2017 at 4:09 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Jun 25, 2017 at 02:50:10AM -0700, scan-admin@coverity.com wrote:
>
>>
>> ** CID 1413016:  Null pointer dereferences  (FORWARD_NULL)
>> /config.c: 213 in include_by_gitdir()
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 1413016:  Null pointer dereferences  (FORWARD_NULL)
>> /config.c: 213 in include_by_gitdir()
>> 207     {
>> 208           struct strbuf text = STRBUF_INIT;
>> 209           struct strbuf pattern = STRBUF_INIT;
>> 210           int ret = 0, prefix;
>> 211           const char *git_dir;
>> 212           int already_tried_absolute = 0;
>> >>>     CID 1413016:  Null pointer dereferences  (FORWARD_NULL)
>> >>>     Assigning: "code" = "NULL".
>> 213           struct wildmatch_compiled *code = NULL;
>> 214
>> 215           if (opts->git_dir)
>> 216                   git_dir = opts->git_dir;
>> 217           else
>> 218                   goto done;
>
> I think this comes the "goto done" at the bottom of the context. After
> that label, we call wildmatch_free() unconditionally. Probably it just
> needs:
>
> diff --git a/config.c b/config.c
> index 515f8518e2..cfee92ebbf 100644
> --- a/config.c
> +++ b/config.c
> @@ -260,7 +260,8 @@ static int include_by_gitdir(const struct config_options *opts,
>  done:
>         strbuf_release(&pattern);
>         strbuf_release(&text);
> -       wildmatch_free(code);
> +       if (code)
> +               wildmatch_free(code);
>         return ret;
>  }
>
>
> though arguably wildmatch_free() should be taught to handle NULL.

Thanks. I'll fix this one way or the other in a future submission;

Junio: Could you please eject the RFC patches in the series (i.e. 2/3
& 3/3), i.e. just keep "wildmatch: remove unused wildopts parameter".
I'm slowly refactoring / fixing bugs / adding tests for wildmatch() &
handling the dir.c / tree-walk.c cases. I'll submit that later after
the current obviously-correct "unused parameter" fix has cooked for a
bit.