[PATCH] receive-pack: fix crash on out-of-namespace symref

[PATCH] receive-pack: fix crash on out-of-namespace symref

[Troels Thomsen via GitGitGadget]

From: Troels Thomsen <troels@thomsen.io>

`check_aliased_update_internal()` detects when a symbolic ref and its
target are being updated in the same push. It does this by building a
list of ref names without the optional namespace prefix. When a symbolic
ref within a namespace points to a ref outside the namespace,
`strip_namespace()` returns NULL which leads to a segfault.

A NULL check preventing this particular issue was repurposed in
ded8393610. Rather than reintroducing it, we can instead build a list of
fully qualified ref names. This prevents the crash, preserves the
consistency check from da3efdb17b, and allows updates to all symbolic
refs.

Signed-off-by: Troels Thomsen <troels@thomsen.io>
---
    receive-pack: fix crash on out-of-namespace symref

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2144%2Ftt%2Ffix-crash-on-out-of-namespace-symref-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2144/tt/fix-crash-on-out-of-namespace-symref-v1
Pull-Request: https://github.com/git/git/pull/2144

 builtin/receive-pack.c           |  9 ++++++---
 t/t5509-fetch-push-namespaces.sh | 13 +++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9c49174616..a9e0568e94 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1685,7 +1685,6 @@ static void check_aliased_update_internal(struct command *cmd,
 		cmd->error_string = "broken symref";
 		return;
 	}
-	dst_name = strip_namespace(dst_name);
 
 	if (!(item = string_list_lookup(list, dst_name)))
 		return;
@@ -1730,10 +1729,13 @@ static void check_aliased_updates(struct command *commands)
 {
 	struct command *cmd;
 	struct string_list ref_list = STRING_LIST_INIT_NODUP;
+	struct strbuf ref_name = STRBUF_INIT;
 
 	for (cmd = commands; cmd; cmd = cmd->next) {
-		struct string_list_item *item =
-			string_list_append(&ref_list, cmd->ref_name);
+		struct string_list_item *item;
+		strbuf_reset(&ref_name);
+		strbuf_addf(&ref_name, "%s%s", get_git_namespace(), cmd->ref_name);
+		item = string_list_append(&ref_list, ref_name.buf);
 		item->util = (void *)cmd;
 	}
 	string_list_sort(&ref_list);
@@ -1743,6 +1745,7 @@ static void check_aliased_updates(struct command *commands)
 			check_aliased_update(cmd, &ref_list);
 	}
 
+	strbuf_release(&ref_name);
 	string_list_clear(&ref_list, 0);
 }
 
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index 095df1a753..3c333ccc5f 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -175,4 +175,17 @@ test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '
 	)
 '
 
+test_expect_success 'pushing to symref pointing outside the namespace' '
+	(
+		cd pushee &&
+		git symbolic-ref refs/namespaces/namespace/refs/heads/main refs/heads/main &&
+		cd ../original &&
+		git push pushee-namespaced main &&
+		git ls-remote pushee-unnamespaced refs/heads/main >actual &&
+		printf "$commit1\trefs/heads/main\n" >expected &&
+		printf "$commit1\trefs/namespaces/namespace/refs/heads/main\n" >>expected &&
+		test_cmp expected actual
+	)
+'
+
 test_done

base-commit: 66ce5f8e8872f0183bb137911c52b07f1f242d13
-- 
gitgitgadget

Re: [PATCH] receive-pack: fix crash on out-of-namespace symref

[Junio C Hamano]

"Troels Thomsen via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Troels Thomsen <troels@thomsen.io>
>
> `check_aliased_update_internal()` detects when a symbolic ref and its
> target are being updated in the same push. It does this by building a
> list of ref names without the optional namespace prefix. When a symbolic
> ref within a namespace points to a ref outside the namespace,
> `strip_namespace()` returns NULL which leads to a segfault.
>
> A NULL check preventing this particular issue was repurposed in
> ded8393610. Rather than reintroducing it, we can instead build a list of
> fully qualified ref names. This prevents the crash, preserves the
> consistency check from da3efdb17b, and allows updates to all symbolic
> refs.
>
> Signed-off-by: Troels Thomsen <troels@thomsen.io>
> ---
>     receive-pack: fix crash on out-of-namespace symref

Fixing crash is certainly a good thing, but when the namespace is
segregated and receive-pack wants to get updates only within the
given namespace, would presence of such a cross namespace symref
cause updates outside the namespace through the symref, defeating
the point of setting up a namespace in the first place?

I am not objecting to the new behaviour, but am not sure if it is a
sensible one.  You _might_ be able to argue that an attempt to update
underlying refs outside the namespace through such a symbolic ref
should result in an error (i.e., a fix to the current crashing
behaviour is to die in a controlled way).

Thoughts?

Re: [PATCH] receive-pack: fix crash on out-of-namespace symref

[Troels Thomsen]

On Sun, Dec 28, 2025, at 15:57, Junio C Hamano wrote:

> Fixing crash is certainly a good thing, but when the namespace is
> segregated and receive-pack wants to get updates only within the
> given namespace, would presence of such a cross namespace symref
> cause updates outside the namespace through the symref, defeating
> the point of setting up a namespace in the first place?
>
> I am not objecting to the new behaviour, but am not sure if it is a
> sensible one.  You _might_ be able to argue that an attempt to update
> underlying refs outside the namespace through such a symbolic ref
> should result in an error (i.e., a fix to the current crashing
> behaviour is to die in a controlled way).
>
> Thoughts?

I think it's important that the symbolic ref needs to be explicitly
created on the receiving side.

An argument in favor of allowing updates is that you can still choose to
reject them by implementing an update hook. Would the opposite be true?
I explictly wanted to share a branch into a namespace and update it from
there.

I suppose the behavior could be configurable. Given this bug has existed
since 2016, I'm assuming namespaces and symbolic refs probably aren't
used in combination frequently enough to justify this over using a hook.

-- 
Troels Thomsen

Re: [PATCH] receive-pack: fix crash on out-of-namespace symref

[Junio C Hamano]

"Troels Thomsen" <troels@thomsen.io> writes:

> On Sun, Dec 28, 2025, at 15:57, Junio C Hamano wrote:
>
>> Fixing crash is certainly a good thing, but when the namespace is
>> segregated and receive-pack wants to get updates only within the
>> given namespace, would presence of such a cross namespace symref
>> cause updates outside the namespace through the symref, defeating
>> the point of setting up a namespace in the first place?
>>
>> I am not objecting to the new behaviour, but am not sure if it is a
>> sensible one.  You _might_ be able to argue that an attempt to update
>> underlying refs outside the namespace through such a symbolic ref
>> should result in an error (i.e., a fix to the current crashing
>> behaviour is to die in a controlled way).
>>
>> Thoughts?
>
> I think it's important that the symbolic ref needs to be explicitly
> created on the receiving side.

Yes, and that can cut both ways.  In an ideal world without any
end-users who make any mistakes, deliberate cross namespace symref
may be a handy feature to break out of the namespace jail on purpose
in a controlled way.

But if the symref was made to point across the namespace boundary by
mistake, catching it as a misconfiguration may be a crucial chance
the user has to prevent it from turning into a security incident.
And that is why I asked.

Re: [PATCH] receive-pack: fix crash on out-of-namespace symref

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Troels Thomsen" <troels@thomsen.io> writes:
>
>> On Sun, Dec 28, 2025, at 15:57, Junio C Hamano wrote:
>>
>>> Fixing crash is certainly a good thing, but when the namespace is
>>> segregated and receive-pack wants to get updates only within the
>>> given namespace, would presence of such a cross namespace symref
>>> cause updates outside the namespace through the symref, defeating
>>> the point of setting up a namespace in the first place?
>>>
>>> I am not objecting to the new behaviour, but am not sure if it is a
>>> sensible one.  You _might_ be able to argue that an attempt to update
>>> underlying refs outside the namespace through such a symbolic ref
>>> should result in an error (i.e., a fix to the current crashing
>>> behaviour is to die in a controlled way).
>>>
>>> Thoughts?
>>
>> I think it's important that the symbolic ref needs to be explicitly
>> created on the receiving side.
>
> Yes, and that can cut both ways.  In an ideal world without any
> end-users who make any mistakes, deliberate cross namespace symref
> may be a handy feature to break out of the namespace jail on purpose
> in a controlled way.
>
> But if the symref was made to point across the namespace boundary by
> mistake, catching it as a misconfiguration may be a crucial chance
> the user has to prevent it from turning into a security incident.
> And that is why I asked.

The review discussion thread ended here.  I am dropping the topic
out of my tree now, but I do not think it would be a bad idea to
resurrect the topic that turns the uncontrolled segmentation fault
into a controlled death that calls die("hey, what is that cross
namespace link doing there?").

Thanks.

Re: [PATCH] receive-pack: fix crash on out-of-namespace symref

[Troels Thomsen]

On Sat, Feb 21, 2026, at 18:00, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> "Troels Thomsen" <troels@thomsen.io> writes:
>>
>>> On Sun, Dec 28, 2025, at 15:57, Junio C Hamano wrote:
>>>
>>>> Fixing crash is certainly a good thing, but when the namespace is
>>>> segregated and receive-pack wants to get updates only within the
>>>> given namespace, would presence of such a cross namespace symref
>>>> cause updates outside the namespace through the symref, defeating
>>>> the point of setting up a namespace in the first place?
>>>>
>>>> I am not objecting to the new behaviour, but am not sure if it is a
>>>> sensible one.  You _might_ be able to argue that an attempt to update
>>>> underlying refs outside the namespace through such a symbolic ref
>>>> should result in an error (i.e., a fix to the current crashing
>>>> behaviour is to die in a controlled way).
>>>>
>>>> Thoughts?
>>>
>>> I think it's important that the symbolic ref needs to be explicitly
>>> created on the receiving side.
>>
>> Yes, and that can cut both ways.  In an ideal world without any
>> end-users who make any mistakes, deliberate cross namespace symref
>> may be a handy feature to break out of the namespace jail on purpose
>> in a controlled way.
>>
>> But if the symref was made to point across the namespace boundary by
>> mistake, catching it as a misconfiguration may be a crucial chance
>> the user has to prevent it from turning into a security incident.
>> And that is why I asked.
>
> The review discussion thread ended here.  I am dropping the topic
> out of my tree now, but I do not think it would be a bad idea to
> resurrect the topic that turns the uncontrolled segmentation fault
> into a controlled death that calls die("hey, what is that cross
> namespace link doing there?").
>
> Thanks.

Do you think your original concern could be addressed by adding a note
to the security section of gitnamespaces?

It seems somewhat relevant that you're unlikely to create a symbolic ref
within a namespace without first consulting the documentation to
understand the ref format. Combined with the lack of interest in this
thread, and the fact that no bug report was filed for years, I suspect
this feature combination is rare. That's not a good reason for a bad
default, but a symbolic ref can already point outside a namespace; you
only can't update it.

If I fix it by rejecting updates as suggested, I still wouldn't be able
to do what I wanted in the first place. Is there a better way to propose
such a change?

In any case, thank you for your time.

-- 
Troels Thomsen

Re: [PATCH] receive-pack: fix crash on out-of-namespace symref

[Junio C Hamano]

"Troels Thomsen" <troels@thomsen.io> writes:

> Do you think your original concern could be addressed by adding a note
> to the security section of gitnamespaces?

Not really.  Nobody reads documentation, so it would be far more
preferrable to make the default strict, with a documented way to
optionally loosen, than the other way around.