Skip to content

[FS] lookupPath should throw an error on an empty string #23366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jan 13, 2025
Merged

[FS] lookupPath should throw an error on an empty string #23366

merged 6 commits into from
Jan 13, 2025

Conversation

hoodmane
Copy link
Collaborator

It's inconsistent that lookupPath returns node:null when the path is an empty string. Normally it either raises ENOENT or node is truthy. This makes empty string into an edge case that every caller has to handle. stat has special handling for the empty string case and correctly returns ENOENT on empty string, but chmod, truncate, and mknod all crash. utime and some other functions mishandle the empty string in an unrelated way.

@hoodmane
FS: lookupPath should throw an error on an empty string
d1a3953 
It's inconsistent that lookupPath returns `node:null` when the path
is an empty string. Normally it either raises `ENOENT` or `node` is truthy.
This makes empty string into an edge case that every caller has to handle.
`stat` has special handling for the empty string case and correctly returns
ENOENT on empty string, but chmod, truncate, and mknod all crash. utime and
some other functions mishandle the empty string in an unrelated way.
@hoodmane hoodmane force-pushed the enoent-on-empty-string branch from e37dc5a to d1a3953 Compare January 10, 2025 16:37
sbc100
sbc100 approved these changes Jan 10, 2025
View reviewed changes
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % comment

@hoodmane
Merge branch 'main' into enoent-on-empty-string
a890bab
@hoodmane
Add curly braces
d09b60c
@hoodmane
Merge branch 'main' into enoent-on-empty-string
4465120
@hoodmane
Automatic rebaseline of codesize expectations. NFC
f9fba53 
This is an automatic change generated by tools/maint/rebaseline_tests.py.

The following (18) test expectation files were updated by
running the tests with `--rebaseline`:

```
other/codesize/test_codesize_cxx_ctors1.gzsize: 8350 => 8343 [-7 bytes / -0.08%]
other/codesize/test_codesize_cxx_ctors1.jssize: 20283 => 20273 [-10 bytes / -0.05%]
other/codesize/test_codesize_cxx_ctors2.gzsize: 8332 => 8327 [-5 bytes / -0.06%]
other/codesize/test_codesize_cxx_ctors2.jssize: 20251 => 20241 [-10 bytes / -0.05%]
other/codesize/test_codesize_cxx_except.gzsize: 9349 => 9343 [-6 bytes / -0.06%]
other/codesize/test_codesize_cxx_except.jssize: 24051 => 24041 [-10 bytes / -0.04%]
other/codesize/test_codesize_cxx_except_wasm.gzsize: 8298 => 8294 [-4 bytes / -0.05%]
other/codesize/test_codesize_cxx_except_wasm.jssize: 20176 => 20166 [-10 bytes / -0.05%]
other/codesize/test_codesize_cxx_except_wasm_exnref.gzsize: 8298 => 8294 [-4 bytes / -0.05%]
other/codesize/test_codesize_cxx_except_wasm_exnref.jssize: 20176 => 20166 [-10 bytes / -0.05%]
other/codesize/test_codesize_cxx_lto.gzsize: 8362 => 8357 [-5 bytes / -0.06%]
other/codesize/test_codesize_cxx_lto.jssize: 20358 => 20348 [-10 bytes / -0.05%]
other/codesize/test_codesize_cxx_mangle.gzsize: 9355 => 9349 [-6 bytes / -0.06%]
other/codesize/test_codesize_cxx_mangle.jssize: 24051 => 24041 [-10 bytes / -0.04%]
other/codesize/test_codesize_cxx_noexcept.gzsize: 8350 => 8343 [-7 bytes / -0.08%]
other/codesize/test_codesize_cxx_noexcept.jssize: 20283 => 20273 [-10 bytes / -0.05%]
other/codesize/test_codesize_files_js_fs.gzsize: 7653 => 7647 [-6 bytes / -0.08%]
other/codesize/test_codesize_files_js_fs.jssize: 18830 => 18820 [-10 bytes / -0.05%]

Average change: -0.06% (-0.08% - -0.04%)
```
@hoodmane
Copy link
Collaborator Author

hoodmane commented Jan 13, 2025 •
edited
Loading

Well this change makes other.test_unistd_fstatfs fail, but it looks to me like the __syscall_fstatfs64 implementation is seriously incorrect. It calls ___syscall_statfs64(0, size, buf). statfs64 does UTF8ToString(first_arg), so we take the null pointer and convert it to a string. The 0th byte of the memory generally seems to be 0 so we get the empty string. Then statfs calls lookupPath on empty string and gets a null node, and if the node is null statfs just returns default values. It will return these same default values no matter what argument we give it. After my change, instead of always returning the same values, fstatfs always returns ENOENT.

@hoodmane
Copy link
Collaborator Author

Opened #23381 to fix the problems in fstatfs turned up by this PR.

@hoodmane hoodmane changed the title FS: lookupPath should throw an error on an empty string [FS] lookupPath should throw an error on an empty string Jan 13, 2025
@hoodmane hoodmane merged commit 714eaea into emscripten-core:main Jan 13, 2025
29 checks passed
@hoodmane hoodmane deleted the enoent-on-empty-string branch January 13, 2025 14:08
@hoodmane hoodmane mentioned this pull request Jan 23, 2025
Open
@dschuff dschuff mentioned this pull request Mar 14, 2025
Merged
@dschuff dschuff mentioned this pull request Mar 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbc100 sbc100 sbc100 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hoodmane @sbc100