Re: git error in tag ...: unterminated header

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 25, 2015, 21:21 UTC
Message-ID: <xmqqoak3wkkq.fsf@gitster.dls.corp.google.com>
In-Reply-To: <2b124e09d9c89ff3892f246ea91aa3c4@www.dscho.org>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 26 quoted lines

> Hi Junio & Wolfgang,
>
> On 2015-06-25 22:24, Junio C Hamano wrote:
>> Wolfgang Denk <wd@denx.de> writes:
>> 
>>> In message <xmqqegkzzoaz.fsf@gitster.dls.corp.google.com> you wrote:
>>>>
>>>> > Question is: how can we fix that?
>>>>
>>>> It could be that 4d0d8975 is buggy and barking at a non breakage.
>
> Well, I would like to believe that this commit made our code *safer*
> by making sure that we would never overrun the buffer. Remember: under
> certain circumstances, the buffer passed to the fsck machinery is
> *not* terminated by a NUL. The code I introduced simply verifies that
> there is an empty line because the fsck code stops there and does not
> look further.
>
> If the buffer does *not* contain an empty line, the fsck code runs the
> danger of looking beyond the allocated memory because it uses
> functions that assume NUL-terminated strings, while the buffer passed
> to the fsck code is a counted string.
>
> The quick & dirty work-around would be to detect when the buffer does
> not contain an empty line and to make a NUL-terminated copy in that
> case.

Yes, I can totally understand its quick-and-dirty-ness would break a valid case where there is no need for a blank after the header.

> A better solution was outlined by Peff back when I submitted those
> patches: change all the code paths that read objects and make sure
> that all of them are terminated by a NUL. AFAIR some code paths did
> that already, but not all of them.

I do not think you necessarily need a NUL. As you said, your input is a counted string, so you know the length of the buffer. And you are verifying line by line. As long as you make sure the buffer ends with "\n" (instead of saying "it has "\n\n" somewhere), updating the existing code that does

	if (buffer is not well formed wrt "tree")
		barf;
	else
        	advance buffer to point at the next line;
	if (buffer is not well formed wrt "parent")
        	barf;
	...

to do this instead:

	if (buffer is not well formed wrt "tree")
		barf;
	else
        	advance buffer to point at the next line;
	if (buffer is now beyond the end of the original length)
		barf; /* missing "parent" */
	if (buffer is not well formed wrt "parent")
        	barf;
	...

shouldn't be rocket science, no?