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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 13, 2023, 17:29 UTC
Message-ID: <xmqqmt5hcw4e.fsf@gitster.g>
In-Reply-To: <876cf920-113a-90cf-f49e-6e1b7b146acf@github.com>

Derrick Stolee <derrickstolee@github.com> writes:

Show 27 quoted lines

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

Show 9 quoted lines

> @@ -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?

Show 19 quoted lines

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