Re: [PATCH 1/5] mktag.c: adjust verify_tag parameters
- From
Junio C Hamano <junio@pobox.com>
- Date
- May 11, 2008, 18:39 UTC
- Message-ID
- <7vmymw3cst.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <3712867.1210299524581.JavaMail.teamon@b301.teamon.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.
Show 5 quoted lines
> 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.
Show 43 quoted lines
> 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