src/libstore/derivation-options.cc (1)

399-400: Consider requiring a system feature for WASM support.

The special-casing of "wasm32-wasip1" allows any system to build WASM derivations, which aligns with the PR's goal of system-independent derivations. However, this assumes Wasmtime is always available.

Consider whether wasm32-wasip1 builds should require a system feature (e.g., "wasm" or "wasmtime") to ensure the runtime is actually available on the build machine. This would be checked by the existing getRequiredSystemFeatures logic at lines 406-408.

🔎 Alternative approach using system features

If you want to gate WASM builds on explicit system capability:

1. Have wasm32-wasip1 derivations declare requiredSystemFeatures = ["wasm"];
2. Remove the special case here, letting the normal system feature check at lines 406-408 handle it:

-    if (drv.platform != settings.thisSystem.get() && drv.platform != "wasm32-wasip1"
-        && !settings.extraPlatforms.get().count(drv.platform) && !drv.isBuiltin())
+    if (drv.platform != settings.thisSystem.get()
+        && !settings.extraPlatforms.get().count(drv.platform) && !drv.isBuiltin())
         return false;

This would make WASM support more explicit and consistent with how other cross-compilation scenarios are handled.

src/libstore/meson.build (1)

383-383: Consider using Meson's dependency system instead of hardcoded linker flags.

Directly appending -lwasmtime to link_args bypasses Meson's dependency resolution, which can lead to portability and configuration issues:

• No verification that the wasmtime library is actually available
• Library search paths may not be configured correctly
• Platform-specific library naming conventions (e.g., .dll vs .so) are not handled
• Build will fail with cryptic linker errors rather than clear dependency errors

🔎 Recommended approach using Meson dependency system

Near the top of the file with other dependencies (around line 109-161), add:

+wasmtime = dependency('wasmtime', required : true)
+deps_private += wasmtime

Then remove the hardcoded linker flag:

-  link_args : linker_export_flags + [ '-lwasmtime' ],
+  link_args : linker_export_flags,

This provides better error messages, respects pkg-config paths, and handles platform differences automatically.

src/libexpr/meson.build (1)

242-242: Consider using Meson's dependency system instead of hardcoded linker flags.

This has the same portability concerns as the identical change in src/libstore/meson.build at line 383. Hardcoded -lwasmtime bypasses Meson's dependency resolution and can cause issues with library paths, platform-specific naming, and error reporting.

🔎 Recommended approach using Meson dependency system

Near the dependencies section (around lines 41-92), add:

+wasmtime = dependency('wasmtime', required : true)
+deps_private += wasmtime

Then remove the hardcoded linker flag:

-  link_args : linker_export_flags + [ '-lwasmtime' ],
+  link_args : linker_export_flags,

This approach provides better error messages, respects pkg-config configurations, and handles cross-platform differences automatically.

src/libstore/unix/build/derivation-builder.cc (1)

2033-2035: Early return for wasm32-wasip1 bypasses later sandbox / uid‑range checks

Routing params.drv.platform == "wasm32-wasip1" directly to WasiDerivationBuilder happens before:

• The diverted store check (storeDir != realStoreDir) and
• The !useSandbox && params.drvOptions.useUidRange(drv) guard.

That means for WASI builds:

• uid-range requests will no longer be rejected when sandboxing is effectively disabled here.
• Diverted stores on non‑Linux platforms will also be allowed for this path, unlike other builders.

If this is intentional (relying on the WASI runtime rather than OS sandboxing), consider making that explicit and either:

• Reject uid-range in WasiDerivationBuilder’s path (or in this branch) so configuration errors are surfaced, or
• Document that uid-range is ignored for wasm32-wasip1 and that OS sandboxing semantics differ from other systems.

src/libexpr/primops.cc (1)

168-188: Making realisePath non‑static for reuse is reasonable

Exposing SourcePath realisePath(...) (no longer static) with the optional SymlinkResolution parameter allows other primop modules (e.g. WASM/WASI ones) to share the same path + context resolution logic. The implementation is consistent with existing callers, and the default (Full) preserves previous behavior.

Just ensure there is a matching declaration in a header (e.g. primops.hh) so external uses stay type‑checked and the function remains the single canonical implementation.

src/libstore/unix/build/wasi-derivation-builder.cc (1)

5-18: Deduplicate unwrap / string2span helpers and tighten const‑correctness

Both unwrap and string2span appear to be cut‑and‑paste from src/libexpr/primops/wasm.cc, which means future changes to the Wasmtime API or error handling will have to be done in two places.

Consider:

• Moving these helpers to a small shared header (e.g. a wasm-runtime-utils.hh) used by both primops and the WASI builder, and
• Switching string2span to use a std::span<const uint8_t> (and avoiding the C‑style cast) if the Wasmtime C++ API accepts a const span, to keep const‑correctness and avoid accidental mutation.

Please double‑check the current wasmtime::Module::compile signature in your pinned Wasmtime version to confirm whether std::span<const uint8_t> is acceptable and adjust the helper accordingly.

src/libexpr/primops/wasm.cc (6)

6-6: Consider removing using namespace wasmtime; for clarity.

While acceptable in implementation files, explicitly qualifying wasmtime types improves code clarity and prevents potential name collisions with future additions to the nix namespace.

---

10-15: Refactor: Remove duplicate declaration and include proper header.

The realisePath function is already defined in src/libexpr/primops.cc (lines 167-187). Instead of forward-declaring it here, consider extracting it to a shared header file that both files can include, or making it part of the EvalState API.

---

94-103: Consider factory method for clearer error handling.

The constructor performs heavy operations (WASM compilation via unwrap()) that can throw. Consider using a static factory method that returns Result<ref<NixWasmInstancePre>, Error> for clearer error handling and easier testing.

---

182-196: Simplify error throwing pattern.

Line 196 uses an immediately-invoked lambda to throw an error, which is less clear than throwing directly. The ternary chain could also benefit from a switch statement for clarity.

🔎 Proposed refactor

 uint32_t get_type(ValueId valueId)
 {
     auto & value = *values.at(valueId);
     state.forceValue(value, noPos);
-    auto t = value.type();
-    return t == nInt        ? 1
-           : t == nFloat    ? 2
-           : t == nBool     ? 3
-           : t == nString   ? 4
-           : t == nPath     ? 5
-           : t == nNull     ? 6
-           : t == nAttrs    ? 7
-           : t == nList     ? 8
-           : t == nFunction ? 9
-                            : []() -> int { throw Error("unsupported type"); }();
+    switch (value.type()) {
+        case nInt: return 1;
+        case nFloat: return 2;
+        case nBool: return 3;
+        case nString: return 4;
+        case nPath: return 5;
+        case nNull: return 6;
+        case nAttrs: return 7;
+        case nList: return 8;
+        case nFunction: return 9;
+        default: throw Error("unsupported type");
+    }
 }

---

230-238: Consider adding documentation for two-phase copy pattern.

This function uses a common but potentially confusing pattern: it only copies data if the buffer is large enough, but always returns the actual size. A brief comment explaining that callers should check returnValue <= maxLen before using the buffer would improve clarity.

---

426-426: Improve readability by splitting complex expression.

Line 426 chains multiple operations in a single expression, making it difficult to read and debug. Consider breaking it into separate steps.

🔎 Proposed refactor

-        v = *instance.values.at(instance.runFunction(functionName, {(int32_t) instance.addValue(args[2])}).at(0).i32());
+        auto argId = instance.addValue(args[2]);
+        auto results = instance.runFunction(functionName, {(int32_t) argId});
+        auto resultId = results.at(0).i32();
+        v = *instance.values.at(resultId);

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/libexpr/primops/wasm.cc (4)

160-176: Add a checked subspan for untrusted WASM pointers.

memory().subspan(ptr, len) is called with untrusted ptr/len, which can lead to OOB access. Introduce a bounds-checked helper and use it at all access sites (panic/warn/make_string/copy_string/…/read_file).

🛡️ Suggested fix (helper + example usage)

     auto memory()
     {
         return memory_.data(wasmCtx);
     }
+
+    std::span<uint8_t> checkedSubspan(uint32_t ptr, uint32_t len)
+    {
+        auto mem = memory();
+        if (ptr > mem.size() || len > mem.size() - ptr)
+            throw Error("Wasm memory access out of bounds: ptr=%u len=%u size=%zu", ptr, len, mem.size());
+        return mem.subspan(ptr, len);
+    }
 
     std::monostate panic(uint32_t ptr, uint32_t len)
     {
-        throw Error("Wasm panic: %s", Uncolored(span2string(memory().subspan(ptr, len))));
+        throw Error("Wasm panic: %s", Uncolored(span2string(checkedSubspan(ptr, len))));
     }

---

281-289: Fix endianness for ValueId reads from WASM memory.

WASM is little‑endian; reading ValueId directly is wrong on big‑endian hosts. Convert each 32‑bit word to host order before indexing values.

🧭 Suggested fix (pattern)

-        for (const auto & [n, v] : enumerate(list))
-            v = values.at(vs[n]); // FIXME: endianness
+        for (const auto & [n, v] : enumerate(list)) {
+            auto id = boost::endian::little_to_native(vs[n]);
+            v = values.at(id);
+        }

---

504-510: Make the Wasm instance cache thread-safe.

instancesPre is a static unordered_map with no synchronization and will race under parallel evaluation. Add locking or replace it with a concurrent map.

🔒 Suggested fix (minimal locking)

-        static std::unordered_map<SourcePath, ref<NixWasmInstancePre>> instancesPre;
+        static std::unordered_map<SourcePath, ref<NixWasmInstancePre>> instancesPre;
+        static std::mutex instancesPreMutex;
 
-        auto instancePre = instancesPre.find(wasmPath);
+        std::lock_guard<std::mutex> lock(instancesPreMutex);
+        auto instancePre = instancesPre.find(wasmPath);

---

519-525: Validate returned ValueId before indexing instance.values.

res[0].i32() is untrusted. An out‑of‑range id throws std::out_of_range (not caught here), which can abort evaluation. Validate the id and raise Error instead.

✅ Suggested fix

-        auto vRes = instance.values.at(res[0].i32());
+        auto resultId = res[0].i32();
+        if (resultId < 0 || static_cast<size_t>(resultId) >= instance.values.size())
+            throw Error(
+                "Wasm function '%s' from '%s' returned invalid ValueId %d",
+                functionName,
+                wasmPath,
+                resultId);
+        auto vRes = instance.values.at(static_cast<size_t>(resultId));

src/libexpr/primops.cc (1)

168-173: Preserve call-site position in path coercion.

noPos discards error locations; pass pos so coercion failures point to the caller.

🐛 Suggested fix

-    auto path = coerceToPath(noPos, v, context, "while realising the context of a path");
+    auto path = coerceToPath(pos, v, context, "while realising the context of a path");

src/libexpr/primops/wasm.cc (3)

160-176: Add bounds-checked WASM memory access.

memory().subspan(ptr, len) is used with untrusted ptr/len and can go OOB. Add a checked helper and use it for all subspan reads/writes (panic/warn/make_* /copy_* /read_file/etc.).

🔒 Suggested helper + example usage

     auto memory()
     {
         return memory_.data(wasmCtx);
     }
+
+    std::span<uint8_t> checkedSubspan(uint32_t ptr, uint32_t len)
+    {
+        auto mem = memory();
+        uint64_t end = (uint64_t) ptr + (uint64_t) len;
+        if (end > mem.size())
+            throw Error("Wasm memory access out of bounds: ptr=%u len=%u size=%zu", ptr, len, mem.size());
+        return mem.subspan(ptr, len);
+    }
@@
     std::monostate panic(uint32_t ptr, uint32_t len)
     {
-        throw Error("Wasm panic: %s", Uncolored(span2string(memory().subspan(ptr, len))));
+        throw Error("Wasm panic: %s", Uncolored(span2string(checkedSubspan(ptr, len))));
     }
@@
     std::monostate warn(uint32_t ptr, uint32_t len)
     {
         nix::warn(
             "'%s' function '%s': %s",
             pre->wasmPath,
             functionName.value_or("<unknown>"),
-            span2string(memory().subspan(ptr, len)));
+            span2string(checkedSubspan(ptr, len)));
         return {};
     }

---

335-355: Ensure deterministic attribute ordering when copying attrsets.

Enumerating *value.attrs() is not guaranteed to be lexicographically sorted, which can make WASM results nondeterministic. Sort by attribute name before writing to memory.

♻️ Suggested fix

-            // FIXME: for determinism, we should return attributes in lexicographically sorted order.
-            for (const auto & [n, attr] : enumerate(*value.attrs())) {
-                buf[n].value = addValue(attr.value);
-                buf[n].nameLen = state.symbols[attr.name].size();
-            }
+            std::vector<const Attr *> sorted;
+            sorted.reserve(value.attrs()->size());
+            for (const auto & attr : *value.attrs())
+                sorted.push_back(&attr);
+            std::sort(sorted.begin(), sorted.end(), [&](const Attr * a, const Attr * b) {
+                return state.symbols[a->name] < state.symbols[b->name];
+            });
+            for (const auto & [n, attr] : enumerate(sorted)) {
+                buf[n].value = addValue(attr->value);
+                buf[n].nameLen = state.symbols[attr->name].size();
+            }

---

503-510: Make instancesPre cache thread-safe.

Parallel evaluation can race on the static instancesPre map. Add locking or switch to a concurrent map.

🧵 Simple locking fix

-        static std::unordered_map<SourcePath, ref<NixWasmInstancePre>> instancesPre;
+        static std::unordered_map<SourcePath, ref<NixWasmInstancePre>> instancesPre;
+        static std::mutex instancesPreMutex;
+        std::lock_guard<std::mutex> lock(instancesPreMutex);

src/libexpr/primops/wasm.cc (1)

358-362: Non-deterministic attribute ordering persists.

The FIXME on line 358 notes that attributes should be returned in lexicographically sorted order for determinism, but this hasn't been implemented. This can cause non-reproducible behavior across different evaluations.

This was flagged in a previous review but remains unaddressed.

src/libexpr/primops/wasm.cc (5)

13-19: Code duplication with wasi-derivation-builder.cc.

This unwrap() helper is identical to the one in src/libstore/unix/build/wasi-derivation-builder.cc. Consider extracting it to a shared header to avoid duplication.

---

111-111: Unused member variable ex.

The std::exception_ptr ex member is declared but never used in this file. Consider removing it or documenting its intended purpose if it's for future use.

---

173-176: Memory access uses std::span::subspan which throws std::out_of_range.

memory().subspan(ptr, len) will throw std::out_of_range on out-of-bounds access, not nix::Error. While this is caught by the generic catch (std::exception & e) in regFun, the error message will lack WASM-specific context.

Consider using a checked helper similar to the subspan template that throws Error with a descriptive message.

🔧 Suggested helper

std::span<uint8_t> checkedMemorySubspan(uint32_t ptr, uint32_t len)
{
    auto mem = memory();
    if (ptr > mem.size() || len > mem.size() - ptr)
        throw Error("Wasm memory access out of bounds: ptr=%u len=%u size=%zu", ptr, len, mem.size());
    return mem.subspan(ptr, len);
}

---

388-398: Clarify the sentinel value convention for get_attr.

Returning 0 for a missing attribute is now valid since ValueId 0 is reserved (line 125-126). However, this convention should be documented in a comment here, as it's not immediately obvious to readers that 0 means "not found" rather than being a valid value ID.

📝 Suggested documentation

     ValueId get_attr(ValueId valueId, uint32_t ptr, uint32_t len)
     {
         auto attrName = span2string(memory().subspan(ptr, len));

         auto & value = getValue(valueId);
         state.forceAttrs(value, noPos, "while getting an attribute from Wasm");

         auto attr = value.attrs()->get(state.symbols.create(attrName));

-        return attr ? addValue(attr->value) : 0;
+        // Return 0 (reserved sentinel) if attribute not found
+        return attr ? addValue(attr->value) : 0;
     }

---

512-515: Cache grows unbounded without eviction.

The static instancesPre cache accumulates entries indefinitely. As noted in the FIXME, this could lead to memory bloat in long-running processes that evaluate many different WASM modules.

Consider tracking cache size and implementing LRU eviction, or moving this to EvalState where it can be cleaned up with the evaluator.