threads / patch / 47570

patchbisect: avoid NULL pointer dereference

Subject: [PATCH] bisect: avoid NULL pointer dereference

## tl;dr

4 messages between Jan 8, 2018 and Jan 8, 2018. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

René Scharfe· Jan 8, 2018, 20:36 UTC · lore

7c117184d7 (bisect: fix off-by-one error in `best_bisection_sorted()`) fixed an off-by-one error, plugged a memory leak and removed a NULL check. However, the pointer p *is* actually NULL if an empty list is passed to the function. Let's add the check back for safety. Bisecting nothing doesn't make too much sense, but that's no excuse for crashing.

Found with GCC's -Wnull-dereference.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 bisect.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Show changes to bisect.c +4 −2

diff --git a/bisect.c b/bisect.c
index 0fca17c02b..2f3008b078 100644
--- a/bisect.c
+++ b/bisect.c
@@ -229,8 +229,10 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
 		if (i < cnt - 1)
 			p = p->next;
 	}
-	free_commit_list(p->next);
-	p->next = NULL;
+	if (p) {
+		free_commit_list(p->next);
+		p->next = NULL;
+	}
 	strbuf_release(&buf);
 	free(array);
 	return list;

-- 
2.15.1

Johannes Schindelin· Jan 8, 2018, 20:45 UTC · re: René Scharfe · lore

Re: [PATCH] bisect: avoid NULL pointer dereference

Hi René,

On Mon, 8 Jan 2018, René Scharfe wrote:

Show 30 quoted lines

> 7c117184d7 (bisect: fix off-by-one error in `best_bisection_sorted()`)
> fixed an off-by-one error, plugged a memory leak and removed a NULL
> check.  However, the pointer p *is* actually NULL if an empty list is
> passed to the function.  Let's add the check back for safety.  Bisecting
> nothing doesn't make too much sense, but that's no excuse for crashing.
> 
> Found with GCC's -Wnull-dereference.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  bisect.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/bisect.c b/bisect.c
> index 0fca17c02b..2f3008b078 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -229,8 +229,10 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
>  		if (i < cnt - 1)
>  			p = p->next;
>  	}
> -	free_commit_list(p->next);
> -	p->next = NULL;
> +	if (p) {
> +		free_commit_list(p->next);
> +		p->next = NULL;
> +	}
>  	strbuf_release(&buf);
>  	free(array);
>  	return list;

Isn't this identical to https://public-inbox.org/git/20180103184852.27271-1-avarab@gmail.com/ ?

Ciao, Dscho

René Scharfe· Jan 8, 2018, 21:50 UTC · re: Johannes Schindelin · lore

Re: [PATCH] bisect: avoid NULL pointer dereference

Hello Dscho,

Am 08.01.2018 um 21:45 schrieb Johannes Schindelin:

> Isn't this identical to
> https://public-inbox.org/git/20180103184852.27271-1-avarab@gmail.com/ ?

yes, indeed, thanks.  So here's an upvote for Ævar's patch then. :)

(I should have sent it earlier, but was not fully convinced it could be triggered in the wild.)

René

Junio C Hamano· Jan 8, 2018, 22:15 UTC · re: René Scharfe · lore

Re: [PATCH] bisect: avoid NULL pointer dereference

René Scharfe <l.s.r@web.de> writes:

Show 10 quoted lines

> 7c117184d7 (bisect: fix off-by-one error in `best_bisection_sorted()`)
> fixed an off-by-one error, plugged a memory leak and removed a NULL
> check.  However, the pointer p *is* actually NULL if an empty list is
> passed to the function.  Let's add the check back for safety.  Bisecting
> nothing doesn't make too much sense, but that's no excuse for crashing.
>
> Found with GCC's -Wnull-dereference.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---

Thanks. I think this is the same as 2e9fdc79 ("bisect: fix a regression causing a segfault", 2018-01-03) but the log we see here explains what goes wrong much better than the other one ;-)

Show 20 quoted lines

>  bisect.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 0fca17c02b..2f3008b078 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -229,8 +229,10 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
>  		if (i < cnt - 1)
>  			p = p->next;
>  	}
> -	free_commit_list(p->next);
> -	p->next = NULL;
> +	if (p) {
> +		free_commit_list(p->next);
> +		p->next = NULL;
> +	}
>  	strbuf_release(&buf);
>  	free(array);
>  	return list;