Re: [PATCH 1/3] validate_headref: NUL-terminate HEAD buffer

From: Jeff King <peff@peff.net>
Date: Sep 27, 2017, 07:16 UTC
Message-ID: <20170927071625.3dtljf5oaiaa5pjq@sigill.intra.peff.net>
In-Reply-To: <xmqqefqs4o41.fsf@gitster.mtv.corp.google.com>

On Wed, Sep 27, 2017 at 04:06:22PM +0900, Junio C Hamano wrote:

> A few tangents I noticed:
> 
>  - the result of readlink should be checked with starts_with() in
>    the modern codebase (#leftoverbits).

Yes, though it needs to NUL-terminate, too (readlink does not do so automatically). Again, we seem to have left room for the NUL but didn't bother to put it there.

>  - buffer[256] would mean that we cannot have a branch whose name is
>    more than a couple of hundred bytes long; as you said, we may be
>    better off using strbuf_read to read the whole thing.

Heh, I almost talked about this in the cover letter, but didn't want to go off on a tangent. But since you mention it...

I had the same concern, but actually truncation is not a problem here (for a symlink or a symref). We are only seeing if the contents look vaguely correct, so really we never parse past "refs/" in either case.

The real symref resolution happens in refs/files-backend.c these days, and does use a strbuf.

-Peff