fix(link,crypto): ELF archive grouping and scrypt cost parameters (from #9311) - #9314
Merged
Conversation
…dlib (#8930) A `bundled-streams` build died with `undefined reference to <futures_channel::mpsc::SenderTask>::notify` out of `libperry_ext_http.a`, even though the `libperry_stdlib.a` on the same command line exports that symbol from its own `futures_channel` member. The mechanism is archive order, but not the obvious one. The wrapper archives already appear twice — once before perry-stdlib, once after — so at a glance every reference has somewhere to go. What the #8930 link map shows is that the FIRST listing pulls nothing at all: the user objects reference only `js_*` symbols that perry-stdlib provides, so nothing in the wrapper is undefined yet when `ld` walks it. All 92 wrapper members that end up in the executable are pulled from the SECOND listing, on references that stdlib's own members had just opened (`js_node_http_*`, …) — and the references those members carry back INTO stdlib have nowhere to go, because GNU `ld` scans each archive once and never revisits one it has passed. That was harmless while each wrapper bundled its own copy of everything it closed over. It becomes a hard error the moment `strip_bundled_shared_deps_from_well_known_lib` drops a bundled member because stdlib provides it — a correct decision by the archive index, but one that only holds if `ld` can still get back to stdlib. That is what `bundled-streams` changes: it is the feature (enabled by a `fs/promises` or `stream/web` import, per `stdlib_features::module_to_features`) that pulls futures_channel into perry-stdlib's own graph, so stdlib starts bundling a name-matching `futures_channel-*.rcgu.o`, so the wrapper's copy becomes eligible to drop. A stdlib built without it carries no futures_channel member at all, the wrapper keeps its copy, and the link stays self-contained. #8939 tightened the pruning rule to the name-matched stdlib member's own exports; here that member does export the symbol, so the rule fires correctly and the link still fails. The bug is on the link line, not in the pruning. Wrap the perry archive block in `-Wl,--start-group` / `-Wl,--end-group` on ELF targets so `ld` re-scans it to a fixed point — the guarantee a mutually recursive archive set needs. Repeating one archive (the codebase's usual "archive twice" trick) only covers a one-step cycle, and this graph is LTO-partitioned across hundreds of codegen units on both sides. Members are still pulled left to right, so the wrappers-before-stdlib and localized-runtime-last first-definition-wins ordering is unaffected; Mach-O `ld64` resolves archives to a fixed point already (and rejects the flag) and `lld-link` / MSVC have no group concept, so both are left alone. Verified against both reported repros in mb24 — `apps/landing` (hono plus a compiled `@skelpo/cms-client`) and `apps/api` (hono + ws + mysql2, four wrapper archives, no `perry.compilePackages` at all). Both fail before and link after; replaying either final link line with only the two group flags removed reproduces the exact undefined reference. A case that already linked (`packages/db/src/migrate.ts`) produces a byte-identical executable with and without the flags. The link/strip-dedup unit tests pass (68), as do the two archive-ordering integration tests — `issue_5920_wrapper_bundled_runtime_async_starvation` (the two-runtime-copies hazard) and `issue_6715_native_wrapper_precedence`. `native_link_cache` fails identically on the unpatched parent commit.
…nd_run.rs build_and_run.rs was at 1994 lines; #8930's archive-group call takes it over the 2000-line gate. The HarmonyOS block needs only cmd/target/format, so it moves to a sibling module unchanged.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe change forwards scrypt options through code generation and runtime validation, adds Node parity tests, extracts HarmonyOS object discovery, and wraps eligible ELF archive links with GNU linker groups. ChangesScrypt option handling
ELF linking and HarmonyOS object collection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NativeDispatch
participant Codegen
participant ScryptRuntime
participant OpenSSL
NativeDispatch->>Codegen: Pass boxed scrypt options
Codegen->>ScryptRuntime: Call scrypt with options_bits
ScryptRuntime->>ScryptRuntime: Validate aliases, ranges, and maxmem
ScryptRuntime->>OpenSSL: Derive key with validated parameters
OpenSSL-->>ScryptRuntime: Return derived bytes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
This was referenced Aug 31, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands the two fixes from #9311 that apply cleanly to current
main, keeping the original commits and authorship.#8930 — group the ELF archive block. The wrappers are already emitted twice, so on paper every reference has somewhere to go; the
ldmap is what settles it (364 stdlib members pulled, then 92 ext members, with the first ext listing pulling nothing).--start-group/--end-group, gated to ELF targets, gives fixed-point semantics rather than one-step-cycle coverage — which matters because the graph is LTO-partitioned across hundreds of CGUs in both directions. The author verified it is a byte-identical no-op where linking already worked.#9289 — honour scrypt cost parameters.
crypto.scrypt(password, salt, keylen, options)discardedoptionsentirely and always computed node's N=2¹⁴/r=8/p=1 defaults, returning one constant digest for every requested cost. This is the security-relevant one: a password hasher records its parameters next to the digest, so the stored record claimed a work factor that was never paid, and aneedsRehashcheck keyed on those parameters never fired. A caller asking for the OWASP N=2¹⁷ baseline silently got an 8× weaker one. The fix honoursN/r/p/keylen/maxmemwith node's aliases, enforces OpenSSL's128 * r * (N + p + 2)boundary, and throws rather than substituting defaults.Also carries a small refactor of mine:
build_and_run.rswas at 1994 lines, and #8930's 8-line archive-group call takes it past the 2000-line gate. The HarmonyOS native-object block needs onlycmd/target/format, so it moves to a sibling module unchanged.Verification. I ran the scrypt parity fixture under both the pinned Node 26.5.1 oracle and the compiled binary — all four sections (sync vectors, async vectors, direct-callback options, invalid-parameter errors) produce identical output, including the
maxmemRangeError. That test is also non-vacuous by construction: it pins six distinct digests across N=2¹²…2¹⁷, and the pre-fix implementation returned a single constant, so it could not have matched more than one row.perry-stdlib+perry-codegen(40 suites) green underRUST_TEST_THREADS=1; release build clean with no new warnings; file-size, raw-handle, thread-local, root-holder and fmt gates all pass.Summary by CodeRabbit
New Features
Bug Fixes
crypto.scryptandscryptSyncto honor cost, block size, parallelization, and memory-limit options.Tests