From: Junio C Hamano <gitster@pobox•com>
To: Eric Sunshine <sunshine@sunshineco•com>
Cc: Git List <git@vger•kernel.org>
Subject: Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type
Date: Mon, 04 May 2015 17:28:54 -0700 [thread overview]
Message-ID: <xmqq4mnrg94p.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CAPig+cS3f2XggxqbvX6Z2Da24QKLOg915Bf-bTVa+4oVzfhA1A@mail.gmail.com> (Eric Sunshine's message of "Mon, 4 May 2015 20:13:18 -0400")
Eric Sunshine <sunshine@sunshineco•com> writes:
>> +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.
> 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.
>> + /* 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.
next prev parent reply other threads:[~2015-05-05 0:29 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-04 7:25 [PATCH 0/3] hash-object crash fix and new doc and tests Eric Sunshine
2015-05-04 7:25 ` [PATCH 1/3] git-hash-object.txt: document --literally option Eric Sunshine
2015-05-04 7:25 ` [PATCH 2/3] t1007: add hash-object --literally tests Eric Sunshine
2015-05-04 7:25 ` [PATCH 3/3] write_sha1_file_prepare: fix buffer overrun with extra-long object type Eric Sunshine
2015-05-04 17:58 ` Junio C Hamano
2015-05-04 17:59 ` Junio C Hamano
2015-05-04 21:37 ` [PATCH 0/4] "hash-object --literally" fixes Junio C Hamano
2015-05-04 21:37 ` [PATCH 1/4] git-hash-object.txt: document --literally option Junio C Hamano
2015-05-04 21:37 ` [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type Junio C Hamano
2015-05-05 0:13 ` Eric Sunshine
2015-05-05 0:28 ` Junio C Hamano [this message]
2015-05-05 17:30 ` Junio C Hamano
2015-05-05 18:49 ` Eric Sunshine
2015-05-04 21:37 ` [PATCH 3/4] t1007: add hash-object --literally tests Junio C Hamano
2015-05-04 21:37 ` [PATCH 4/4] write_sha1_file(): do not use a separate sha1[] array Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq4mnrg94p.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox