Re: [PATCH] sha1_name: cache readdir(3) results in find_short_object_filename()
On Sat, Jun 24, 2017 at 04:09:39PM +0200, René Scharfe wrote:
Show 18 quoted lines
> Am 24.06.2017 um 14:20 schrieb Jeff King:
> > On Sat, Jun 24, 2017 at 02:12:30PM +0200, René Scharfe wrote:
> >
> >>> That's redundant with "subdir_nr". Should we push that logic down into
> >>> the function, and basically do:
> >>>
> >>> if (subdir_nr < 0 || subdir_nr > 256)
> >>> BUG("object subdir number out of range");
> >>
> >> Hmm. I don't expect many more callers, so do we really need this safety
> >> check? It's cheap compared to the readdir(3) call, of course.
> >
> > To me it's as much about documenting the assumptions as it is about
> > catching buggy callers. I'd be OK with a comment, too.
>
> I didn't catch the off-by-one error above in the first reading. Did you
> add it intentionally? ;-) In any case, I'm convinced now that we need
> such a check, but with hexadecimal notation to avoid such mistakes.Err...yeah, it was totally intentional. ;)
I agree writing it in hex is better.
Show 11 quoted lines
> -- >8 --
> Subject: [PATCH] sha1_file: guard against invalid loose subdirectory numbers
>
> Loose object subdirectories have hexadecimal names based on the first
> byte of the hash of contained objects, thus their numerical
> representation can range from 0 (0x00) to 255 (0xff). Change the type
> of the corresponding variable in for_each_file_in_obj_subdir() and
> associated callback functions to unsigned int and add a range check.
>
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
This looks great. Thanks.
-Peff
Jeff King <peff@peff.net>