Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much

From: Junio C Hamano <gitster@pobox.com>
Date: May 18, 2017, 04:19 UTC
Message-ID: <xmqq1srmvlt8.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <eecc015af8d7ed71223b591b13847fdb56ee69f0.1495014840.git.mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 13 quoted lines

> The `trim` parameter can be set independently of `prefix`. So if some
> caller were to set `trim` to be greater than `strlen(prefix)`, we
> could end up pointing the `refname` field of the iterator past the NUL
> of the actual reference name string.
>
> That can't happen currently, because `trim` is always set either to
> zero or to `strlen(prefix)`. But even the latter could lead to
> confusion, if a refname is exactly equal to the prefix, because then
> we would set the outgoing `refname` to the empty string.
>
> And we're about to decouple the `prefix` and `trim` arguments even
> more, so let's be cautious here. Skip over any references whose names
> are not longer than `trim`.

Should we be silently continuing, or should we use die("BUG") here instead, if the motivation is to be cautions? Personally, I do not find this memchr() implementation too bad, especially when our objective is to play cautious, but strlen() based one is fine, too.

It's not like refname field would point at a run of non-NUL bytes at the end of the last-mapped page and taking strlen() would segfault, right?

Show 16 quoted lines

> +		if (iter->trim) {
> +			/*
> +			 * If there wouldn't be at least one character
> +			 * left in the refname after trimming, skip
> +			 * over it:
> +			 */
> +			if (memchr(iter->iter0->refname, '\0', iter->trim + 1))
> +				continue;
> +			iter->base.refname = iter->iter0->refname + iter->trim;
> +		} else {
> +			iter->base.refname = iter->iter0->refname;
> +		}
> +
>  		iter->base.oid = iter->iter0->oid;
>  		iter->base.flags = iter->iter0->flags;
>  		return ITER_OK;