Sync with upstream 2.33.1 #318
Conversation
(cherry picked from commit 819a61a)
…tenance [Backport 2.33-maintenance] [libstore]: Fix a heap-use-after-free bug
Unfortunately previous tarball caches had loose objects written to them and subsequent switch to thin packfiles. This results in possibly broken thin packfiles when the loose objects backend is disabled. Thin packfiles do not necessarily contain the whole closure of objects. When packfilesOnly is true we end up with an inconsistent state where a tree lives in a packfiles which refers to a blob in the loose objects backend. In the future we might want to nuke old cache directories and repack the tarball cache. (cherry picked from commit 0ffe83a)
…tenance libfetchers: Bump tarball-cache version to v2 [backport 2.33]
Bumps [korthout/backport-action](https://github.com/korthout/backport-action) from 3.4.1 to 4.0.1. - [Release notes](https://github.com/korthout/backport-action/releases) - [Commits](korthout/backport-action@d074166...c656f5d) --- updated-dependencies: - dependency-name: korthout/backport-action dependency-version: 4.0.1 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> (cherry picked from commit 4227d24)
…tenance [Backport 2.33-maintenance] build(deps): bump korthout/backport-action from 3.4.1 to 4.0.1
- Skip packages that don't build for Windows when building for windows - Automatically disable kaitai / json schema, fixing todo - Skip native build of Nix for manual (cherry picked from commit a5edc2d)
…tenance [Backport 2.33-maintenance] Fix up dev shell in a few ways
There are other types of sockets. (cherry picked from commit 79750a3)
These functions use `SOCKET` not `int`, despite them being unix functions. (cherry picked from commit 208ed3c)
…tenance [Backport 2.33-maintenance] Windows fixes
Some filesystems, notably most FUSE-based ones and some top-level overlaysfs ones do not support this and we need a graceful fallback. (cherry picked from commit 06f2159)
…tenance [Backport 2.33-maintenance] libutil: Gracefully fall back from unsupported O_TMPFILE
…tenance [Backport 2.33-maintenance] libstore/store-api: Do not query all substituters for substitutable p…
The previous error message was ambiguous about which specific directory failed the check. This commit updates checkNotWorldWritable to return the failing path so it can be included in the error message, making debugging easier. (cherry picked from commit a1e24fa)
…tenance [Backport 2.33-maintenance] libstore: include path in the world-writable error
(cherry picked from commit 7541129)
…tenance [Backport 2.33-maintenance] Fix `curl` with `c-ares` failing to resolve DNS inside sandbox on macOS
Runs the tests against the new daemon as well as the cli. This more reliably shares the artifact (not relying directly on github actions cache). We've seen github evict our caches super fast, so it would be nice to move away from it entirely if possible. (cherry picked from commit 6eebfe6)
Also bumps download-artifact to v7.0.0. (cherry picked from commit c54af23)
…tenance [Backport 2.33-maintenance] ci: Run flake-regressions also with the newly built daemon
Best reviewed with -w --color-moved. This just moves the code into a separate workflow. This will allow us to reuse it in the release job for github releng of releases. (cherry picked from commit 745983d)
(cherry picked from commit fb05f6d)
This should allow reusing this workflow (with more tweaks) in the releng workflow. (cherry picked from commit c867ed6)
…tenance [Backport 2.33-maintenance] ci: Move docker_push_image into a separate workflow
This allows for testing with a local minio deployment like: ./upload-release.pl --skip-docker --skip-git --s3-endpoint http://localhost:9000 --s3-host localhost:9000 1821360 (cherry picked from commit d19b8d5)
Previously it was only Eeclo doing releases that were signed with B541D55301270E0BCF15CA5D8170B4726D7198DE. Other linux distributions have the expectation (rightfully so) that our tags are signed. Let's document this. We could do cross-signing to make tracing the chain of trust easier for all Nix team members [1]. [1]: https://nixos.org/community/teams/nix/ (cherry picked from commit 6cb8b58)
This workflow is supposed to automate release uploads by using OIDC for AWS setup. DockerHub still uses long-lived credentials, but that's not fixable. In a follow-up we could set up release uploads to GHCR too. (cherry picked from commit 4599daa)
(cherry picked from commit a156945)
(cherry picked from commit 3933e45)
(cherry picked from commit 84ff2ef)
…tenance [Backport 2.33-maintenance] ci: GitHub releng for release automation
I messed up and accidentally configured the S3 client to use the same host as the nix-releases bucket, but nix-channels is us-east-1 and nix-releases is eu-west-1. (cherry picked from commit 0900638)
…tenance [Backport 2.33-maintenance] upload-release.pl: Fix up nix-channels bucket location, use awscli2
Previously builtins.readDir would return an empty attribute set instead of barfing on non-existent paths. This is a regression from 2.32 for impure eval. (cherry picked from commit 4ab2cda)
…tenance [Backport 2.33-maintenance] libutil/union-source-accessor: Barf on non-existent directories
Tagging release 2.33.1
📝 WalkthroughWalkthroughThis PR performs a version bump to 2.33.1, reorganizes socket utility abstractions into a dedicated header, refactors the release process to use GitHub Actions workflows, removes the backport workflow, updates dev-shell dependencies with platform-conditional logic, improves anonymous temp file creation with O_TMPFILE optimization, adds error handling for missing directories in UnionSourceAccessor, and introduces comprehensive test coverage for readDir behavior on symlinks and non-existent paths. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
✏️ Tip: You can disable this entire section by setting 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: 5
🤖 Fix all issues with AI agents
In @.github/workflows/backport.yml:
- Line 29: Update the backport action usage to either revert to the v3.x pin or
verify runner compatibility with v4.0.1: if keeping
korthout/backport-action@c656f5d5851037b2b38fb5db2691a03fa229e3b2 (v4.0.1),
ensure CI runners use Node.js 24 and GitHub Actions runner >= 2.327.1 and run a
full workflow test; alternatively pin to the last v3.x release to avoid Node 24
and runner requirements. Also review any commands calling execa in your
workflows or scripts and test them against the refactor to `@actions/exec` to
confirm behavior parity before merging.
In @.github/workflows/docker-push.yml:
- Around line 55-59: The install-nix-action step (uses:
./.github/actions/install-nix-action) is missing the required github_token
input; update that action invocation to include github_token: ${{
secrets.GITHUB_TOKEN }} alongside the existing inputs (dogfood and
extra_nix_config) so the action receives the mandatory token parameter.
In @.github/workflows/upload-release.yml:
- Around line 24-29: The workflow step using the local action install-nix-action
is missing the required github_token input; update the step that calls
install-nix-action to pass the repository token by adding the github_token input
(e.g., set github_token to the built-in secrets.GITHUB_TOKEN) alongside
dogfood/use_cache/extra_nix_config so the action’s required input is provided
when invoking install-nix-action.
In `@packaging/dev-shell.nix`:
- Around line 299-308: Fix the typo in the inline comment inside the dev shell
build expression: change the word "ross" to "cross" in the comment that explains
why we filter out "nix" from component nativeBuildInputs (the comment near the
expression using lib.filter, lib.lists.concatMap, and
activeComponents/nativeBuildInputs). Leave the surrounding logic untouched; only
correct the comment text to read "cross dev shell" instead of "ross dev shell".
In `@src/libstore/unix/build/derivation-builder.cc`:
- Around line 696-707: The error message in checkNotWorldWritable refers to
symlinks but the code only tests S_IWOTH; update the function to detect symlinks
using the lstat result (test S_ISLNK(st.st_mode)) and throw a clear error when
the path is a symlink or when it's world-writable (use separate messages or
include which condition triggered), keep the traversal via path.parent_path()
as-is, and remove the redundant final return; also ensure you call lstat
correctly and use the obtained struct stat (from lstat) when testing S_IWOTH and
S_ISLNK.
🧹 Nitpick comments (7)
maintainers/keys/README.md (1)
7-11: Consider adding an OpenPGP link for Eelco Dolstra's key for consistency.Sergei Zimmerman's fingerprint is linked to
keys.openpgp.org, while Eelco Dolstra's is not. Consider adding a similar link for consistency, which would also make it easier to verify and import the key.Suggested change
- **Eelco Dolstra** - GPG Fingerprint: `B541 D553 0127 0E0B CF15 CA5D 8170 B472 6D71 98DE` + GPG Fingerprint: [`B541 D553 0127 0E0B CF15 CA5D 8170 B472 6D71 98DE`](https://keys.openpgp.org/vks/v1/by-fingerprint/B541D55301270E0BCF15CA5D8170B4726D7198DE)maintainers/upload-release.pl (2)
95-120: Potential issue with undefineds3_endpointin regex match.On line 105,
$opt->s3_endpointcould be undefined when evaluating the regex=~ /^http:/. While Perl handles this gracefully (treats as empty string), it may generate a warning underuse warnings. Consider adding an explicit check.Similarly, lines 115-116 have redundant conditions—
$opt->s3_endpoint ? (host => $opt->s3_host)sets host tos3_hostonly whens3_endpointis provided, but this seems intentional for the channels bucket to use the default host unless a custom endpoint is specified.♻️ Optional: Explicit check for s3_endpoint
- secure => ($opt->s3_endpoint && $opt->s3_endpoint =~ /^http:/) ? 0 : 1, + secure => ($opt->s3_endpoint // '') =~ /^http:/ ? 0 : 1,
237-239: Missing semicolon after statement.Line 238 is missing a trailing semicolon. While Perl tolerates this as the last statement in a block, it's better practice to include it for consistency and to avoid issues if code is added later.
♻️ Suggested fix
$dockerManifest .= " --amend $tag"; - $dockerManifestLatest .= " --amend $latestTag" + $dockerManifestLatest .= " --amend $latestTag"; }.github/workflows/upload-release.yml (1)
53-69: Redundant execution of upload-release.pl script.The script is executed twice: once for S3/Docker Hub upload (lines 54-57) and once for GHCR (lines 63-67). Consider whether this could be consolidated into a single invocation with appropriate flags, or if the current separation is intentional for isolation purposes.
.github/workflows/docker-push.yml (2)
75-81: Quote shell variables to prevent word splitting.Variables
$NIX_VERSIONand$DOCKERHUB_REPOshould be quoted to prevent issues if they ever contain spaces or special characters.♻️ Proposed fix
run: | - docker tag nix:$NIX_VERSION $DOCKERHUB_REPO:$NIX_VERSION - docker push $DOCKERHUB_REPO:$NIX_VERSION + docker tag "nix:$NIX_VERSION" "$DOCKERHUB_REPO:$NIX_VERSION" + docker push "$DOCKERHUB_REPO:$NIX_VERSION" if [ "$IS_MASTER" = "true" ]; then - docker tag nix:$NIX_VERSION $DOCKERHUB_REPO:master - docker push $DOCKERHUB_REPO:master + docker tag "nix:$NIX_VERSION" "$DOCKERHUB_REPO:master" + docker push "$DOCKERHUB_REPO:master" fi
92-101: Quote shell variables consistently in GHCR push step.Same quoting concern as the Docker Hub push step.
♻️ Proposed fix
run: | IMAGE_ID=ghcr.io/${{ github.repository_owner }}/nix IMAGE_ID=$(echo $IMAGE_ID | tr '[A-Z]' '[a-z]') - docker tag nix:$NIX_VERSION $IMAGE_ID:$NIX_VERSION - docker push $IMAGE_ID:$NIX_VERSION + docker tag "nix:$NIX_VERSION" "$IMAGE_ID:$NIX_VERSION" + docker push "$IMAGE_ID:$NIX_VERSION" if [ "$IS_MASTER" = "true" ]; then - docker tag nix:$NIX_VERSION $IMAGE_ID:master - docker push $IMAGE_ID:master + docker tag "nix:$NIX_VERSION" "$IMAGE_ID:master" + docker push "$IMAGE_ID:master" fisrc/libutil/include/nix/util/socket.hh (1)
17-56: Ensure Descriptor↔Socket casts are size-safe on Windows.These
reinterpret_casts assumeDescriptorandSocketare the same width and that only socket-backed descriptors are passed on Windows. Please confirm the invariant; a compile-time guard would make the contract explicit.♻️ Possible guard
`#ifdef` _WIN32 +static_assert(sizeof(Descriptor) == sizeof(Socket), + "Descriptor and Socket must be the same size on Windows"); /** * Windows gives this a different name */ # define SHUT_WR SD_SEND
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.github/workflows/backport.yml.github/workflows/docker-push.yml.github/workflows/upload-release.yml.versionmaintainers/keys/158A6F530EA202E5F651611314FAEA63448E1DF9.ascmaintainers/keys/B541D55301270E0BCF15CA5D8170B4726D7198DE.ascmaintainers/keys/README.mdmaintainers/release-process.mdmaintainers/upload-release.plpackaging/dev-shell.nixsrc/libstore/include/nix/store/build/derivation-builder.hhsrc/libstore/store-api.ccsrc/libstore/unix/build/derivation-builder.ccsrc/libstore/unix/build/sandbox-network.sbsrc/libutil/file-system.ccsrc/libutil/include/nix/util/meson.buildsrc/libutil/include/nix/util/socket.hhsrc/libutil/include/nix/util/unix-domain-socket.hhsrc/libutil/serialise.ccsrc/libutil/union-source-accessor.ccsrc/nix/unix/daemon.cctests/functional/lang/eval-fail-readDir-nonexistent-1.err.exptests/functional/lang/eval-fail-readDir-nonexistent-1.nixtests/functional/lang/eval-fail-readDir-nonexistent-2.err.exptests/functional/lang/eval-fail-readDir-nonexistent-2.nixtests/functional/lang/eval-fail-readDir-not-a-directory-1.err.exptests/functional/lang/eval-fail-readDir-not-a-directory-1.nixtests/functional/lang/eval-fail-readDir-not-a-directory-2.err.exptests/functional/lang/eval-fail-readDir-not-a-directory-2.nixtests/functional/lang/eval-okay-readDir-symlinked-directory.exptests/functional/lang/eval-okay-readDir-symlinked-directory.nix
🧰 Additional context used
🧬 Code graph analysis (2)
src/nix/unix/daemon.cc (1)
src/libutil/include/nix/util/file-descriptor.hh (1)
getStandardInput(115-115)
src/libutil/union-source-accessor.cc (3)
src/libutil/include/nix/util/source-path.hh (1)
accessor(117-120)src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh (15)
path(35-35)path(37-37)path(39-39)path(41-41)path(43-43)path(45-45)path(47-47)path(49-49)path(51-51)path(53-53)path(55-55)path(61-61)path(66-66)path(98-98)path(100-100)src/libfetchers/filtering-source-accessor.cc (3)
path(99-102)showPath(59-62)showPath(59-59)
🪛 actionlint (1.7.10)
.github/workflows/upload-release.yml
24-24: missing input "github_token" which is required by action "Install Nix" defined at "./.github/actions/install-nix-action". all required inputs are "dogfood", "github_token"
(action)
.github/workflows/docker-push.yml
55-55: missing input "github_token" which is required by action "Install Nix" defined at "./.github/actions/install-nix-action". all required inputs are "dogfood", "github_token"
(action)
🪛 Clang (14.0.6)
src/libutil/include/nix/util/socket.hh
[error] 4-4: 'nix/util/file-descriptor.hh' file not found
(clang-diagnostic-error)
🪛 markdownlint-cli2 (0.18.1)
maintainers/release-process.md
10-10: Bare URL used
(MD034, no-bare-urls)
110-110: Dollar signs used before commands without showing output
(MD014, commands-show-output)
183-183: Dollar signs used before commands without showing output
(MD014, commands-show-output)
⏰ 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_x86_64-linux / build
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (39)
src/libstore/unix/build/sandbox-network.sb (1)
17-19: Scoped DNS permission looks appropriate.This adds a narrowly targeted mach-lookup for DNS configuration, aligned with the DNS lookup allowance above.
tests/functional/lang/eval-okay-readDir-symlinked-directory.nix (1)
1-1: Test input looks good.Simple, focused readDir invocation that matches the intended scenario.
tests/functional/lang/eval-okay-readDir-symlinked-directory.exp (1)
1-1: Expectation fixture is consistent.Config flag aligns with the test case context.
tests/functional/lang/eval-fail-readDir-nonexistent-2.nix (1)
1-3: LGTM for eval-fail fixture.
Matches the intended non-existent relative path scenario and is minimal/clear.tests/functional/lang/eval-fail-readDir-nonexistent-1.nix (1)
1-3: LGTM for eval-fail fixture.
Covers absolute-path non-existence case cleanly.tests/functional/lang/eval-fail-readDir-nonexistent-1.err.exp (1)
1-16: Expected error output looks consistent.
Line/column pointers and final “does not exist” message align with the fixture.tests/functional/lang/eval-fail-readDir-nonexistent-2.err.exp (1)
1-16: Expected error output looks consistent.
The resolved absolute path for the relative input is consistent with /pwd/lang.src/libstore/store-api.cc (1)
452-463: Confirm substituter ordering before short‑circuiting.
Breaking after the first successful substituter changes selection semantics (first match now wins). Please verifygetDefaultSubstituters()is deterministic and already ordered by desired priority; otherwise this could select a lower‑priority substituter compared to the previous “last match wins” behavior.src/libutil/union-source-accessor.cc (1)
38-49: LGTM! Behavioral fix aligns with sibling methods.The addition of the
existsflag and theFileNotFoundthrow makesreadDirectoryconsistent withreadFileandreadLink, which both throw when the path doesn't exist in any accessor. Previously, this method would silently return an emptyDirEntriesfor non-existent paths, which could mask errors.maintainers/keys/158A6F530EA202E5F651611314FAEA63448E1DF9.asc (1)
1-110: LGTM!The PGP public key block is properly formatted with valid ASCII armor delimiters. The filename correctly corresponds to the GPG fingerprint, and the key identity matches Sergei Zimmerman as documented in the README.
maintainers/keys/B541D55301270E0BCF15CA5D8170B4726D7198DE.asc (1)
1-51: LGTM!The PGP public key block is properly formatted with valid ASCII armor delimiters. The filename correctly corresponds to the GPG fingerprint, and the key identities match Eelco Dolstra as documented in the README.
tests/functional/lang/eval-fail-readDir-not-a-directory-1.nix (1)
1-3: LGTM!This test correctly exercises the error path when
builtins.readDiris called on a regular file rather than a directory. The test structure follows the standardeval-fail-*pattern and pairs properly with its.err.expexpectation file.tests/functional/lang/eval-fail-readDir-not-a-directory-2.nix (1)
1-3: LGTM!This test complements the first test by verifying error handling when
readDiris applied to a symlink that resolves to a non-directory. The attribute namesymlinkedRegularFileclearly documents the test scenario.tests/functional/lang/eval-fail-readDir-not-a-directory-2.err.exp (1)
1-16: LGTM!The expected error output correctly captures:
- The evaluation trace through
symlinkedRegularFileattribute- The
readDirbuiltin call context with accurate line/column references- The resolved symlink target path in the final error message
The path difference between
./readDir/linked(input) and/pwd/lang/readDir/foo/git-hates-directories(error) correctly demonstrates thatreadDirfollows symlinks before attempting to read the directory.tests/functional/lang/eval-fail-readDir-not-a-directory-1.err.exp (1)
1-16: LGTM!The expected error output properly documents the evaluation failure trace with accurate source locations. The error path
/pwd/lang/readDir/bardirectly corresponds to the test input./readDir/bar, confirming this tests the direct regular-file case (vs. the symlink case in test 2).src/libstore/include/nix/store/build/derivation-builder.hh (1)
82-82: Good fix for lifetime safety.Changing
initialOutputsfrom a const reference to by-value storage eliminates the risk of dangling references. This aligns with the PR's stated fix for heap-use-after-free in libstore. The constructor inderivation-builder.cccorrectly movesDerivationBuilderParams, so the copy overhead is minimized.src/libstore/unix/build/derivation-builder.cc (1)
728-729: LGTM!The refactored call site is cleaner - exception-based error handling removes the need for explicit return value checks and is consistent with error handling patterns used elsewhere in this codebase.
src/libutil/file-system.cc (2)
735-737: LGTM!Correct platform guard for Windows, where file deletion semantics while the file is open differ from Unix. On Windows, the temp file will be cleaned up when the process closes the descriptor or exits, rather than being immediately unlinked.
716-729: Well-designed graceful fallback for O_TMPFILE unsupported filesystems.The implementation correctly:
- Uses
O_EXCLwithO_TMPFILEto prevent accidental linking vialinkat().- Handles both
EOPNOTSUPP(POSIX) andEISDIR(historical Linux < 3.11 indicator) for proper fallback.- Uses
std::atomic_flagfor thread-safe memoization with benign race conditions.The project uses C++23 as its standard, which fully supports
std::atomic_flag::test()(C++20+ feature), so the code is compatible..version (1)
1-1: LGTM!Version bump to 2.33.1 aligns with the PR objective to sync with upstream.
maintainers/release-process.md (2)
107-117: LGTM!The updated release process now properly separates concerns: local tagging via
upload-release.pl(with--skip-docker --skip-s3) followed by triggering theupload-release.ymlworkflow for artifact uploads. This provides a cleaner, more auditable release pipeline.
180-191: LGTM!Point release documentation correctly mirrors the main release flow with the same workflow-driven approach.
packaging/dev-shell.nix (5)
151-158: LGTM!Clean implementation of
activeComponentNamesandisActiveComponentfor efficient O(1) membership testing. Usinglib.listToAttrsto create a set-like lookup is idiomatic Nix.
202-214: LGTM!The
lib.intersectAttrswithlib.genAttrspattern is a clean way to select a subset of components. This is more readable than the previous override approach.
282-293: LGTM!Conditional Meson flags for
kaitai-struct-checksandjson-schema-checksbased on active component membership is well-implemented.
325-325: LGTM!Platform-specific inclusion of
gdbonly on Unix hosts is appropriate since gdb may not be available or functional on non-Unix platforms like Windows.
341-347: LGTM!Conditional inclusion of
gbenchmarkon Unix platforms addresses the Windows build compatibility noted in the TODO comment.maintainers/upload-release.pl (4)
17-40: LGTM!Comprehensive CLI option parsing with
Getopt::Long::Descriptive. The help text includes environment variables documentation, and the validation ensuring--project-rootis required unless--skip-gitis appropriate.
204-260: LGTM!Docker handling is well-refactored:
- Configurable
docker_ownerenables pushing to different registries- Multi-platform manifest creation is properly gated
- The
isLatestconditional correctly controls latest tag behavior
264-320: LGTM!The S3 upload logic is properly gated by
--skip-s3. The cross-platform tarball downloads are wrapped ineval/warnblocks for graceful degradation when builds aren't available. Content-type detection for uploads is appropriate.
323-329: LGTM!Git tagging logic properly respects
--skip-gitand uses--force-with-leasefor safer force-push of thelatest-releasebranch..github/workflows/upload-release.yml (1)
1-69: Workflow structure looks reasonable.The workflow appropriately:
- Uses pinned action versions with SHA commits
- Configures OIDC for AWS credentials
- Sets minimal required permissions
- Uses environment secrets for the
releasesenvironment.github/workflows/docker-push.yml (2)
27-40: Good practice: secrets check before attempting push.The
check_secretsjob pattern is a good approach to gracefully handle missing secrets in fork environments, avoiding workflow failures when secrets aren't configured.
42-48: Appropriate permission scoping.The
pushjob correctly requests onlycontents: readandpackages: writepermissions, following the principle of least privilege.src/libutil/include/nix/util/meson.build (1)
69-69: LGTM — exportingsocket.hhmatches the new public API.src/nix/unix/daemon.cc (1)
440-456: LGTM —Socketwrapper is applied consistently in the stdio forwarding loop.src/libutil/include/nix/util/unix-domain-socket.hh (1)
4-7: LGTM — centralized socket helpers include.src/libutil/serialise.cc (2)
1-5: LGTM — include aligns with the new socket abstraction.
184-200: LGTM —hasData()now uses the socket abstraction consistently.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| static void checkNotWorldWritable(std::filesystem::path path) | ||
| { | ||
| while (true) { | ||
| auto st = lstat(path); | ||
| if (st.st_mode & S_IWOTH) | ||
| return false; | ||
| throw Error("Path %s is world-writable or a symlink. That's not allowed for security.", path); | ||
| if (path == path.parent_path()) | ||
| break; | ||
| path = path.parent_path(); | ||
| } | ||
| return true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Error message mentions symlinks but code doesn't check for them.
The error message at line 701 states "Path %s is world-writable or a symlink", but the condition only checks S_IWOTH (world-writable bit). The symlink portion of the message appears to be outdated or incorrect.
Suggested fix
if (st.st_mode & S_IWOTH)
- throw Error("Path %s is world-writable or a symlink. That's not allowed for security.", path);
+ throw Error("Path %s is world-writable. That's not allowed for security.", path);Also, the return; statement at line 706 is redundant at the end of a void function.
Optional cleanup
path = path.parent_path();
}
- return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static void checkNotWorldWritable(std::filesystem::path path) | |
| { | |
| while (true) { | |
| auto st = lstat(path); | |
| if (st.st_mode & S_IWOTH) | |
| return false; | |
| throw Error("Path %s is world-writable or a symlink. That's not allowed for security.", path); | |
| if (path == path.parent_path()) | |
| break; | |
| path = path.parent_path(); | |
| } | |
| return true; | |
| return; | |
| } | |
| static void checkNotWorldWritable(std::filesystem::path path) | |
| { | |
| while (true) { | |
| auto st = lstat(path); | |
| if (st.st_mode & S_IWOTH) | |
| throw Error("Path %s is world-writable. That's not allowed for security.", path); | |
| if (path == path.parent_path()) | |
| break; | |
| path = path.parent_path(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/libstore/unix/build/derivation-builder.cc` around lines 696 - 707, The
error message in checkNotWorldWritable refers to symlinks but the code only
tests S_IWOTH; update the function to detect symlinks using the lstat result
(test S_ISLNK(st.st_mode)) and throw a clear error when the path is a symlink or
when it's world-writable (use separate messages or include which condition
triggered), keep the traversal via path.parent_path() as-is, and remove the
redundant final return; also ensure you call lstat correctly and use the
obtained struct stat (from lstat) when testing S_IWOTH and S_ISLNK.
Motivation
Context
Summary by CodeRabbit
Bug Fixes
readDirbuiltin to properly error when reading non-existent or non-directory paths.Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.