Conversation
boost::concurrent_flat_map (used in libutil and libstore) includes the C++17 <execution> header. GCC's libstdc++ implements parallel algorithms using Intel TBB as the backend, which creates a link-time dependency on libtbb even though we don't actually use any parallel algorithms. Disable the TBB backend for libstdc++ by setting _GLIBCXX_USE_TBB_PAR_BACKEND=0. This makes parallel algorithms fall back to serial execution, which is acceptable since we don't use them anyway. This only affects libstdc++ (GCC's standard library); other standard libraries like libc++ (LLVM) are unaffected. (cherry picked from commit 2f3ec16)
…tenance [Backport 2.32-maintenance] build: Disable libstdc++ TBB backend to avoid unnecessary dependency
These will change in the next commit to fix the silent regression from 2.23 in the handling of multiple subsequent path separators. (cherry picked from commit 86f0908)
This gets us back to pre-2.23 behavior of this primop. Done by inlining the code of `nix::dirOf` from 2.2-maintenance. (cherry picked from commit a33fccf)
…tenance [Backport 2.32-maintenance] libexpr: Don't use nix::dirOf in prim_dirOf (fix 2.23 regression)
This early return was lost in d4ef822. By doing some https://en.wikipedia.org/wiki/Non-virtual_interface_pattern, we can ensure that we don't make this mistake again --- implementations are no longer responsible for implementing the caching/memoization mechanism. (cherry picked from commit 496e43e)
…tenance [Backport 2.32-maintenance] Restore isAllowed check in ChrootLinuxDerivationBuilder
Tagging release 2.32.4
edolstra
added
the
flake-regression-test
WalkthroughBumps version to 2.32.4, refactors Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Wrapper as addDependency (wrapper)
participant Check as isAllowed check
participant Impl as addDependencyImpl (virtual)
participant Sub as Subclass impl
Caller->>Wrapper: addDependency(path)
Wrapper->>Check: isAllowed(path)?
alt Path allowed
Check-->>Wrapper: true
Wrapper-->>Caller: return (early exit)
else Path not allowed
Check-->>Wrapper: false
Wrapper->>Impl: addDependencyImpl(path)
Impl->>Sub: dispatch to subclass
Note over Sub: DerivationBuilderImpl: always record<br/>ChrootLinuxBuilder: guard + process
Sub-->>Wrapper: complete
Wrapper-->>Caller: done
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/libstore/unix/build/linux-derivation-builder.cc (2)
706-733: Fix infinite recursion and missing addedPaths update in addDependencyImpl.Current flow:
- addDependencyImpl → ChrootDerivationBuilder::addDependencyPrep(path)
- addDependencyPrep → DerivationBuilderImpl::addDependency(path) wrapper
- wrapper → virtual addDependencyImpl(path) → back here
This recurses. Also, this override never inserts into addedPaths, so subsequent isAllowed() never flips.
Implement recording locally and compute bind targets without re-entering the wrapper; then bind-mount.
Apply:
- void addDependencyImpl(const StorePath & path) override - { - if (isAllowed(path)) - return; - - auto [source, target] = ChrootDerivationBuilder::addDependencyPrep(path); + void addDependencyImpl(const StorePath & path) override + { + // Guard if already present via inputs or prior additions. + if (isAllowed(path)) + return; + + // Record first to flip isAllowed() and avoid re-entry. + DerivationBuilderImpl::addDependencyImpl(path); + + // Compute source/target like addDependencyPrep(), but avoid calling it to prevent recursion. + Path source = store.Store::toRealPath(path); + Path target = chrootRootDir + store.printStorePath(path); + if (pathExists(target)) { + // Mirror original diagnostic. + debug("bind-mounting %s -> %s", target, source); + throw Error("store path '%s' already exists in the sandbox", store.printStorePath(path)); + } /* Bind-mount the path into the sandbox. This requires entering its mount namespace, which is not possible in multithreaded programs. So we do this in a child process.*/ Pid child(startProcess([&]() { if (usingUserNamespace && (setns(sandboxUserNamespace.get(), 0) == -1)) throw SysError("entering sandbox user namespace"); if (setns(sandboxMountNamespace.get(), 0) == -1) throw SysError("entering sandbox mount namespace"); doBind(source, target); _exit(0); })); int status = child.wait(); if (status != 0) throw Error("could not add path '%s' to sandbox", store.printStorePath(path)); }Alternatively, change ChrootDerivationBuilder::addDependencyPrep() to call DerivationBuilderImpl::addDependencyImpl(path) (base) instead of the wrapper, and keep your code; but that requires a coordinated change in that file as well.
181-197: Follow-up (in chroot-derivation-builder.cc): avoid wrapper to prevent recursion.In addDependencyPrep(), replace DerivationBuilderImpl::addDependency(path) with the base impl call to avoid virtual re-entry.
Apply in src/libstore/unix/build/chroot-derivation-builder.cc:
- DerivationBuilderImpl::addDependency(path); + // Record directly to avoid virtual dispatch back into platform impl. + DerivationBuilderImpl::addDependencyImpl(path);This change is required only if you keep using addDependencyPrep() from the Linux override.
🧹 Nitpick comments (4)
nix-meson-build-support/common/meson.build (1)
45-63: Harden libstdc++ detection header.Including to probe GLIBCXX can be unreliable; prefer a standard C++ header that always pulls in the C++ library config (e.g., ), keeping the same compile-time test.
Apply:
- #include <ciso646> + #include <string>src/libexpr/primops.cc (1)
2006-2014: String dirOf logic is correct and matches tests.
- No slash → "."
- Leading slash only → "/"
- Otherwise substring up to last '/'
- Respects multiple separators without normalization.
Minor: you can use mkStringMove for the substring to avoid one copy, but not required.
src/libstore/unix/build/linux-derivation-builder.cc (1)
708-710: Redundant allow-check (optional).The public wrapper already checks isAllowed() before dispatching. If you adopt the local-recording approach above (which bypasses the wrapper), keeping this guard is useful; otherwise it’s redundant.
src/libstore/include/nix/store/restricted-store.hh (1)
64-69: Consider clarifying the idempotency guarantee in the comment.The comment states "the caller will ensure... that idempotent calls are a no-op", which could be read as the caller making
addDependencyImplitself idempotent. A clearer phrasing would explain that the wrapper makes the overall operation idempotent by preventing redundant calls to the implementation.Consider revising the comment to:
/** - * This is the underlying implementation to be defined. The caller - * will ensure that this is only called on newly added dependencies, - * and that idempotent calls are a no-op. + * Underlying implementation called by addDependency(). The wrapper + * ensures this is only invoked for paths not already allowed (i.e., + * not in originalPaths() or addedPaths), making the overall + * addDependency() operation idempotent. */ virtual void addDependencyImpl(const StorePath & path) = 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.version(1 hunks)nix-meson-build-support/common/meson.build(1 hunks)src/libexpr/primops.cc(1 hunks)src/libstore/include/nix/store/restricted-store.hh(1 hunks)src/libstore/unix/build/derivation-builder.cc(2 hunks)src/libstore/unix/build/linux-derivation-builder.cc(1 hunks)tests/functional/lang/eval-okay-builtins-dirOf.exp(1 hunks)tests/functional/lang/eval-okay-builtins-dirOf.nix(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/libstore/unix/build/derivation-builder.cc (3)
src/libstore/include/nix/store/restricted-store.hh (3)
path(55-60)path(55-55)path(69-69)src/libstore/unix/build/linux-derivation-builder.cc (2)
path(706-732)path(706-706)src/libstore/unix/build/chroot-derivation-builder.cc (2)
path(182-198)path(182-182)
src/libstore/include/nix/store/restricted-store.hh (3)
src/libstore/unix/build/derivation-builder.cc (7)
path(194-197)path(194-194)path(328-328)path(336-336)path(408-408)addDependencyImpl(1189-1192)addDependencyImpl(1189-1189)src/libstore/unix/build/linux-derivation-builder.cc (2)
path(706-732)path(706-706)src/libstore/restricted-store.cc (2)
isAllowed(29-32)isAllowed(29-29)
src/libexpr/primops.cc (1)
src/libexpr/include/nix/expr/eval.hh (26)
pos(712-713)pos(729-736)pos(759-759)pos(764-764)pos(769-773)pos(790-790)pos(904-904)pos(1042-1042)v(654-657)v(654-654)v(659-659)v(665-665)v(670-670)v(671-671)v(672-672)v(674-674)v(679-679)v(683-683)v(684-684)v(685-690)v(691-691)v(710-710)v(875-875)v(952-952)context(738-738)context(1026-1026)
src/libstore/unix/build/linux-derivation-builder.cc (3)
src/libstore/include/nix/store/restricted-store.hh (3)
path(55-60)path(55-55)path(69-69)src/libstore/unix/build/derivation-builder.cc (5)
path(194-197)path(194-194)path(328-328)path(336-336)path(408-408)src/libstore/unix/build/chroot-derivation-builder.cc (2)
path(182-198)path(182-182)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (7)
.version (1)
1-1: Version bump looks good.No concerns.
nix-meson-build-support/common/meson.build (1)
61-63: Define placement is correct; keep it libstdc++-only.The project argument to disable the TBB backend is scoped right. No change requested.
src/libstore/unix/build/derivation-builder.cc (2)
328-329: Interface rename/override looks consistent.Protected virtual addDependencyImpl is the right seam for the wrapper pattern.
1189-1193: Base impl should only record. LGTM.addDependencyImpl inserting into addedPaths centralizes the allow-check semantics.
tests/functional/lang/eval-okay-builtins-dirOf.exp (1)
1-1: Coverage is solid.Good mix of edge cases: empty, no sep, root variants, repeated slashes, ., .., and non-existent absolute paths.
tests/functional/lang/eval-okay-builtins-dirOf.nix (1)
1-21: Nice, exhaustive test matrix.Covers strings vs paths, roots, multi-seps, and dot components. Reads clearly.
src/libstore/include/nix/store/restricted-store.hh (1)
55-60: Thread safety assumption contradicts the review's own concerns.The review claims the
isAllowed(path)check inLinuxDerivationBuilder::addDependencyImplis redundant and "will never evaluate to true." However, this conclusion depends on single-threaded execution with no concurrent state changes. The same review flags "no apparent thread safety mechanisms," which undermines this assumption.In a multithreaded context, another thread could add the path to
addedPathsbetween the wrapper's check (line 57) and the impl's execution, making the defensive check in the implementation (line 708) prudent rather than redundant. The comment on lines 65-68 states the caller will ensure preconditions, but this doesn't guarantee thread safety across concurrent calls.Before removing the check, verify whether the codebase is guaranteed single-threaded or whether the defensive check is intentional protection against concurrent modification.
Likely an incorrect or invalid review comment.
4 participants
Motivation
Context
Summary by CodeRabbit
Bug Fixes
Tests
Chores