Tracking issue: `std::filesystem::path` headaches, and breakage on Windows with non-ASCII paths

[Renegade334]

Version

v24.1.0

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

No response

What steps will reproduce the bug?

Couldn't find a tracking issue for this one.

There are places in the source that handle std::filesystem::path objects, and on Windows, this involves conversion between UTF-8 strings and wchar-based paths. This can lead to path corruption, as previously discussed.

One example of this is #56049, which has an open PR to fix this specific usage. However, #58764 has just been reported, which affects a completely different area of the API. There are other examples of usage elsewhere in the source.

It feels like this probably needs a library-wide approach – even if not removing std::filesystem::path entirely as previously discussed, then at least some sort of internal wrapper API to abstract out potential footguns.

[joyeecheung]

I have written something about this a while ago: #53063 (comment)

then at least some sort of internal wrapper API to abstract out potential footguns.

+1, I think there are at least two places where we need some helpers and consistently use them to guard against corruption:

1. Conversion from Local<String> to std::filesystem::path - this is usually done via a UTF-8 intermediate value in the middle (usually via Utf8Value), and it's too easy to omit doing the UTF-16 transcoding on Windows unless this is always done through a wrapper
2. Conversion from std::filesystem::path to std::string/char* - this commonly eventually go into some libuv API, which generally assume that the path is encoded in UTF-8. Using the implicit conversion on Windows therefore causes corruption.

[mertcanaltin]

@joyeecheung I'd like to contribute a solution for this. Based on your analysis, I propose implementing a PathHelper utility class that addresses both conversion points:

class PathHelper {
public:
    // Handles Local<String> → std::filesystem::path safely
    static std::filesystem::path FromV8String(v8::Local<v8::String> v8_path);
    
    // Safe conversion to UTF-8 for libuv APIs
    static std::string ToUTF8String(const std::filesystem::path& path);
    
    // For cases where we need to return to V8
    static v8::Local<v8::String> ToV8String(v8::Isolate* isolate, 
                                           const std::filesystem::path& path);
};

[jasnell]

Just connecting dots here... I believe this is also contributing to many of the issues we have with the fs.cpSync(...) API as discussed in #58869 ... generally, we're not handling non-UTF8 paths correctly at all in some of these APIs. The recent refactoring that was done in fs.cpSync(...) to move large chunks of the implementation to c++ using std::filesystem APIs just contributed to making the problem worse and more difficult to fix. I'm thinking we likely need to take a step back and figure out a more comprehensive approach to fixing these but it might be easiest to actually revert away from using std::fileystem and the more recent changes that moved large parts of the impl into c++ as a first step.

[mertcanaltin]

Just connecting dots here... I believe this is also contributing to many of the issues we have with the fs.cpSync(...) API as discussed in #58869 ... generally, we're not handling non-UTF8 paths correctly at all in some of these APIs. The recent refactoring that was done in fs.cpSync(...) to move large chunks of the implementation to c++ using std::filesystem APIs just contributed to making the problem worse and more difficult to fix. I'm thinking we likely need to take a step back and figure out a more comprehensive approach to fixing these but it might be easiest to actually revert away from using std::fileystem and the more recent changes that moved large parts of the impl into c++ as a first step.

@jasnell I agree with your assessment. The std::filesystem approach seems to have introduced more complexity than it solved. Would you like me to help with:

1. Identifying the specific commits/PRs that moved fs.cpSync to std::filesystem
2. Assessing the scope of reversion needed
3. Creating a plan for gradual rollback?

[jasnell]

I think it's going to be important to plan out the necessary changes before making actual changes, so I'd want to make sure we're taking things a bit slow... or we run the risk of just adding to the problems. I think it's also clear that it's not just cpSync that is impacted here, so taking some effort to identify the full scope of the issue is worthwhile.

Specifically there are number of concerns: file name encodings are not only different from one OS to another, on some OS's a single path can contain names in multiple encodings. While this is fairly rare, it does happen. For instance, the original reason we allow Buffer paths in fs APi in the first place was because I had a former customer whose application was dealing with file paths that contained some names encoded as Shift-JIS, some names encoded as UTF8, and others encoded as Latin1... all the in the same path... like /{latin1}/{utf8}/{shift-jis} ... which ended up making the in app details super funky.

So before opening any PRs here, let's explore what the current bugs are and come up with a proposed plan to fix and then work through PRs from there.

[jasnell]

... this is usually done via a UTF-8 intermediate value in the middle (usually via Utf8Value), and it's too easy to omit doing the UTF-16 transcoding on Windows unless this is always done through a wrapper

I think it's critical that we keep in mind that UTF8 transcoding is not always something we should do at all. If the path is provided as a Buffer then we should not transcode at all, but we are still doing so in some of these places.

Things are a bit more ambiguous when a file:/// URL is provided as the path. It's possible for the URL to contain non-UTF8 percent encoded bytes that are perfectly valid in the URL itself but need to be interpreted. We should decide if we are going to accept file URLs that contain something other than UTF-8 encodings (in which case we should generally treat those the same as Buffer paths) or require those to be UTF8 (in which case we should generally treat those as string paths).

Conversion from std::filesystem::path to std::string/char* - this commonly eventually go into some libuv API, which generally assume that the path is encoded in UTF-8.

Are we certain that libuv is making any assumptions about how these paths are encoded? Are those assumptions consistent across OS's? From what I've seen, libuv tends to just treat paths as opaque byte strings and it's our (node.js') interpretation of those that is often incorrect. What we need here is a way of saying "return paths as byte strings" or "return paths as strings". For the former, we never transcode or make assumptions about the encoding and pass those through as is.

[mertcanaltin]

@jasnell You're absolutely right - this is way more complex than I initially thought. The mixed encoding scenario (latin1/utf8/shift-jis in same path) is eye-opening.

Let me start with a focused investigation:

Step 1: libuv behavior analysis - Since you questioned whether libuv actually assumes UTF-8 or treats paths as opaque bytes, I'll test this across Windows/macOS/Linux with non-UTF8 paths.

Step 2: Current bug inventory - Map all std::filesystem issues and where we're incorrectly transcoding Buffer paths.

I'll document findings in a comprehensive analysis before proposing any changes. The libuv investigation seems like the foundational piece - should I start there?

[jasnell]

Sounds good. Thank you!

[mertcanaltin]

Hi @jasnell,

@dario-piotrowicz and I looked into the std::filesystem::path issues you mentioned. You're right
there are systematic encoding problems beyond just fs.cpSync.

We found encoding inconsistencies between modules (node_file.cc uses CP_UTF8 while node_modules.cc uses GetACP()) and confirmed Buffer paths are being transcoded when they shouldn't be, causing corruption.

Your suggestion for partial std::filesystem revert makes sense. Should we start with reverting fs.cpSync first?

[joyeecheung]

I think it's critical that we keep in mind that UTF8 transcoding is not always something we should do at all. If the path is provided as a Buffer then we should not transcode at all, but we are still doing so in some of these places.

Note that the original comment was specifically about String, not Buffer, which is a different can of worms (I think taking Buffer without encoding in the API is already a mistake, and is not something the proposed helper can fix, unless the document start to enforce that data in the Buffer must always be in a specific encoding). When it's a V8 string the underlying encoding is clear from the V8 API, and when std::filesystem is used, it doesn't matter so much what the system encoding is - that part is already handled by the standard library. The footgun lies in the use of char instead of char8_t. When char8_t or u8string is used, std::filesystem::path will convert from UTF8 to the native encoding, this is well-specified.

[joyeecheung]

Are we certain that libuv is making any assumptions about how these paths are encoded?

The libuv documentation states:

On Windows uv_fs_* functions use utf-8 encoding.

So if any API happens to not use UTF8 on Windows, that should be a bug in libuv. On non-Windows, it's generally UTF-8, or before the std::filesystem::path that's what we have always been doing. While that might not be perfect, at least people weren't complaining much in practice and reverting to the previous state would probably already be "good enough".

[jasnell]

@joyeecheung ... it's not super clear what you're suggesting here in response to my comments.

[jasnell]

@mertcanaltin ... I would recommend syncing up with @anonrig to see if there's a better path forward than a full revert of the work that was done on cpSync and some of the other work that's introduced use of std::filesystem::path. Unfortunately it's just... complicated.

[jasnell]

... On non-Windows, it's generally UTF-8, or before the std::filesystem::path that's what we have always been doing

libuv does not apply or assume any encoding on any other OS. It Simply passes through whatever bytes it receives. Node.js has a long history of mishandling filenames on Linux, particularly those that use legacy encodings. The fact that Node.js inconsistently handles this by treating paths in some apis as byte sequences and others as utf8 without actually verifying they are utf8 has long been a problem that pops up every so often. Switching some apis to std::filesystem::path just appears to have made the issue more difficult to fix by introducing even more inconsistency. I've been thinking more and more that we likely need a better abstraction for paths, both in C++ and JS.