[PATCH] bisect: avoid NULL pointer dereference

[PATCH] bisect: avoid NULL pointer dereference

[René Scharfe]

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;
-- 
2.15.1

Re: [PATCH] bisect: avoid NULL pointer dereference

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

Hi René,

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

> 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

Re: [PATCH] bisect: avoid NULL pointer dereference

[René Scharfe]

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é

Re: [PATCH] bisect: avoid NULL pointer dereference

[Junio C Hamano]

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

> 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 ;-)

>  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;