src: fix bugs and refactor NativeSymbolDebuggingContext::GetLoadedLibraries

[Whitecx]

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:

1. 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:

1. 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;
}

2. 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)

3. 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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.25%. Comparing base (668a0b8) to head (80624e0).
Report is 128 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.

[nodejs-github-bot]

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

[Whitecx]

@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

[Whitecx]

@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.

[nodejs-github-bot]

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

[H4ad]

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)

[Whitecx]

@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

[nodejs-github-bot]

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

[H4ad]

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