Skip to content

fix(pdump): exclude build environments from loadup images - #579

Open
eval-exec wants to merge 2 commits into
mainfrom
fix/issue578-pdump-environment
Open

eval-exec wants to merge 2 commits into
mainfrom
fix/issue578-pdump-environment

Conversation

@eval-exec

Copy link
Copy Markdown
Owner

Dump construction captured the builder's entire environment in initial-environment, retaining build-only Nix store paths even though runtime startup replaced the environment. GNU Emacs 31.1 skips this snapshot while constructing a dump.

This change uses the typed loadup mode to omit the construction-time snapshot and seed a portable shell before customization records its standard value. It preserves exec-path while loadup queries Git, then excludes it only during image serialization. Native construction state resets on restoration and runtime activation; ordinary runtime redumps retain their existing behavior. The Context layout remains unchanged, with compile-time size and existing offset checks.

Public dump/load regressions cover production and bootstrap images, inspect image bytes for build markers, preserve repository metadata, and verify runtime environment lists, real subprocesses, and subsequent user redumps. The Nix package also rejects references to build-only inputs.

Fixes #578.

Verification

The branch is rebased onto 48ec663ed2 (origin/main). The dump regression tests were rerun after rebasing. Before rebasing, the same fix passed:

  • cargo nextest run -p neovm-core --lib --no-fail-fast: 14,769 passed, 67 skipped.
  • Public dump/load regressions: 2 passed; GNU oracle environment checks: 7 passed.
  • TUI shell/process checks: 2 passed; GUI process checks passed under both X11 and Wayland.
  • cargo check --workspace --all-targets, cargo xtask fresh-build --release, and cargo fmt --all --check.
  • Full Nix package build and installed-runtime environment/subprocess checks. The measured closure was 2.10 GiB / 307 paths, with none of the eight forbidden build inputs in the closure. Both installed dump images contained no /nix/store paths. This is a current-build measurement, not a controlled comparison with the reporter's older build.
  • An actual production dump generated with synthetic build-environment, PATH, and SHELL markers contained none of those markers.

Verification was performed on Linux. Whole-flake Nix formatting still fails on the unchanged nix/dependencies.nix; the changed nix/package.nix passes the pinned formatter.

GNU Emacs 31.1 skips set_initial_environment while constructing a dump.
Neomacs instead retained the entire builder environment through
initial-environment, pinning build-only Nix dependencies in the output.

Use the typed loadup dump mode to omit that snapshot, seed a portable
shell before cus-start captures its standard value, and exclude exec-path
only during serialization so loadup can still run Git. Keep construction
state native and reset it when restoring or activating a runtime image.
Keep Context offsets stable using trailing padding, an ABI guard, and a
compile-time size assertion. Ordinary runtime redumps retain their
existing environment policy.

Exercise both image modes through public dump/load APIs in isolated
processes, scan image bytes, preserve repository metadata, and verify
runtime environment, subprocesses, and subsequent user redumps. Reject
build-only references in the Nix package output.

Fixes #578.
Copilot AI balanced review requested due to automatic review settings October 18, 2026 16:19
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6692ae97-457c-4051-a1f6-35488c6f4a05
📥 Commits

Reviewing files that changed from the base of the PR and between 3d7d239 and 4e99d07.

📒 Files selected for processing: 1
  • crates/neovm-core/tests/dump_environment.rs
🚧 Files skipped from review as they are similar to previous changes: 1
  • crates/neovm-core/tests/dump_environment.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

📝 Walkthrough

Dump construction now uses dump-specific environment and shell initialization. Dump serialization temporarily clears exec-path in dump mode and restores it afterward. Integration tests check production and bootstrap images, runtime activation, and ordinary runtime redumps. The Nix package build rejects outputs that retain listed build inputs.

Changes: Dump environment isolation

Layer / File(s) Summary
Initialize dump-specific state
crates/neovm-core/src/emacs_core/runtime/eval/mod.rs, crates/neovm-core/src/emacs_core/runtime/eval/construct.rs, crates/neovm-core/src/emacs_core/runtime/eval/pdump_reconstruct.rs, crates/neovm-core/src/emacs_core/system/shell_file_name/mod.rs, crates/neovm-core/src/emacs_core/system/environment/mod.rs, crates/neovm-core/src/emacs_core/lisp/load/mod.rs
Context now stores optional loadup dump state. Loadup selects environment and shell initialization by invocation, using a dump-specific shell default during dump construction.
Serialize without build environment
crates/neovm-core/src/emacs_core/lisp/native/builtins/symbols.rs, crates/neovm-core/src/emacs_core/lisp/load/mod.rs
When dump mode is set, builtin_dump_emacs_portable temporarily clears exec-path during serialization and restores it afterward. Runtime activation clears the dump mode.
Validate dump contents and package references
crates/neovm-core/Cargo.toml, crates/neovm-core/tests/dump_environment.rs, nix/package.nix
Production and bootstrap integration tests check dump contents, runtime environment activation, and ordinary runtime redumps. The Nix package build rejects outputs that retain listed build inputs.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Loadup
  participant Context
  participant DumpFunction as builtin_dump_emacs_portable
  participant DumpFile as dump_to_file
  participant RuntimeActivation
  Loadup->>Context: Set loadup_dump_mode for Dump
  Context->>DumpFunction: Retain dump mode
  DumpFunction->>DumpFile: Clear exec-path and write dump
  DumpFile-->>DumpFunction: Return from dump
  DumpFunction->>Context: Restore exec-path
  RuntimeActivation->>Context: Clear loadup_dump_mode
Loading
Merge Risk: ⚪ Minimal · up to 4e99d

No merge-blocking issue is identified from the supplied changes; normal checks remain appropriate.

Security architecture risk: 🔵 Low · up to 3d7d2

The change reduces build-host data carried into distributed images while retaining runtime settings from the user's environment. No introduced security defect was established. Cleanup after abrupt interruption remains less fully demonstrated than normal execution and returned errors.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The relevant propagation scope is an image and its consumers: builder-controlled environment or executable paths could otherwise be distributed with production or bootstrap state. The PR reduces that propagation rather than adding a remote entrypoint or establishing a new privilege transition.

Trust Boundaries and Controls

  • observed — Construction-time shell installation supplies neither inherited SHELL nor an executable-directory input. Runtime shell installation still accepts the launching environment's SHELL and feeds shell-process consumers, as before this PR. That existing runtime authority is distinct from retaining the builder's shell in an image.

Resilience and Maintainability Implications

  • inferred — Returned-error cleanup is supported, but a Rust unwind during serialization could bypass the linear restoration sequence. Whether a caller catches such a panic and reuses the construction evaluator was not established. This remains a recovery proof gap, not a verified introduced security failure.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: excluding build environments from portable dump images.
Description check Passed The description directly explains the dump-environment changes, implementation details, tests, packaging safeguards, and verification results.
Linked Issues check Passed Issue #578 requires dump images to omit build-time environment data and prevent build-only Nix references. The loadup path now uses dump-specific environment initialization, and serialization temporar…
Out of Scope Changes check Passed The changes stay within issue #578. The Context field and layout assertion support non-serialized dump state. Runtime reset logic and redump tests protect behavior affected by the environment change…
Full details: Linked Issues check

Explanation

Issue #578 requires dump images to omit build-time environment data and prevent build-only Nix references. The loadup path now uses dump-specific environment initialization, and serialization temporarily removes exec-path for dump construction. Native dump state resets during restoration and runtime activation. Production and bootstrap tests inject build markers, inspect dump bytes, verify runtime environment and subprocess behavior, and test ordinary redumps. nix/package.nix rejects Rust, build-tool, source, and Cargo artifact references.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #578. The Context field and layout assertion support non-serialized dump state. Runtime reset logic and redump tests protect behavior affected by the environment change. Loadup, shell, serialization, test, and Nix package changes implement or verify removal of build-time environment data.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 2
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The change touches ABI-pinned Context layout, dump serialization internals, and platform/packaging behavior for every produced image, which warrants final human review despite being well-tested.

1 open issue
What changed in this PR

This PR fixes issue #578, where the portable dump (neomacs.pdump) captured the builder's entire process environment (initial-environment/process-environment, plus the build-time SHELL and exec-path), turning build-only Nix store paths into runtime closure dependencies (and leaking CI env into .deb assets). The fix mirrors GNU Emacs 31.1's !will_dump_p() behavior: when loadup.el is evaluated to construct a dump, host environment state is not snapshotted into the image, while exec-path is deliberately retained until the moment of serialization (so loadup can still query Git) and then excluded only while writing the image. Runtime activation re-derives the environment, shell, and search path from the real process, and ordinary user redumps keep their existing behavior.

Changes:

  • Thread a non-serialized loadup_dump_mode flag through Context (placed in trailing repr(C) padding, with a size assertion) and set it only on the LoadupInvocation::Dump path; dump-emacs-portable uses it to blank exec-path around serialization.
  • Skip the host-environment snapshot during dump construction and seed a portable shell-file-name default (install_dump_default) before cus-start records its standard value.
  • Add a Nix disallowedReferences regression guard and a new dump_environment integration test covering production and bootstrap images, byte-level marker checks, runtime activation, and user redump.
File Description
nix/​package.nix Adds disallowedReferences guard to fail the build if build-only inputs leak into the output; reflows a lib.optionals call.
crates/​neovm-core/​tests/​dump_environment.rs New integration test verifying dumps exclude build env and runtime re-derives it.
crates/​neovm-core/​Cargo.toml Registers the new explicit [[test]] target (required under autotests = false).
crates/​neovm-core/​src/​emacs_core/​lisp/​load/​mod.rs Branches env/shell setup on PreloadOnly vs Dump; sets/resets loadup_dump_mode; retains exec-path until serialization.
crates/​neovm-core/​src/​emacs_core/​lisp/​native/​builtins/​symbols.rs Excludes exec-path from the image only for loadup dumps, saving/restoring it in native locals.
crates/​neovm-core/​src/​emacs_core/​system/​shell_file_name/​mod.rs Adds install_dump_default for a portable, builder-independent shell.
crates/​neovm-core/​src/​emacs_core/​runtime/​eval/​mod.rs Adds the loadup_dump_mode field plus a compile-time size assertion.
crates/​neovm-core/​src/​emacs_core/​runtime/​eval/​construct.rs, pdump_reconstruct.rs Initialize the new field to None.
crates/​neovm-core/​src/​emacs_core/​system/​environment/​mod.rs Doc-comment update noting the dump-time exception.

Review effort: Balanced

Comment thread crates/neovm-core/tests/dump_environment.rs Outdated

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: neomacs.pdump embeds the full build environment (Nix runtime closure 7.4 GiB)

2 participants