[fetch] Fix double free in fetch_free() with streaming fetch requests #25856
+62
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fetch->data will be allocated and freed in onprogress if fetch attribute EMSCRIPTEN_FETCH_STREAM_DATA is used, however it will still be freed in fetch_free() causing double free problem.
Collaborator
|
Maybe we can run some of the browser tests with lsan in order catch this? |
Contributor
Author
OK, I've added a test called After applying this fix, the test will pass. |
Collaborator
|
Thanks for adding a test! @brendandahl WDYT? |
sbc100
reviewed
Nov 24, 2025
system/lib/fetch/emscripten_fetch.c
Outdated
| emscripten_fetch_free(fetch->id); | ||
| fetch->id = 0; | ||
| free((void*)fetch->data); | ||
| if (!(fetch->__attributes.attributes & EMSCRIPTEN_FETCH_STREAM_DATA)) { |
Collaborator
There was a problem hiding this comment.
When EMSCRIPTEN_FETCH_STREAM_DATA is used where is ->data free'd? i.e. where is the double free exacly?
Collaborator
There was a problem hiding this comment.
Ah, I see the problem.
Can you instead set this pointer back to NULL when the data is free'd.
i.e. can you add {{{ makeSetValue('fetch', C_STRUCTS.emscripten_fetch_t.data, 0, '*') }}} after the free call on line 671 of Fetch.js instead of this change?
Contributor
Author
There was a problem hiding this comment.
I'm afraid not. Closing the fetch directly in onprogress callback will free fetch->data immediately. However, the ptr variable in xhr.onprogress remains non-zero even after setting fetch->data to NULL in fetch_free. This leads to a double free when ptr is freed at the end of xhr.onprogress.
Lines 642 to 672 in 797cf32
This problem can be reproduced by modifying the test to following code:
attr.onprogress = [](emscripten_fetch_t *fetch) {
printf("Abort fetch when downloading\n");
emscripten_fetch_close(fetch);
};ASAN output:
[47964:34952:1125/232003.321:INFO:CONSOLE:146] "Downloading.. 0.58% complete. Received chunk [0, 785039[", source: http://localhost:8888/test.html (146)
[47964:34952:1125/232003.322:INFO:CONSOLE:146] "Abort fetch when downloading", source: http://localhost:8888/test.html (146)
[47964:34952:1125/232003.322:INFO:CONSOLE:146] "Downloading failed with status code: 65535.", source: http://localhost:8888/test.html (146)
[47964:34952:1125/232003.324:INFO:CONSOLE:1431] "=================================================================", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.324:INFO:CONSOLE:1431] "==42==ERROR: AddressSanitizer: attempting double-free on 0x13e20800 in thread T0:", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.327:INFO:CONSOLE:1431] " #0 0x00019cc7 (this.program+0x19cc7)", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.327:INFO:CONSOLE:1431] " #1 0x800002ae (JavaScript+0x2ae)", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.327:INFO:CONSOLE:1431] " #2 0x80001123 in xhr.onprogress http://localhost:8888/test.js:4387:5", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.327:INFO:CONSOLE:1431] " #3 0x800012aa in FetchXHR.send http://localhost:8888/test.js:4778:16", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.327:INFO:CONSOLE:1431] "0x13e20800 is located 0 bytes inside of 785039-byte region [0x13e20800,0x13ee028f)", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.327:INFO:CONSOLE:1431] "freed by thread T0 here:", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.328:INFO:CONSOLE:1431] " #0 0x00019cc7 (this.program+0x19cc7)", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.329:INFO:CONSOLE:1431] " #1 0x00001b72 (this.program+0x1b72)", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.329:INFO:CONSOLE:1431] " #2 0x00001cad (this.program+0x1cad)", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.329:INFO:CONSOLE:1431] " #3 0x0000187f (this.program+0x187f)", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.329:INFO:CONSOLE:1431] " #4 0x00001320 (this.program+0x1320)", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.329:INFO:CONSOLE:1431] " #5 0x800012f9 (JavaScript+0x12f9)", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.329:INFO:CONSOLE:1431] " #6 0x8000117e in callUserCallback http://localhost:8888/test.js:4478:5", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.329:INFO:CONSOLE:1431] "previously allocated by thread T0 here:", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.329:INFO:CONSOLE:1431] " #0 0x00019e9c (this.program+0x19e9c)", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.330:INFO:CONSOLE:1431] " #1 0x800002ae (JavaScript+0x2ae)", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.330:INFO:CONSOLE:1431] " #2 0x80001115 in xhr.onprogress http://localhost:8888/test.js:4373:13", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.330:INFO:CONSOLE:1431] " #3 0x800012aa in FetchXHR.send http://localhost:8888/test.js:4778:16", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.330:INFO:CONSOLE:1431] "SUMMARY: AddressSanitizer: double-free (this.program+0x19cc3) ", source: http://localhost:8888/test.js (1431)
[47964:34952:1125/232003.330:INFO:CONSOLE:1431] "==42==ABORTING", source: http://localhost:8888/test.js (1431)
Collaborator
There was a problem hiding this comment.
Ok, that makes sense.
Howabout we switch to using realloc, avoiding the need to the inbetween free calls completely. Like this: #25863
Contributor
Author
There was a problem hiding this comment.
Yes, I think it's better than my fix. It keeps the lifecycle of fetch->data consistent between streaming and non-streaming fetch requests.
Collaborator
There was a problem hiding this comment.
Feel free the copy the changes from my PR into this one.
Contributor
Author
There was a problem hiding this comment.
Thank you!
Updated, and my newly added test test_fetch_stream_abort passed.
sbc100
approved these changes
Nov 25, 2025
brendandahl
approved these changes
Nov 25, 2025
Collaborator
|
This PR added a new test Does not seem to occur 100% of the time. |
Collaborator
Looks like maybe something to do with the fact that the fetch is being cancelled .. the server looks like its throwing: That doesn't explain why it would only be the asan version that failed though.. |
Collaborator
|
Hmm.. actually it looks like that exception is shown even on successful runs: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fetch->data will be allocated and freed in onprogress if fetch attribute EMSCRIPTEN_FETCH_STREAM_DATA is used, however it will still be freed in fetch_free() causing double free problem.