On Tue, Mar 21, 2017 at 6:17 AM, brian m. carlson
<sandals@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> 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