Re: [PATCH 3/6] Make sure fsck_commit_buffer() does not run out of the buffer

[Junio C Hamano <gitster@pobox.com>]

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

> So far, we assumed that the buffer is NUL terminated, but this is not
> a safe assumption, now that we opened the fsck_object() API to pass a
> buffer directly.
>
> So let's make sure that there is at least an empty line in the buffer.
> That way, our checks would fail if the empty line was encountered
> prematurely, and consequently we can get away with the current string
> comparisons even with non-NUL-terminated buffers are passed to
> fsck_object().
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Heh, I probably should have read this one before commenting on 2/6.

It makes me feel somewhat uneasy to insist that there must be a
blank line in the commit object, even though from the very first
implementation of "commit-tree" I think we have always had a blank
there at the end of the header, even when you feed nothing as the
message to it.

I think the new check is OK, but the message should be s/empty
line/end of header/ or something.  It is not like we require an
empty line in the log message proper.

>  fsck.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/fsck.c b/fsck.c
> index dd77628..db6aaa4 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
>  	return retval;
>  }
>  
> +static int must_have_empty_line(const void *data, unsigned long size,
> +	struct object *obj, fsck_error error_func)
> +{
> +	const char *buffer = (const char *)data;
> +	int i;
> +
> +	for (i = 0; i < size; i++) {
> +		switch (buffer[i]) {
> +		case '\0':
> +			return error_func(obj, FSCK_ERROR,
> +				"invalid message: NUL at offset %d", i);
> +		case '\n':
> +			if (i + 1 < size && buffer[i + 1] == '\n')
> +				return 0;
> +		}
> +	}
> +
> +	return error_func(obj, FSCK_ERROR, "invalid buffer: missing empty line");
> +}
> +
>  static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)
>  {
>  	char *end;
> @@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
>  	unsigned parent_count, parent_line_count = 0;
>  	int err;
>  
> +	if (must_have_empty_line(buffer, size, &commit->object, error_func))
> +		return -1;
> +
>  	if (!skip_prefix(buffer, "tree ", &buffer))
>  		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'tree' line");
>  	if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')