Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,12 @@
}, {
'use_openssl_def%': 0,
}],
[ 'OS=="linux"', {
'nsolid_sources': [
'src/nsolid/nsolid_elf_utils.cc',
'src/nsolid/nsolid_elf_utils.h',
]
}],
],
},

Expand Down
11 changes: 7 additions & 4 deletions node.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -550,10 +550,13 @@
[ 'OS=="sunos"', {
'ldflags': [ '-Wl,-M,/usr/lib/ld/map.noexstk' ],
}],
[ 'OS=="linux" and not nsolid_use_librt', {
'libraries!': [
'-lrt'
],
[ 'OS=="linux"', {
'libraries': [ '-lelf' ],
'conditions': [
[ 'not nsolid_use_librt', {
'libraries!': [ '-lrt' ],
}]
]
}],
[ 'OS in "freebsd linux"', {
'ldflags': [ '-Wl,-z,relro',
Expand Down
97 changes: 97 additions & 0 deletions src/nsolid/nsolid_elf_utils.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
#include "nsolid_elf_utils.h"
#include "nsolid_util.h"

#include <fcntl.h>
#include <libelf.h>
#include <gelf.h>
#include <unistd.h>
#include <cstring>
#include <map>
#include "uv.h"

namespace node {
namespace nsolid {
namespace elf_utils {


int GetBuildId(const std::string& path, std::string* build_id) {
static std::map<std::string, std::string> build_id_cache_;
Comment thread
santigimeno marked this conversation as resolved.

Elf* e;
Elf_Scn* scn = nullptr;
GElf_Shdr shdr;

if (build_id_cache_.find(path) != build_id_cache_.end()) {
*build_id = build_id_cache_[path];
return 0;
}

int ret;

ret = 0;
if (elf_version(EV_CURRENT) == EV_NONE) {
return elf_errno();
}

int fd = open(path.c_str(), O_RDONLY);
if (fd < 0) {
return -errno;
}

e = elf_begin(fd, ELF_C_READ, nullptr);
if (!e) {
ret = elf_errno();
goto error;
}

size_t shstrndx;
if (elf_getshdrstrndx(e, &shstrndx) != 0) {
ret = elf_errno();
goto end_error;
}

*build_id = std::string("");
while ((scn = elf_nextscn(e, scn)) != nullptr) {
if (gelf_getshdr(scn, &shdr) != &shdr) {
ret = elf_errno();
goto end_error;
}

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) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.

// 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];
// Name starts at offset 12
// Descriptor (build-id) starts at next aligned offset
size_t name_end = 12 + ((namesz + 3) & ~3);
uint8_t* id = reinterpret_cast<uint8_t*>(data->d_buf) + name_end;
*build_id = utils::buffer_to_hex(id, descsz);
break;
}
}
}

if (build_id->empty()) {
ret = UV_ENOENT;
} else {
build_id_cache_[path] = *build_id;
}
Comment on lines +79 to +83
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.


end_error:
elf_end(e);

error:
close(fd);

return ret;
}

} // namespace elf_utils
} // namespace nsolid
} // namespace node

19 changes: 19 additions & 0 deletions src/nsolid/nsolid_elf_utils.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#ifndef SRC_NSOLID_NSOLID_ELF_UTILS_H_
#define SRC_NSOLID_NSOLID_ELF_UTILS_H_

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include <string>

namespace node {
namespace nsolid {

namespace elf_utils {
int GetBuildId(const std::string& path, std::string* build_id);
} // namespace elf_utils
} // namespace nsolid
} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#endif // SRC_NSOLID_NSOLID_ELF_UTILS_H_
32 changes: 32 additions & 0 deletions test/addons/nsolid-elf-utils/binding.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#include <node.h>
#include <v8.h>
#include <string>
#if defined(__linux__)
#include "../../../src/nsolid/nsolid_elf_utils.h"
#endif

using v8::FunctionCallbackInfo;
using v8::Isolate;
using v8::String;
using v8::Value;

static void GetBuildId(const FunctionCallbackInfo<Value>& args) {
#if defined(__linux__)
Isolate* isolate = args.GetIsolate();
assert(args[0]->IsString());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.

v8::String::Utf8Value path_utf8(isolate, args[0]);
std::string path(*path_utf8, path_utf8.length());
std::string build_id;
int res = node::nsolid::elf_utils::GetBuildId(path, &build_id);
if (res != 0) {
return;
}

args.GetReturnValue().Set(
String::NewFromUtf8(isolate, build_id.c_str()).ToLocalChecked());
#endif
}
Comment thread
santigimeno marked this conversation as resolved.

NODE_MODULE_INIT(/* exports, module, context */) {
NODE_SET_METHOD(exports, "getBuildId", GetBuildId);
}
10 changes: 10 additions & 0 deletions test/addons/nsolid-elf-utils/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
'targets': [
{
'target_name': 'binding',
'sources': [ 'binding.cc' ],
'includes': ['../common.gypi'],
'defines': [ 'NODE_WANT_INTERNALS=1' ],
}
]
}
23 changes: 23 additions & 0 deletions test/addons/nsolid-elf-utils/nsolid-elf-utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';
const common = require('../../common');
const assert = require('assert');
const { execSync } = require('child_process');
const process = require('process');

// Only run on Linux
if (process.platform !== 'linux') {
console.log('Skipping: nsolid-elf-utils only supported on Linux');
process.exit(0);
}

const bindingPath = require.resolve(`./build/${common.buildType}/binding`);
const binding = require(bindingPath);
Comment thread
santigimeno marked this conversation as resolved.

const expected =
execSync(`readelf -n ${process.execPath} | awk '/Build ID/ { print $3 }'`,
{ encoding: 'utf8' }).trim();
Comment thread
santigimeno marked this conversation as resolved.

const buildId = binding.getBuildId(process.execPath);
assert.strictEqual(buildId,
expected,
`Mismatch: addon='${buildId}', readelf='${expected}'`);
Loading