Re: [PATCH] commit-reach: avoid NULL dereference

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

Derrick Stolee <derrickstolee@github.com> writes:

>> The above seems to indicate that the expectation by callers is a bit
>> uneven.  Shouldn't the first onetrust the callee to clear the flag?
>
> Yes, that makes sense. (But there's more!)
> ...
> Thanks for digging into the details here. I agree that this extra
> assignment of the flag to these non-commit objects is unnecessary
> since we intend to clear them by the end of the method and don't
> do anything with the flags otherwise.
>
> What you seem to be suggesting is this diff:
>
> diff --git a/commit-reach.c b/commit-reach.c
> index 2e33c599a82..8c387911228 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -740,10 +740,8 @@ int can_all_from_reach_with_flag(struct object_array *from,
>  			/*
>  			 * no way to tell if this is reachable by
>  			 * looking at the ancestry chain alone, so
> -			 * leave a note to ourselves not to worry about
> -			 * this object anymore.
> +			 * skip it.
>  			 */
> -			from->objects[i].item->flags |= assign_flag;
>  			continue;
>  		}

Agreed with this part.

> @@ -856,17 +854,6 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,
>  
>  	result = can_all_from_reach_with_flag(&from_objs, PARENT2, PARENT1,
>  					      min_commit_date, min_generation);
> -
> -	while (from) {
> -		clear_commit_marks(from->item, PARENT1);
> -		from = from->next;
> -	}

This too.

> -	while (to) {
> -		clear_commit_marks(to->item, PARENT2);
> -		to = to->next;
> -	}

But I didn't think this is redundant.  PARENT2 is used to mark "to"
commits by this function, not the callee, before making the call.
The callee may clear PARENT1 used to mark the ones visited by the
traversal starting from the "from" commits, but does not do anything
to "with_flag", does it?

>  	object_array_clear(&from_objs);
>  	return result;
>  }
>
> And instead of the current patch, this should allow the
> following diff hunk instead:
>
> @@ -807,9 +805,6 @@ int can_all_from_reach_with_flag(struct object_array *from,
>  	clear_commit_marks_many(nr_commits, list, RESULT | assign_flag);
>  	free(list);
>  
> -	for (i = 0; i < from->nr; i++)
> -		from->objects[i].item->flags &= ~assign_flag;
> -
>  	return result;
>  }
>
> This avoids the need for the NULL check, since we are skipping the
> entire loop. The clear_commit_marks_many() is sufficient. 

Yeah, that was my reading based on very limited and sufrace level of
understanding of what this function was doing, although I think my
understanding of it is still very skimpy (if I understand it well
enough, I would give it a bit more understandable name but I cannot
come up with one yet).

Thanks.