Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>> + fd = xopen("/dev/null", O_RDONLY);
>> + if (fd > 0)
>> + close(fd);
>> + if (fd > 2)
>> + return;
>> +
>> + fd = xopen("/dev/null", O_WRONLY);
>> while (fd < 2)
>> fd = xdup(fd);
>> if (fd > 2)
>
> I don't really get this, if it's safe under seccomp to open this
> O_RDONLY wasn't it always redundant to use O_WRONLY, and this change
> should just be switching to O_RDONLY?
The first "if we successfully opened /dev/null and also found that
stdin is already connected to something above 0, then close the fd"
may be tricky to reason about. There are four cases.
(1) The first xopen(/dev/null, O_RDONLY) fails and returns -1.
In this case, we go down the original codepath.
(2) It returns 0 (upon startup, stdin was closed). We keep the fd
to use as the standard input, and go down the original
codepath, but now standard input is connected, the original
codepath needs to deal with only stdout and stderr, so we can
safely use O_WRONLY and dup it into stdout and stderr.
(3) It returns 1 or 2 (upon startup, stdin was open, stdin or
stderr was closed). The fd is closed, and we go to the
original codepath. Again, the original codepath does not have
to touch stdin, so we can afford to use O_WRONLY.
(4) It returns 3 or above, indicating that the three fd's we care
about was already open. We return without making further
damage after closing the fd.
The updated code does a single system call to probe for the lowest
unused file descriptor and returns in the common case. And that
single system call is an extra overhead for the "oops some of the
low FD were not open" case compared to the original code, but the
read-only FD may get reused when we need to.
So, I think the updated code does what it explains in its proposed
log message. I.e.
Check the need of sanitization with a file descriptor in read-only mode,
keep it as replacement for stdin and open replacement file descriptors
for stdout and stderr in write-only mode.
v2:
- switch to xopen("/dev/null", O_RDONLY) to stay at 2 syscalls in the
common case and use O_WRONLY for stdout and stderr, as suggested
by René Scharfe
