fs.rmSync('速') crash without throw

[kanasimi]

Version

23.3.0

Platform

Windows 10

Subsystem

No response

What steps will reproduce the bug?

When fs.rmSync('速') files with name containing “速”, node 23.3.0 will crash without throw.

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

Everytime

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

Delete the file normally.

What do you see instead?

The program just crashed.

Additional information

There are other special characters that can cause similar problems, such as “請”. This problem did not occur in previous versions.

[islandryu]

It might be related to #55773.

[kanasimi]

v22.11.0 is OK, but v23.0.0 also has problems.

[joyeecheung]

This seems, once again, a problem from using std::filesystem::path on Windows, and wasn't taken care of as part of https://github.com/nodejs/node/pull/55015 (cc @anonrig who authored https://github.com/nodejs/node/pull/53617). See https://github.com/nodejs/node/pull/53063#issuecomment-2361447818 on an explanation about this class of bugs.

(With the amount of crashes reported for this bug on Windows, I start to feel that we might as well should just forbid std::filesystem::path in the code base unless it's absolutely justified, as it's so easy to miss the encoding inconsistency on Windows.....)

[anonrig]

I recommend forbidding std::filesystem::path as well. It's not worth it.

[joyeecheung]

It seems no one is working on it yet, so marking it as good first issue. See https://github.com/nodejs/node/pull/53063#issuecomment-2361447818 on how this sort of bug happens and how they can be fixed. In this case I believe one just needs to change

https://github.com/nodejs/node/blob/56e5bd8d2acd3698547b9d18d16620ed4e11a99f/src/node_file.cc#L1629-L1630

To use ToU8StringView() instead, since the call was actually changed to use std::filesystem calls, which only takes std::filesystem::paths.

[geeksilva97]

I will give it a try

[Yeaseen]

@geeksilva97 I would like to try this if you haven't done it yet. I started building with the suggested change and will write the test to check. This would be my first contribution here. Thank you.

[geeksilva97]

@geeksilva97 I would like to try this if you haven't done it yet. I started building with the suggested change and will write the test to check. This would be my first contribution here. Thank you.

Of course. Go for it.

[kanasimi]

Do we need to do an extensive check to see if there are other places with similar problems?

[jimmywarting]

off-topic: @anonrig just borrowing this issue instead of creating a new one. but do you think you could maybe try to unlock #40844 agian?

maybe also delete all unnecessary +1 comments and github-action bots.