{"thread":{"id":"64197","subject":"[PATCH] refs/ref-cache: stop seeking into empty directories","startedAt":"2025-09-24T07:56:13Z","lastAt":"2025-10-01T12:17:34Z","messageCount":10,"participants":["Karthik Nayak","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"527194","messageId":"20250924-583-git-for-each-ref-start-after-v1-1-c73be2b5db5a@gmail.com","threadId":"64197","inReplyTo":null,"subject":"[PATCH] refs/ref-cache: stop seeking into empty directories","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-24T07:55:51Z","receivedAt":"2025-09-24T07:56:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The 'cache_ref_iterator_seek()' function is used to seek the\n`ref_iterator` to the desired reference in the ref-cache mechanism. We\nuse the seeking functionality to implement the '--start-after' flag in\n'git-for-each-ref(1)'.\n\nWhen using the files-backend with packed-refs, it is possible that some\nof the refs directories are empty. For e.g. just after repacking, the\n'refs/heads' directory would be empty. The ref-cache seek mechanism\ndoesn't take this into consideration, causing SEGFAULT as we try to\naccess entries within the directory. Fix this by breaking out of the\nloop when we enter an empty directory.\n\nAdd tests which simulate this behavior and also provide coverage over\nusing the feature over packed-refs.\n\nSigned-off-by: Karthik Nayak \n---\n refs/ref-cache.c | 3 ++\n t/t6302-for-each-ref-filter.sh | 65 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 68 insertions(+)\n\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex c180e0aad7..8a260028ec 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -507,6 +507,9 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,\n \t\t\tslash = strchr(slash, '/');\n \t\t\tlen = slash ? (size_t)(slash - refname) : strlen(refname);\n \n+\t\t\tif (dir->nr == 0)\n+\t\t\t\tbreak;\n+\n \t\t\tfor (idx = 0; idx < dir->nr; idx++) {\n \t\t\t\tcmp = strncmp(refname, dir->entries[idx]->name, len);\n \t\t\t\tif (cmp <= 0)\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 9b80ea1e3b..d14567cb62 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -754,4 +754,69 @@ test_expect_success 'start after used with custom sort order' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'start after with packed refs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit default &&\n+\n+\t\tgit update-ref --stdin <<-EOF &&\n+\t\tcreate refs/heads/branch @\n+\t\tcreate refs/heads/side @\n+\t\tcreate refs/odd/spot @\n+\t\tcreate refs/tags/one @\n+\t\tcreate refs/tags/two @\n+\t\tcommit\n+\t\tEOF\n+\n+\t\tcat >expect <<-\\EOF &&\n+\t\trefs/tags/default\n+\t\trefs/tags/one\n+\t\trefs/tags/two\n+\t\tEOF\n+\n+\t\tgit pack-refs --all &&\n+\t\tgit for-each-ref --format=\"%(refname)\" --start-after=refs/odd/spot >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'start after with packed refs and some loose refs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit default &&\n+\n+\t\tgit update-ref --stdin <<-EOF &&\n+\t\tcreate refs/heads/branch @\n+\t\tcreate refs/heads/side @\n+\t\tcreate refs/odd/spot @\n+\t\tcreate refs/tags/one @\n+\t\tcreate refs/tags/two @\n+\t\tcommit\n+\t\tEOF\n+\n+\t\tgit pack-refs --all &&\n+\n+\t\tgit update-ref --stdin <<-EOF &&\n+\t\tcreate refs/heads/foo @\n+\t\tcreate refs/odd/tee @\n+\t\tcommit\n+\t\tEOF\n+\n+\t\tcat >expect <<-\\EOF &&\n+\t\trefs/odd/tee\n+\t\trefs/tags/default\n+\t\trefs/tags/one\n+\t\trefs/tags/two\n+\t\tEOF\n+\n+\n+\t\tgit for-each-ref --format=\"%(refname)\" --start-after=refs/odd/spot >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n\n\n\n"},{"id":"527207","messageId":"aNPkA9UZtEqHsa3f@pks.im","threadId":"64197","inReplyTo":"20250924-583-git-for-each-ref-start-after-v1-1-c73be2b5db5a@gmail.com","subject":"Re: [PATCH] refs/ref-cache: stop seeking into empty directories","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-24T12:28:51Z","receivedAt":"2025-09-24T12:28:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 24, 2025 at 09:55:51AM +0200, Karthik Nayak wrote:\n> The 'cache_ref_iterator_seek()' function is used to seek the\n> `ref_iterator` to the desired reference in the ref-cache mechanism. We\n> use the seeking functionality to implement the '--start-after' flag in\n> 'git-for-each-ref(1)'.\n> \n> When using the files-backend with packed-refs, it is possible that some\n> of the refs directories are empty. For e.g. just after repacking, the\n> 'refs/heads' directory would be empty. The ref-cache seek mechanism\n> doesn't take this into consideration, causing SEGFAULT as we try to\n> access entries within the directory.\n\nWhy do we even try to access any entry in an empty directory? We have\n`dir->nr`, so shouldn't we check that `idx < dir->nr` every time we try\nto deref an entry?\n\nIn other words, I wonder whether we fix a symptom of a missing bounds\ncheck instead of addressing that missing bounds check as the root cause\ndirectly.\n\n> Fix this by breaking out of the\n> loop when we enter an empty directory.\n> \n> Add tests which simulate this behavior and also provide coverage over\n> using the feature over packed-refs.\n\nNit: the commit subject might be adjusted to mention the symptom we're\nfixing rather than what we're doing in the commit. At least, it feels\nlike the segfault is the more important thing to talk about here.\n\n> Signed-off-by: Karthik Nayak \n> ---\n> refs/ref-cache.c | 3 ++\n> t/t6302-for-each-ref-filter.sh | 65 ++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 68 insertions(+)\n> \n> diff --git a/refs/ref-cache.c b/refs/ref-cache.c\n> index c180e0aad7..8a260028ec 100644\n> --- a/refs/ref-cache.c\n> +++ b/refs/ref-cache.c\n> @@ -507,6 +507,9 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,\n> \t\t\tslash = strchr(slash, '/');\n> \t\t\tlen = slash ? (size_t)(slash - refname) : strlen(refname);\n> \n> +\t\t\tif (dir->nr == 0)\n> +\t\t\t\tbreak;\n\nCan't we break before sorting already? Avoids a couple of no-op changes.\n\n> \t\t\tfor (idx = 0; idx < dir->nr; idx++) {\n> \t\t\t\tcmp = strncmp(refname, dir->entries[idx]->name, len);\n> \t\t\t\tif (cmp <= 0)\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index 9b80ea1e3b..d14567cb62 100755\n> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -754,4 +754,69 @@ test_expect_success 'start after used with custom sort order' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'start after with packed refs' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit default &&\n> +\n> +\t\tgit update-ref --stdin <<-EOF &&\n\ns/EOF/\\EOF/\n\nThe same is true for the other test.\n\nPatrick\n"},{"id":"527302","messageId":"CAOLa=ZS=y5Q-_BH+q2xhzh6mpg2oQJptv1rp_T0jdqH-jBf0+g@mail.gmail.com","threadId":"64197","inReplyTo":"aNPkA9UZtEqHsa3f@pks.im","subject":"Re: [PATCH] refs/ref-cache: stop seeking into empty directories","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-25T08:20:11Z","receivedAt":"2025-09-25T08:20:15Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Wed, Sep 24, 2025 at 09:55:51AM +0200, Karthik Nayak wrote:\n>> The 'cache_ref_iterator_seek()' function is used to seek the\n>> `ref_iterator` to the desired reference in the ref-cache mechanism. We\n>> use the seeking functionality to implement the '--start-after' flag in\n>> 'git-for-each-ref(1)'.\n>>\n>> When using the files-backend with packed-refs, it is possible that some\n>> of the refs directories are empty. For e.g. just after repacking, the\n>> 'refs/heads' directory would be empty. The ref-cache seek mechanism\n>> doesn't take this into consideration, causing SEGFAULT as we try to\n>> access entries within the directory.\n>\n> Why do we even try to access any entry in an empty directory? We have\n> `dir->nr`, so shouldn't we check that `idx < dir->nr` every time we try\n> to deref an entry?\n>\n> In other words, I wonder whether we fix a symptom of a missing bounds\n> check instead of addressing that missing bounds check as the root cause\n> directly.\n>\n\nWe do a bounds check to ensure that we don't overflow the entries in a\ndirectory with `idx = idx >= dir->nr ? dir->nr - 1 : idx;`.\n\nThe issue is that when `dir->nr = 0`, this will set the `idx = -1`. So\nthis is where it fails.\n\nTaking a step back, the while loop shouldn't even operate when we hit a\ndirectory with no entries. So I'll modify the loop condition to include\na check for `dir->nr`.\n\n>> Fix this by breaking out of the\n>> loop when we enter an empty directory.\n>>\n>> Add tests which simulate this behavior and also provide coverage over\n>> using the feature over packed-refs.\n>\n> Nit: the commit subject might be adjusted to mention the symptom we're\n> fixing rather than what we're doing in the commit. At least, it feels\n> like the segfault is the more important thing to talk about here.\n>\n\nThat's fair, let me modify that.\n\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> refs/ref-cache.c | 3 ++\n>> t/t6302-for-each-ref-filter.sh | 65 ++++++++++++++++++++++++++++++++++++++++++\n>> 2 files changed, 68 insertions(+)\n>>\n>> diff --git a/refs/ref-cache.c b/refs/ref-cache.c\n>> index c180e0aad7..8a260028ec 100644\n>> --- a/refs/ref-cache.c\n>> +++ b/refs/ref-cache.c\n>> @@ -507,6 +507,9 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,\n>> \t\t\tslash = strchr(slash, '/');\n>> \t\t\tlen = slash ? (size_t)(slash - refname) : strlen(refname);\n>>\n>> +\t\t\tif (dir->nr == 0)\n>> +\t\t\t\tbreak;\n>\n> Can't we break before sorting already? Avoids a couple of no-op changes.\n>\n\nYup, in the next iteration, I'll move it to the loop condition, so we\ndon't even enter the loop.\n\n>> \t\t\tfor (idx = 0; idx < dir->nr; idx++) {\n>> \t\t\t\tcmp = strncmp(refname, dir->entries[idx]->name, len);\n>> \t\t\t\tif (cmp <= 0)\n>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n>> index 9b80ea1e3b..d14567cb62 100755\n>> --- a/t/t6302-for-each-ref-filter.sh\n>> +++ b/t/t6302-for-each-ref-filter.sh\n>> @@ -754,4 +754,69 @@ test_expect_success 'start after used with custom sort order' '\n>> \ttest_cmp expect actual\n>> '\n>>\n>> +test_expect_success 'start after with packed refs' '\n>> +\ttest_when_finished \"rm -rf repo\" &&\n>> +\tgit init repo &&\n>> +\t(\n>> +\t\tcd repo &&\n>> +\t\ttest_commit default &&\n>> +\n>> +\t\tgit update-ref --stdin <<-EOF &&\n>\n> s/EOF/\\EOF/\n>\n> The same is true for the other test.\n>\n> Patrick\n\nThis always trips me up. Will fix.\n\nThanks for the review.\n"},{"id":"527303","messageId":"20250925-583-git-for-each-ref-start-after-v2-1-3613b5a27ff1@gmail.com","threadId":"64197","inReplyTo":"20250924-583-git-for-each-ref-start-after-v1-1-c73be2b5db5a@gmail.com","subject":"[PATCH v2] refs/ref-cache: fix SEGFAULT when seeking in empty directories","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-25T09:15:07Z","receivedAt":"2025-09-25T09:15:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The 'cache_ref_iterator_seek()' function is used to seek the\n`ref_iterator` to the desired reference in the ref-cache mechanism. We\nuse the seeking functionality to implement the '--start-after' flag in\n'git-for-each-ref(1)'.\n\nWhen using the files-backend with packed-refs, it is possible that some\nof the refs directories are empty. For e.g. just after repacking, the\n'refs/heads' directory would be empty. The ref-cache seek mechanism\ndoesn't take this into consideration, causing SEGFAULT as we try to\naccess entries within the directory. Fix this by breaking out of the\nloop when we enter an empty directory.\n\nAdd tests which simulate this behavior and also provide coverage over\nusing the feature over packed-refs.\n\nSigned-off-by: Karthik Nayak \n---\nChanges in v2:\n- Moved the `dir-nr` check to the loop to provide better bound checks.\n- Modified the commit subject to talk about the issue at hand.\n- Substituted EOF with \\EOF since we don't do any variable parsing.\n- Link to v1: https://lore.kernel.org/r/20250924-583-git-for-each-ref-start-after-v1-1-c73be2b5db5a@gmail.com\n---\n refs/ref-cache.c | 2 +-\n t/t6302-for-each-ref-filter.sh | 65 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex c180e0aad7..e5e5df16d8 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -539,7 +539,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,\n \t\t\t\t */\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t} while (slash);\n+\t\t} while (slash && dir->nr);\n \t}\n \n \treturn 0;\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 9b80ea1e3b..7f060d97bf 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -754,4 +754,69 @@ test_expect_success 'start after used with custom sort order' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'start after with packed refs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit default &&\n+\n+\t\tgit update-ref --stdin <<-\\EOF &&\n+\t\tcreate refs/heads/branch @\n+\t\tcreate refs/heads/side @\n+\t\tcreate refs/odd/spot @\n+\t\tcreate refs/tags/one @\n+\t\tcreate refs/tags/two @\n+\t\tcommit\n+\t\tEOF\n+\n+\t\tcat >expect <<-\\EOF &&\n+\t\trefs/tags/default\n+\t\trefs/tags/one\n+\t\trefs/tags/two\n+\t\tEOF\n+\n+\t\tgit pack-refs --all &&\n+\t\tgit for-each-ref --format=\"%(refname)\" --start-after=refs/odd/spot >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'start after with packed refs and some loose refs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit default &&\n+\n+\t\tgit update-ref --stdin <<-\\EOF &&\n+\t\tcreate refs/heads/branch @\n+\t\tcreate refs/heads/side @\n+\t\tcreate refs/odd/spot @\n+\t\tcreate refs/tags/one @\n+\t\tcreate refs/tags/two @\n+\t\tcommit\n+\t\tEOF\n+\n+\t\tgit pack-refs --all &&\n+\n+\t\tgit update-ref --stdin <<-\\EOF &&\n+\t\tcreate refs/heads/foo @\n+\t\tcreate refs/odd/tee @\n+\t\tcommit\n+\t\tEOF\n+\n+\t\tcat >expect <<-\\EOF &&\n+\t\trefs/odd/tee\n+\t\trefs/tags/default\n+\t\trefs/tags/one\n+\t\trefs/tags/two\n+\t\tEOF\n+\n+\n+\t\tgit for-each-ref --format=\"%(refname)\" --start-after=refs/odd/spot >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n\n\n\n"},{"id":"527336","messageId":"xmqqbjmyh353.fsf@gitster.g","threadId":"64197","inReplyTo":"20250925-583-git-for-each-ref-start-after-v2-1-3613b5a27ff1@gmail.com","subject":"Re: [PATCH v2] refs/ref-cache: fix SEGFAULT when seeking in empty directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-25T17:07:36Z","receivedAt":"2025-09-25T17:07:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> diff --git a/refs/ref-cache.c b/refs/ref-cache.c\n> index c180e0aad7..e5e5df16d8 100644\n> --- a/refs/ref-cache.c\n> +++ b/refs/ref-cache.c\n> @@ -539,7 +539,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,\n> \t\t\t\t */\n> \t\t\t\tbreak;\n> \t\t\t}\n> -\t\t} while (slash);\n> +\t\t} while (slash && dir->nr);\n> \t}\n\nThis is at the tail of a \"do { ... } while (...);\" loop, but inside\nthe loop I see this construct:\n\n\t\t\tfor (idx = 0; idx < dir->nr; idx++) {\n\t\t\t\tcmp = strncmp(refname, dir->entries[idx]->name, len);\n\t\t\t\tif (cmp <= 0)\n\t\t\t\t\tbreak;\n\t\t\t}\n\t\t\t/* don't overflow the index */\n\t\t\tidx = idx >= dir->nr ? dir->nr - 1 : idx;\n\ni.e., if we scan all the dir->entries[] elements in the innter loop\nand did not find any hit, idx would become dir->nr and this inner\nloop runs to the end. If (dir->nr == 0), then ?: operator [*] would\nbecome the idx = (dir->nr - 1); And that idx is used for a while\nbefore we get to this \"while (slash && dir->nr)\".\n\nAnd then tha tis used like this.\n\n\t\t\tif (slash)\n\t\t\t\tslash = slash + 1;\n\n\t\t\tlevel->index = idx;\n\t\t\tif (dir->entries[idx]->flag & REF_DIR) {\n\t\t\t\t...\n\nIOW, isn't this check a bit too late? I wonder if we can leave at\nthe beginning of the outer loop, even before sort_ref_dir(dir), when\ndir->nr is zero, or something?\n\n\n\n[Side note]\n\n * I found the problematic ?: extremely hard to read, given its\n contrast with the terminating condnition of for loop. If you\n apply the discipline to keep the textual order match the actual\n order, i.e.\n\n\tfor (idx = 0; idx < dir->nr; idx++)\n\t\t...;\n\n\tidx = dir->nr <= idx ? dir->nr - 1 : idx;\n\n people would have spotted this more easily.\n\n But perhaps that may be just me. Anyway...\n"},{"id":"527403","messageId":"CAOLa=ZSfL1CVCNjD0Vi3kpQqLM6t7YVa=G2ChGpaza6WOjZTCw@mail.gmail.com","threadId":"64197","inReplyTo":"xmqqbjmyh353.fsf@gitster.g","subject":"Re: [PATCH v2] refs/ref-cache: fix SEGFAULT when seeking in empty directories","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-26T07:41:58Z","receivedAt":"2025-09-26T07:42:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> diff --git a/refs/ref-cache.c b/refs/ref-cache.c\n>> index c180e0aad7..e5e5df16d8 100644\n>> --- a/refs/ref-cache.c\n>> +++ b/refs/ref-cache.c\n>> @@ -539,7 +539,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,\n>> \t\t\t\t */\n>> \t\t\t\tbreak;\n>> \t\t\t}\n>> -\t\t} while (slash);\n>> +\t\t} while (slash && dir->nr);\n>> \t}\n>\n> This is at the tail of a \"do { ... } while (...);\" loop, but inside\n> the loop I see this construct:\n>\n> \t\t\tfor (idx = 0; idx < dir->nr; idx++) {\n> \t\t\t\tcmp = strncmp(refname, dir->entries[idx]->name, len);\n> \t\t\t\tif (cmp <= 0)\n> \t\t\t\t\tbreak;\n> \t\t\t}\n> \t\t\t/* don't overflow the index */\n> \t\t\tidx = idx >= dir->nr ? dir->nr - 1 : idx;\n>\n> i.e., if we scan all the dir->entries[] elements in the innter loop\n> and did not find any hit, idx would become dir->nr and this inner\n> loop runs to the end. If (dir->nr == 0), then ?: operator [*] would\n> become the idx = (dir->nr - 1); And that idx is used for a while\n> before we get to this \"while (slash && dir->nr)\".\n>\n\nThis wouldn't happen because before the loop starts, we set:\n\n dir = get_ref_dir(iter->cache->root);\n\nSo, `dir` will always have an entry (i.e. 'refs') in the first\niteration, after that, the checks in the while section of the 'do {}\nwhile (...)' loop will kick in.\n\n> And then tha tis used like this.\n>\n> \t\t\tif (slash)\n> \t\t\t\tslash = slash + 1;\n>\n> \t\t\tlevel->index = idx;\n> \t\t\tif (dir->entries[idx]->flag & REF_DIR) {\n> \t\t\t\t...\n>\n> IOW, isn't this check a bit too late? I wonder if we can leave at\n> the beginning of the outer loop, even before sort_ref_dir(dir), when\n> dir->nr is zero, or something?\n>\n>\n\nWe could add it there too. I can't see the merit of one over the other.\nBut if you see it being more readable. I'll happily make that change.\n\nAs the author of this code, I do find it complex already.\n\n>\n> [Side note]\n>\n> * I found the problematic ?: extremely hard to read, given its\n> contrast with the terminating condnition of for loop. If you\n> apply the discipline to keep the textual order match the actual\n> order, i.e.\n>\n> \tfor (idx = 0; idx < dir->nr; idx++)\n> \t\t...;\n>\n> \tidx = dir->nr <= idx ? dir->nr - 1 : idx;\n>\n> people would have spotted this more easily.\n>\n> But perhaps that may be just me. Anyway...\n\nI'm not really sold on this being easier to read. But that could be me,\nSo I'll make a second commit to make this change, we can drop it if\nunwanted.\n"},{"id":"527404","messageId":"CAOLa=ZSrzq0yeLJpMMg4Jf2vWkRC-9D9=9hxHveK+=F+ZF2L+w@mail.gmail.com","threadId":"64197","inReplyTo":"CAOLa=ZSfL1CVCNjD0Vi3kpQqLM6t7YVa=G2ChGpaza6WOjZTCw@mail.gmail.com","subject":"Re: [PATCH v2] refs/ref-cache: fix SEGFAULT when seeking in empty directories","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-26T07:44:13Z","receivedAt":"2025-09-26T07:44:17Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Karthik Nayak writes:\n\n> Junio C Hamano writes:\n>\n>> Karthik Nayak writes:\n>>\n>>> diff --git a/refs/ref-cache.c b/refs/ref-cache.c\n>>> index c180e0aad7..e5e5df16d8 100644\n>>> --- a/refs/ref-cache.c\n>>> +++ b/refs/ref-cache.c\n>>> @@ -539,7 +539,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,\n>>> \t\t\t\t */\n>>> \t\t\t\tbreak;\n>>> \t\t\t}\n>>> -\t\t} while (slash);\n>>> +\t\t} while (slash && dir->nr);\n>>> \t}\n>>\n>> This is at the tail of a \"do { ... } while (...);\" loop, but inside\n>> the loop I see this construct:\n>>\n>> \t\t\tfor (idx = 0; idx < dir->nr; idx++) {\n>> \t\t\t\tcmp = strncmp(refname, dir->entries[idx]->name, len);\n>> \t\t\t\tif (cmp <= 0)\n>> \t\t\t\t\tbreak;\n>> \t\t\t}\n>> \t\t\t/* don't overflow the index */\n>> \t\t\tidx = idx >= dir->nr ? dir->nr - 1 : idx;\n>>\n>> i.e., if we scan all the dir->entries[] elements in the innter loop\n>> and did not find any hit, idx would become dir->nr and this inner\n>> loop runs to the end. If (dir->nr == 0), then ?: operator [*] would\n>> become the idx = (dir->nr - 1); And that idx is used for a while\n>> before we get to this \"while (slash && dir->nr)\".\n>>\n>\n> This wouldn't happen because before the loop starts, we set:\n>\n> dir = get_ref_dir(iter->cache->root);\n>\n> So, `dir` will always have an entry (i.e. 'refs') in the first\n> iteration, after that, the checks in the while section of the 'do {}\n> while (...)' loop will kick in.\n>\n>> And then tha tis used like this.\n>>\n>> \t\t\tif (slash)\n>> \t\t\t\tslash = slash + 1;\n>>\n>> \t\t\tlevel->index = idx;\n>> \t\t\tif (dir->entries[idx]->flag & REF_DIR) {\n>> \t\t\t\t...\n>>\n>> IOW, isn't this check a bit too late? I wonder if we can leave at\n>> the beginning of the outer loop, even before sort_ref_dir(dir), when\n>> dir->nr is zero, or something?\n>>\n>>\n>\n> We could add it there too. I can't see the merit of one over the other.\n> But if you see it being more readable. I'll happily make that change.\n>\n\nJust to clarify, I will hold off on re-rolling till we make a decision\naround this, as if we decide to keep the check as it currently is, there\nis no reason to re-roll.\n\n[snip]\n"},{"id":"527433","messageId":"xmqq5xd5ch1h.fsf@gitster.g","threadId":"64197","inReplyTo":"CAOLa=ZSfL1CVCNjD0Vi3kpQqLM6t7YVa=G2ChGpaza6WOjZTCw@mail.gmail.com","subject":"Re: [PATCH v2] refs/ref-cache: fix SEGFAULT when seeking in empty directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-26T16:30:50Z","receivedAt":"2025-09-26T16:30:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> This wouldn't happen because before the loop starts, we set:\n>\n> dir = get_ref_dir(iter->cache->root);\n>\n> So, `dir` will always have an entry (i.e. 'refs') in the first\n> iteration, after that, the checks in the while section of the 'do {}\n> while (...)' loop will kick in.\n\nAh, OK. So we know dir->nr != 0 when the loop is entered for the\nfirst time. We may descend into a new subdirectory and at that\npoint we might hit dir->nr == 0 and that is why we want to catch it\nat the end of \"do {} while ()\". Makes sense.\n\n>> I wonder if we can leave at\n>> the beginning of the outer loop, even before sort_ref_dir(dir), when\n>> dir->nr is zero, or something?\n>\n> We could add it there too. I can't see the merit of one over the other.\n\nNo, I don't, either. As long as we know dir->nr != 0 upon the entry\ninto the loop, the check at the tail end in while() is sufficient.\n\nThanks for clarifying. I wonder if a bit of update to the log\nmessage would help future readers of the code, though.\n\n When using the files-backend with packed-refs, it is possible that some\n of the refs directories are empty. For e.g. just after repacking, the\n 'refs/heads' directory would be empty. The ref-cache seek mechanism,\n doesn't take this into consideration when descending into a subdirectory,\n and makes an out of bounds access, causing SEGFAULT as we try to\n access entries within the directory. Fix this by breaking out of the\n loop when we enter an empty directory.\n\nor something? I dunno.\n\nThanks.\n\n"},{"id":"527589","messageId":"CAOLa=ZSB90Ne=8VdJcXX0U9MhSWGAi1tTdzAkE9KdE=XuaOBCQ@mail.gmail.com","threadId":"64197","inReplyTo":"xmqq5xd5ch1h.fsf@gitster.g","subject":"Re: [PATCH v2] refs/ref-cache: fix SEGFAULT when seeking in empty directories","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-29T14:47:50Z","receivedAt":"2025-09-29T14:47:52Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> This wouldn't happen because before the loop starts, we set:\n>>\n>> dir = get_ref_dir(iter->cache->root);\n>>\n>> So, `dir` will always have an entry (i.e. 'refs') in the first\n>> iteration, after that, the checks in the while section of the 'do {}\n>> while (...)' loop will kick in.\n>\n> Ah, OK. So we know dir->nr != 0 when the loop is entered for the\n> first time. We may descend into a new subdirectory and at that\n> point we might hit dir->nr == 0 and that is why we want to catch it\n> at the end of \"do {} while ()\". Makes sense.\n>\n>>> I wonder if we can leave at\n>>> the beginning of the outer loop, even before sort_ref_dir(dir), when\n>>> dir->nr is zero, or something?\n>>\n>> We could add it there too. I can't see the merit of one over the other.\n>\n> No, I don't, either. As long as we know dir->nr != 0 upon the entry\n> into the loop, the check at the tail end in while() is sufficient.\n>\n> Thanks for clarifying. I wonder if a bit of update to the log\n> message would help future readers of the code, though.\n>\n> When using the files-backend with packed-refs, it is possible that some\n> of the refs directories are empty. For e.g. just after repacking, the\n> 'refs/heads' directory would be empty. The ref-cache seek mechanism,\n> doesn't take this into consideration when descending into a subdirectory,\n> and makes an out of bounds access, causing SEGFAULT as we try to\n> access entries within the directory. Fix this by breaking out of the\n> loop when we enter an empty directory.\n>\n> or something? I dunno.\n>\n> Thanks.\n\nYeah, I think something like this would be much better, I'll amend the\ncommit and send in a new version soon.\n\nThanks!\nKarthik\n"},{"id":"527678","messageId":"20251001-583-git-for-each-ref-start-after-v3-1-000f03837a92@gmail.com","threadId":"64197","inReplyTo":"20250924-583-git-for-each-ref-start-after-v1-1-c73be2b5db5a@gmail.com","subject":"[PATCH v3] refs/ref-cache: fix SEGFAULT when seeking in empty directories","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-01T12:17:29Z","receivedAt":"2025-10-01T12:17:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The 'cache_ref_iterator_seek()' function is used to seek the\n`ref_iterator` to the desired reference in the ref-cache mechanism. We\nuse the seeking functionality to implement the '--start-after' flag in\n'git-for-each-ref(1)'.\n\nWhen using the files-backend with packed-refs, it is possible that some\nof the refs directories are empty. For e.g. just after repacking, the\n'refs/heads' directory would be empty. The ref-cache seek mechanism,\ndoesn't take this into consideration when descending into a\nsubdirectory, and makes an out of bounds access, causing SEGFAULT as we\ntry to access entries within the directory. Fix this by breaking out of\nthe loop when we enter an empty directory.\n\nSince we start with the base directory of 'refs/' which is never empty,\nit is okay to perform this check after the first iteration in the\n`do..while` clause.\n\nAdd tests which simulate this behavior and also provide coverage over\nusing the feature over packed-refs.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Karthik Nayak \n---\nChanges in v3:\n- Modify the commit message to also explain why it is safe to add the\n check for empty directories in a `do..while` clause.\n- Link to v2: https://lore.kernel.org/r/20250925-583-git-for-each-ref-start-after-v2-1-3613b5a27ff1@gmail.com\n\nChanges in v2:\n- Moved the `dir-nr` check to the loop to provide better bound checks.\n- Modified the commit subject to talk about the issue at hand.\n- Substituted EOF with \\EOF since we don't do any variable parsing.\n- Link to v1: https://lore.kernel.org/r/20250924-583-git-for-each-ref-start-after-v1-1-c73be2b5db5a@gmail.com\n---\n refs/ref-cache.c | 2 +-\n t/t6302-for-each-ref-filter.sh | 65 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex c180e0aad7..e5e5df16d8 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -539,7 +539,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,\n \t\t\t\t */\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t} while (slash);\n+\t\t} while (slash && dir->nr);\n \t}\n \n \treturn 0;\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 9b80ea1e3b..7f060d97bf 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -754,4 +754,69 @@ test_expect_success 'start after used with custom sort order' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'start after with packed refs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit default &&\n+\n+\t\tgit update-ref --stdin <<-\\EOF &&\n+\t\tcreate refs/heads/branch @\n+\t\tcreate refs/heads/side @\n+\t\tcreate refs/odd/spot @\n+\t\tcreate refs/tags/one @\n+\t\tcreate refs/tags/two @\n+\t\tcommit\n+\t\tEOF\n+\n+\t\tcat >expect <<-\\EOF &&\n+\t\trefs/tags/default\n+\t\trefs/tags/one\n+\t\trefs/tags/two\n+\t\tEOF\n+\n+\t\tgit pack-refs --all &&\n+\t\tgit for-each-ref --format=\"%(refname)\" --start-after=refs/odd/spot >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'start after with packed refs and some loose refs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit default &&\n+\n+\t\tgit update-ref --stdin <<-\\EOF &&\n+\t\tcreate refs/heads/branch @\n+\t\tcreate refs/heads/side @\n+\t\tcreate refs/odd/spot @\n+\t\tcreate refs/tags/one @\n+\t\tcreate refs/tags/two @\n+\t\tcommit\n+\t\tEOF\n+\n+\t\tgit pack-refs --all &&\n+\n+\t\tgit update-ref --stdin <<-\\EOF &&\n+\t\tcreate refs/heads/foo @\n+\t\tcreate refs/odd/tee @\n+\t\tcommit\n+\t\tEOF\n+\n+\t\tcat >expect <<-\\EOF &&\n+\t\trefs/odd/tee\n+\t\trefs/tags/default\n+\t\trefs/tags/one\n+\t\trefs/tags/two\n+\t\tEOF\n+\n+\n+\t\tgit for-each-ref --format=\"%(refname)\" --start-after=refs/odd/spot >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n\n\n\n"}]}