drafnel@xxxxxxxxx writes:
> From: Brandon Casey <casey@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
