Re: [PATCH 3/6] revert: fix buffer overflow in insn sheet parser

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 20, 2011, 09:09 UTC
Message-ID: <20111020090912.GA21471@elie.hsd1.il.comcast.net>
In-Reply-To: <4E9FE061.3080601@gmail.com>

Ramkumar Ramachandra wrote:

Show 7 quoted lines

> Okay.  How about putting this after 5/6?
>
> -- 8< --
> Subject: [PATCH] t3510: guard against buffer overflows in parser
> 
> To guard against a buffer overflow in the parser, verify that
> instruction sheets with overly long object names are parsed.

Looks good, except I would explain it differently, to avoid referring to hypothetical implementation details ("What buffer overflow?"):

	test: git cherry-pick --continue should cope with long object names

	A naive implementation that uses a commit-id-shaped buffer
	to store the word after "pick" in .git/sequencer/todo lines
	would crash often.  Our implementation is not so naive, but
	add a test anyway to futureproof it.

Or:

	test: make sure the "cherry-pick --continue" buffer overflow doesn't come back

	Before commit ..., "git cherry-pick --continue" would overflow
	under ... circumstance.  Add a test to make sure it doesn't
	happen again.

Though the implementation is actually better than that --- it can even cope with a valid object name (e.g., a long name of a branch, or something like "HEAD^{/refs.c: ensure struct whose member}") that is that long, without truncating it. So if you have time for it, I think it would be worth a test where the "git cherry-pick --continue" succeeds, too.

Thanks, Jonathan