On Tue, Jul 15, 2025 at 01:28:28PM +0200, Karthik Nayak wrote:
> +static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
> + const char *refname, unsigned int flags)
> [...]
> + do {
> + int len, idx;
> + int cmp = 0;
> +
> + sort_ref_dir(dir);
> +
> + slash = strchr(slash, '/');
> + len = slash ? slash - refname : (int)strlen(refname);
I was looking at this code due to a nearby thread and noticed this funny
cast to int. I guess you added it to silence -Wsign-compare, but Why are
we not using a size_t in the first place?
This kind of conversion can sometimes have security implications because
a very large "refname" would cause "len" to become negative (i.e., if
it's between 2GB and 4GB).
In this particular case it ends up cast back to a size_t via strncmp:
> + for (idx = 0; idx < dir->nr; idx++) {
> + cmp = strncmp(refname, dir->entries[idx]->name, len);
> + if (cmp <= 0)
> + break;
> + }
so we get the original value back. We'd still get truncation for a
refname value over 4GB, which would presumably give us a slightly wrong
answer. But I don't think we'd ever look outside the array.
Such sizes are probably unlikely if we are feeding filesystem paths. But
we probably should not set a bad example, and just do;
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index 1d95b56d40..3949d145e8 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -498,13 +498,14 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
* indexing to each level as needed.
*/
do {
- int len, idx;
+ size_t len;
+ int idx;
int cmp = 0;
sort_ref_dir(dir);
slash = strchr(slash, '/');
- len = slash ? slash - refname : (int)strlen(refname);
+ len = slash ? slash - refname : strlen(refname);
for (idx = 0; idx < dir->nr; idx++) {
cmp = strncmp(refname, dir->entries[idx]->name, len);
-Peff
