Re: [PATCH] sha1_name: cache readdir(3) results in find_short_object_filename()
- From
René Scharfe <l.s.r@web.de>
- Date
- Jun 24, 2017, 14:09 UTC
- Message-ID
- <03dc365d-41f7-dbe7-3c22-bdeee77a817c@web.de>
- In-Reply-To
- <20170624122057.ndmlsa7e7dksejg3@sigill.intra.peff.net>
Am 24.06.2017 um 14:20 schrieb Jeff King:
Show 13 quoted lines
> 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.
Show 8 quoted lines
>> But wouldn't it make more sense to use an unsigned data type to avoid >> the first half? And an unsigned char specifically to only accept valid >> values, leaving the overflow concern to the caller? It warrants a >> separate patch in any case IMHO. > > Using unsigned makes sense. Using "char" because you know it only goes > to 256 is a bit too subtle for my taste. And yes, I'd do it in a > preparatory patch (or follow-on if you prefer).
It's subtle on the caller's side, as big numbers would just wrap if the programmer ignored the limits of the type. On the callee's side it's fundamental, though.
Anyway, here's a patch on top of the others.
-- >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> --- builtin/fsck.c | 2 +- builtin/prune-packed.c | 2 +- builtin/prune.c | 2 +- cache.h | 4 ++-- sha1_file.c | 5 ++++- 5 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c index 3a2c27f241..5d113f8bc0 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -536,7 +536,7 @@ static int fsck_cruft(const char *basename, const char *path, void *data) return 0; } -static int fsck_subdir(int nr, const char *path, void *progress) +static int fsck_subdir(unsigned int nr, const char *path, void *progress) { display_progress(progress, nr + 1); return 0; diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c index c026299e78..ac978ad401 100644 --- a/builtin/prune-packed.c +++ b/builtin/prune-packed.c @@ -10,7 +10,7 @@ static const char * const prune_packed_usage[] = { static struct progress *progress; -static int prune_subdir(int nr, const char *path, void *data) +static int prune_subdir(unsigned int nr, const char *path, void *data) { int *opts = data; display_progress(progress, nr + 1); diff --git a/builtin/prune.c b/builtin/prune.c index f0e2bff04c..c378690545 100644 --- a/builtin/prune.c +++ b/builtin/prune.c @@ -68,7 +68,7 @@ static int prune_cruft(const char *basename, const char *path, void *data) return 0; } -static int prune_subdir(int nr, const char *path, void *data) +static int prune_subdir(unsigned int nr, const char *path, void *data) { if (!show_only) rmdir(path); diff --git a/cache.h b/cache.h index 00a017dfcb..04dd2961ad 100644 --- a/cache.h +++ b/cache.h @@ -1819,10 +1819,10 @@ typedef int each_loose_object_fn(const struct object_id *oid, typedef int each_loose_cruft_fn(const char *basename, const char *path, void *data); -typedef int each_loose_subdir_fn(int nr, +typedef int each_loose_subdir_fn(unsigned int nr, const char *path, void *data); -int for_each_file_in_obj_subdir(int subdir_nr, +int for_each_file_in_obj_subdir(unsigned int subdir_nr, struct strbuf *path, each_loose_object_fn obj_cb, each_loose_cruft_fn cruft_cb, diff --git a/sha1_file.c b/sha1_file.c index 98ce85acf9..90b9414170 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -3735,7 +3735,7 @@ void assert_sha1_type(const unsigned char *sha1, enum object_type expect) typename(expect)); } -int for_each_file_in_obj_subdir(int subdir_nr, +int for_each_file_in_obj_subdir(unsigned int subdir_nr, struct strbuf *path, each_loose_object_fn obj_cb, each_loose_cruft_fn cruft_cb, @@ -3747,6 +3747,9 @@ int for_each_file_in_obj_subdir(int subdir_nr, struct dirent *de; int r = 0; + if (subdir_nr > 0xff) + BUG("invalid loose object subdirectory: %x", subdir_nr); + origlen = path->len; strbuf_complete(path, '/'); strbuf_addf(path, "%02x", subdir_nr);
-- 2.13.1