Re: [PATCH 1/5] mktag.c: adjust verify_tag parameters

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

drafnel@gmail.com writes:

> From: Brandon Casey <casey@nrlssc.navy.mil>
>
> The size parameter should be a size_t since it is a string length.

Correct.

> There is no reason that the buffer argument should not be constant except
> for the nul termination that is performed in the first few lines of this
> function. This is not necessary, since a valid c string must always be
> nul terminated and we can check whether we have exceeded the caller's
> size parameter at the end of parsing the buffer.

Wait a minute.  The point of passing a stringlet as a tuple of <pointer to
the beginning, length> is that you may not have a valid C string to begin
with, isn't it?  We shouldn't be assuming that reading past the given size
is Ok --- you may not be lucky enough to have any NUL byte after the given
string before you hit the page boundary and encounter unmapped page.

The generic-looking argument you made is bogus, but for this particular
code it is true, as the parameter you are passing to the function is
prepared by strbuf_read() which gives you a NUL terminated buffer.

So the code is correct -- justification is not.

> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>  mktag.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/mktag.c b/mktag.c
> index 0b34341..5489ad6 100644
> --- a/mktag.c
> +++ b/mktag.c
> @@ -39,7 +39,7 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
>  #define PD_FMT "%td"
>  #endif
>  
> -static int verify_tag(char *buffer, unsigned long size)
> +static int verify_tag(const char *buffer, size_t size)
>  {
>  	int typelen;
>  	char type[20];
> @@ -50,8 +50,6 @@ static int verify_tag(char *buffer, unsigned long size)
>  	if (size < 84)
>  		return error("wanna fool me ? you obviously got the size wrong !");
>  
> -	buffer[size] = 0;
> -
>  	/* Verify object line */
>  	object = buffer;
>  	if (memcmp(object, "object ", 7))
> @@ -145,6 +143,13 @@ static int verify_tag(char *buffer, unsigned long size)
>  		return error("char" PD_FMT ": trailing garbage in tag header",
>  			tagger_line - buffer);
>  
> +	/*
> +	 * Make sure we haven't advanced past what the caller said the
> +	 * buffer size was.
> +	 */
> +	if (tagger_line - buffer >= size)
> +		return error("char" PD_FMT ": tag truncated", size);
> +
>  	/* The actual stuff afterwards we don't care about.. */
>  	return 0;
>  }
> -- 
> 1.5.5.67.g9a49