Re: [PATCH] revision.c: fix possible null pointer access

[Junio C Hamano <gitster@pobox.com>]

Stefan Naewe <stefan.naewe@gmail.com> writes:

> Two functions dereference a tree pointer before checking

Reading them a bit carefully, a reader would notice that they
actually do not dereference the pointer at all.  It just computes
another pointer and that is done by adding the offset of object
member in the tree struct.

> if the pointer is valid. Fix that by doing the check first.
>
> Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
> ---
> This has been reported through the CppHints newsletter (http://cpphints.com/hints/40)
> but doesn't seem to have made its way to the ones who care (the git list
> that is...)

Nobody would be surprised, unless the newsletter was sent to this
list, which I do not think it was (but if it was sent while I was
away, then it is very possible that I didn't see it).

>  revision.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 0fbb684..bb40179 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -104,7 +104,12 @@ static void mark_tree_contents_uninteresting(struct tree *tree)
>  {
>  	struct tree_desc desc;
>  	struct name_entry entry;
> -	struct object *obj = &tree->object;
> +	struct object *obj;
> +
> +	if (!tree)
> +		return;
> +
> +	obj = &tree->object;

This is questionable; if you check all the callers of this function
(there are two of them, I think), you would notice that they both
know that tree cannot be NULL here.

>  
>  	if (!has_sha1_file(obj->sha1))
>  		return;
> @@ -135,10 +140,13 @@ static void mark_tree_contents_uninteresting(struct tree *tree)
>  
>  void mark_tree_uninteresting(struct tree *tree)
>  {
> -	struct object *obj = &tree->object;
> +	struct object *obj;
>  
>  	if (!tree)
>  		return;
> +
> +	obj = &tree->object;
> +
>  	if (obj->flags & UNINTERESTING)
>  		return;

This one is not wrong per-se, but an unnecessary change, because no
deferencing is involved.  At least, please lose the blank line after
the new assignment.

>  	obj->flags |= UNINTERESTING;

Thanks.