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