XMLHttpRequest: stop truncating responses at the first NUL byte - #231
Conversation
ResponseString() returns a string_view over a body that is not null terminated and may legitimately contain embedded nulls, but only its data() pointer was handed to Napi, so the string was measured with strlen. Emscripten's EXPORT_ES6 output inlines the .wasm payload as a JavaScript string literal, so such files are full of nulls - spz.js arrived as 1955 of 805507 characters. Both responseText and response now construct the string with an explicit length.
There was a problem hiding this comment.
Pull request overview
This PR fixes XMLHttpRequest string responses being truncated at the first embedded NUL (\0) by ensuring Node-API string creation uses an explicit length rather than relying on C-string strlen behavior.
Changes:
- Build
responseTextand stringresponseviaNapi::String::New(env, data, size)to preserve embedded NULs. - Add a unit test that fetches an asset containing embedded NUL bytes and asserts the full 16-character string is returned.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Tests/UnitTests/Scripts/tests.ts | Adds a regression test asserting XHR string responses preserve embedded NUL bytes. |
| Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp | Switches XHR string conversion to explicit-length N-API string creation to avoid truncation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // ResponseString() is a string_view over the raw body, which is not null terminated and | ||
| // may legitimately contain embedded nulls: Emscripten's EXPORT_ES6 output, for example, | ||
| // inlines the .wasm payload as a JavaScript string literal. Passing .data() alone would | ||
| // hand a const char* to Napi and truncate the body at the first null byte, so the length | ||
| // has to be supplied explicitly. | ||
| const std::string_view responseString{m_request.ResponseString()}; | ||
| return Napi::String::New(Env(), responseString.data(), responseString.size()); |
There was a problem hiding this comment.
Real bug, same class — thanks. I'm deliberately leaving it out of this PR though, because it cannot be fixed the same way.
Napi::Eval is declared identically in all six engine backends:
Napi::Value Eval(Napi::Env env, const char* source, const char* sourceUrl);(Core/Node-API-JSI plus Core/Node-API/Include/Engine/{Chakra,Hermes,JavaScriptCore,QuickJS,V8}.) There is no length-taking form, so there is no local change at ScriptLoader.cpp:34 that fixes it — a real fix means adding a length-aware overload to each of the six backends and routing Eval through it. That is a much larger, riskier change than this one, so it belongs in its own PR. I'll open one.
One correction to your framing, which also applies to a comment I had written here: the buffer is NUL terminated. ResponseString() returns a string_view over UrlRequestImpl::m_responseString, a std::string, so .data() is guaranteed NUL terminated and there is no out-of-bounds read at either site. The only defect is truncation at an embedded NUL. I've corrected my comment in 635964f so it does not claim otherwise.
Worth noting the ScriptLoader path is the more user-visible of the two: an Emscripten EXPORT_ES6 bundle loaded through LoadScript gets silently cut at the first NUL and then fails with a syntax error a long way from the cause.
5246526 to
635964f
Compare
## Summary Update JsRuntimeHost from [`cb988baa`](BabylonJS/JsRuntimeHost@cb988ba) to [`0d263627`](BabylonJS/JsRuntimeHost@0d26362), picking up three fixes merged since the current pin: - [BabylonJS/JsRuntimeHost#220](BabylonJS/JsRuntimeHost#220) — prevent zero-delay `setInterval` from flooding and starving the JS dispatch queue. In BN this could indefinitely delay async shader-compilation continuations and hang scene readiness. - [BabylonJS/JsRuntimeHost#230](BabylonJS/JsRuntimeHost#230) — support UTF-16LE/UTF-16BE in `TextDecoder`, needed by Emscripten `EXPORT_ES6` modules such as SPZ and CSG2/manifold. - [BabylonJS/JsRuntimeHost#231](BabylonJS/JsRuntimeHost#231) — preserve embedded NUL bytes in XHR string responses, needed for Emscripten modules with inline WASM payloads. ## Validation - Configured Win32 x64 against the updated dependency and confirmed FetchContent resolved the exact pinned SHA. - Built the Debug Playground target successfully. - Ran the `Nested BBG` validation test, which exercises the timer-starvation regression path: **1/1 passed, exit 0**. - The three upstream changes include their own focused JsRuntimeHost unit coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601
ResponseString()returns astring_viewover a body that is not NUL terminated and may legitimately contain embedded NULs, but only itsdata()pointer was handed to Napi, so the string was measured withstrlen.Emscripten's
EXPORT_ES6output inlines the.wasmpayload as a JavaScript string literal, so such files are full of NULs —spz.jsarrived as 1955 of 805507 characters, and the module failed to load with a syntax error rather than anything that pointed at XHR.Both
responseTextandresponsenow construct the string with an explicit length.The new test fetches a 16-byte asset containing two embedded NULs; without the fix it reports
'start'instead of'start\0middle\0end'.