Repository navigation
fix: combined WASM hardening (stack guard, v18 errors, scan OOM, sync recovery, ESM, native parity) - #190
fix: combined WASM hardening (stack guard, v18 errors, scan OOM, sync recovery, ESM, native parity)#190pyramation wants to merge 21 commits into
Conversation
libpg_query's check_stack_depth() never had a base pointer, so recursion past the WASM stack trapped (memory access out of bounds). Call the fork's pg_query_set_stack_limit() at every export with the stack actually left (weak symbol: upstream pins skip it) and pin 17/18 to the fork branches. Restore the shadow stack pointer in callWasm after a throw so sync calls on the stale instance do not start half-way down it. Regression test: 10 parseSync of a 100k-term expression under --stack-size=8000.
…tial or empty tokens
…ut of memory as out of WASM memory
…ever runs on a broken one
…in/1792902893-combined-hardening
…792902893-combined-hardening
…/1792902893-combined-hardening # Conflicts: # templates/full/wasm_wrapper.c # versions/18/Makefile # versions/18/package.json # versions/18/src/wasm_wrapper.c
…into devin/1792902893-combined-hardening
…in/1792902893-combined-hardening # Conflicts: # templates/full/index.ts # templates/index.ts # versions/13/src/index.ts # versions/14/src/index.ts # versions/15/src/index.ts # versions/16/src/index.ts # versions/17/src/index.ts # versions/18/src/index.ts
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟠 1 high · 🟡 1 medium 💬 Inline comments (2)
This PR reworks the WASM wrappers to recover from out-of-memory and stack-overflow failures by replacing the Emscripten instance and unwinding the shadow stack, adds a detailed-result ABI (
Reviewed commit: 7abd626 |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds WASM OOM/stack-recovery handling with an instance-replacement loader, control-character escaping and a detailed-result ABI in the C wrappers, a native-parity harness, and ESM packaging updates, copied across versions 13–18.
Key findings
- 🟠 Size
escaped_textto fit the\u00XXescape — wasm_wrapper.c:362 - 🟡 Reset
largeUnfreedonly after memory can be reclaimed — index.ts:231
| @@ -326,6 +381,8 @@ static char* build_scan_json(PgQuery__ScanResult *scan_result, const char* origi | |||
| } else if (c == '\t') { | |||
| escaped_text[escaped_pos++] = '\\'; | |||
| escaped_text[escaped_pos++] = 't'; | |||
| } else if ((unsigned char) c < 0x20) { | |||
| escaped_pos += snprintf(escaped_text + escaped_pos, 7, "\\u%04x", (unsigned char) c); | |||
There was a problem hiding this comment.
🟠 security · high
Size escaped_text to fit the \u00XX escape
In build_scan_json, the control-character escape can write past the end of escaped_text: the buffer is sized token_length * 6 + 1 (templates/full/wasm_wrapper.c:362), but after the final control character escaped_pos equals 6 * token_length and snprintf(..., 7, ...) writes 7 bytes (6 chars plus NUL), overflowing up to 6 bytes beyond the allocation (templates/full/wasm_wrapper.c:385). Attacker-supplied SQL containing control characters not explicitly handled (e.g. \f, \v) reaches this path through wasm_scan, so the overflow corrupts adjacent WASM heap and can yield corrupted scan JSON or a crash.
📋 Prompt for AI Agents
In templates/full/wasm_wrapper.c around lines 362-385, the escape buffer is allocated as safe_malloc((size_t) token_length * 6 + 1) but the \u%04x escape uses snprintf(escaped_text + escaped_pos, 7, ...) which can write 7 bytes starting at offset 6 * token_length, overflowing by up to 6 bytes. Change the allocation to safe_malloc((size_t) token_length * 6 + 8) so the NUL terminator of the last snprintf always fits. Apply the same fix to the identical code in versions/18/src/wasm_wrapper.c (same pattern around line 369).
| if (wasmModule.HEAPU8.length > LARGE_INSTANCE_BYTES && largeUnfreed++ === 0) { | ||
| queueMicrotask(() => { largeUnfreed = 0; }); | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
Reset largeUnfreed only after memory can be reclaimed
The repeated-OOM guard resets largeUnfreed to 0 in a queueMicrotask (templates/full/index.ts:232), which fires at the first yield even though the replaced >512 MiB instance is only reclaimed by a later GC. In the common try { await parse(huge) } catch {} loop, every iteration yields, so largeUnfreed returns to 0 and each OOM failure replaces the instance again while previously dropped 1 GiB instances may still be resident — the opposite of the cap the comment at templates/full/index.ts:154-156 promises. Users looping over oversized inputs with awaits between attempts can accumulate multiple GiB of un-reclaimed WASM memory.
📋 Prompt for AI Agents
In templates/full/index.ts (mirrored in templates/index.ts and versions/13..18/src/index.ts), callWasm's catch block around lines 231-233 schedules queueMicrotask(() => { largeUnfreed = 0; }) when a >512 MiB instance is replaced. Replace the microtask with setTimeout(() => { largeUnfreed = 0; }, 0) (or decrement by one per yielded large failure) so the one-large-instance-per-run budget does not reset across microtask-only yields while V8 has not yet reclaimed the dropped instance's WebAssembly.Memory. Keep the largeUnfreed++ === 0 scheduling condition unchanged so only one reset is ever pending per run, and update test/wasm-recovery.test.js expectations accordingly.
Summary
One PR that folds in every open hardening PR, plus pins for the combined fork branches: v18 → libpg_query #13 tip
3606788, v17 → #11 tipab619b8.set_stack_guard()on every export) plusstackSave/stackRestorearound each WASM call.@pgsql/parserESMimportfailed on Node 18 (Unexpected token 'export') → ESM entry points ship as.mjs.fingerprint/normalizereturned lexer errors as the result,scanthrew a JSONSyntaxErroror returned empty tokens → structuredSqlErrorviaWasmDetailedResult, andout of WASM memorywhen there isn't enough memory.scanaborted inside libpg_query and corrupted the instance → pinned fork with checked scan allocations; its"out of memory"is reported asout of WASM memory.scripts/native-parity.jsin CI.replaceInstance(), plus a same-tick cap on repeated over-limit calls.parsePlPgSQLSyncof aBEGIN ATOMICSQL function aborted (proc_source != NULLassertion) → new test inversions/18/test/plpgsql.test.js.callWasmkeeps both behaviours: #180'sstackRestore(stackPointer)runs first in thecatch, then #189's replacement logic. One extra commit addsstackSave/stackRestoreto the full-APIWasmModuleinterface, because v18build:jsfailedtscwithout them.Local results (combined pins, Docker emsdk build)
See the comment below for v17/v18 suites,
test:stressand native parity.Expected
wasm/libpg-query.wasmsha256:ab619b8):97ceae55660c18b2b6e379c9a3a93f667f477947740a7532aaf0056075db06c93606788):bbad03f9c704f4b76838c0e15a62a0dd67ad37f7baea1577823eeb98ba3f2864Merge order
18-constructive) and Bump tar from 6.1.0 to 6.1.5 #11 (17-constructive) with a merge commit, not squash. These pins are their head commits.Until step 1, the v17/v18
Build WASM Objectjobs fail withfatal: reference is not a treeand the dependent jobs are skipped. That is expected.Link to Devin session: https://app.devin.ai/sessions/68876ce060664609ab6b63c8bed91e78
Open in Devin Desktop: https://app.devin.ai/desktop/session/68876ce060664609ab6b63c8bed91e78?variant=devin
Requested by: @pyramation