On Mon, Mar 20, 2017 at 08:07:09PM +0700, Duy Nguyen wrote: > On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson > <sandals@xxxxxxxxxxxxxxxxxxxx> wrote: > > @@ -332,7 +332,7 @@ static int init_object_disambiguation(const char *name, int len, > > ds->hex_pfx[i] = c; > > if (!(i & 1)) > > val <<= 4; > > - ds->bin_pfx[i >> 1] |= val; > > + ds->bin_pfx.hash[i >> 1] |= val; > > The indexing makes me a bit nervous, especially since diff context > here is too narrow to see. It would be nice if this code (at the > beginning of init_object_disambiguation) is converted here too > > if (len < MINIMUM_ABBREV || len > GIT_SHA1_HEXSZ) > return -1; Well, I think that's the way I would have written that text at the top of the function. I expect that we'll end up turning GIT_SHA1_HEXSZ into a global named something like current_hash_len via global search-and-replace, so it will always be the right length. The indexing should be safe because len is guaranteed to be sufficiently small, and I feel like it we would have seen it break by now if it had had an overflow. i will always be in the range [0, 40) (for SHA-1), so i >> 1 should always be in [0, 20). Am I understanding you correctly and if so, does that assuage your concerns, or did you mean something else? -- 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:
signature.asc
Description: PGP signature