fix(bwrap): honor namespace fd 0

[danielchristiancazares]

Summary

• treat namespace FDs consistently using the -1 sentinel check
• accept fd

[giuseppe]

please address the DCO failure. Need to add a Signed-off-by line to the commit, using a real name

[danielchristiancazares]

please address the DCO failure. Need to add a Signed-off-by line to the commit, using a real name

Amended. DCO check is green.

[giuseppe]

LGTM

[smcv]

Is there a user-visible bug in some larger component or script that this is fixing, or is this just an "I was looking at the code and noticed an inconsistency" sort of thing?

On one hand, as you say, 0 is an entirely valid fd, and it's consistent with everything else if we accept it as such.

On the other hand, having the namespace fd be bubblewrap's stdin seems like a bad idea in general: the "payload" command inside the container will inherit that namespace fd as its stdin, which I suspect is not going to do anything particularly useful if the "payload" command tries to read from it. So if there is some larger component or script that is passing the userns as fd 0, I'd recommend giving it some other fd as stdin (probably /dev/null, unless you have something better available), and using a fd >= 3 for the userns.

The same would be equally true for fd 1 or 2 (stdout and stderr), and we don't reject those bad ideas, so it would be consistent to give fd 0 (stdout) equal treatment.

[smcv]

Is there a user-visible bug in some larger component or script that this is fixing?

Not merging this right now because I'd prefer to know the answer to that question, but I think this change seems fine.

[danielchristiancazares]

Is there a user-visible bug in some larger component or script that this is fixing, or is this just an "I was looking at the code and noticed an inconsistency" sort of thing?

On one hand, as you say, 0 is an entirely valid fd, and it's consistent with everything else if we accept it as such.

On the other hand, having the namespace fd be bubblewrap's stdin seems like a bad idea in general: the "payload" command inside the container will inherit that namespace fd as its stdin, which I suspect is not going to do anything particularly useful if the "payload" command tries to read from it. So if there is some larger component or script that is passing the userns as fd 0, I'd recommend giving it some other fd as stdin (probably /dev/null, unless you have something better available), and using a fd >= 3 for the userns.

The same would be equally true for fd 1 or 2 (stdout and stderr), and we don't reject those bad ideas, so it would be consistent to give fd 0 (stdout) equal treatment.

Yes, there's a user-visible behavioral bug here. It's not just an internal inconsistency fix.

I put together a small reproducer and ran it on Ubuntu 24.04.4 arm64 (Azure VM, kernel 6.17.0-1008-azure).

The reproducer:

1. Starts a bwrap sandbox with --unshare-user --unshare-pid and records its pid namespace identity.
2. Starts a second bwrap joining that namespace via --userns 3 --pidns 4 as a control. This succeeds and reports the same pid namespace.
3. Starts a third bwrap identically, except the pid namespace is on fd 0 instead of fd 4.
4. Compares the reported pid namespace from the fd 0 case with the original.

On an unpatched build, the fd 0 case enters the wrong pid namespace.

creating a reusable user namespace + pid namespace
control: the same pid namespace on fd 4 should succeed
repro: the same pid namespace on fd 0 should behave the same
fd 0 case returned the wrong pid namespace.
expected: pid:[4026532168]
actual: pid:[4026531836]

On the patched build, the same reproducer passes:

creating a reusable user namespace + pid namespace
control: the same pid namespace on fd 4 should succeed
repro: the same pid namespace on fd 0 should behave the same
fd 0 case succeeded; no bug reproduced.

fd 0 is accepted by argument parsing, so it's valid input. The > 0 checks end up silently treating it as option not supplied, which causes the sandbox to skip the setns() call and changes the expected behavior of valid input.

[rusty-snake]

Is there a user-visible bug in some larger component or script that this is fixing

Any real-world one?

[smcv]

Starts a third bwrap identically, except the pid namespace is on fd 0 instead of fd 4.

Sure, but is there a reason why you'd want to do this outside an artificial reproducer? fds 0, 1 and 2 are special because they're expected to be used by the "payload" command (they're standard input, output and error).

[danielchristiancazares]

Starts a third bwrap identically, except the pid namespace is on fd 0 instead of fd 4.

Sure, but is there a reason why you'd want to do this outside an artificial reproducer? fds 0, 1 and 2 are special because they're expected to be used by the "payload" command (they're standard input, output and error).

Great question. I agree that 0/1/2 are usually poor choices for namespace fds, because the payload will inherit them as stdio. I'm not arguing that callers should prefer fd 0.

The issue I'm trying to fix is narrower. bwrap currently accepts any non-negative fd for --userns, --userns2 and --pidns, and fd 0 can arise naturally if a launcher has stdin closed before opening the namespace fd (or intentionally via shell redirection like 0< /proc/$pid/ns/pid).

On the unpatched build, that accepted input is later treated as though the option hadn't been supplied, without an error. That's the issue the script demonstrates.

If the policy intent should be that stdio fds should be rejected, that seems like a separate decision, and I think it should be enforced explicitly at argument validation time for 0/1/2, rather than implicitly as a side effect of using > 0 instead of the -1 sentinel check.

[smcv]

I'll merge this, because, yes, treating fd 0 as special is an inconsistency; but fd 0 is a bad choice for this purpose, and if there is some layer of the stack that is causing misbehaviour for you by closing stdin, then that layer should really be fixed so that fd 0 is always open as "the fd that I expect my child processes to receive as stdin" (and if that's /dev/null that's fine, it's a completely reasonable stdin).

Or if this is completely hypothetical and it's just that the inconsistency annoyed you, please be honest about that, rather than inventing hypothetical broken components.

fd 0 can arise naturally if a launcher has stdin closed before opening the namespace fd

If a launcher is doing that, that's a bug in the launcher (or the parent or ancestor from which it inherited a closed fd 0): anything that runs child processes should have something open on fds 0, 1 and 2 (using /dev/null if they need to be "closed"). I've seen very hard-to-diagnose issues in the past in other desktop components like dbus-daemon when the desktop environment (I think it was KDE Plasma?) was accidentally running them with stdin closed, resulting in random other fds becoming their stdin when opened. Please find and fix whichever layer of the stack is closing stdin so that it doesn't break the rest of your environment.

or intentionally via shell redirection like 0< /proc/$pid/ns/pid

This seems just as much "but why would you do that?" as any other use of stdin for this purpose. Shell redirection can open any other fd, it was someone's choice to use fd 0 (and they could/should have chosen differently).