Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type

From: Junio C Hamano <gitster@pobox.com>
Date: May 5, 2015, 00:28 UTC
Message-ID: <xmqq4mnrg94p.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAPig+cS3f2XggxqbvX6Z2Da24QKLOg915Bf-bTVa+4oVzfhA1A@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 9 quoted lines

>> +extern int hash_sha1_file_literally(struct strbuf *buf, const char
>> *type, unsigned char *return_sha1, unsigned flags);
>
> A few questions:
>
> What's the value of making the first argument of
> hash_sha1_file_literally() a strbuf rather than the two-argument
> buf & len accepted by hash_sha1_file() and write_sha1_file()? Is
> the inconsistency warranted?

I do not care either way, as this is really meant to be a single-purpose wrapper for a single caller.

Show 5 quoted lines

> Would it make sense to name the third argument "sha1" instead of
> "return_sha1" to match the argument name of hash_sha1_file()?
>
> And, as an aside, should your new patch 4/4 rename "return_sha1" to
> "sha1" in the write_sha1_file() prototype also?

Because most of the read-cache.c functions take object name and do something about it, but these small number of functions compute and return object name, I actually think it is a good way to keep "return" in the name to remind those who are writing or reading callers.

Show 8 quoted lines

>> +       /* type string, SP, %lu of the length plus NUL must fit this */
>> +       strbuf_grow(&header, strlen(type) + 20);
>
> A couple comments:
>
> First, given that the largest 64-bit unsigned long value
> (18,446,744,073,709,551,615) is 20 characters, do we want to be really
> pedantic and add 22 instead of 20?

32 is fine ;-)

> Second, is strbuf overkill in this situation when a simple
> xmalloc()/free() would do?

I think underneath the number of xmalloc()/free() calls are the same. The code that uses strbuf abstraction is easier to read, I would think.