Skip to content

JS FS: Don't return ELOOP on a path with a large number of ..'s #24591

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
Jun 18, 2025
Merged

JS FS: Don't return ELOOP on a path with a large number of ..'s #24591

merged 6 commits into from
Jun 18, 2025

Conversation

hoodmane
Copy link
Collaborator

Decrement nlinks before doing continue linkloop; due to a .. at a file system loop. We're not actually traversing a symlink here.

Caught by the Python test suite.

@hoodmane
JS FS: Don't return ELOOP on a path with a large number of ..'s
120de73 
Decrement nlinks before doing `continue linkloop;` due to a .. at a
file system loop. We're not actually traversing a symlink here.
This was referenced Jun 17, 2025
Closed
Merged
sbc100
sbc100 reviewed Jun 17, 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.

Thanks!

@hoodmane
Copy link
Collaborator Author

Okay updated it.

sbc100
sbc100 reviewed Jun 17, 2025
View reviewed changes
@hoodmane
Copy link
Collaborator Author

Now let's see if I can convince rebaseline_tests to work correctly. I reran ./emsdk install tot and then tools/maint/rebaseline_tests.py but the size diffs looked way too big for me, so I tried ./emcc --clear-cache and am running again. I get a couple of closure tests tests failing with roughly this error:

test_codesize_minimal_O1 (test_other.other) ... FAIL
Running codesize test: mem_no_argv.c: ['-O3', '-sSTANDALONE_WASM'] True False
building:ERROR: Closure compiler run failed:

building:ERROR: /tmp/emtest_y5rraz1l/emscripten_temp_2bt05snf/a.out.jso4.js:43:262: ERROR - [JSC_UNDEFINED_VARIABLE] variable assert is undeclared
  43| if(typeof module!="undefined"){module["exports"]=Module}quit_=(status,toThrow)=>{process.exitCode=status;throw toThrow}}else if(ENVIRONMENT_IS_SHELL){readBinary=f=>{if(typeof readbuffer=="function"){return new Uint8Array(readbuffer(f))}let data=read(f,"binary");assert(typeof data=="object");return data};readAsync=async f=>readBinary(f);globalThis.clearTimeout??=id=>{};// spidermonkey lacks setTimeout but we use it above in readAsync.
                                                                                                                                                                                                                                                                            ^^^^^^

1 error(s), 0 warning(s)

@hoodmane
Copy link
Collaborator Author

The jssizes have all gone up by ~600 bytes and the gzsizes by ~1300 bytes. Seems too big for this change.

@sbc100
Copy link
Collaborator

sbc100 commented Jun 17, 2025

If you want I can try to generate the codesize change for this PR?

@hoodmane
Copy link
Collaborator Author

That would be helpful. It would be good to eventually figure out how to generate these myself. Maybe we could run the rebasline_tests.py in a docker image like @kleisauke suggested.

@sbc100
Automatic rebaseline of codesize expectations. NFC
fc85fd2 
This is an automatic change generated by tools/maint/rebaseline_tests.py.

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

```
test/other/codesize/test_codesize_cxx_ctors1.gzsize updated
test/other/codesize/test_codesize_cxx_ctors1.jssize updated
test/other/codesize/test_codesize_cxx_ctors2.gzsize updated
test/other/codesize/test_codesize_cxx_ctors2.jssize updated
test/other/codesize/test_codesize_cxx_except.gzsize updated
test/other/codesize/test_codesize_cxx_except.jssize updated
test/other/codesize/test_codesize_cxx_except_wasm.gzsize updated
test/other/codesize/test_codesize_cxx_except_wasm.jssize updated
test/other/codesize/test_codesize_cxx_except_wasm_legacy.gzsize updated
test/other/codesize/test_codesize_cxx_except_wasm_legacy.jssize updated
test/other/codesize/test_codesize_cxx_lto.gzsize updated
test/other/codesize/test_codesize_cxx_lto.jssize updated
test/other/codesize/test_codesize_cxx_mangle.gzsize updated
test/other/codesize/test_codesize_cxx_mangle.jssize updated
test/other/codesize/test_codesize_cxx_noexcept.gzsize updated
test/other/codesize/test_codesize_cxx_noexcept.jssize updated
test/other/codesize/test_codesize_files_js_fs.gzsize updated
test/other/codesize/test_codesize_files_js_fs.jssize updated
test/other/codesize/test_codesize_hello_dylink.gzsize updated
test/other/codesize/test_codesize_hello_dylink.jssize updated
```
@sbc100
Copy link
Collaborator

sbc100 commented Jun 17, 2025

Done

@hoodmane hoodmane closed this Jun 18, 2025
@hoodmane hoodmane reopened this Jun 18, 2025
@hoodmane
Copy link
Collaborator Author

All the tests passed but something went wrong with the status report for test-wasm-js and GitHub shows it as in progress while circle ci shows it as passed. I guess I can push an empty commit and retrigger.

@sbc100 sbc100 merged commit cc1de1a into emscripten-core:main Jun 18, 2025
30 of 32 checks passed
@hoodmane hoodmane deleted the no-eloop-due-to-dotdot branch June 19, 2025 14:34
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