dolthub / dolthub/doltlite

Stack frames up to 113 KB against the 64 KB wasm stack; no build uses -Wframe-larger-than

Closed
#2,910 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
268
Forks
18
Avg merge
2h 27m
Merged PRs (30d)
447

Description

Found in a full-repo review at `0ba280f06f`.

Compiling `src/*.c` with the build's own flags plus `-Wframe-larger-than=4096` reports stack frames up to 113 KB. The WebAssembly build sets no `-sSTACK_SIZE` anywhere in `ext/wasm/`, so it gets emscripten's 64 KB default — which `ext/wasm/GNUmakefile:900` already documents as a hazard:

> ACHTUNG: emsdk 3.1.27 reduced the default stack size from 5MB to a mere 64KB, which leads to silent memory corruption via the kvvfs VFS

The wasm build compiles the amalgamation, which contains all of these functions.

| bytes | function | site |
|---|---|---|
| **113,344** | `normalizeSideToMergedLayout` | `src/doltlite_merge_schema.c:1210` |
| **66,576** | `gcWriteCompactedTo` | `src/doltlite_gc.c:677` |
| 32,960 | `statusSequenceTrackedEqualOneWay` | `src/doltlite_status.c:119` |
| 32,880 | `detectFkViolationsForSpec` | `src/doltlite_merge_constraints_fk.c:264` |
| 32,352 | `doltliteBuildIndexEntry` | `src/doltlite_merge_rows.c:374` |
| 32,336 | `advanceToNextRow` | `src/doltlite_diff_table.c:1033` |
| 32,176 | `dsCountModifiedRow` | `src/doltlite_diff_stat.c:244` |
| 32,144 | `diffRecordsEqualFieldwise` | `src/prolly_diff.c:147` |
| 32,112 | `recordPrefixEquals` | `src/doltlite_merge_constraints.c:351` |
| 16,960 | `mergeSplitWorkingCatalog` | `src/doltlite_merge_cmd.c:299` |
| 16,720 | `detectUniqueViolationsForIndexWithoutRowid` | `src/doltlite_merge_constraints_unique.c:530` |
| 16,608 | `loadSchemaFromCatalogMode` | `src/doltlite_schema_diff.c:109` |
| 16,576 | `masterCollectViewTriggerRows` | `src/doltlite_diff.c:48` |
| 16,560 | `loadSchemaEntryFromCatalog` | `src/doltlite_schema_diff.c:300` |
| 16,544 | `loadSchemaCatalogRows` | `src/prolly_btree_catalog.c:729` |
| 16,544 | `checkoutLoadSourceTableSql` | `src/doltlite_checkout.c:67` |
| 16,496 | `statusSchemaHasViewOrTrigger` / `schemaHasAnyViewOrTrigger` | `src/doltlite_status.c:329`, `src/doltlite_diff.c:138` |
| 16,208 | `doltlitePartialIndexMatchesRecord` | `src/doltlite_merge_constraints_unique.c:142` |
| 16,112 | `resultUserCol` | `src/doltlite_record.c:593` |
| 16,096 | `bmColumn` | `src/doltlite_blame.c:974` |
| 16,080 | `patchGetValue` | `src/doltlite_patch.c:937` |
| 15,648 | `diffIterPrefetchPairs` | `src/prolly_diff.c:719` |
| 14,032 | `doltliteSyncChunks` | `src/doltlite_remote.c:269` |
| 10,576 | `prollyDiffIterStep` | `src/prolly_diff.c:1024` |
| 8,560 | `compareCatalogs` | `src/doltlite_status.c:956` |
| 6,384 | `serverWorkerEntry` | `src/doltlite_remotesrv.c:1244` |
| 5,328 | `csReplayWalFrom` | `src/chunk_wal.c:655` |
| 5,280 | `csOriginGetMany` | `src/doltlite_chunk_source.c:67` |
| 4,304 | `csWriteWalCheckpoint` | `src/chunk_wal.c:354` |

`normalizeSideToMergedLayout` alone overflows the whole wasm stack 1.8x on a schema-changing merge; `gcWriteCompactedTo` does so on `dolt_gc`.

Two contributing causes:

1. **`DoltliteRecordInfo` is 16,004 bytes** (`src/prolly_record.h:83-87`: `4 + 2 × SQLITE_MAX_COLUMN × 4`) and lives on the stack. Every 16 KB / 32 KB entry above is one or two of these.

2. **The new diff prefetch allocates 10 KB before checking whether it is needed.** `diffIterPrefetchFrame` (`src/prolly_diff.c:802`) declares `DiffPrefetchPair aPair[256]` and is inlined into `prollyDiffIterStep`; it tail-calls `diffIterPrefetchPairs` (`:719`, another `aPair[256]` plus `aHash[256]`) passing that array, so both frames are live together — **26,224 bytes, 41% of the wasm stack, in two frames**. Both declare their arrays *before* the `if( !pIter->pStore->pChunkSource ) return SQLITE_OK;` guard, so a purely local database pays the full 26 KB on every diff step even though prefetch is a no-op there.

This is the same defect class as #2567 / #2568 / #2570, where one silent stack overflow produced three unrelated-looking wasm bugs.

## Fix

- Add `-Wframe-larger-than` to the build so this cannot recur silently. It is currently used nowhere in the repo, and it is what finds this class first.
- Set an explicit `-sSTACK_SIZE` for the wasm build rather than inheriting emscripten's default.
- Heap-allocate the diff prefetch buffer once per `ProllyDiffIter`, or move it behind the chunk-source check.
- Shrink `DoltliteRecordInfo` to a small inline array with heap spill, which removes most of the table at once.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reproducing the report with the build's flags plus -Wframe-larger-than=4096, then read ext/wasm/GNUmakefile:900, src/prolly_diff.c:719 and :802, and src/prolly_record.h:83-87. Trace the listed large-frame functions and wasm build settings. Done means frame warnings are enabled, the wasm stack is explicit, and the identified stack allocations no longer overflow it.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, sqlite, wasm
Domain
build-system, databases, performance
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.