abetlen / abetlen/pikchr-js

Inputs over ~41 KB crash: ccall passes the markup on the 64 KiB C stack

未關閉
#23 0 則留言 1 個 reaction 已指派 0 人 在 GitHub 檢視
主要語言
C
星號
8
分支
2
PR 合併指標
30 天內沒有已合併 PR

描述

## Summary

Any diagram source larger than roughly 41 KB crashes the module with
`RuntimeError: memory access out of bounds` (or an assertion failure just below
that size). The failure point tracks the **byte size of the input**, not the
number of objects or tokens.

The cause is in the JS wrapper rather than in pikchr. `index.js` passes the
markup through `ccall` with a `"string"` argument type:

```js
cstring = module.ccall(
"pikchr",
"number",
["string", "string", "number", "number", "number"],
[markup, svgClass, flags, widthPtr, heightPtr]
);
```

Emscripten converts each `"string"` argument with `stringToUTF8OnStack()`, which
is `stackAlloc()`. **The whole markup is copied onto the module's C stack**,
whose size is fixed at compile time. `build.sh` does not set `-sSTACK_SIZE`, so
it is the emscripten default of 64 KiB. A 41 KB diagram plus pikchr's own frame
does not fit, the copy runs off the end of the stack and corrupts whatever sits
next to it.

The two out-parameters are already allocated on the heap with `_malloc`, so the
fix is a small and consistent one — see below.

## Reproduction

`pikchr-js@0.1.4`, Node v22.16.0, Windows 11 (also reproduced in Chrome).

```js
// A fresh module per case: once the WASM instance traps, every later call on it
// fails too, which would hide the threshold.
const loadPikchr = require("pikchr-js");

const stmt = (i) =>
`line from (${100 + (i % 400)}px,-${100 + ((i * 7) % 400)}px)` +
` to (${101 + (i % 400)}px,-${103 + ((i * 7) % 400)}px)`;

(async () => {
for (const n of [800, 900, 950, 1000, 1100]) {
const pikchr = await loadPikchr();
const src = Array.from({ length: n }, (_, i) => stmt(i)).join("\n");
let out;
try {
const svg = pikchr(src, "pikchr", 1);
out = svg.startsWith(" ${out}`);
}
})();
```

Output:

```
800 statements, 34399 bytes -> ok (77992 bytes of SVG)
900 statements, 38699 bytes -> ok (87611 bytes of SVG)
950 statements, 40849 bytes -> ok (92492 bytes of SVG)
1000 statements, 42999 bytes -> CRASH: Aborted(Assertion failed: yy_lookahead[i]==iLookAhead, at: pikchr.c,2028,yy_find_reduce_action)
1100 statements, 47299 bytes -> CRASH: memory access out of bounds
```

The assertion at 43 KB is worth noting: the parser is reading a corrupted stack,
not hitting a real grammar limit. Slightly below the crash threshold the same
corruption can surface as a *spurious* `syntax error` on input pikchr accepts —
a wrong answer rather than a visible failure. That is what makes this awkward to
diagnose from the outside.

The threshold moves with the shape of the statements, because it is the total
byte count that matters. Measurements on four different shapes:

| source, repeated *n* times | last *n* that works | bytes |
|-------------------------------------|--------------------:|-------:|
| `box wid 005px ht 005px` | 1936 | 44 527 |
| `box at (x,y)` | 2024 | 44 527 |
| `box at (x,y) wid 5px ht 5px` | 1086 | 44 525 |
| `line from (x,y) to (x,y)` | 1152 | 49 535 |

## This is not a pikchr limit

The same inputs render fine with the C engine. Building `pikchr.exe` from
current upstream trunk (lemon + MSVC) and feeding it a 77 KB, 1057-statement
drawing gives a clean 121 KB SVG, exit code 0. The same binary also handles the
synthetic cases above scaled up to 4000 statements (92 KB and 172 KB of source).

Rebuilding the same `pikchr.c` to WASM and calling it through the heap instead of
`ccall` produces output **identical byte for byte** to the native engine on those
inputs.

## Suggested fix

Copy the strings to the heap and call the export directly, mirroring what the
code already does for `widthPtr`/`heightPtr`. This removes the dependence on
input size entirely — the heap grows, the stack cannot.

```js
function toHeap(module, text) {
const n = module.lengthBytesUTF8(text) + 1;
const p = module._malloc(n);
if (!p) throw new Error(`failed to allocate ${n} bytes`);
module.stringToUTF8(text, p, n);
return p;
}

// inside render(), with pMarkup/pClass freed in the existing finally block:
const pMarkup = toHeap(module, String(markup));
const pClass = toHeap(module, String(svgClass));
cstring = module._pikchr(pMarkup, pClass, flags, widthPtr, heightPtr);
```

This needs `_pikchr` in `-sEXPORTED_FUNCTIONS` and
`stringToUTF8,lengthBytesUTF8` in `-sEXPORTED_RUNTIME_METHODS`.

Raising `-sSTACK_SIZE` alone would also push the threshold out, but it only moves
the ceiling rather than removing it. Some headroom there is worth having anyway:
`struct Pik` is a local of `pikchr()` and contains `PPoint aTPath[1000]`, about
16 KiB on its own, so 64 KiB is tight even before the markup is copied in.

I have both changes running locally against upstream trunk and would be glad to
open a PR if that is useful.

## Environment

- `pikchr-js@0.1.4` (published 2026-06-04), unmodified from npm
- Node v22.16.0, Windows 11; also reproduced in Chrome
- Comparison engine: pikchr built from trunk with MSVC 19.44

貢獻指南

這個儲存庫沒有索引到貢獻指南

評估

這個 Issue 還沒有評估資料。

把新 issue 寄到你的電子郵件信箱

精選適合新手參與的 GitHub issue 摘要。