Repository navigation
libutil-tests: Skip leakedFDsAreClosed without close_range on musl - #16568
Conversation
| long n = ::syscall(SYS_getdents64, dirFd, buf, sizeof(buf)); | ||
| if (n <= 0) { | ||
| ok = n == 0; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Is this actually necessary? readdir in musl seems like they would be perfectly fine to use here tbh.
|
Can we just disable the fallback tests when we don't have close_range? This closing is already best-effort tbh and I'm not sure we should be bending over backwards to support musl and ancient kernels. For most of nix's existence runProgram2 didn't close fds so while using glibc's fallback is nice because it's readily available, it seems like maybe we shouldn't bother overcomplicating this pile of complex code. |
Is this an actual concern? I have previously did some fallbacks but now I'm doubting how useful that would be considering that some things are just impossible to do without /proc/self/fd still... |
8a54750 to
0bdfc90
Compare
philiptaron
changed the title
musl has no closefrom(), so doExecChild() closes leaked descriptors only via close_range, which is deliberately best-effort. The `run-without-new-syscalls` test run blocks close_range with `enosys`, so `runProgram2.leakedFDsAreClosed` has failed there on every musl build, including `nix-everything-static`, since it was added. Skip the test when close_range returns ENOSYS on a non-glibc Linux libc. glibc's closefrom() has its own /proc/self/fd fallback, so the test still runs under `enosys` there. Fixes NixOS#16520 (the leakedFDsAreClosed part).
0bdfc90 to
3aacdcc
Compare
Yeah, that's the better path. Did that instead of this code. |
2 participants
Motivation
nix-util-tests-run-without-new-syscallsfails on every musl build, includingnix-everything-static:That test run uses
enosysto blockclose_range, among other syscalls, so the fallback paths get exercised. musl has noclosefrom(), sodoExecChild()closes leaked descriptors only throughclose_range. That's deliberately best-effort, so underenosysthe descriptors stay open and the test fails.leakedFDsAreClosed(#16250) landed a day afterclose_rangewas added to theenosysrun (#16247), and this combination has failed since then.Context
Closes: #16520 (the leakedFDsAreClosed part; the ParseURL failures in the original report were a boost regression and no longer reproduce with the current nixpkgs pin).
As discussed in the review of a previous version of this PR, we skip the test instead of adding a
/proc/self/fdfallback todoExecChild(). On a non-glibc Linux libc, the test now probesclose_rangeand calls GTEST_SKIP() if it returns ENOSYS. glibc’s closefrom() has its own procfs fallback, so the test still runs under enosys there.Testing
On
x86_64-linux:.#nix-util-tests-static.tests.run-without-new-syscallson master (5ca019f)runProgram2.leakedFDsAreClosed.#nix-util-tests-static.tests.run-without-new-syscallswith this change.#nix-util-tests-static.tests.runwith this change.#nix-everything-staticwith this changeAdd 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.