git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: "brian m. carlson" <sandals@crustytoothpaste.net>
To: Duy Nguyen <pclouds@gmail.com>
Cc: 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: Mon, 20 Mar 2017 23:17:53 +0000
Message-ID: <20170320231752.gzo2uk6d2qob5ovk@genre.crustytoothpaste.net> (raw)
In-Reply-To: <CACsJy8Ch0=ovmmfHikQG04K249TEpA3HONWpknUn_kWbQa=8Fw@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2666 bytes --]

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.

Going back to linelen, I think it's probably safe to remove, since even
if *boc is a newline (and we get an empty linelen), we're still
guaranteed to have another character, since this is a strbuf.  The
amount of double-checking I had to do there makes me nervous, though.

I'll squash in a change.

> > +           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.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

  reply index

Thread overview: 20+ 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 05/20] builtin/pull: convert portions to struct object_id 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 [this message]
2017-03-21 10:26       ` Duy Nguyen
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 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-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=20170320231752.gzo2uk6d2qob5ovk@genre.crustytoothpaste.net \
    --to=sandals@crustytoothpaste.net \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.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