{"thread":{"id":"64686","subject":"[PATCH] receive-pack: fix crash on out-of-namespace symref","startedAt":"2025-12-27T15:40:18Z","lastAt":"2026-02-22T20:35:49Z","messageCount":7,"participants":["Troels Thomsen via GitGitGadget","Junio C Hamano","Troels Thomsen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"532785","messageId":"pull.2144.git.git.1766850014289.gitgitgadget@gmail.com","threadId":"64686","inReplyTo":null,"subject":"[PATCH] receive-pack: fix crash on out-of-namespace symref","fromName":"Troels Thomsen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-12-27T15:40:14Z","receivedAt":"2025-12-27T15:40:18Z","isPatch":true,"sender":{"key":"troels@thomsen.io","avatar":"https://gravatar.com/avatar/25f46623c7122b8f97a0dcea37e0dc67d14610eb601a7d2c9c1904e3819febf7?d=mp&s=160"},"body":"From: Troels Thomsen \n\n`check_aliased_update_internal()` detects when a symbolic ref and its\ntarget are being updated in the same push. It does this by building a\nlist of ref names without the optional namespace prefix. When a symbolic\nref within a namespace points to a ref outside the namespace,\n`strip_namespace()` returns NULL which leads to a segfault.\n\nA NULL check preventing this particular issue was repurposed in\nded8393610. Rather than reintroducing it, we can instead build a list of\nfully qualified ref names. This prevents the crash, preserves the\nconsistency check from da3efdb17b, and allows updates to all symbolic\nrefs.\n\nSigned-off-by: Troels Thomsen \n---\n receive-pack: fix crash on out-of-namespace symref\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2144%2Ftt%2Ffix-crash-on-out-of-namespace-symref-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2144/tt/fix-crash-on-out-of-namespace-symref-v1\nPull-Request: https://github.com/git/git/pull/2144\n\n builtin/receive-pack.c | 9 ++++++---\n t/t5509-fetch-push-namespaces.sh | 13 +++++++++++++\n 2 files changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9c49174616..a9e0568e94 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1685,7 +1685,6 @@ static void check_aliased_update_internal(struct command *cmd,\n \t\tcmd->error_string = \"broken symref\";\n \t\treturn;\n \t}\n-\tdst_name = strip_namespace(dst_name);\n \n \tif (!(item = string_list_lookup(list, dst_name)))\n \t\treturn;\n@@ -1730,10 +1729,13 @@ static void check_aliased_updates(struct command *commands)\n {\n \tstruct command *cmd;\n \tstruct string_list ref_list = STRING_LIST_INIT_NODUP;\n+\tstruct strbuf ref_name = STRBUF_INIT;\n \n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tstruct string_list_item *item =\n-\t\t\tstring_list_append(&ref_list, cmd->ref_name);\n+\t\tstruct string_list_item *item;\n+\t\tstrbuf_reset(&ref_name);\n+\t\tstrbuf_addf(&ref_name, \"%s%s\", get_git_namespace(), cmd->ref_name);\n+\t\titem = string_list_append(&ref_list, ref_name.buf);\n \t\titem->util = (void *)cmd;\n \t}\n \tstring_list_sort(&ref_list);\n@@ -1743,6 +1745,7 @@ static void check_aliased_updates(struct command *commands)\n \t\t\tcheck_aliased_update(cmd, &ref_list);\n \t}\n \n+\tstrbuf_release(&ref_name);\n \tstring_list_clear(&ref_list, 0);\n }\n \ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex 095df1a753..3c333ccc5f 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -175,4 +175,17 @@ test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '\n \t)\n '\n \n+test_expect_success 'pushing to symref pointing outside the namespace' '\n+\t(\n+\t\tcd pushee &&\n+\t\tgit symbolic-ref refs/namespaces/namespace/refs/heads/main refs/heads/main &&\n+\t\tcd ../original &&\n+\t\tgit push pushee-namespaced main &&\n+\t\tgit ls-remote pushee-unnamespaced refs/heads/main >actual &&\n+\t\tprintf \"$commit1\\trefs/heads/main\\n\" >expected &&\n+\t\tprintf \"$commit1\\trefs/namespaces/namespace/refs/heads/main\\n\" >>expected &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n test_done\n\nbase-commit: 66ce5f8e8872f0183bb137911c52b07f1f242d13\n-- \ngitgitgadget\n"},{"id":"532791","messageId":"xmqqfr8uk61i.fsf@gitster.g","threadId":"64686","inReplyTo":"pull.2144.git.git.1766850014289.gitgitgadget@gmail.com","subject":"Re: [PATCH] receive-pack: fix crash on out-of-namespace symref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-12-28T14:57:45Z","receivedAt":"2025-12-28T14:57:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Troels Thomsen via GitGitGadget\" writes:\n\n> From: Troels Thomsen \n>\n> `check_aliased_update_internal()` detects when a symbolic ref and its\n> target are being updated in the same push. It does this by building a\n> list of ref names without the optional namespace prefix. When a symbolic\n> ref within a namespace points to a ref outside the namespace,\n> `strip_namespace()` returns NULL which leads to a segfault.\n>\n> A NULL check preventing this particular issue was repurposed in\n> ded8393610. Rather than reintroducing it, we can instead build a list of\n> fully qualified ref names. This prevents the crash, preserves the\n> consistency check from da3efdb17b, and allows updates to all symbolic\n> refs.\n>\n> Signed-off-by: Troels Thomsen \n> ---\n> receive-pack: fix crash on out-of-namespace symref\n\nFixing crash is certainly a good thing, but when the namespace is\nsegregated and receive-pack wants to get updates only within the\ngiven namespace, would presence of such a cross namespace symref\ncause updates outside the namespace through the symref, defeating\nthe point of setting up a namespace in the first place?\n\nI am not objecting to the new behaviour, but am not sure if it is a\nsensible one. You _might_ be able to argue that an attempt to update\nunderlying refs outside the namespace through such a symbolic ref\nshould result in an error (i.e., a fix to the current crashing\nbehaviour is to die in a controlled way).\n\nThoughts?\n"},{"id":"532796","messageId":"a16bf8a6-2f57-4794-91b5-92615f184c4b@app.fastmail.com","threadId":"64686","inReplyTo":"xmqqfr8uk61i.fsf@gitster.g","subject":"Re: [PATCH] receive-pack: fix crash on out-of-namespace symref","fromName":"Troels Thomsen","fromEmail":"troels@thomsen.io","sentAt":"2025-12-28T16:26:45Z","receivedAt":"2025-12-28T16:27:06Z","isPatch":true,"sender":{"key":"troels@thomsen.io","avatar":"https://gravatar.com/avatar/25f46623c7122b8f97a0dcea37e0dc67d14610eb601a7d2c9c1904e3819febf7?d=mp&s=160"},"body":"On Sun, Dec 28, 2025, at 15:57, Junio C Hamano wrote:\n\n> Fixing crash is certainly a good thing, but when the namespace is\n> segregated and receive-pack wants to get updates only within the\n> given namespace, would presence of such a cross namespace symref\n> cause updates outside the namespace through the symref, defeating\n> the point of setting up a namespace in the first place?\n>\n> I am not objecting to the new behaviour, but am not sure if it is a\n> sensible one. You _might_ be able to argue that an attempt to update\n> underlying refs outside the namespace through such a symbolic ref\n> should result in an error (i.e., a fix to the current crashing\n> behaviour is to die in a controlled way).\n>\n> Thoughts?\n\nI think it's important that the symbolic ref needs to be explicitly\ncreated on the receiving side.\n\nAn argument in favor of allowing updates is that you can still choose to\nreject them by implementing an update hook. Would the opposite be true?\nI explictly wanted to share a branch into a namespace and update it from\nthere.\n\nI suppose the behavior could be configurable. Given this bug has existed\nsince 2016, I'm assuming namespaces and symbolic refs probably aren't\nused in combination frequently enough to justify this over using a hook.\n\n-- \nTroels Thomsen\n"},{"id":"532817","messageId":"xmqqbjjgiz3a.fsf@gitster.g","threadId":"64686","inReplyTo":"a16bf8a6-2f57-4794-91b5-92615f184c4b@app.fastmail.com","subject":"Re: [PATCH] receive-pack: fix crash on out-of-namespace symref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-12-30T00:37:45Z","receivedAt":"2025-12-30T00:37:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Troels Thomsen\" writes:\n\n> On Sun, Dec 28, 2025, at 15:57, Junio C Hamano wrote:\n>\n>> Fixing crash is certainly a good thing, but when the namespace is\n>> segregated and receive-pack wants to get updates only within the\n>> given namespace, would presence of such a cross namespace symref\n>> cause updates outside the namespace through the symref, defeating\n>> the point of setting up a namespace in the first place?\n>>\n>> I am not objecting to the new behaviour, but am not sure if it is a\n>> sensible one. You _might_ be able to argue that an attempt to update\n>> underlying refs outside the namespace through such a symbolic ref\n>> should result in an error (i.e., a fix to the current crashing\n>> behaviour is to die in a controlled way).\n>>\n>> Thoughts?\n>\n> I think it's important that the symbolic ref needs to be explicitly\n> created on the receiving side.\n\nYes, and that can cut both ways. In an ideal world without any\nend-users who make any mistakes, deliberate cross namespace symref\nmay be a handy feature to break out of the namespace jail on purpose\nin a controlled way.\n\nBut if the symref was made to point across the namespace boundary by\nmistake, catching it as a misconfiguration may be a crucial chance\nthe user has to prevent it from turning into a security incident.\nAnd that is why I asked.\n"},{"id":"536595","messageId":"xmqq8qcmt4kq.fsf@gitster.g","threadId":"64686","inReplyTo":"xmqqbjjgiz3a.fsf@gitster.g","subject":"Re: [PATCH] receive-pack: fix crash on out-of-namespace symref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-21T17:00:05Z","receivedAt":"2026-02-21T17:00:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Troels Thomsen\" writes:\n>\n>> On Sun, Dec 28, 2025, at 15:57, Junio C Hamano wrote:\n>>\n>>> Fixing crash is certainly a good thing, but when the namespace is\n>>> segregated and receive-pack wants to get updates only within the\n>>> given namespace, would presence of such a cross namespace symref\n>>> cause updates outside the namespace through the symref, defeating\n>>> the point of setting up a namespace in the first place?\n>>>\n>>> I am not objecting to the new behaviour, but am not sure if it is a\n>>> sensible one. You _might_ be able to argue that an attempt to update\n>>> underlying refs outside the namespace through such a symbolic ref\n>>> should result in an error (i.e., a fix to the current crashing\n>>> behaviour is to die in a controlled way).\n>>>\n>>> Thoughts?\n>>\n>> I think it's important that the symbolic ref needs to be explicitly\n>> created on the receiving side.\n>\n> Yes, and that can cut both ways. In an ideal world without any\n> end-users who make any mistakes, deliberate cross namespace symref\n> may be a handy feature to break out of the namespace jail on purpose\n> in a controlled way.\n>\n> But if the symref was made to point across the namespace boundary by\n> mistake, catching it as a misconfiguration may be a crucial chance\n> the user has to prevent it from turning into a security incident.\n> And that is why I asked.\n\nThe review discussion thread ended here. I am dropping the topic\nout of my tree now, but I do not think it would be a bad idea to\nresurrect the topic that turns the uncontrolled segmentation fault\ninto a controlled death that calls die(\"hey, what is that cross\nnamespace link doing there?\").\n\nThanks.\n"},{"id":"536634","messageId":"ead4041f-bbc3-41ea-8729-9534e69e5e83@app.fastmail.com","threadId":"64686","inReplyTo":"xmqq8qcmt4kq.fsf@gitster.g","subject":"Re: [PATCH] receive-pack: fix crash on out-of-namespace symref","fromName":"Troels Thomsen","fromEmail":"troels@thomsen.io","sentAt":"2026-02-22T07:56:55Z","receivedAt":"2026-02-22T07:58:06Z","isPatch":true,"sender":{"key":"troels@thomsen.io","avatar":"https://gravatar.com/avatar/25f46623c7122b8f97a0dcea37e0dc67d14610eb601a7d2c9c1904e3819febf7?d=mp&s=160"},"body":"On Sat, Feb 21, 2026, at 18:00, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n>> \"Troels Thomsen\" writes:\n>>\n>>> On Sun, Dec 28, 2025, at 15:57, Junio C Hamano wrote:\n>>>\n>>>> Fixing crash is certainly a good thing, but when the namespace is\n>>>> segregated and receive-pack wants to get updates only within the\n>>>> given namespace, would presence of such a cross namespace symref\n>>>> cause updates outside the namespace through the symref, defeating\n>>>> the point of setting up a namespace in the first place?\n>>>>\n>>>> I am not objecting to the new behaviour, but am not sure if it is a\n>>>> sensible one. You _might_ be able to argue that an attempt to update\n>>>> underlying refs outside the namespace through such a symbolic ref\n>>>> should result in an error (i.e., a fix to the current crashing\n>>>> behaviour is to die in a controlled way).\n>>>>\n>>>> Thoughts?\n>>>\n>>> I think it's important that the symbolic ref needs to be explicitly\n>>> created on the receiving side.\n>>\n>> Yes, and that can cut both ways. In an ideal world without any\n>> end-users who make any mistakes, deliberate cross namespace symref\n>> may be a handy feature to break out of the namespace jail on purpose\n>> in a controlled way.\n>>\n>> But if the symref was made to point across the namespace boundary by\n>> mistake, catching it as a misconfiguration may be a crucial chance\n>> the user has to prevent it from turning into a security incident.\n>> And that is why I asked.\n>\n> The review discussion thread ended here. I am dropping the topic\n> out of my tree now, but I do not think it would be a bad idea to\n> resurrect the topic that turns the uncontrolled segmentation fault\n> into a controlled death that calls die(\"hey, what is that cross\n> namespace link doing there?\").\n>\n> Thanks.\n\nDo you think your original concern could be addressed by adding a note\nto the security section of gitnamespaces?\n\nIt seems somewhat relevant that you're unlikely to create a symbolic ref\nwithin a namespace without first consulting the documentation to\nunderstand the ref format. Combined with the lack of interest in this\nthread, and the fact that no bug report was filed for years, I suspect\nthis feature combination is rare. That's not a good reason for a bad\ndefault, but a symbolic ref can already point outside a namespace; you\nonly can't update it.\n\nIf I fix it by rejecting updates as suggested, I still wouldn't be able\nto do what I wanted in the first place. Is there a better way to propose\nsuch a change?\n\nIn any case, thank you for your time.\n\n-- \nTroels Thomsen\n"},{"id":"536663","messageId":"xmqqcy1wqzx9.fsf@gitster.g","threadId":"64686","inReplyTo":"ead4041f-bbc3-41ea-8729-9534e69e5e83@app.fastmail.com","subject":"Re: [PATCH] receive-pack: fix crash on out-of-namespace symref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-22T20:35:46Z","receivedAt":"2026-02-22T20:35:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Troels Thomsen\" writes:\n\n> Do you think your original concern could be addressed by adding a note\n> to the security section of gitnamespaces?\n\nNot really. Nobody reads documentation, so it would be far more\npreferrable to make the default strict, with a documented way to\noptionally loosen, than the other way around.\n\n"}]}