Memory leak in node::fs::ReadFileUtf8(const FunctionCallbackInfo<Value>& args) when args[0] is a string.

[Justin-Nietzel]

Version

v22.14.0

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

node::fs

What steps will reproduce the bug?

Open a file with ReadFileUtf8() using a path (not a file handle).

How often does it reproduce? Is there a required condition?

Every time, including loading JavaScript files for execution.

What is the expected behavior? Why is that the expected behavior?

Calling readFileSync() with an encoding of UTF-8 should not permanently increase the amount of rss memory being used by node. It's (hopefully) the expected behavior because memory leaks are bad.

What do you see instead?

Calling will cause Node's memory usage to go up indefinitely.

This was discovered when a process using node:worker_threads would continuously increase it's memory consumption until it was forcibly closed. JavaScript debug tools showed that the managed heap's memory usage was constant, but the rss memory usage kept rising. Debugging the node runtime showed that loading modules was calling the underlying ReadFileUtf8 method. The process that was showing this memory leak was not caching workers and was re-loading all code files each time a worker thread was created.

Minimum code example:

import { readFileSync } from 'node:fs';

const options = { encoding: 'utf-8' };
for (let i = 1; true; i++) {
  readFileSync('./empty.txt', options);
  if (i % 1000 === 0) {
    i = 0;
    gc();
  }
}

Memory usage in task manager:

Output from process.memoryUsage():

2025-04-08 20:23:38:       rss -   357.03 MB
2025-04-08 20:23:38: heapTotal -     7.03 MB
2025-04-08 20:23:38:  heapUsed -     4.28 MB
2025-04-08 20:23:38:  external -     1.73 MB

Additional information

This is my best guess at the bug. I'm like 85% sure of what's going, but this is the first time I'm looking at the NodeJS codebase.

Allocation:

• void ReadFileUtf8(const FunctionCallbackInfo<Value>& args) calls uv_fs_open which calls fs__capture_path.
• fs__capture_path allocates a UTF16 buffer and assigns it to uv_fs_s.file.pathw.
• uv_fs_s.file is a union type with the definition: union { WCHAR* pathw; int fd; }

Corruption

• uv_file file; is allocated and assigned the return value from uv_fs_open.
• ReadFileUtf8 calls uv_fs_read passing in the file handle file.
• uv_fs_read overwrites the least significant half of uv_fs_s.file.pathw with this call req->file.fd = fd;.
• The pointer to the buffer formerly assigned to uv_fs_s.file.pathw is lost.

[juanarbol]

Humble ping to @nodejs/platform-windows and @nodejs/libuv

[bnoordhuis]

At a quick look it's missing a uv_fs_req_cleanup(&req) call after uv_fs_open(). Untested but the fix probably looks like this:

diff --git a/src/node_file.cc b/src/node_file.cc
index b2e3889b412..ecc682d2f0c 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -2709,8 +2709,8 @@ static void ReadFileUtf8(const FunctionCallbackInfo<Value>& args) {
     FS_SYNC_TRACE_BEGIN(open);
     file = uv_fs_open(nullptr, &req, *path, flags, 0666, nullptr);
     FS_SYNC_TRACE_END(open);
+    uv_fs_req_cleanup(&req);
     if (req.result < 0) {
-      uv_fs_req_cleanup(&req);
       // req will be cleaned up by scope leave.
       return env->ThrowUVException(
           static_cast<int>(req.result), "open", nullptr, path.out());

The uv_fs_read() call further down below is also missing a concomitant uv_fs_req_cleanup(&req) call but that doesn't cause issues, if only by accident.

[tniessen]

@bnoordhuis There is a deferred uv_fs_cleanup(&req) for when the file was opened successfully but I suppose that will only take care of the most recent value of req, which in this case likely is from uv_fs_close()?

node/src/node_file.cc

Lines 2720 to 2727 in 437c6aa

	auto defer_close = OnScopeLeave([file, is_fd, &req]() {
	if (!is_fd) {
	FS_SYNC_TRACE_BEGIN(close);
	CHECK_EQ(0, uv_fs_close(nullptr, &req, file, nullptr));
	FS_SYNC_TRACE_END(close);
	}
	uv_fs_req_cleanup(&req);
	});

Before #49691, there was a separate defer_req_cleanup as well as a separate uv_fs_t close_req.

cc @anonrig since #49691 appears to have changed this part.

[bnoordhuis]

@tniessen yes, it's tied to the uv_fs_close() call; or if it isn't, it's still a bug because req gets repurposed for uv_fs_read() operations.

[Justin-Nietzel]

At a quick look it's missing a uv_fs_req_cleanup(&req) call after uv_fs_open(). Untested but the fix probably looks like this:

diff --git a/src/node_file.cc b/src/node_file.cc
index b2e3889b412..ecc682d2f0c 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -2709,8 +2709,8 @@ static void ReadFileUtf8(const FunctionCallbackInfo& args) {
FS_SYNC_TRACE_BEGIN(open);
file = uv_fs_open(nullptr, &req, *path, flags, 0666, nullptr);
FS_SYNC_TRACE_END(open);

• uv_fs_req_cleanup(&req);
  if (req.result < 0) {

•  uv_fs_req_cleanup(&req);
   // req will be cleaned up by scope leave.
   return env->ThrowUVException(
       static_cast<int>(req.result), "open", nullptr, path.out());
  

The uv_fs_read() call further down below is also missing a concomitant uv_fs_req_cleanup(&req) call but that doesn't cause issues, if only by accident.

I tested this change locally too. It fixed the issue without any obvious side effects.

[mcollina]

@bnoordhuis are you sending a PR?

[bnoordhuis]

No. Feel free to take the diff, no attribution needed.

[Justin-Nietzel]

I can make a PR with my branch. I'm re-running all the tests now.

[Justin-Nietzel]

After pulling latest, something changed and started preventing me from running vcbuild test. It worked when I first pulled the repo. I think something broke the test script if python is installed in C:/Program Files/ because of the space in the path name. It's probably a me thing. I put together a draft PR for now and did my best to follow all of the contribution guidelines.

Feel free to take over if I'm not doing this right. I'm not super familiar with working on open-source projects.

#57811

Is this issue with spaces in paths on Windows a known thing? Any way to get around it?

Failed to build addon in C:\Source\node\test\addons\esm:
(node:17384) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.
(Use `node --trace-deprecation ...` to show where the warning was created)
'C:\Program' is not recognized as an internal or external command,
operable program or batch file.
gyp: Call to 'C:\Program Files\Python311\python.exe -c "import os; print(os.getcwd())"' returned exit status 1 while in binding.gyp. while trying to load binding.gyp