piecrust: prevent dangling commit index element references

[d-sonuga]

Closes #493

Fix undefined behavior where a reference is made from a pointer to a contract index element after the owning mutex guard is dropped.

[Copilot]

Pull request overview

This PR hardens Piecrust’s commit index lookup path to avoid creating references to commit-store-backed index elements that can dangle if the underlying commit is removed concurrently.

Changes:

• Replace inherited Commit::index_get results with a guard-bound reference type (ContractIndexElementRef) to keep commit-store data alive while being read.
• Update session contract loading to avoid holding inherited index element references while building Memory (clone needed data instead).
• Add a Miri-only regression test for the dangling-reference scenario and wire it into the crate Makefile.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
piecrust/src/store/session.rs	Adjusts inherited contract loading to avoid holding commit-store-backed element refs while constructing Memory.
piecrust/src/store/commit.rs	Introduces ContractIndexElementRef to keep inherited index lookups tied to a held store lock; removes unsafe raw-pointer based access and adds Miri regression test.
piecrust/src/store/commit_store.rs	Reworks inherited element lookup to return an owner handle and adds a guarded get_element accessor used by the new ref type.
piecrust/Makefile	Adds a Miri test invocation for the new regression test (currently under the default test target).
piecrust/CHANGELOG.md	Adds an Unreleased “Fixed” entry describing the dangling-reference fix.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[moCello]

COMMENT — The core fix is correct and minimal: the guard-bound ContractIndexElementRef keeps the store locked exactly as long as a caller reads an inherited entry, the borrow-not-clone change to deep_index_get makes the returned guard sound, and the unsafe raw-pointer helpers are gone with no leftover references. Lifetimes are compiler-enforced, and the prior review's double-lookup and redundant-Deref comments are addressed. One item needs a maintainer decision: the Makefile change folds Miri into the default test target, forcing a nightly toolchain and a network component install on every make test. The single note — a missing explicit assertion in the Miri-only regression test — is polish, not a blocker.

[moCello]

REQUEST CHANGES — The undefined behaviour is genuinely fixed and the raw pointers are gone cleanly, but the guard-holding handle that replaces them trades a memory-safety hazard for a liveness one that only unwritten caller discipline prevents.

The deciding item is index_get: it can return a value that silently holds the process-wide CommitStore mutex, and on the other side of that lock is the single sync_loop thread servicing every commit, delete and finalize in the VM. Nothing in the type or the signature says so, and whether the lock is taken at all depends on where the contract happens to live. Bounding the guard to a callback, or returning an owned element, removes the hazard along with the Deref re-lookup and the expect.

[moCello]

COMMENT — the undefined behavior is genuinely fixed and the design is sound: raw pointers gone, no reference escapes the lock, Commit::insert's replacement lookup is infallible by construction, and all callers pass pure closures. The one item worth a maintainer's decision is the inline WARNING — with_index_element runs the caller's closure under the process-wide CommitStore mutex, undocumented, so a future caller could reintroduce a VM-wide deadlock; a one-line doc comment closes it. Everything from the prior rounds is resolved.