fs.rmSync('速') crash without throw

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.

It might be related to #55773.

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

@anonrig

This seems, once again, a problem from using std::filesystem::path on Windows, and wasn't taken care of as part of #55015 (cc @anonrig who authored #53617). See #53063 (comment) 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.....)

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

It seems no one is working on it yet, so marking it as good first issue. See #53063 (comment) on how this sort of bug happens and how they can be fixed. In this case I believe one just needs to change

node/src/node_file.cc

Lines 1629 to 1630 in 56e5bd8

    
               env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); 
        
           auto file_path = std::filesystem::path(path.ToStringView());

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

I will give it a try

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

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

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

fs: fix rmSync to handle non-ASCII characters

fix(гейт): снятие и очистка состояния переживают пути с не-ASCII симв…

fix(desktop): harden before-pack cleanup against the same non-ASCII r…

Navigation Menu

Repository navigation

fs.rmSync('速') crash without throw #56049

Version

Platform

Subsystem

What steps will reproduce the bug?

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

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

What do you see instead?

Additional information

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	env, permission::PermissionScope::kFileSystemWrite, path.ToStringView());
	auto file_path = std::filesystem::path(path.ToStringView());

fs.rmSync('速') crash without throw #56049

Description

Version

Platform

Subsystem

What steps will reproduce the bug?

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

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

What do you see instead?

Additional information

Activity

islandryu commented on Nov 29, 2024

kanasimi commented on Nov 29, 2024

joyeecheung commented on Nov 29, 2024

anonrig commented on Nov 29, 2024

joyeecheung commented on Dec 2, 2024

geeksilva97 commented on Dec 2, 2024

Yeaseen commented on Dec 3, 2024

geeksilva97 commented on Dec 3, 2024

kanasimi commented on Dec 3, 2024

jimmywarting commented on Dec 12, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions