Skip to content

Memory safety - #17

Open
connormanning wants to merge 1 commit into
masterfrom
decompression-leak-safety
Open

Memory safety#17
connormanning wants to merge 1 commit into
masterfrom
decompression-leak-safety

Conversation

@connormanning

Copy link
Copy Markdown
Owner
  • Free memory allocated in the emscripten heap in decompressFile, analogous to the correct decompressChunk behavior
  • Make both decompressChunk and decompressFile allocate within the try block so that both are exception-safe without leaking

Fixes #16.

@dsmithn

dsmithn commented Aug 4, 2026

Copy link
Copy Markdown

Verified — this branch fixes the benchmarked case from #16.

Method. Built this branch at 60f1698. The built lib/ on master can't currently be loaded by Node directly (see the note at the end), so the harness consumes it bundled: esbuild lib/index.js --bundle --format=cjs --platform=node --external:laz-perf. laz-perf stays external so the harness's instrumented instance — the malloc/free accounting from #16 — is still the allocator under measurement, pinned at 0.0.6 across all arms. The harness gained one change for this: its copc import is now require(process.env.COPC_MODULE || 'copc'), so arm A can target a bundled build; the default is unchanged. Exact commands are in the repro repo: Verifying the fix — copc.js PR #17.

Results — the real EPT-node fixture from #16 (289,466 B; header reads 39,207 points × 29 B records) ×200 iterations, after the harness's common 50-iteration warmup; Node v26.3.0, macOS arm64:

build leaked mallocs / frees
copc@0.0.8 (npm, as filed) 57,899,000 B 400 / 0
master @ 9f9e447 (this PR's parent, bundled) 57,899,000 B 400 / 0
this PR @ 60f1698 (bundled) 0 B 400 / 400

Small fixture (5,894 B) ×2000: 0 B leaked, 4000/4000 mallocs/frees. On output correctness: within each run the harness asserts the real exported decompressFile decodes byte-identically to its freeing-mirror control arm, and separately, decompressFile's decoded output for this fixture is sha256-identical between copc@0.0.8 and this PR's build (1,137,003 B, consistent with the header's 39,207 × 29). The parent-commit control pins the change to this PR rather than to 0.0.8→master drift. npx jest on this branch: 11 suites / 29 tests pass. This exercises the success path only — the PR also moves the allocations inside the try, and that throwing path wasn't separately exercised.

On the ordering question from #16. The issue flagged the free-before-delete() order — which this PR uses, matching decompressChunk — as unverified. We've since read laz-perf's teardown at the trees matching the shipped packages (the 0.0.6-era and 0.0.7-era commits; identical on current master): the binding's LASZip holds a shared_ptr<reader::mem_file> and declares no destructor, mem_file::~mem_file() is empty, and its Private holds a charbuf — a std::streambuf view with no custom destructor — plus a std::istream. We didn't exhaustively trace every inherited member's teardown, so read this as "the destructors we traced don't touch the input buffer" rather than a formal proof — and note the benchmark can't decide this question either way (a _free'd block stays mapped in WASM linear memory), so the source read, not the 400/400, is the evidence here. It's also the order decompressChunk already uses.

One thing we hit while verifying, mentioned only because you said a release is coming: the built lib/ from npm run build on master can't be loaded by Node directly — loading lib/index.js fails with ERR_UNSUPPORTED_DIR_IMPORT on lib/ept, and the individual modules fail on extensionless relative specifiers like ./header (that's why we bundled). Published copc@0.0.8 is a CJS build and loads fine, so it seemed worth flagging before the release — if the ESM build is intended for bundler consumption only, feel free to ignore.

Thanks for the fast turnaround on this.

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 this pull request may close these issues.

Las.PointData.decompressFile never frees its two WASM allocations

2 participants