src: fix bugs and refactor NativeSymbolDebuggingContext::GetLoadedLibraries by Whitecx · Pull Request #57738 · nodejs/node · GitHub

tldr:

GetLoadedLibraries is used in process.report.GetReport to generate a list of loaded libraries. This PR fixes the following:

Random failures when retrieving module file paths
Retrieve only up to half the max file path length intended
Allocating unused memory for module handles
Memory leak from converting file path wchar* strings to utf8 std::strings

--

Refactor:

GetModuleFileNameExW can fail randomly: Microsoft's Documentation recommends using GetModuleFileNameW over GetModuleFileNameExW when a process is trying to get a list of it's own dlls. GetModuleFileNameExW can fail unexpectedly for some calls.

Sources from Docs + Raymond Chen

"To retrieve the name of a module in the current process, use the GetModuleFileName function" -- WinApi Docs: Remarks
Raymond Chen explains how GetModuleFileNameExW can fail randomly, in a way that GetModuleFileNameW avoids -- Article

Bug Fixes:

Unused Buffer Memory Allocated: The constructor for modules allocates memory by multiplying the constructor arg size_t n by the sizeof it's type. In this case, modules would be allocated with size_1 * sizeof(HMODULE) bytes. But EnumProcessModules sets size_1 to the number of bytes that need to be allocated for the module buffer. Using size_1/sizeof(HMODULE) gives the constructor what it's asking for i.e the number of HMODULES to allocate space for.

Exapnd to Step through MallocedBuffer constructor behavior

src/util.h MallocedBuffer

  explicit MallocedBuffer(size_t size) : data(Malloc<T>(size)), size(size) {}

src/util-inl.h UncheckedMalloc

template <typename T>
inline T* UncheckedMalloc(size_t n) {
  return UncheckedRealloc<T>(nullptr, n);
}

src/util-inl.h UncheckedRealloc

template <typename T>
T* UncheckedRealloc(T* pointer, size_t n) {
  size_t full_size = MultiplyWithOverflowCheck(sizeof(T), n); // <- Expects n to be number of T elements
/* other code not shown... */

  void* allocated = realloc(pointer, full_size);

/* other code not shown... */

  return static_cast<T*>(allocated);
}

src/util-inl.h MultiplyWithOverflowCheck

template <typename T>
inline T MultiplyWithOverflowCheck(T a, T b) {
  auto ret = a * b;
  if (a != 0)
    CHECK_EQ(b, ret / a);

  return ret;
}

Unnecessary Module path truncation: GetModuleFileName is called using array_size(module_name) / sizeof(WCHAR) for the nSize param. nSize is supposed to be the number of characters in the wchar array supplied. module_name is already a WCHAR array, so nSize should just be array_size(module_name). The current code causes GetModuleFileName to truncate after reaching half the capacity of the buffer (i.e half of MAX_PATH)
Memory Leak: When converting the file path written by GetModuleFileName from a wchar array to a char array with UTF8 encoding, a char array str is added to the heap using new. Next, it's added to list, a std::vector<std::string> using emplace_back. emplace_back will pass str intro the std::string constructor. This constructor is defined to copy values from a null terminated char * array into the std::string. I assume str was created on the heap so that it could be dynamically allocated based on the number of bytes returned by WideCharToMultiByte. Once it's added to list, the memory is no longer needed, but is never deleted from the heap.

missing parenthesis format c++

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.25%. Comparing base (668a0b8) to head (80624e0).
Report is 311 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #57738   +/-   ##
=======================================
  Coverage   90.24%   90.25%           
=======================================
  Files         630      630           
  Lines      184949   184989   +40     
  Branches    36207    36218   +11     
=======================================
+ Hits       166902   166954   +52     
+ Misses      11002    10997    -5     
+ Partials     7045     7038    -7

Files with missing lines	Coverage Δ
src/debug_utils.cc	60.90% <ø> (ø)

... and 43 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

CI: https://ci.nodejs.org/job/node-test-pull-request/66176/

@addaleax There seems to be a fair amount of Builds that failed on pummel.test-buffer-large-size due to a timeout after 12mins, like this one. Is this a test that's known to fail from time to time?

This Build Failure

Other Builds

@H4ad if you have a moment, does the build failure I linked in the comment above look to be from a flaky test? The code I changed shouldn't have affected this test, and there's a pattern of timeout failures for this test on other builds too.

If that is the case, could you add the request-ci label again? I think it just needs to be re-run.

CI: https://ci.nodejs.org/job/node-test-pull-request/66338/

19:11:01 not ok 4038 pummel/test-buffer-large-size
19:11:01 ---
19:11:01 duration_ms: 720098.82500
19:11:01 severity: fail
19:11:02 exitcode: -15
19:11:02 stack: |-
19:11:02 timeout
19:11:02 (node:3252246) [DEP0030] DeprecationWarning: SlowBuffer() is deprecated. Please use Buffer.allocUnsafeSlow()
19:11:02 (Use node --trace-deprecation ... to show where the warning was created)
19:11:02 ...

Looks like a flaky test, I resume the CI in the Jenkins, you can ask to resume the CI instead of add request-ci again, adding the label again will re-run all the tests again (instead of just the one that failed)

@H4ad A test that completed w/ "unstable" last time failed this time, but it seems the test never actually ran. It died while being setup. Are there some artifacts relative to the initial ci run that no longer exist? Hopefully that doesn't mean having to re-run more than what failed 😅

Build Failure

00:52:14 + git archive --format=tar --remote=binary_tmp@67.158.54.159:binary_tmp.git jenkins-node-test-commit-windows-fanned-80624e0eecfa92dce184ae3d27c806a049d9cafc src/node_version.h -o node_version.h.tar
...
remote: fatal: no such ref: jenkins-node-test-commit-windows-fanned-80624e0eecfa92dce184ae3d27c806a049d9cafc        
00:52:15 remote: git upload-archive: archiver died with error
00:52:15 fatal: sent error to the client: git upload-archive: archiver died with error

CI: https://ci.nodejs.org/job/node-test-pull-request/66363/ 💛

friendly ping to @nodejs/platform-windows before adding commit-queue

@jasnell if you have a moment, would you mind confirming whether or not this can be commit-queued? No worries at all if you’re busy (please ignore if so!😊)

PR-URL: #57738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs/node#57738

* chore: bump node in DEPS to v22.17.0 * build: use //third_party/simdutf by default in GN nodejs/node#58115 * chore: adjust crypto specs: - nodejs/node#58117 - nodejs/node#58387 * deps: update libuv to 1.51.0 nodejs/node#58124 * test: fix test-buffer-tostring-range on allocation failure nodejs/node#58416 * build: use FILE_OFFSET_BITS=64 esp. on 32-bit arch nodejs/node#58090 * build: use //third_party/simdutf by default in GN nodejs/node#58115 * inspector: add protocol method Network.dataReceived nodejs/node#58001 * test: force slow JSON.stringify path for overflow nodejs/node#58181 * chore: fixup patch indices * 6049967: Remove protocol::Maybe and roll inspector_protocol https://chromium-review.googlesource.com/c/chromium/src/+/6049967 * chore: fixup crypto test patch * src: fix module buffer allocation nodejs/node#57738 * crypto: expose process.features.openssl_is_boringssl nodejs/node#58387 * util: add internal assignFunctionName() function nodejs/node#57916 * build: fix pointer compression builds nodejs/node#58171 * chore: put back config options * fixup! deps: update libuv to 1.51.0 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

nodejs/node#57738

* chore: bump node in DEPS to v22.17.0 * build: use //third_party/simdutf by default in GN nodejs/node#58115 * chore: adjust crypto specs: - nodejs/node#58117 - nodejs/node#58387 * deps: update libuv to 1.51.0 nodejs/node#58124 * test: fix test-buffer-tostring-range on allocation failure nodejs/node#58416 * build: use FILE_OFFSET_BITS=64 esp. on 32-bit arch nodejs/node#58090 * build: use //third_party/simdutf by default in GN nodejs/node#58115 * inspector: add protocol method Network.dataReceived nodejs/node#58001 * test: force slow JSON.stringify path for overflow nodejs/node#58181 * chore: fixup patch indices * 6049967: Remove protocol::Maybe and roll inspector_protocol https://chromium-review.googlesource.com/c/chromium/src/+/6049967 * chore: fixup crypto test patch * src: fix module buffer allocation nodejs/node#57738 * crypto: expose process.features.openssl_is_boringssl nodejs/node#58387 * util: add internal assignFunctionName() function nodejs/node#57916 * build: fix pointer compression builds nodejs/node#58171 * chore: put back config options * fixup! deps: update libuv to 1.51.0 * chore: update patches --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> Co-authored-by: patchup[bot] <73610968+patchup[bot]@users.noreply.github.com>

nodejs/node#57738

* chore: bump node in DEPS to v22.17.0 * build: use //third_party/simdutf by default in GN nodejs/node#58115 * chore: adjust crypto specs: - nodejs/node#58117 - nodejs/node#58387 * deps: update libuv to 1.51.0 nodejs/node#58124 * test: fix test-buffer-tostring-range on allocation failure nodejs/node#58416 * build: use FILE_OFFSET_BITS=64 esp. on 32-bit arch nodejs/node#58090 * build: use //third_party/simdutf by default in GN nodejs/node#58115 * inspector: add protocol method Network.dataReceived nodejs/node#58001 * chore: fixup patch indices * 6049967: Remove protocol::Maybe and roll inspector_protocol https://chromium-review.googlesource.com/c/chromium/src/+/6049967 * chore: fixup crypto test patch * src: fix module buffer allocation nodejs/node#57738 * crypto: expose process.features.openssl_is_boringssl nodejs/node#58387 * util: add internal assignFunctionName() function nodejs/node#57916 * build: fix pointer compression builds nodejs/node#58171 * chore: put back config options * fixup! deps: update libuv to 1.51.0 * chore: update patches --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> Co-authored-by: patchup[bot] <73610968+patchup[bot]@users.noreply.github.com>

PR-URL: #57738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

* chore: bump node in DEPS to v22.17.0 * build: use //third_party/simdutf by default in GN nodejs/node#58115 * chore: adjust crypto specs: - nodejs/node#58117 - nodejs/node#58387 * deps: update libuv to 1.51.0 nodejs/node#58124 * test: fix test-buffer-tostring-range on allocation failure nodejs/node#58416 * build: use FILE_OFFSET_BITS=64 esp. on 32-bit arch nodejs/node#58090 * build: use //third_party/simdutf by default in GN nodejs/node#58115 * inspector: add protocol method Network.dataReceived nodejs/node#58001 * test: force slow JSON.stringify path for overflow nodejs/node#58181 * chore: fixup patch indices * 6049967: Remove protocol::Maybe and roll inspector_protocol https://chromium-review.googlesource.com/c/chromium/src/+/6049967 * chore: fixup crypto test patch * src: fix module buffer allocation nodejs/node#57738 * crypto: expose process.features.openssl_is_boringssl nodejs/node#58387 * util: add internal assignFunctionName() function nodejs/node#57916 * build: fix pointer compression builds nodejs/node#58171 * chore: put back config options * fixup! deps: update libuv to 1.51.0 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

Whitecx added 3 commits April 3, 2025 13:46

src: fix module buffer allocation

48d4baa

src: Replace GetModuleFileNameExW with GetModuleFileNameW

0f261b5

src: fix GetModuleFileName param

6584451

Whitecx marked this pull request as draft April 3, 2025 18:31

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. debugger Issues and PRs related to the debugger subsystem. needs-ci PRs that need a full CI run. labels Apr 3, 2025

This comment was marked as off-topic.

Sign in to view

src: fix memory leak from string conversion

80624e0

missing parenthesis format c++

Whitecx force-pushed the reportWin32Modules branch from 8aea264 to 80624e0 Compare April 3, 2025 20:37

Whitecx changed the title Report win32 modules src: Fix bugs and refactor NativeSymbolDebuggingContext::GetLoadedLibraries Apr 3, 2025

Whitecx marked this pull request as ready for review April 3, 2025 22:34

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2025

addaleax approved these changes Apr 9, 2025

View reviewed changes

Comment thread src/debug_utils.cc

addaleax changed the title src: Fix bugs and refactor NativeSymbolDebuggingContext::GetLoadedLibraries src: fix bugs and refactor NativeSymbolDebuggingContext::GetLoadedLibraries Apr 9, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2025

H4ad added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 19, 2025

H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label May 9, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 9, 2025

nodejs-github-bot merged commit e4e80c5 into nodejs:main May 9, 2025

github-actions Bot mentioned this pull request May 20, 2025

2025-05-21, Version 24.1.0 (Current) #58406

Merged

aduh95 pushed a commit that referenced this pull request Jun 10, 2025

src: fix module buffer allocation

3f834da

PR-URL: #57738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

github-actions Bot mentioned this pull request Jun 10, 2025

2025-06-24, Version 22.17.0 'Jod' (LTS) #58665

Merged

codebytere mentioned this pull request Jun 26, 2025

src: -Wmismatched-new-delete in debug_utils.cc #58844

Merged

codebytere added a commit to electron/electron that referenced this pull request Jun 26, 2025

src: fix module buffer allocation

4401a74

nodejs/node#57738

codebytere added a commit to electron/electron that referenced this pull request Jun 27, 2025

src: fix module buffer allocation

d7ace93

nodejs/node#57738

codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025

src: fix module buffer allocation

5066736

nodejs/node#57738

codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025

src: fix module buffer allocation

9a16e0b

nodejs/node#57738

codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025

src: fix module buffer allocation

027bc76

nodejs/node#57738

codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025

src: fix module buffer allocation

92ad23b

nodejs/node#57738

codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025

src: fix module buffer allocation

d6d1bbf

nodejs/node#57738

codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025

src: fix module buffer allocation

b8b7832

nodejs/node#57738

codebytere added a commit to electron/electron that referenced this pull request Jul 2, 2025

src: fix module buffer allocation

2c3f810

nodejs/node#57738

marco-ippolito pushed a commit that referenced this pull request Aug 18, 2025

src: fix module buffer allocation

e44a550

PR-URL: #57738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

marco-ippolito pushed a commit that referenced this pull request Aug 20, 2025

src: fix module buffer allocation

0ba36c6

PR-URL: #57738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

marco-ippolito mentioned this pull request Aug 20, 2025

2025-09-03, Version 20.19.5 'Iron' (LTS) #59551

Merged

marco-ippolito pushed a commit that referenced this pull request Aug 20, 2025

src: fix module buffer allocation

7bf05c3

PR-URL: #57738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

marco-ippolito pushed a commit that referenced this pull request Aug 23, 2025

src: fix module buffer allocation

4f8580b

PR-URL: #57738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

marco-ippolito pushed a commit that referenced this pull request Aug 25, 2025

src: fix module buffer allocation

5df8511

PR-URL: #57738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

marco-ippolito pushed a commit that referenced this pull request Aug 25, 2025

src: fix module buffer allocation

b6a458c

PR-URL: #57738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

marco-ippolito pushed a commit that referenced this pull request Aug 25, 2025

src: fix module buffer allocation

0f8968e

PR-URL: #57738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

marco-ippolito pushed a commit that referenced this pull request Aug 27, 2025

src: fix module buffer allocation

5daa7fe

PR-URL: #57738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Brooooooklyn mentioned this pull request Oct 22, 2025

npm oxc-parser: importing package in Node worker thread leads to heap corruption error on Windows oxc-project/oxc#14732

Closed

src: fix bugs and refactor NativeSymbolDebuggingContext::GetLoadedLibraries by Whitecx · Pull Request #57738 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: fix bugs and refactor NativeSymbolDebuggingContext::GetLoadedLibraries - #57738

src: fix bugs and refactor NativeSymbolDebuggingContext::GetLoadedLibraries#57738nodejs-github-bot merged 4 commits intonodejs:mainnodejs/node:mainfrom Whitecx:reportWin32ModulesWhitecx/node:reportWin32ModulesCopy head branch name to clipboard

Conversation

Whitecx commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

tldr:

Refactor:

Bug Fixes:

Uh oh!

This comment was marked as off-topic.

codecov Bot commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 9, 2025

Uh oh!

Whitecx commented Apr 10, 2025

Uh oh!

Whitecx commented Apr 18, 2025

Uh oh!

nodejs-github-bot commented Apr 18, 2025

Uh oh!

H4ad commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Whitecx commented Apr 18, 2025

Uh oh!

nodejs-github-bot commented Apr 19, 2025 • edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

H4ad commented Apr 19, 2025

Uh oh!

Whitecx commented May 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

src: fix bugs and refactor NativeSymbolDebuggingContext::GetLoadedLibraries#57738
nodejs-github-bot merged 4 commits into
nodejs:mainfrom
Whitecx:reportWin32Modules

Whitecx commented Apr 3, 2025 •
edited
Loading

codecov Bot commented Apr 3, 2025 •
edited
Loading

H4ad commented Apr 18, 2025 •
edited
Loading

nodejs-github-bot commented Apr 19, 2025 •
edited by jasnell
Loading