Conversation
|
Review requested:
|
|
Can we split the LIEF source update to a separate PR, ideally generated by the bot? It would make reviewing much easier |
|
Doesn't look like it was added to the GHA workflow, have opened #66242. |
|
Thanks! I'll split the LIEF source update into a separate PR and keep the Node-side changes here. |
|
Can we land the Node.js side independently from the LIEF update? I.e. can we make the codebase compatible with both LIEF 0.17.x and 1.x? |
Yes, I'll make the Node.js changes compatible with both LIEF 0.17.x and 1.x so this PR can land independently, then leave the LIEF update to a separate PR. |
|
I’m working on local builds and validation, and I expect to update the PR later. |
Keep bundled LIEF at 0.17.0 while selecting the Mach-O section API from the headers in use and the bundled build settings by version. Prepare the updater for Mbed TLS 4 and TF-PSA-Crypto so the vendor update can land separately. Assisted-by: Codex Signed-off-by: inoway46 <inoueyuya416@gmail.com>
173cf8c to
6a9b5b4
Compare
|
Updated as discussed: this PR now keeps bundled LIEF at 0.17.0 and supports both LIEF 0.17.x and 1.x, so it can land independently of the vendor update. Local validation results are in the PR description. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66240 +/- ##
==========================================
+ Coverage 90.27% 90.38% +0.10%
==========================================
Files 789 790 +1
Lines 272907 274292 +1385
Branches 52117 52504 +387
==========================================
+ Hits 246373 247916 +1543
+ Misses 16988 16852 -136
+ Partials 9546 9524 -22
🚀 New features to boost your workflow:
|
|
I tried building with a shared LIEF 1.0.0 and got the following error: The following diff fixes it: diff --git a/src/node_sea_bin.cc b/src/node_sea_bin.cc
index bd61ea2942e..44ac892a531 100644
--- a/src/node_sea_bin.cc
+++ b/src/node_sea_bin.cc
@@ -306,7 +306,12 @@ InjectOutput InjectIntoPE(const std::vector<uint8_t>& executable,
cfg.resources = true;
cfg.rsrc_section = ".rsrc"; // ensure section name
LIEF::PE::Builder builder(*binary, cfg);
+#if LIEF_VERSION_MAJOR >= 1
+ builder.build();
+ if (builder.get_build().empty()) {
+#else
if (!builder.build()) {
+#endif
return {InjectResult::kError, {}, "Failed to build modified PE binary"};
} |
|
@aduh95 I reproduced this on macOS x64 with shared LIEF 1.0.0 and optimization disabled. d91e91d6 added Two possible workarounds:
Which would you prefer pending an upstream fix? If we use option 2, should we restrict it to 1.0.0 rather than all 1.x versions? |
|
You're asking me but I have no idea. We should maybe ask upstream to add that to their migration guide |
Avoid the hidden ok_error_t bool conversion in shared LIEF 1.0.0 by checking the PE builder output instead. Restrict the workaround to 1.0.0 so other versions retain the existing return-value check. Refs: lief-project/LIEF#1387 Assisted-by: Codex Signed-off-by: inoway46 <inoueyuya416@gmail.com>
|
Reported upstream: lief-project/LIEF#1387. Added the |
| #if LIEF_VERSION_MAJOR == 1 && LIEF_VERSION_MINOR == 0 && \ | ||
| LIEF_VERSION_PATCH == 0 |
There was a problem hiding this comment.
What's the reasoning for restricting it to a specific version number?
There was a problem hiding this comment.
I restricted it to 1.0.0 because get_build().empty() is a workaround and isn’t exactly equivalent to checking build()’s return value, so I wanted other versions to keep the normal check.
That said, the full CI passed even without this workaround, so pinning it to 1.0.0 could silently reintroduce the issue on the next LIEF update if it hasn’t been fixed upstream yet. Applying it to 1.x and explicitly reverting it once the upstream issue is fixed seems safer and simpler to maintain. I’ll update it that way.
Apply the shared LIEF link workaround to major versions >= 1 instead of restricting it to 1.0.0. Keep it until the upstream issue is fixed so a dependency update does not silently reintroduce the link failure. Refs: lief-project/LIEF#1387 Assisted-by: Codex Signed-off-by: inoway46 <inoueyuya416@gmail.com>
Keep bundled LIEF at 0.17.0 while selecting the Mach-O section API from the headers in use and the bundled source list and Mbed TLS configuration by version.
Prepare the updater for Mbed TLS 4 and TF-PSA-Crypto, including its generated sources, so the LIEF vendor update can land separately.
Validated bundled 0.17.0 with a Node.js build on macOS x64, and bundled and shared 1.0.0 with Node.js builds and 47 SEA/related tests each on Linux x64.
Fixes: #66238
Refs: #66242
Note: The build fix for #63530 and the fix for CVE-2025-15504 remain in the follow-up LIEF vendor update.
Refs: #63530
Refs: nodejs/nodejs-dependency-vuln-assessments#360
Refs: nodejs/nodejs-dependency-vuln-assessments#342