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

> 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.

>> > +           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.