Re: [PATCH 06/20] builtin/receive-pack: convert portions to struct object_id

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- 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 --]