Free req.file.pathw in fs::ReadFileUtf8().

[Justin-Nietzel]

Always call uv_fs_req_cleanup after calling uv_fs_open instead of just when uv_fs_open returns a negative result. I referenced ReadFileSync from node:js2c when making this change.

https://github.com/bnoordhuis made the same suggestion based on the PR #49691.

Fixes: #57800

[mcollina]

lgtm

[nodejs-github-bot]

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

[Justin-Nietzel]

I updated the commit comments after the first CI failure with module names in the title. I think it was mistaking some variable names as modules? I moved the original title into the description. Will that description also get flagged. Is there a safe way to format the variable name without triggering the module name CI check?

[tniessen]

@Justin-Nietzel You need to force-push to your branch issue-57800 with an amended commit message. Since req.file.pathw is an implementation detail in an external dependency, I'd suggest a commit message title such as fs: add missing call to uv_fs_req_cleanup or something like that.

Is it safe to also unconditionally call us_fs_req_cleanup(&req) in defer_close? It seems very odd to me that defer_close will always call uv_fs_req_cleanup(&req) while conditionally reusing &req for uv_fs_close. Before #49691, there was a seperate uv_fs_t for the close operation (cc @anonrig).

[Justin-Nietzel]

ReadFileSync in js2c.cc follows a very similar workflow to ReadFileUTF8 when using opening a file by file path. It re-uses the req object and always calls uv_fs_close after calling uv_fs_req_cleanup. I can't say with any certainty whether this behavior is "correct" or not, but I found it to be a useful reference.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (8dce721) to head (951437d).
Report is 128 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57811      +/-   ##
==========================================
- Coverage   90.23%   90.23%   -0.01%     
==========================================
  Files         630      630              
  Lines      185465   185471       +6     
  Branches    36368    36374       +6     
==========================================
- Hits       167362   167353       -9     
+ Misses      10999    10996       -3     
- Partials     7104     7122      +18     

Files with missing lines	Coverage Δ
src/node_file.cc	76.72% <100.00%> (+<0.01%)	⬆️

... and 25 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.

[mcollina]

@Justin-Nietzel do you think you'd be able to add a test that verify the memory leak is fixed?

[nodejs-github-bot]

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

[Justin-Nietzel]

I probably could add a unit test. I would reference the readFileSync test and this worker thread memory test. I just had a few questions.

1. This leak scales with path length of the file bring read. I would probably want to make sure my test file has a path length of at least 100. Should I create a new test folder in fixtures for this issue so I'm not putting a really long file name directly in the fixtures folder? Alternatively, I could create the long file name in the temporary directory based on the current path instead.
2. For a noticable difference I would probably be doing 100,00 plus file reads. Should I put this in the pummel folder?
3. Based on my understanding of the documentation, each test is ran as a separate process. I don't have to worry about other tests effecting the RSS memory usage, right?

[Justin-Nietzel]

@mcollina I added a unit test for this fix and did my best to follow all the documentation I could find on style and testing. Let me know if you need me to make any adjustments.

[mcollina]

lgtm

[nodejs-github-bot]

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

[jasnell]

Landed in d0a5bd6...09ecd2e

[tniessen]

On a side note for future PRs, the second commit technically does not satisfy our requirements for commit messages since the leading verb is not imperative (added -> add).

[marco-ippolito]

I think there is something wrong with the test for this fix.
If I revert the fix commit, the test is still passing.
For example on v20.x:

marcoippolito@marcos-MacBook-Pro-3 node % git revert 9e13bf0a81
[v20.x 27c78e4265] Revert "fs: add missing call to uv_fs_req_cleanup"
 1 file changed, 1 insertion(+), 1 deletion(-)
marcoippolito@marcos-MacBook-Pro-3 node % make
ninja -C out/Release 
ninja: Entering directory `out/Release'
[16/16] LINK node, POSTBUILDS
if [ ! -r node ] || [ ! -L node ]; then ln -fs out/Release/node node; fi
marcoippolito@marcos-MacBook-Pro-3 node % ./node test/pummel/test-fs-read-file-sync-utf8-memory.js
marcoippolito@marcos-MacBook-Pro-3 node % make test
// tests are passing

Same thing for all release lines, you can try running the test with a pre-security release version.
On another note the test is not passing on v18 where this bug should not be present 😕

[Justin-Nietzel]

I think there is something wrong with the test for this fix. If I revert the fix commit, the test is still passing. For example on v20.x:

marcoippolito@marcos-MacBook-Pro-3 node % git revert 9e13bf0a81
[v20.x 27c78e4265] Revert "fs: add missing call to uv_fs_req_cleanup"
 1 file changed, 1 insertion(+), 1 deletion(-)
marcoippolito@marcos-MacBook-Pro-3 node % make
ninja -C out/Release 
ninja: Entering directory `out/Release'
[16/16] LINK node, POSTBUILDS
if [ ! -r node ] || [ ! -L node ]; then ln -fs out/Release/node node; fi
marcoippolito@marcos-MacBook-Pro-3 node % ./node test/pummel/test-fs-read-file-sync-utf8-memory.js
marcoippolito@marcos-MacBook-Pro-3 node % make test
// tests are passing

Same thing for all release lines, you can try running the test with a pre-security release version. On another note the test is not passing on v18 where this bug should not be present 😕

Pummel tests are not ran as part of the CI tests.
If you're look at CI output when referring to "all release lines", that may be why you're not seeing it?

node_file.cc isn't OS specific, but the underlying libuv libraries are, so this bug may not be detected on Unix systems depending on the version being used either.

Here's the commit we think may have introduced the bug: a415654f9771642c109a9abc9c0b1fc28ffa06bf.
If your branch is also missing this commit, you may not see the issue either.

Here the code used for formatting the memory usage in the original bug report.
I'd be interested in seeing what output you get on your system when you run the test.

function formatBytes(bytes) {
  const megaBytes = (Math.round(bytes / 1024 / 1024 * 100) / 100);
  return `${megaBytes} MB`.padStart(12, ' ');
}

export function printMemoryUsage() {
  const timeStamp = new Date().toISOString().replace('T', ' ').slice(0, -5);
  const { rss, heapTotal, heapUsed, external } = process.memoryUsage();
  console.log('')
  console.log(`${timeStamp}:       rss -${formatBytes(rss)}`);
  console.log(`${timeStamp}: heapTotal -${formatBytes(heapTotal)}`);
  console.log(`${timeStamp}:  heapUsed -${formatBytes(heapUsed)}`);
  console.log(`${timeStamp}:  external -${formatBytes(external)}`);
  console.log('')
}

Let me know if there's anything else I can help with.

[marco-ippolito]

I'm compiling node on my machine and testing on the release branches (20.x etc...)
I'm running pummel tests with tools/test.py pummel so I'm quite confident they are being executed.
This the machine:

marcoippolito@marcos-MacBook-Pro-3 node % uname -a
Darwin marcos-MacBook-Pro-3.local 24.4.0 Darwin Kernel Version 24.4.0: Fri Apr 11 18:33:47 PDT 2025; root:xnu-11417.101.15~117/RELEASE_ARM64_T6030 arm64

I'm going to test this on windows x64, unfortunately I don't have a linux x64 machine
Here is the output of the before after:

marcoippolito@marcos-MacBook-Pro-3 node % ./node test/pummel/test-fs-read-file-sync-utf8-memory.js

2025-05-15 17:28:58:       rss -    33.14 MB
2025-05-15 17:28:58: heapTotal -      4.8 MB
2025-05-15 17:28:58:  heapUsed -     4.57 MB
2025-05-15 17:28:58:  external -     1.88 MB


2025-05-15 17:28:58:       rss -    39.45 MB
2025-05-15 17:28:58: heapTotal -      6.3 MB
2025-05-15 17:28:58:  heapUsed -      4.3 MB
2025-05-15 17:28:58:  external -     1.88 MB

[Justin-Nietzel]

I took a peek at libuv and the struct that originally enabled this pointer corruption is Windows specific.
Not seeing this issue on Unix builds seems right to me actually.

• uv_fs_t (uv_fs_s) has a private file field.
  • The original type definition is found here: https://github.com/libuv/libuv/blob/29edf310890634af02555c96e8f2fb4dab509fe8/include/uv.h#L1443
• That file field has this type on Windows: union { WCHAR* pathw; int fd; }.
  • Winodws: https://github.com/libuv/libuv/blob/29edf310890634af02555c96e8f2fb4dab509fe8/include/uv/win.h#L615
• That file field has this type on Unix: typedef int uv_file
  • Unix: https://github.com/libuv/libuv/blob/29edf310890634af02555c96e8f2fb4dab509fe8/include/uv/unix.h#L364

[marco-ippolito]

I took a peek at libuv and the struct that originally enabled this pointer corruption is Windows specific.
Not seeing this issue on Unix builds seems right to me actually.

• uv_fs_t (uv_fs_s) has a private file field.
  • The original type definition is found here: https://github.com/libuv/libuv/blob/29edf310890634af02555c96e8f2fb4dab509fe8/include/uv.h#L1443
• That file field has this type on Windows: union { WCHAR* pathw; int fd; }.
  • Winodws: https://github.com/libuv/libuv/blob/29edf310890634af02555c96e8f2fb4dab509fe8/include/uv/win.h#L615
• That file field has this type on Unix: typedef int uv_file
  • Unix: https://github.com/libuv/libuv/blob/29edf310890634af02555c96e8f2fb4dab509fe8/include/uv/unix.h#L364

Ok so knowing this is windows specific helps a lot. It would have been good to specify it in the release notes or CVE text.
Im testing it right now on my windows machine.
All right I can confirm it's working as expected. Sorry for the trouble. I think we should make it clear that this is a windows only issue

[Justin-Nietzel]

All right I can confirm it's working as expected. Sorry for the trouble. I think we should make it clear that this is a windows only issue

Sure, what would the best way to do this be? The original issue was Windows only, but the unit test would catch the issue if it was ever introduced to any other operating systems too. Should we add some more background info to the original issue report?

I don't think anyone knew this was Windows only issue until just now.