git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Duy Nguyen <pclouds@gmail.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>,
	Duy Nguyen <pclouds@gmail.com>,
	Git Mailing List <git@vger.kernel.org>, Jeff King <peff@peff.net>
Subject: Re: [PATCH 06/20] builtin/receive-pack: convert portions to struct object_id
Date: Tue, 21 Mar 2017 17:26:16 +0700
Message-ID: <CACsJy8Drb8vuL2mqk_NC4w8Z78bx_eDREyG3Xo0KYWXispi3rA@mail.gmail.com> (raw)
In-Reply-To: <20170320231752.gzo2uk6d2qob5ovk@genre.crustytoothpaste.net>

On Tue, Mar 21, 2017 at 6:17 AM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On Mon, Mar 20, 2017 at 07:56:17PM +0700, Duy Nguyen wrote:
>> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
>> <sandals@crustytoothpaste.net> wrote:
>> > @@ -1489,23 +1489,24 @@ static struct command **queue_command(struct command **tail,
>> >                                       const char *line,
>> >                                       int linelen)
>> >  {
>> > -       unsigned char old_sha1[20], new_sha1[20];
>> > +       struct object_id old_oid, new_oid;
>> >         struct command *cmd;
>> >         const char *refname;
>> >         int reflen;
>> > +       const char *p;
>> >
>> > -       if (linelen < 83 ||
>> > -           line[40] != ' ' ||
>> > -           line[81] != ' ' ||
>> > -           get_sha1_hex(line, old_sha1) ||
>> > -           get_sha1_hex(line + 41, new_sha1))
>> > +       if (!linelen ||
>>
>> I think you can skip this. The old code needed "< 83" because of the
>> random accesses to [40] and [81] but you don't do that anymore.
>> parse_oid_hex() can handle empty hex strings fine.
>
> I just realized this looks fishy:
>
>         while (boc < eoc) {
>                 const char *eol = memchr(boc, '\n', eoc - boc);
>                 tail = queue_command(tail, boc, eol ? eol - boc : eoc - eol);
>                 boc = eol ? eol + 1 : eoc;
>         }
>
> If eol is NULL, we subtract it from eoc?  I mean, eol will be zero, so
> we get eoc, which seems like what we want.  I think I'm going to send in
> a separate patch to fix that, because clearly that's bizarre and not at
> all compliant with the C standard.

Eck.. Good eyes!

>> > +           parse_oid_hex(line, &old_oid, &p) ||
>> > +           *p++ != ' ' ||
>> > +           parse_oid_hex(p, &new_oid, &p) ||
>> > +           *p++ != ' ')
>>
>> maybe "|| *p)" as well? I think the old code, with "linelen < 83",
>> makes sure reflen is at least one. Not sure what FLEX_ALLOC_MEM would
>> do if reflen is zero.
>
> I don't think that line is actually guaranteed to be NUL-terminated.  It
> may be terminated instead by a newline, such as by
> queue_commands_from_cert.
>
> If we did get an empty reflen, we'd call xcalloc, where we will allocate
> exactly the size of the struct otherwise.  We'd then try to memcpy zero
> bytes into that location, and succeed.

Actually I think we allocate an extra byte for NUL as well, so
cmd->ref_name is still valid (as an empty string). Yes we should be
fine.
-- 
Duy

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
2017-03-18 21:19 ` [PATCH 01/20] Define new hash-size constants for allocating memory brian m. carlson
2017-03-18 21:19 ` [PATCH 02/20] Convert GIT_SHA1_HEXSZ used for allocation to GIT_MAX_HEXSZ brian m. carlson
2017-03-18 21:19 ` [PATCH 03/20] Convert GIT_SHA1_RAWSZ used for allocation to GIT_MAX_RAWSZ brian m. carlson
2017-03-18 21:19 ` [PATCH 04/20] builtin/diff: convert to struct object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 05/20] builtin/pull: convert portions " brian m. carlson
2017-03-18 21:19 ` [PATCH 06/20] builtin/receive-pack: " brian m. carlson
2017-03-20 12:56   ` Duy Nguyen
2017-03-20 23:17     ` brian m. carlson
2017-03-21 10:26       ` Duy Nguyen [this message]
2017-03-18 21:19 ` [PATCH 07/20] fsck: convert init_skiplist " brian m. carlson
2017-03-18 21:19 ` [PATCH 08/20] parse-options-cb: convert sha1_array_append caller " brian m. carlson
2017-03-18 21:19 ` [PATCH 09/20] test-sha1-array: convert most code " brian m. carlson
2017-03-18 21:19 ` [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id brian m. carlson
2017-03-20 13:07   ` Duy Nguyen
2017-03-20 22:32     ` brian m. carlson
2017-03-21 10:17       ` Duy Nguyen
2017-03-18 21:19 ` [PATCH 11/20] sha1_name: convert disambiguate_hint_fn to take object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 12/20] submodule: convert check_for_new_submodule_commits to object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 13/20] builtin/pull: convert to struct object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 14/20] sha1-array: convert internal storage for struct sha1_array to object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 15/20] Make sha1_array_append take a struct object_id * brian m. carlson
2017-03-18 21:19 ` [PATCH 16/20] Convert remaining callers of sha1_array_lookup to object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 17/20] Convert sha1_array_lookup to take struct object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 18/20] Convert sha1_array_for_each_unique and for_each_abbrev to object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 19/20] Rename sha1_array to oid_array brian m. carlson
2017-03-20 12:25   ` Duy Nguyen
2017-03-21  0:54     ` brian m. carlson
2017-03-18 21:19 ` [PATCH 20/20] Documentation: update and rename api-sha1-array.txt brian m. carlson
2017-03-20 13:14 ` [PATCH 00/20] object_id part 7 Duy Nguyen
2017-03-21  1:16   ` brian m. carlson

Reply instructions:

You may reply publically 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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACsJy8Drb8vuL2mqk_NC4w8Z78bx_eDREyG3Xo0KYWXispi3rA@mail.gmail.com \
    --to=pclouds@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox