src: add GetBuildId helper function#339
Conversation
WalkthroughThe changes introduce ELF build ID extraction utilities for Linux, including new C++ source and header files, a Node.js native addon, and a test script. Build configuration files are updated to include the new sources and adjust Linux-specific linking logic. The test verifies the new functionality by comparing extracted build IDs with system values. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript Test
participant Addon as Native Addon (binding.cc)
participant Utils as ELF Utils (GetBuildId)
participant System as Linux System
JS->>Addon: getBuildId(path)
Addon->>Utils: GetBuildId(path, &build_id)
Utils->>System: Open ELF file, parse sections
System-->>Utils: ELF data (build ID or error)
Utils-->>Addon: build_id or error code
Addon-->>JS: build_id string or undefined
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
To allow us to read the Build-Id from ELF headers in a specific binary.
santigimeno
force-pushed
the
santi/add_build_id
branch
from
b554e8a to
ab9a12f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
test/addons/nsolid-elf-utils/nsolid-elf-utils.js (1)
1-1: Remove redundant use strict directive.The
'use strict';directive is redundant in JavaScript modules as they are automatically in strict mode.-'use strict';src/nsolid/nsolid_elf_utils.cc (2)
32-34: Consider using more specific error handling for elf_version.The function returns
elf_errno()but this might not provide meaningful error information to callers.if (elf_version(EV_CURRENT) == EV_NONE) { - return elf_errno(); + return UV_ENOSYS; // System function not implemented }
66-73: Improve note parsing robustness.The note parsing logic looks correct but could benefit from additional validation.
uint32_t* note = reinterpret_cast<uint32_t*>(data->d_buf); uint32_t namesz = note[0]; uint32_t descsz = note[1]; + uint32_t type = note[2]; + + // Verify this is actually a build-id note (NT_GNU_BUILD_ID = 3) + if (type != 3) { + continue; + } + // Name starts at offset 12 // Descriptor (build-id) starts at next aligned offset size_t name_end = 12 + ((namesz + 3) & ~3);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
node.gyp(1 hunks)node.gypi(1 hunks)src/nsolid/nsolid_elf_utils.cc(1 hunks)src/nsolid/nsolid_elf_utils.h(1 hunks)test/addons/nsolid-elf-utils/binding.cc(1 hunks)test/addons/nsolid-elf-utils/binding.gyp(1 hunks)test/addons/nsolid-elf-utils/nsolid-elf-utils.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
test/addons/nsolid-elf-utils/nsolid-elf-utils.js (1)
Learnt from: santigimeno
PR: nodesource/nsolid#327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
🧬 Code Graph Analysis (1)
test/addons/nsolid-elf-utils/binding.cc (3)
test/addons/nsolid-elf-utils/nsolid-elf-utils.js (1)
assert(3-3)src/nsolid/nsolid_elf_utils.h (1)
GetBuildId(12-12)src/nsolid/nsolid_elf_utils.cc (2)
GetBuildId(17-92)GetBuildId(17-17)
🪛 Biome (1.9.4)
test/addons/nsolid-elf-utils/nsolid-elf-utils.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: coverage-linux-without-intl
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-macOS
- GitHub Check: build-tarball
- GitHub Check: coverage-linux
- GitHub Check: lint-js-and-md
- GitHub Check: coverage-windows
- GitHub Check: build-docs
🔇 Additional comments (9)
test/addons/nsolid-elf-utils/binding.gyp (1)
1-10: LGTM - Standard addon build configurationThe build configuration is well-structured and appropriate for a native addon that needs access to internal Node.js APIs.
node.gyp (1)
576-581: LGTM - Proper Linux-specific integrationThe conditional addition of ELF utility sources for Linux builds is correctly implemented and follows the existing patterns in the build configuration.
src/nsolid/nsolid_elf_utils.h (1)
1-20: LGTM - Well-structured header fileThe header file follows Node.js conventions with proper include guards, namespace structure, and conditional compilation for internal APIs.
node.gypi (1)
553-560: LGTM - Proper linking configuration for ELF supportThe changes correctly ensure
-lelfis linked on Linux builds while maintaining the conditional-lrtremoval logic. This supports the new ELF utility functionality.test/addons/nsolid-elf-utils/nsolid-elf-utils.js (2)
7-11: LGTM: Proper platform check for Linux-only functionality.The platform check correctly ensures the test only runs on Linux, which is appropriate for ELF utilities.
20-23: LGTM: Test logic is correct and comprehensive.The test correctly compares the addon's output with the expected value from
readelf, with a helpful error message showing both values.src/nsolid/nsolid_elf_utils.cc (3)
1-11: LGTM: Proper includes and dependencies.The includes are appropriate for ELF processing, with both low-level POSIX headers and libelf-specific headers.
36-39: LGTM: Proper file opening with error handling.The file opening logic correctly handles errors and returns appropriate errno values.
85-91: LGTM: Proper resource cleanup with goto pattern.The cleanup logic correctly handles both ELF and file descriptor resources using the goto pattern, which is appropriate for C-style error handling.
| static void GetBuildId(const FunctionCallbackInfo<Value>& args) { | ||
| #if defined(__linux__) | ||
| Isolate* isolate = args.GetIsolate(); | ||
| assert(args[0]->IsString()); |
There was a problem hiding this comment.
Add missing include for assert
The code uses assert() without including the necessary header.
+#include <cassert>
#include <node.h>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert(args[0]->IsString()); | |
| #include <cassert> | |
| #include <node.h> |
🤖 Prompt for AI Agents
In test/addons/nsolid-elf-utils/binding.cc at line 16, the code uses assert()
but does not include the required header file. Add the appropriate include
directive for the assert header, typically #include <cassert>, at the top of the
file to ensure assert() is properly declared.
| if (build_id->empty()) { | ||
| ret = UV_ENOENT; | ||
| } else { | ||
| build_id_cache_[path] = *build_id; | ||
| } |
There was a problem hiding this comment.
Fix thread safety issue with cache update.
The cache update outside the mutex protection (from the earlier thread safety fix) will cause race conditions.
If implementing the mutex fix suggested earlier, ensure the cache update is also protected:
if (build_id->empty()) {
ret = UV_ENOENT;
} else {
+ std::lock_guard<std::mutex> lock(cache_mutex_);
build_id_cache_[path] = *build_id;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (build_id->empty()) { | |
| ret = UV_ENOENT; | |
| } else { | |
| build_id_cache_[path] = *build_id; | |
| } | |
| if (build_id->empty()) { | |
| ret = UV_ENOENT; | |
| } else { | |
| std::lock_guard<std::mutex> lock(cache_mutex_); | |
| build_id_cache_[path] = *build_id; | |
| } |
🤖 Prompt for AI Agents
In src/nsolid/nsolid_elf_utils.cc around lines 79 to 83, the update to
build_id_cache_ is done outside the mutex lock, causing potential race
conditions. To fix this, move the cache update inside the mutex protection block
so that both the check and the update to build_id_cache_ happen atomically under
the same mutex lock.
| char* name = elf_strptr(e, shstrndx, shdr.sh_name); | ||
| if (name && strcmp(name, ".note.gnu.build-id") == 0) { | ||
| Elf_Data* data = elf_getdata(scn, nullptr); | ||
| if (data && data->d_size >= 16) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds checking for note data size.
The current check data->d_size >= 16 is insufficient. The actual required size depends on the note header fields.
- if (data && data->d_size >= 16) {
+ if (data && data->d_size >= 12) { // Minimum for note header
// ELF Note header: namesz(4), descsz(4), type(4) + name padding
// Compute offset to build-id properly
uint32_t* note = reinterpret_cast<uint32_t*>(data->d_buf);
uint32_t namesz = note[0];
uint32_t descsz = note[1];
+
+ // Verify we have enough data for the complete note
+ size_t name_end = 12 + ((namesz + 3) & ~3);
+ if (data->d_size < name_end + descsz) {
+ continue; // Skip malformed note
+ }
+
- // Name starts at offset 12
- // Descriptor (build-id) starts at next aligned offset
- size_t name_end = 12 + ((namesz + 3) & ~3);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (data && data->d_size >= 16) { | |
| if (data && data->d_size >= 12) { // Minimum for note header | |
| // ELF Note header: namesz(4), descsz(4), type(4) + name padding | |
| // Compute offset to build-id properly | |
| uint32_t* note = reinterpret_cast<uint32_t*>(data->d_buf); | |
| uint32_t namesz = note[0]; | |
| uint32_t descsz = note[1]; | |
| // Verify we have enough data for the complete note | |
| size_t name_end = 12 + ((namesz + 3) & ~3); | |
| if (data->d_size < name_end + descsz) { | |
| continue; // Skip malformed note | |
| } | |
| // ... rest of processing for a well-formed note ... | |
| } |
🤖 Prompt for AI Agents
In src/nsolid/nsolid_elf_utils.cc at line 63, the current check for note data
size using `data->d_size >= 16` is too simplistic. Update the condition to
calculate the required size based on the note header fields (such as namesz,
descsz, and header size) and ensure `data->d_size` is at least this computed
size before proceeding. This will prevent out-of-bounds access by validating the
actual expected size of the note data.
2 participants
To allow us to read the Build-Id from ELF headers in a specific binary.
Summary by CodeRabbit
readelfcommand.