Skip to content

FetchFS: getFileRange issues out-of-bounds chunk requests #24438

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.

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

Open
foxtacles opened this issue May 29, 2025 · 5 comments · May be fixed by #24442
Open

FetchFS: getFileRange issues out-of-bounds chunk requests #24438

foxtacles opened this issue May 29, 2025 · 5 comments · May be fixed by #24442

Comments

@foxtacles
Copy link

foxtacles commented May 29, 2025

Version of emscripten/emsdk:

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 4.0.9 (bbf1caa6e24f64fca9eb6a13a9e02d3f42123e77)
clang version 21.0.0git (https:/github.com/llvm/llvm-project 2f05451198e2f222ec66cec4892ada0509519290)
Target: wasm32-unknown-emscripten
Thread model: posix

The getFileRange function in FetchFS currently allows requests for chunks outside the file boundaries. The server will then respond with 416 Range Not Satisfiable, which is properly handled in the code but I think we should save ourselves the request in the first place.

I've applied the attached patch to resolve this problem. I'd submit a pull request but I don't have the time right now to set up a local test environment.

CC @JoeOsborn

bounds.patch

@JoeOsborn
Copy link
Contributor

It looks good to me!

sbc100 added a commit to sbc100/emscripten that referenced this issue May 29, 2025
@sbc100
Copy link
Collaborator

sbc100 commented May 29, 2025

I opened #24442 with this patch. But we really should write a test too.

Do either of you know how to re-produce the issue and make a test?

@foxtacles
Copy link
Author

foxtacles commented May 30, 2025

I know how to re-produce it in principle (simply by trying to read from the file past the end of it). However I can't get the local tests to work (yet), they are all failing, so I can't write a test right now:

EMTEST_BROWSER=chromium ./test/runner.py browser.test_wasmfs_fetch*

--- expected
+++ actual
@@ -1 +1 @@
-/report_result?exit:0
+/report_result?abort:Assertion failed: read_now > 0, at: /home/foxtacles/emsdk/upstream/emscripten/test/wasmfs/wasmfs_fetch.c,136,read_chunks_check

Probably missing something obvious since it's my first time running the tests. Other tests seem to pass.

@foxtacles
Copy link
Author

foxtacles commented May 30, 2025

I've managed to get the tests running using Firefox.

Unfortunately, reproducing this issue with tests turned out much harder for 2 reasons:

  1. Using the read API, I somehow can't manage to nudge the file offset past the end of the file (which actually seems like sound behavior). I'm not sure how it was possible that this happened in the first place in my Emscripten app (but it definitely happens and I can still reproduce it there). This might hint to another bug somewhere else.
  2. Whenever I set the offset manually past the end of the file (subsequent Range request has a start offset past the file size), I'm unable to reproduce the problem because the test server the request is issued against never returns 416 Range Not Satisfiable, but instead returns a 2xx with an empty body. That seems incorrect.

@sbc100
Copy link
Collaborator

sbc100 commented May 30, 2025

If you are referring to test server using by the emscripten test suite that seems like something we should be able to fix (re generating 416 error).

The fact that you cannot make it happen using normal read calls is more worrying. If you are able to reproduce in your full app can you capture a full backtrace so we can see where call comes from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants