On Tue, Mar 21, 2017 at 5:32 AM, brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx> wrote: > 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? There's a disconnect between object_id (which goes with GIT_MAX_RAWSZ) and the code here which still checks upper bound as GIT_SHA1_HEXSZ. But I guess eventually GIT_SHA1_HEXSZ will be undefined and gone. This is just a temporary state. So forget about my paranoid comment. All good. -- Duy