ICU: shrink the data package 13.5% with unreachable-item removal, a compact TOC, resource-bundle rewrites, and front-coded pool strings - #272
Jarred-Sumner wants to merge 9 commits into
Conversation
…es raw Every item in icu/remove-items.txt was shown to be unreachable by enumerating every ICU call site that can open it and finding no caller in JSC, WTF, or Bun: the NFKC_Casefold/SimpleCasefold normalizers, the Indic layout and emoji property tables, ugender / TZDB-abbreviation-parse / numeric-currency-code data, and the six per-tree available-locale indexes that nothing enumerates (the root and coll trees' indexes, which ARE enumerated, stay). The Dockerfiles assert each listed name still exists in the upstream .dat so an ICU upgrade cannot silently re-ship one or leave a stale name behind. keep-raw.txt's root.res / en*.res globs are single-segment and never matched zone/root.res + zone/en*.res, which JSC's eager time-zone warm-up opens inside VM::VM() -- so every process paid a ZSTD_DCtx + 128 KB dictionary init to inflate ~4.5 KB at startup. Match the zone tree's root/en bundles explicitly. Android and FreeBSD run the same item filter now; they previously only had the category filter. linux-x64 libicudata.a: 11,578,422 -> 11,447,310 bytes (-131,112). The repacked data, relinked into bun-debug, is byte-identical across a 64-case Intl/Segmenter/Collator/normalize corpus and passes test/js/web/intl and test/js/web/encoding.
The CmnD table of contents stores a full "icudt75l/tree/name.ext" string
plus an 8-byte {nameOffset,dataOffset} entry per item: 115 KB, 81% of it
the repeated package/tree prefixes and the 7 copies of every locale
basename that recur across the locale trees. Replace it with a
directory-prefix table, one global pool of unique basenames (16-way
front-coded), a u16 name id per entry, and a sentinel-terminated offset
column (which also gives the last item an explicit length instead of -1).
Item bodies and their DataHeaders are unchanged.
icu/ucmndata-toc.patch adds the reader as a new "CmnD" formatVersion 2
arm in ucmndata.cpp; a stock ICU rejects the package with
U_INVALID_FORMAT_ERROR instead of misreading it. compress-data.ts writes
the new TOC and, since stock `icupkg -l` can no longer read the output,
replays the reader's exact lookup for every item (and deliberate misses)
against the emitted bytes.
TOC: 114,364 -> 30,976 bytes on the current package (3,755 items).
linux-x64 libicudata.a: 11,447,310 -> 11,363,342 (-83,968; -215,080
cumulative vs the shipped baseline).
Verified by building ICU 75.1 with the patch and running the same
resource/collation/break-iterator/number/date/display-name test binary
against the v1-uncompressed, v1+zstd, and v2+zstd packages: all three
outputs are byte-identical.
…alues Adds icu/resbundle.ts, a reader/rewriter for the .res binary format (formatVersion 2/3, pool-bundle aware): parse to a resource graph, apply edits, re-serialize with hole compaction and full offset relocation across the 16-bit and 32-bit areas. Its identity rewrite is byte-for-byte identical on all 3,736 bundles in the package. icu/rewrite-items.ts applies it per item from compress-data.ts: - every bundle: store byte-identical sibling resources (containers, aliases, intvectors, binaries, strings) once -- value sharing is a documented property of the format that genrb only exploits for strings. root.res alone has 912 alias resources with 180 distinct targets. - supplementalData.res: drop `subdivisionContainment` and the dead `idValidity` classes; no reachable ICU reader opens either. - root-tree locale bundles: drop `characterLabel`, `personNames` (no linked ICU4C reader) and the per-locale `parse` tables (only root's is read). - brkitr: drop the UBRK_LINE / UBRK_TITLE boundary entries and their %%DEPENDENCY references, which lets the packer drop the ten line*/title .brk rule files themselves (icupkg's referential-integrity check is why they cannot go in remove-items.txt). - collation: delete coll/root.res `UCARules` and stub every tailoring's compiled-away `Sequence` rule source to a single code unit. The stub is deliberately non-empty: JSC keys its ASCII collation fast path on ucol_getRules() being non-empty, so deleting the rules outright would change Intl.Collator ordering for tailored locales. The packer proves each rewrite at build time: the rewritten bundle's resolved resource tree must equal the original's with exactly the intended edits applied (expectedDump), and the whole-package TOC verification runs as before. linux-x64 libicudata.a: 11,363,342 -> 10,432,526 bytes (-930,816; -1,145,896 cumulative vs the shipped baseline). The ICU-level behavior test (collation incl. zh/cs tailorings, word/sentence segmentation, number/date/display-name formatting, locale enumeration) is byte-identical before and after.
icu/test-package.cpp links the patched libicuuc/libicui18n and the repacked libicudata.a produced moments earlier in the same stage, then exercises every ICU data family a JavaScript engine reaches: locale enumeration, display names, number/currency and date/time-zone formatting, collation (including the zh/cs tailorings and the ucol_getRules() non-emptiness that JSC's ASCII fast path keys on), word + sentence break iteration through all four dictionaries, normalization, and the removed items (which must report U_MISSING_RESOURCE_ERROR). It defines the same weak per-item zstd hook Bun does, so the compact TOC, the lazy decompression, and the rewritten bundles are all exercised on the real artifact. Any failure fails the image build. Enabled on the linux and musl images (the windows one is a cross build and cannot run its artifacts). The zstd stanzas now also install libzstd.a + zstd.h for it.
Fixes found while reviewing the previous three commits, plus the build guards that make the whole pipeline fail loudly instead of silently drifting across ICU upgrades: - Preserve URES_BINARY payload alignment through bundle rewrites: genrb 16-aligns binary payloads and the collation reader casts them to int64_t*, but the 16-bit-area shrink was shifting the 32-bit area by arbitrary word counts (7 of 17 collation binaries landed misaligned). Quantize the shrink, keep 32-bit holes 16-byte-sized in front of live binaries, and assert the invariant in the serializer. - Resolve Table32 keys with RES_GET_KEY32 semantics (the sign bit selects the pool), not the key16 localKeyLimit rule. - Bound every copy into the TOC reader's name buffer and validate the front-coding lcp byte, so a corrupt package fails the lookup instead of overrunning the stack. - Refuse string replacements that land inside a live, unreplaced string (genrb suffix-shares string storage; coll/es.res does this today). - Restore the "category filter matched something" guard the filter rewrite dropped from the Dockerfiles, and assert in the packer that the filter ran. - Fail the build when a named rewrite no longer fires or a keep-raw glob no longer matches (both guards immediately caught real stale entries, including a keep-raw entry dead since ICU 64), derive the .brk drop list from the references actually removed, and filter dangling %%DEPENDENCY entries instead of leaving them behind. - Enforce (not just print) the ucol_getRules() non-emptiness that JSC's collation fast path keys on, and probe removed/kept items. - Run the package test on the Windows cross build too, host-natively against the emitted .dat (new --emit-dat flag), and align the test hook's constants with the runtime hook.
WalkthroughThis PR reworks ICU data packaging across Dockerfiles and new TypeScript/C++ tooling: it introduces a compact TOC formatVersion-2 layout, front-coded pool string support, resource-bundle rewriting/dedup, explicit removal manifests, static zstd builds, and a build-time validation binary that gates the packaged ICU data. ChangesICU packaging pipeline rework
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@icu/compress-data.ts`:
- Around line 260-271: Update the Compact TOC format comment in compress-data.ts
so the header layout matches the actual V2 writer/reader contract. The last
header word is not a reserved 0; it is nameBlockSize, written by writePackageV2
and validated by verifyPackageV2. Adjust the ASCII-art header field list and
surrounding prose to document that slot accurately, keeping the rest of the TOC
layout description unchanged.
In `@icu/test-package.cpp`:
- Around line 84-95: `readAll` needs explicit failure handling for `ftell` and
`malloc`: after seeking to the end, verify `ftell` did not return a negative
size before using it, and check the allocation result before calling `fread`. If
either step fails, emit a clear error and exit cleanly so the Windows argv input
path in `test-package.cpp` does not proceed with an invalid length or null
buffer.
In `@icu/ucmndata-toc.patch`:
- Around line 210-215: The offsetTOC2LookupFn branch writes through pLength
without checking whether it is null, unlike the TOC1 path where LookupFn treats
it as optional. Update the offsetTOC2LookupFn logic around the dataOffsets
lookup so it only assigns to pLength when it is non-null, while keeping the
existing return behavior for the DataHeader unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b21589da-f509-4ec0-bac3-a9681790b29e
📒 Files selected for processing (12)
DockerfileDockerfile.androidDockerfile.freebsdDockerfile.muslDockerfile.windowsicu/compress-data.tsicu/keep-raw.txticu/remove-items.txticu/resbundle.tsicu/rewrite-items.tsicu/test-package.cppicu/ucmndata-toc.patch
| // --------------------------------------------------------------------------- | ||
| // Compact TOC ("CmnD" formatVersion 2) — the reader is icu/ucmndata-toc.patch. | ||
| // | ||
| // Layout after the (verbatim, formatVersion-bumped) DataHeader; every offset | ||
| // is relative to the TOC start, exactly like formatVersion 1: | ||
| // | ||
| // u32[12] header: count, treeCount, nameCount, bucketCount, maxNameLength, | ||
| // dataOffsetsOff, nameIdsOff, treesOff, bucketDirOff, 0,0,0 | ||
| // u32 dataOffsets[count+1] ([count] is a sentinel; length = next - this) | ||
| // u16 nameIds[count] (ascending within each tree) | ||
| // { u32 dirNameOffset; u32 firstEntry; u32 entryCount; } trees[treeCount] | ||
| // u32 bucketDir[bucketCount] (start offset of each front-coded block) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Stale header layout comment: last word documented as 0 is actually nameBlockSize.
The ASCII-art header layout says the last three u32 header words are 0,0,0, but writePackageV2 writes V2_BLOCK into the 10th word (line 371), and verifyPackageV2 reads/validates that exact slot as nameBlockSize (lines 402-403, 406). The implementation is internally consistent, but the comment documenting this binary format — which is also the reader contract for icu/ucmndata-toc.patch — is misleading.
📝 Proposed fix
// u32[12] header: count, treeCount, nameCount, bucketCount, maxNameLength,
-// dataOffsetsOff, nameIdsOff, treesOff, bucketDirOff, 0,0,0
+// dataOffsetsOff, nameIdsOff, treesOff, bucketDirOff,
+// nameBlockSize, 0, 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // --------------------------------------------------------------------------- | |
| // Compact TOC ("CmnD" formatVersion 2) — the reader is icu/ucmndata-toc.patch. | |
| // | |
| // Layout after the (verbatim, formatVersion-bumped) DataHeader; every offset | |
| // is relative to the TOC start, exactly like formatVersion 1: | |
| // | |
| // u32[12] header: count, treeCount, nameCount, bucketCount, maxNameLength, | |
| // dataOffsetsOff, nameIdsOff, treesOff, bucketDirOff, 0,0,0 | |
| // u32 dataOffsets[count+1] ([count] is a sentinel; length = next - this) | |
| // u16 nameIds[count] (ascending within each tree) | |
| // { u32 dirNameOffset; u32 firstEntry; u32 entryCount; } trees[treeCount] | |
| // u32 bucketDir[bucketCount] (start offset of each front-coded block) | |
| // --------------------------------------------------------------------------- | |
| // Compact TOC ("CmnD" formatVersion 2) — the reader is icu/ucmndata-toc.patch. | |
| // | |
| // Layout after the (verbatim, formatVersion-bumped) DataHeader; every offset | |
| // is relative to the TOC start, exactly like formatVersion 1: | |
| // | |
| // u32[12] header: count, treeCount, nameCount, bucketCount, maxNameLength, | |
| // dataOffsetsOff, nameIdsOff, treesOff, bucketDirOff, | |
| // nameBlockSize, 0, 0 | |
| // u32 dataOffsets[count+1] ([count] is a sentinel; length = next - this) | |
| // u16 nameIds[count] (ascending within each tree) | |
| // { u32 dirNameOffset; u32 firstEntry; u32 entryCount; } trees[treeCount] | |
| // u32 bucketDir[bucketCount] (start offset of each front-coded block) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@icu/compress-data.ts` around lines 260 - 271, Update the Compact TOC format
comment in compress-data.ts so the header layout matches the actual V2
writer/reader contract. The last header word is not a reserved 0; it is
nameBlockSize, written by writePackageV2 and validated by verifyPackageV2.
Adjust the ASCII-art header field list and surrounding prose to document that
slot accurately, keeping the rest of the TOC layout description unchanged.
| static void* readAll(const char* path, long* outLen) { | ||
| FILE* f = fopen(path, "rb"); | ||
| if (!f) { perror(path); exit(2); } | ||
| fseek(f, 0, SEEK_END); | ||
| long n = ftell(f); | ||
| fseek(f, 0, SEEK_SET); | ||
| void* d = malloc(n); | ||
| if (fread(d, 1, n, f) != (size_t)n) exit(2); | ||
| fclose(f); | ||
| if (outLen) *outLen = n; | ||
| return d; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard ftell/malloc failures in readAll.
ftell can return -1 (then malloc(n) requests SIZE_MAX) and malloc isn't null-checked, so a failed allocation feeds a null pointer to fread. This path is reachable via the argv input mode used by the Windows build. Fail with a clear error instead of an ambiguous crash.
🛡️ Proposed hardening
fseek(f, 0, SEEK_END);
long n = ftell(f);
+ if (n < 0) { perror(path); exit(2); }
fseek(f, 0, SEEK_SET);
void* d = malloc(n);
+ if (!d) { fprintf(stderr, "%s: out of memory\n", path); exit(2); }
if (fread(d, 1, n, f) != (size_t)n) exit(2);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static void* readAll(const char* path, long* outLen) { | |
| FILE* f = fopen(path, "rb"); | |
| if (!f) { perror(path); exit(2); } | |
| fseek(f, 0, SEEK_END); | |
| long n = ftell(f); | |
| fseek(f, 0, SEEK_SET); | |
| void* d = malloc(n); | |
| if (fread(d, 1, n, f) != (size_t)n) exit(2); | |
| fclose(f); | |
| if (outLen) *outLen = n; | |
| return d; | |
| } | |
| static void* readAll(const char* path, long* outLen) { | |
| FILE* f = fopen(path, "rb"); | |
| if (!f) { perror(path); exit(2); } | |
| fseek(f, 0, SEEK_END); | |
| long n = ftell(f); | |
| if (n < 0) { perror(path); exit(2); } | |
| fseek(f, 0, SEEK_SET); | |
| void* d = malloc(n); | |
| if (!d) { fprintf(stderr, "%s: out of memory\n", path); exit(2); } | |
| if (fread(d, 1, n, f) != (size_t)n) exit(2); | |
| fclose(f); | |
| if (outLen) *outLen = n; | |
| return d; | |
| } |
🧰 Tools
🪛 Cppcheck (2.21.0)
[warning] 91-91: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@icu/test-package.cpp` around lines 84 - 95, `readAll` needs explicit failure
handling for `ftell` and `malloc`: after seeking to the end, verify `ftell` did
not return a negative size before using it, and check the allocation result
before calling `fread`. If either step fails, emit a clear error and exit
cleanly so the Windows argv input path in `test-package.cpp` does not proceed
with an invalid length or null buffer.
Source: Linters/SAST tools
| + dataOffsets=(const uint32_t *)(base+toc->dataOffsetsOff); | ||
| +#ifdef UDATA_DEBUG | ||
| + fprintf(stderr, "%s: Found.\n", tocEntryName); | ||
| +#endif | ||
| + *pLength=(int32_t)(dataOffsets[i+1]-dataOffsets[i]); | ||
| + return (const DataHeader *)(base+dataOffsets[i]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the v1 reader handles pLength and whether any Lookup caller passes null.
fd -t f 'udata-decompress-hook.patch' /icu-bun icu 2>/dev/null | xargs -r sed -n '1,200p'
echo "----- offsetTOCLookupFn pLength handling (patched v1) -----"
rg -nP -C3 'pLength' --glob '*ucmndata*' --glob '*.patch'
echo "----- Lookup() call sites -----"
rg -nP -C3 '->Lookup\s*\(' --glob '*.patch' --glob '*ucmndata*' --glob '*udata*'Repository: oven-sh/WebKit
Length of output: 1411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant patch and source files =="
fd -a 'ucmndata-toc.patch|udata.cpp|ucmndata.cpp|udata.h|ucmndata.h' .
echo
echo "== inspect offsetTOC2LookupFn in icu/ucmndata-toc.patch =="
patch_file="$(fd -a 'ucmndata-toc.patch' . | head -n1)"
if [ -n "${patch_file:-}" ]; then
rg -n -C 8 'offsetTOC2LookupFn|pLength|Lookup' "$patch_file"
fi
echo
echo "== inspect any Lookup call sites and contracts in relevant ICU sources/patches =="
rg -n -C 3 '->Lookup\s*\(|Lookup\s*\(' . \
--glob '!*node_modules*' --glob '!*dist*' --glob '!*build*' \
--glob '*.cpp' --glob '*.h' --glob '*.patch' \
| sed -n '1,240p'
echo
echo "== inspect udata.cpp around the TOC lookup path =="
udata_file="$(fd -a 'udata.cpp' . | head -n1)"
if [ -n "${udata_file:-}" ]; then
rg -n -C 6 'bun_icu_maybe_decompress|checkDataItem|doLoadFromCommonData|Lookup' "$udata_file"
fiRepository: oven-sh/WebKit
Length of output: 3238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant source files =="
fd -a 'udata.cpp|udata.h|ucmndata-toc.patch' .
echo
echo "== udata.h contract around lookup APIs =="
udata_h="$(fd -a 'udata.h' . | head -n1)"
if [ -n "${udata_h:-}" ]; then
rg -n -C 4 'Lookup|pLength|null|length' "$udata_h"
fi
echo
echo "== udata.cpp lookup flow and callers =="
udata_cpp="$(fd -a 'udata.cpp' . | head -n1)"
if [ -n "${udata_cpp:-}" ]; then
rg -n -C 4 'Lookup|pLength|commonDataFuncs|vFuncs|checkDataItem|doLoadFromCommonData' "$udata_cpp"
fi
echo
echo "== patch references to offsetTOC2LookupFn and call sites =="
patch_file="$(fd -a 'ucmndata-toc.patch' . | head -n1)"
if [ -n "${patch_file:-}" ]; then
rg -n -C 4 'offsetTOC2LookupFn|offsetTOCLookupFn|pLength|Lookup' "$patch_file"
fi
echo
echo "== any explicit NULL/ nullptr pLength passing in ICU-related code =="
rg -n -C 3 'nullptr\s*\)|NULL\s*\)|pLength\s*=\s*nullptr|pLength\s*==\s*nullptr' . \
--glob '*.cpp' --glob '*.h' --glob '*.patch' \
| sed -n '1,240p'Repository: oven-sh/WebKit
Length of output: 379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== list ICU files of interest =="
git ls-files 'Source/WTF/icu/**' 'icu/**' | sed -n '1,200p'
echo
echo "== inspect udata.h for lookup-related declarations =="
hdr="$(git ls-files 'Source/WTF/icu/unicode/udata.h' 'icu/unicode/udata.h' | head -n1)"
if [ -n "${hdr:-}" ]; then
cat -n "$hdr" | sed -n '1,260p'
fi
echo
echo "== inspect the TOC2 patch around lookup and any pLength guards =="
patch_file="$(git ls-files 'icu/ucmndata-toc.patch' | head -n1)"
if [ -n "${patch_file:-}" ]; then
cat -n "$patch_file" | sed -n '140,230p'
fi
echo
echo "== search for lookup function types and call sites in ICU sources =="
rg -n -C 4 'commonDataFuncs|LookupFn|Lookup\(|pLength' icu Source/WTF/icu \
--glob '*.cpp' --glob '*.h' --glob '*.patch' | sed -n '1,240p'Repository: oven-sh/WebKit
Length of output: 38882
🏁 Script executed:
#!/bin/bash
set -euo pipefail
patch_file="$(git ls-files 'icu/ucmndata-toc.patch' | head -n1)"
echo "== function pointer typedefs and commonDataFuncs in the patch =="
if [ -n "${patch_file:-}" ]; then
rg -n -C 5 'typedef.*Lookup|commonDataFuncs|LookupFn|offsetTOC2LookupFn|offsetTOCLookupFn' "$patch_file"
fi
echo
echo "== any nullability docs for pLength in ICU headers =="
rg -n -C 3 'pLength.*NULL|NULL.*pLength|may be NULL|can be NULL|Ignored if NULL|pointer may be <code>NULL</code>' \
Source/WTF/icu/unicode icu \
--glob '*.h' --glob '*.patch' | sed -n '1,240p'
echo
echo "== search for lookup callback invocations/struct wiring in patch =="
if [ -n "${patch_file:-}" ]; then
rg -n -C 4 'vFuncs|commonDataFuncs|Lookup\)|LookupFn|pLength' "$patch_file"
fiRepository: oven-sh/WebKit
Length of output: 23510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
patch_file="$(git ls-files 'icu/ucmndata-toc.patch' | head -n1)"
echo "== existing offsetTOCLookupFn body in the patch =="
if [ -n "${patch_file:-}" ]; then
rg -n -C 20 'offsetTOCLookupFn' "$patch_file" | sed -n '1,220p'
fi
echo
echo "== search for pLength guards in the same file =="
if [ -n "${patch_file:-}" ]; then
rg -n -C 3 'if\s*\(\s*pLength\s*!=\s*nullptr\s*\)|if\s*\(\s*pLength\s*\)' "$patch_file"
fi
echo
echo "== search for lookup callbacks in ICU headers to infer nullability patterns =="
rg -n -C 3 'typedef.*\*.*Lookup|pLength.*NULL|can be NULL|may be NULL' \
Source/WTF/icu/unicode icu \
--glob '*.h' --glob '*.patch' | sed -n '1,240p'Repository: oven-sh/WebKit
Length of output: 2097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate any source definition of offsetTOCLookupFn or related ICU lookup callbacks =="
rg -n -C 6 'offsetTOCLookupFn|pointerTOCLookupFn|commonDataFuncs|LookupFn|Lookup\(' \
. --glob '*.cpp' --glob '*.h' --glob '*.patch' | sed -n '1,260p'
echo
echo "== inspect any ICU source file that defines commonDataFuncs or data lookup dispatch =="
for f in $(git ls-files 'icu/**' 'Source/WTF/icu/**' | grep -E 'ucmndata|udata'); do
rg -n -C 4 'commonDataFuncs|offsetTOC|pointerTOC|LookupFn|pLength' "$f" && echo "----- $f -----"
doneRepository: oven-sh/WebKit
Length of output: 28282
🌐 Web query:
ICU ucmndata.cpp offsetTOCLookupFn pLength null commonDataFuncs Lookup signature
💡 Result:
In the International Components for Unicode (ICU) library, the function offsetTOCLookupFn is a static helper defined in ucmndata.cpp that implements a lookup mechanism for common data files formatted with an offset-based Table of Contents (TOC) [1][2][3]. The function signature is defined as [1][2]: static const DataHeader * offsetTOCLookupFn(const UDataMemory *pData, const char *tocEntryName, int32_t *pLength, UErrorCode *pErrorCode); This function is part of the commonDataFuncs structure, which acts as a virtual function dispatch table for ICU common data handling [1][4]. In this structure, the Lookup member follows the LookupFn function pointer type [5][4]: typedef const DataHeader ( LookupFn)(const UDataMemory *pData, const char *tocEntryName, int32_t *pLength, UErrorCode *pErrorCode); Regarding the parameters mentioned: - pLength: This is a pointer to an int32_t that the function populates with the length of the found entry [2]. If an entry is found, it calculates the length by subtracting the current entry's data offset from the next entry's data offset [2]. If the entry is the last in the table, it may be set to -1 [2]. - null handling: The function checks if the internal TOC structure is null (e.g., if(toc!=NULL)) [2]. If the TOC is not available, it typically returns the header associated with the UDataMemory directly [2]. If the binary search fails to find the entry name, the function returns NULL [2]. The commonDataFuncs struct essentially pairs the Lookup function with a function to count entries (NumEntriesFn) to allow the library to abstract the difference between different common data formats (offset-based versus pointer-based TOCs) [5][4].
Citations:
- 1: https://fossies.org/dox/icu-release-78.3/ucmndata_8cpp_source.html
- 2: http://svn.code.sf.net/p/firebird/code/firebird/branches/B2_5_Release/extern/icu/source/common/ucmndata.c
- 3: https://fossies.org/dox/wine-11.10/ucmndata_8cpp_source.html
- 4: http://svn.code.sf.net/p/firebird/code/firebird/branches/B2_5_Release/extern/icu/source/common/ucmndata.h
- 5: https://fossies.org/dox/icu-release-78.3/ucmndata_8h_source.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
patch_file="$(git ls-files 'icu/ucmndata-toc.patch' | head -n1)"
echo "== lookup callback typedefs and dispatch =="
if [ -n "${patch_file:-}" ]; then
rg -n -C 6 'typedef.*LookupFn|LookupFn|commonDataFuncs|vFuncs->Lookup|->Lookup\(' "$patch_file"
fi
echo
echo "== broader lookup/pLength references in the patch =="
if [ -n "${patch_file:-}" ]; then
rg -n -C 4 '\bpLength\b|Lookup' "$patch_file" | sed -n '1,260p'
fiRepository: oven-sh/WebKit
Length of output: 2948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
patch_file="$(git ls-files 'icu/ucmndata-toc.patch' | head -n1)"
echo "== wider context around offsetTOCLookupFn and offsetTOC2LookupFn =="
if [ -n "${patch_file:-}" ]; then
sed -n '80,220p' "$patch_file"
fi
echo
echo "== search for any pLength null-check in the TOC lookup code =="
if [ -n "${patch_file:-}" ]; then
rg -n -C 3 'pLength|offsetTOC1|offsetTOCLookupFn|offsetTOC2LookupFn' "$patch_file"
fiRepository: oven-sh/WebKit
Length of output: 5989
🌐 Web query:
ICU ucmndata.cpp offsetTOCLookupFn source pLength guard if (pLength != nullptr)
💡 Result:
The function offsetTOCLookupFn is located in the ICU source file ucmndata.cpp (or ucmndata.c in some older versions) [1][2]. It is used to look up data within a common data file that uses an offset-based Table of Contents (TOC) [2]. The parameter pLength is an int32_t pointer that the function uses to return the size of the found data entry [2]. The check if (pLength!= nullptr) is a safety guard used to ensure that the function only attempts to dereference or write to pLength if a valid pointer was provided by the caller [2]. If the caller passes nullptr for pLength, this check prevents a null pointer dereference, allowing the function to proceed with the lookup without attempting to report the length of the found entry [2]. In the context of ICU's common data lookups, this pattern is standard practice for optional output parameters [2][3]. When a TOC entry is successfully located, the function calculates the length—typically by subtracting the offset of the current entry from the offset of the subsequent entry—and assigns this value to *pLength if pLength is not null [2].
Citations:
- 1: https://fossies.org/dox/icu-release-78.3/ucmndata_8cpp_source.html
- 2: http://svn.code.sf.net/p/firebird/code/firebird/branches/B2_5_Release/extern/icu/source/common/ucmndata.c
- 3: https://fossies.org/dox/wine-11.11/udatpg_8cpp_source.html
🌐 Web query:
ICU commonDataFuncs LookupFn pLength nullptr optional output parameter ucmndata source
💡 Result:
In ICU, the commonDataFuncs struct defines a dispatch interface for managing common data files, containing a Lookup function pointer defined as follows [1][2]: typedef const DataHeader* (LookupFn)(const UDataMemory pData, const char* tocEntryName, int32_t* pLength, UErrorCode* pErrorCode); The pLength parameter is an optional output parameter used to return the length of the data item being looked up [3][4]. Within the ICU implementation (such as in udata.cpp), it is common to pass a pointer to a local variable (e.g., &length) to receive this size information when invoking the function via pData->vFuncs->Lookup [5]. The lookup functions themselves (like offsetTOCLookupFn and pointerTOCLookupFn) are designed to handle this pointer; callers typically provide a valid address to receive the length [6][7]. While nullptr could technically be passed if the caller does not require the size, the internal design assumes a valid address for receiving this output [3][6]. These functions are part of ICU's internal implementation and are not intended for direct use by external applications [1].
Citations:
- 1: http://svn.code.sf.net/p/firebird/code/firebird/branches/B2_5_Release/extern/icu/source/common/ucmndata.h
- 2: https://fossies.org/dox/wine-11.11/structcommonDataFuncs.html
- 3: https://fossies.org/dox/icu-release-78.3/ucmndata_8h_source.html
- 4: https://fossies.org/dox/icu-release-78.3/ucmndata_8h.html
- 5: https://fossies.org/dox/icu-release-78.3/udata_8cpp_source.html
- 6: https://fossies.org/dox/icu-release-78.3/ucmndata_8cpp_source.html
- 7: http://svn.code.sf.net/p/firebird/code/firebird/branches/B2_5_Release/extern/icu/source/common/ucmndata.c
Guard pLength before writing it in offsetTOC2LookupFn. LookupFn treats pLength as optional, and the TOC1 path already checks for nullptr; this branch should do the same before *pLength = ....
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@icu/ucmndata-toc.patch` around lines 210 - 215, The offsetTOC2LookupFn branch
writes through pLength without checking whether it is null, unlike the TOC1 path
where LookupFn treats it as optional. Update the offsetTOC2LookupFn logic around
the dataOffsets lookup so it only assigns to pLength when it is non-null, while
keeping the existing return behavior for the DataHeader unchanged.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // u32[12] header: count, treeCount, nameCount, bucketCount, maxNameLength, | ||
| // dataOffsetsOff, nameIdsOff, treesOff, bucketDirOff, 0,0,0 |
There was a problem hiding this comment.
🟡 Nit: this layout comment says the 12-word header ends in bucketDirOff, 0,0,0, but the writer actually emits [..., bucketDirOff, V2_BLOCK, 0, 0] and the C reader's UDataOffsetTOC2 struct has uint32_t nameBlockSize; uint32_t reserved[2]. Should read ..., bucketDirOff, nameBlockSize, 0, 0. No runtime impact — just keeping the authoritative layout spec accurate.
Extended reasoning...
This is a documentation-only drift in the block comment that specifies the compact-TOC v2 binary layout. The comment reads:
// u32[12] header: count, treeCount, nameCount, bucketCount, maxNameLength,
// dataOffsetsOff, nameIdsOff, treesOff, bucketDirOff, 0,0,0
i.e. nine named fields followed by three reserved zeros. But that is not what the code writes or reads.
Step-by-step proof:
-
In
writePackageV2, the header words are emitted with:for (const v of [count, dirs.length, names.length, bucketCount, maxNameLength, dataOffsetsOff, nameIdsOff, treesOff, bucketDirOff, V2_BLOCK, 0, 0]) { ... }
The 10th word is
V2_BLOCK(= 16), not0. -
In
icu/ucmndata-toc.patch, the C reader'sUDataOffsetTOC2struct declares the 10th word asuint32_t nameBlockSize; /* names per front-coded block (the writer uses 16) */followed byuint32_t reserved[2];— nine named fields plusnameBlockSizeplus two reserved, not nine plus three reserved. -
In
verifyPackageV2, the destructuring reads offset 36 asnameBlockSizeand assertsif (nameBlockSize !== V2_BLOCK) die(...)— so the field is load-bearing at build time, further confirming it is not a reserved zero.
So three independent pieces of code agree on nameBlockSize being the 10th header word; only the layout comment disagrees.
Impact: None at runtime — the code is correct, only the comment is stale. However, this comment is explicitly positioned as the authoritative binary-layout description ("Layout after the ... DataHeader"), and it is the natural place a future maintainer would look when touching the writer, the C reader, or the verifier. A stale spec here is exactly the kind of thing that causes confusion later.
Fix: One-word edit — change bucketDirOff, 0,0,0 to bucketDirOff, nameBlockSize, 0, 0 on the second line of that comment.
The named-removal step requires every listed item to exist in the input package, so an ICU rename cannot silently re-ship one. Dockerfile.windows builds ICU 73.2, and nfkc_scf.nrm (the NFKC simple-casefold normalizer) only exists from ICU 74, so that guard would have failed the Windows image build. Entries can now carry the ICU major that introduced them (`74:nfkc_scf.nrm`) and every Dockerfile filters the list against the major it builds. Verified against the real icudt73l.dat item list.
A tree's pool.res holds the value strings shared by every bundle of that tree. The six pools ship uncompressed (they sit on the locale lookup path, so compressing them would cost a per-process decode) and half their bytes are prefixes shared with the lexicographic neighbor -- redundancy zstd never sees. libicudata.a: 10,433,790 -> 10,014,274 B. icu/frontcode.ts rebuilds each pool's 16-bit area as a verbatim region (the traced hot set, anything a member must address through a small poolStringIndex16Limit, and ICU's no-inheritance marker) plus a front-coded region (blocks of 16, WTF-8 tails), renumbers every pool reference in every member bundle, and proves at build time that each member's resolved view is unchanged and that every front-coded string round-trips through a reference decoder exactly as strict as the C one. icu/uresdata-frontcode.patch adds the reader: one branch in the only two functions that resolve pool string offsets. Verbatim offsets take exactly today's path. Higher offsets are ids, decoded one 16-string block at a time, on first access, into a side arena the data archive itself carries as zero-cost .bss symbols together with their sizes (a package that claims a slice the archive did not allocate, or whose directories are not monotone and contained, is rejected at pool open). Materialized strings use the stock string-v2 encoding, so every downstream consumer is unchanged, and they live for the whole process like the mapped strings they replace. Without the archive (ICU tools) the tables are allocated once per pool. The cost model is enforced rather than asserted: a traced Bun startup reads no pool string at all; the first use of every common Intl API reads only the 429 strings pinned verbatim in icu/pin-strings.txt (~6 KB); and icu/test-package.cpp fails the image build if that phase materializes a single block, separately asserts the front-coded path was exercised, and byte-compares three outputs that flow through it. icu/trace-pool-strings.patch (development only) regenerates the pin list.
Jarred-Sumner
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@icu/uresdata-frontcode.patch`:
- Around line 210-227: The frontcode setup in the ICU archive handling currently
falls back to heap allocation when `bun_icu_fc_slots` or `bun_icu_fc_arena` are
present but `slotBase` or `arenaBase` is out of range, which hides malformed
package data. Update the `fc` initialization logic to treat invalid slice bounds
as an error instead of entering the allocation branch, and return failure
through the existing `errorCode` path. Keep the valid in-archive fast path in
place, but reject any out-of-range `slotBase`/`arenaBase` cases before
`fc->slots` and `fc->arena` are assigned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3644419-335c-4f95-a30a-3a4eac7e09d8
📒 Files selected for processing (13)
DockerfileDockerfile.androidDockerfile.freebsdDockerfile.muslDockerfile.windowsicu/compress-data.tsicu/frontcode.tsicu/pin-strings.txticu/remove-items.txticu/resbundle.tsicu/test-package.cppicu/trace-pool-strings.patchicu/uresdata-frontcode.patch
| + if(bun_icu_fc_slots!=nullptr && bun_icu_fc_arena!=nullptr && | ||
| + &bun_icu_fc_slot_count!=nullptr && &bun_icu_fc_arena_unit_count!=nullptr && | ||
| + slotBase<=bun_icu_fc_slot_count && (uint32_t)dirCount<=bun_icu_fc_slot_count-slotBase && | ||
| + arenaBase<=bun_icu_fc_arena_unit_count && (uint32_t)arenaUnits<=bun_icu_fc_arena_unit_count-arenaBase) { | ||
| + fc->slots=bun_icu_fc_slots+slotBase; | ||
| + fc->arena=bun_icu_fc_arena+arenaBase; | ||
| + } else { | ||
| + fc->slots=(char16_t **)uprv_malloc((size_t)dirCount*sizeof(char16_t *)); | ||
| + fc->arena=(char16_t *)uprv_malloc((size_t)arenaUnits*2); | ||
| + if(fc->slots==nullptr || fc->arena==nullptr) { | ||
| + uprv_free(fc->slots); | ||
| + uprv_free(fc->arena); | ||
| + uprv_free(fc); | ||
| + *errorCode=U_MEMORY_ALLOCATION_ERROR; | ||
| + return nullptr; | ||
| + } | ||
| + uprv_memset(fc->slots, 0, (size_t)dirCount*sizeof(char16_t *)); | ||
| + } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files of interest:\n'
git ls-files 'icu/uresdata-frontcode.patch' 'icu/frontcode.ts' | sed 's#^`#-` #'
printf '\nLocate the relevant code paths:\n'
rg -n "bun_icu_fc_slots|bun_icu_fc_arena|U_INVALID_FORMAT_ERROR|U_MEMORY_ALLOCATION_ERROR|slotBase|arenaBase|archive|frontcode" icu/uresdata-frontcode.patch icu/frontcode.ts
printf '\nRead the patch around the cited lines:\n'
sed -n '150,260p' icu/uresdata-frontcode.patch
printf '\nRead the TypeScript source around matching logic/comments:\n'
sed -n '1,220p' icu/frontcode.tsRepository: oven-sh/WebKit
Length of output: 17794
Reject invalid archive slices instead of falling back to heap allocation. When bun_icu_fc_slots/bun_icu_fc_arena are present but slotBase or arenaBase is out of range, this path allocates fresh tables and continues. That masks a malformed package that the header says should be rejected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@icu/uresdata-frontcode.patch` around lines 210 - 227, The frontcode setup in
the ICU archive handling currently falls back to heap allocation when
`bun_icu_fc_slots` or `bun_icu_fc_arena` are present but `slotBase` or
`arenaBase` is out of range, which hides malformed package data. Update the `fc`
initialization logic to treat invalid slice bounds as an error instead of
entering the allocation branch, and return failure through the existing
`errorCode` path. Keep the valid in-archive fast path in place, but reject any
out-of-range `slotBase`/`arenaBase` cases before `fc->slots` and `fc->arena` are
assigned.
| + fflush(gF); | ||
| +} | ||
| +static void bunTraceFCBlock(int32_t b) { | ||
| + static FILE *gF2 = (FILE *)(intptr_t)-1; | ||
| + if (gF2 == (FILE *)(intptr_t)-1) { | ||
| + const char *path = getenv("BUN_ICU_TRACE_FC"); | ||
| + gF2 = path ? fopen(path, "a") : nullptr; | ||
| + } | ||
| + if (!gF2) return; | ||
| + fprintf(gF2, "block %d\n", (int)b); |
There was a problem hiding this comment.
🟡 Nit: bunTraceFCBlock is defined and its BUN_ICU_TRACE_FC=<file> env var documented in the header, but the patch has no hunk that inserts a call to it — only bunTracePoolString is wired in. So the FC-block trace does nothing and the unused static will emit -Wunused-function. Either add a call after ++bun_icu_fc_materialized_blocks; in bunFCMaterialize, or drop the function and its doc line. (Dev-only patch, never applied by the Dockerfiles.)
Extended reasoning...
What the bug is
icu/trace-pool-strings.patch is a development-only instrumentation patch (its first line is DEVELOPMENT ONLY -- never applied by the Dockerfiles) that a maintainer applies by hand when regenerating icu/pin-strings.txt. Its header documents two env-var hooks:
BUN_ICU_TRACE_POOL=<file> append "<poolPtr>\t<offset>\t<utf8 text>" for
every pool string read through the verbatim path
BUN_ICU_TRACE_FC=<file> append a line per materialized block
The first hunk defines two static functions to implement those: bunTracePoolString (reads BUN_ICU_TRACE_POOL) and bunTraceFCBlock (reads BUN_ICU_TRACE_FC). But the patch's only call-site hunk (@@ -601,6 +639,7 @@) inserts a single call to bunTracePoolString in the verbatim-path branch of res_getStringNoTrace. There is no hunk that inserts a call to bunTraceFCBlock anywhere.
Step-by-step proof
- The patch file is 71 lines long and contains exactly two
@@hunks againstsource/common/uresdata.cpp. - Hunk 1 (
@@ -104,6 +104,44 @@) adds 38 lines: two#includes, the definition ofbunTracePoolString(const void*, int32_t, const char16_t*), and the definition ofbunTraceFCBlock(int32_t b), bothstatic. - Hunk 2 (
@@ -601,6 +639,7 @@) adds one line:bunTracePoolString(pResData->poolBundleStrings, (int32_t)offset, p);inside the verbatim pool-string branch. grep bunTraceFCBlock icu/trace-pool-strings.patchmatches exactly once — the definition. There is no call.- The natural call site would be in
bunFCMaterialize(defined byicu/uresdata-frontcode.patch, on top of which this trace patch applies), right after++bun_icu_fc_materialized_blocks;. No hunk targets that function.
Therefore bunTraceFCBlock is a defined-but-unreferenced static function, and setting BUN_ICU_TRACE_FC has no observable effect.
Why existing code doesn't prevent it
Nothing in the build applies or compiles this patch — it is explicitly excluded from every Dockerfile — so no CI step catches it. And the patch's own recipe ("link Bun against … a package whose pools are NOT front-coded (--pins none produces one)") uses only BUN_ICU_TRACE_POOL; against a non-front-coded package bunFCMaterialize never runs anyway, so the primary regeneration workflow is unaffected and would not have surfaced this.
Impact
Very low. The shipped artifact is untouched: none of the Dockerfiles apply this patch, and bun_icu_fc_materialized_blocks (the counter test-package.cpp gates on) lives in the always-applied uresdata-frontcode.patch, not here. The two consequences are (a) the documented BUN_ICU_TRACE_FC env var is dead — a maintainer following the header will get an empty file — and (b) compiling with this patch applied emits -Wunused-function for bunTraceFCBlock. ICU's stock ./configure build does not default to -Werror, so this is a warning rather than a build break in the documented flow.
How to fix
Either wire the call in — add a third hunk that inserts bunTraceFCBlock(b); immediately after ++bun_icu_fc_materialized_blocks; inside bunFCMaterialize — or, since the documented recipe never runs against a front-coded package anyway, delete bunTraceFCBlock and the BUN_ICU_TRACE_FC=<file> line from the header.
…step
Two build failures in the in-image package gate:
- zstd >= 1.5.6's zstd.h unconditionally includes zstd_errors.h, so copying
zstd.h alone breaks every compile against /usr/local/include (the gate,
test-package.cpp). Copy both headers on linux, musl, and windows.
- Alpine's base image has no /usr/local/{bin,lib,include} and busybox cp
does not create the destination directory; mkdir them first on musl.
Verified by building the ICU stage of Dockerfile and Dockerfile.musl
locally: both complete with every in-image proof and gate passing.
The pinned verbatim hot set is CLDR text, so it is specific to an ICU major: Dockerfile.windows builds ICU 73, whose date/number patterns differ textually from the ICU-75-traced list, and test-package's startup-phase materialization gate correctly refused to ship that (19 blocks would have materialized during the first Intl formatter construction on Windows). compress-data.ts now selects icu/pin-strings-<major>.txt from the package it is given; the ICU 73 list is traced from the gate's own startup phase (new BUN_ICU_TEST_STARTUP_ONLY hook, documented in icu/trace-pool-strings.patch) against a patched ICU 73. Verified with an ICU-73-built gate binary on the repacked icudt73l.dat: 0 startup-phase materializations, the wide sweep still exercises the front-coded path, and the byte-exact output assertions hold on CLDR 43.
| if (bare.startsWith("brkitr/") && bare.endsWith(".res") && d.boundaries) { | ||
| for (const k of Object.keys(d.boundaries)) if (isLineOrTitle(k)) delete d.boundaries[k]; | ||
| if (Object.keys(d.boundaries).length === 0) delete d.boundaries; | ||
| if (Array.isArray(d["%%DEPENDENCY"])) { | ||
| const kept = d["%%DEPENDENCY"].filter((x: any) => !isLineOrTitle(String(x?.s ?? "").replace(/\.brk$/, ""))); | ||
| if (kept.length === 0) delete d["%%DEPENDENCY"]; | ||
| else d["%%DEPENDENCY"] = kept; | ||
| } |
There was a problem hiding this comment.
🟡 Nit: rewriteItem() handles %%DEPENDENCY for any brkitr .res as a sibling of if (bnd !== null) (line 97), but expectedDump() gates the same edit on && d.boundaries (line 202) — so the two "independent statements of the edits" aren't structurally in lockstep as the header comment claims. Unreachable on real ICU data (genrb only emits those %%DEPENDENCY entries from boundaries values, so they co-occur by construction), but moving the %%DEPENDENCY block out from under && d.boundaries makes the lockstep invariant hold structurally rather than incidentally.
Extended reasoning...
What the divergence is
expectedDump() is documented (lines 184–190) as "deliberately a second, independent statement of the edits" that rewriteItem() performs; compress-data.ts compares them and throws "rewrite verification failed" if they disagree. For the brkitr edits the two functions are gated differently:
rewriteItem() (lines 77–113):
if (bare.startsWith("brkitr/") && bare.endsWith(".res")) {
const bnd = b.find(["boundaries"]);
if (bnd !== null) { /* delete boundaries/line*, title */ }
// sibling of the null-check — runs regardless of whether boundaries exists
const depRes = b.find(["%%DEPENDENCY"]);
if (depRes !== null) { /* delete line*.brk / title.brk entries */ }
}expectedDump() (lines 202–210):
if (bare.startsWith("brkitr/") && bare.endsWith(".res") && d.boundaries) {
/* delete boundaries/line*, title */
if (Array.isArray(d["%%DEPENDENCY"])) { /* delete line*.brk / title.brk entries */ }
}So rewriteItem() will edit %%DEPENDENCY even when boundaries is absent, while expectedDump() will not.
Step-by-step proof of the divergence
Consider a hypothetical brkitr/xx.res whose resolved dump is { "%%DEPENDENCY": [{s: "line.brk"}] } with no boundaries key:
rewriteItem():bnd = b.find(["boundaries"])→null, so the boundaries block is skipped.depRes = b.find(["%%DEPENDENCY"])→ non-null;isLineOrTitle("line")→ true; the array has one element, all dead, sodeleteTableKeys([], ["%%DEPENDENCY"])runs andnotes.push("-%%DEPENDENCY"). The rewritten bundle's resolved dump is{}.expectedDump():d.boundariesisundefined, so the whole brkitr block is skipped. Returns{ "%%DEPENDENCY": [{s: "line.brk"}] }unchanged.compress-data.tscomparesJSON.stringify({ "%%DEPENDENCY": [...] })againstJSON.stringify({})→ mismatch → throwsrewrite verification failed (-%%DEPENDENCY)→ build fails.
Why this cannot fire on the actual input
The only input this pipeline consumes is a stock icupkg-extracted ICU package, whose brkitr bundles are produced by genrb. genrb auto-generates %%DEPENDENCY from :process(dependency){"file"} items in the source .txt, and the only place a brkitr source references line*.brk/title.brk via that mechanism is inside the boundaries{} table. So a brkitr bundle with a line*/title entry in %%DEPENDENCY but no boundaries table cannot be produced by genrb — the two co-occur by construction on ICU 73 and 75, and structurally on any genrb output. Bundles that do have %%DEPENDENCY without boundaries (e.g. brkitr/th.res, dictionaries only) contain no line*/title entries, so rewriteItem()'s filter finds nothing to delete and both functions leave the dump unchanged. And if a future ICU renamed boundaries, assertExpectedRewrites (["brkitr/root.res", "-boundaries/line*"]) would fail the build first with a clearer message.
This is the substance of the counter-argument, and it is correct: there is no reachable failure on any input this code sees, and the failure mode if it were ever reached is a loud build error — the proof mechanism doing its job — not silent corruption.
Why it's still worth the one-line tidy
The header comment above expectedDump() explicitly claims the two functions are independent restatements of the same edits. That claim is what gives the want !== got check its force as a proof. Right now the claim holds only incidentally (because genrb's output shape happens to make the extra guard vacuous), not structurally — a future reader auditing the two functions against each other will trip over the asymmetry and have to convince themselves it's harmless. Moving the if (Array.isArray(d["%%DEPENDENCY"])) block out from under && d.boundaries (or, equivalently, gating rewriteItem()'s depRes block on bnd !== null) makes the lockstep hold by construction. It's a one-line, zero-risk change.
Fix
if (bare.startsWith("brkitr/") && bare.endsWith(".res")) {
if (d.boundaries) {
for (const k of Object.keys(d.boundaries)) if (isLineOrTitle(k)) delete d.boundaries[k];
if (Object.keys(d.boundaries).length === 0) delete d.boundaries;
}
if (Array.isArray(d["%%DEPENDENCY"])) {
const kept = d["%%DEPENDENCY"].filter((x: any) => !isLineOrTitle(String(x?.s ?? "").replace(/\.brk$/, "")));
if (kept.length === 0) delete d["%%DEPENDENCY"];
else d["%%DEPENDENCY"] = kept;
}
}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
icu/uresdata-frontcode.patch (1)
151-188: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap
arenaUnitsbefore allocating the fallback arena. The per-block write path is already bounded tightly enough thatentryOffwon’t wrap, but a crafted archive-absent bundle can still forceuprv_malloc((size_t)arenaUnits*2)into a huge allocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@icu/uresdata-frontcode.patch` around lines 151 - 188, The fallback arena allocation path in the frontcode loader still trusts arenaUnits too much, allowing a crafted archive-absent bundle to request an oversized malloc. Tighten the validation in the same loader function that reads stringCount, dirCount, and arenaUnits by capping arenaUnits to a reasonable upper bound before any allocation, and keep the existing block-slice checks so the per-block write path remains safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@icu/uresdata-frontcode.patch`:
- Around line 151-188: The fallback arena allocation path in the frontcode
loader still trusts arenaUnits too much, allowing a crafted archive-absent
bundle to request an oversized malloc. Tighten the validation in the same loader
function that reads stringCount, dirCount, and arenaUnits by capping arenaUnits
to a reasonable upper bound before any allocation, and keep the existing
block-slice checks so the per-block write path remains safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3fefe4ec-95fb-4028-b490-c11425e82e77
📒 Files selected for processing (7)
icu/compress-data.tsicu/frontcode.tsicu/pin-strings-73.txticu/pin-strings-75.txticu/test-package.cppicu/trace-pool-strings.patchicu/uresdata-frontcode.patch
💤 Files with no reviewable changes (1)
- icu/pin-strings-75.txt
Preview Builds
|
1 participant
What
Shrinks the ICU data package embedded in every Bun binary by 1,564,148 bytes (−13.5%) on linux/musl/windows —
libicudata.a11,578,422 → 10,014,274 bytes — with no behavior change and no runtime cost of any kind (nothing is compressed that wasn't already, no read path gets slower, nothing new is decoded at startup or first use).Four independent changes, one per commit:
libicudata.akeep-raw.txtglob so the two zone-tree bundles JSC's eager time-zone warm-up opens stop being zstd frames.resbundle rewriter: within-bundle value sharing, dead subtrees, brkitrline*/titlerules, collation rule-source stubs%%DEPENDENCYcleanup, the Windows artifact gate74:nfkc_scf.nrm): ICU 73 (Windows) does not have every ICU 75 item.resformatVersion 4): reader patch + packer + traced verbatim pin set* alignment padding retained in the collation bundles (below) plus zstd dictionary-training noise; the −131,112/−83,968/−930,816 figures were each measured at their commit.
Why these are safe
Item removals (1). Every removed item is unreachable from any ICU call site JSC, WTF, or Bun can reach;
icu/remove-items.txtdocuments the loader + caller proof per item. Two near-misses were explicitly kept (metadata.resis reached via CLDR unit-alias resolution; the root andcollres_index.resare the two a JS engine enumerates). The Dockerfiles assert every listed name still exists in the upstream.dat, so an ICU upgrade can't silently strand or re-ship one.Compact TOC (2). The stock TOC spends 115 KB storing
icudt75l/tree/name.extstrings plus 8 bytes per item; 81% of it is the repeated prefixes and the 7 copies of every locale basename recurring across the locale trees. The v2 TOC (30,976 B) factors names into a directory table + one global 16-way front-coded pool of unique basenames, and adds a sentinel offset so even the last item has an explicit length. Item bodies and theirDataHeaders are untouched.icu/ucmndata-toc.patchadds the reader as a newformatVersion 2arm next to the v1 one; a stock ICU rejects the package withU_INVALID_FORMAT_ERRORinstead of misreading it.compress-data.tsreplays the reader's exact lookup for every item + deliberate misses against the emitted bytes on every build (stockicupkg -lcan no longer read the output, by design). Runtime cost: one lookup perudata_open— ≤10 boundedstrncmps + two binary searches, replacing a binary search over full name strings; ~17 opens in a whole startup.Bundle rewrites (3).
icu/resbundle.tsparses a.res(formatVersion 2/3, pool-aware) into its resource graph, applies edits, and re-serializes with hole compaction and full offset relocation across both unit areas. Its identity rewrite is byte-for-byte identical on all 3,736 bundles in the package. On top of it,icu/rewrite-items.ts:root.resalone has 912 alias resources with 180 distinct targets);supplementalData.res'ssubdivisionContainment+ deadidValidityclasses, andcharacterLabel/personNames/ per-localeparsefrom the locale bundles (no linked reader for any of them);UBRK_LINE/UBRK_TITLEboundary entries and their%%DEPENDENCYreferences from the brkitr bundles, which is what lets the packer drop the tenline*/title.brkrule files (icupkg -rrefuses them because it enforces referential integrity — correctly);coll/root.res'sUCARulesand stubs every tailoring's compiled-awaySequencerule source to one code unit, deliberately not zero: JSC keys its ASCII collation fast path onucol_getRules()being non-empty, so deleting the rules outright would changeIntl.Collatorordering for tailored locales.Every rewrite is proven at build time: the output bundle's resolved resource tree must equal the input's with exactly the intended edits applied (an independently-stated expected-edit model), and a manifest asserts each named edit actually fired — so an ICU upgrade that renames a key fails the image build instead of silently no-oping.
In-image test (4).
icu/test-package.cpplinks the patchedlibicuuc/libicui18nand the just-producedlibicudata.a(with the same weak zstd hook Bun defines) and drives locale enumeration, display names, number/currency/date formatting, collation (incl. thezh/cstailorings and theucol_getRules()predicate), word + sentence segmentation through all four dictionaries, normalization, and the removed items. Nonzero exit fails the image.Hardening (5). Review of the first four commits, fixed in the same PR. The two substantive ones: bundle rewrites were shifting the 32-bit resource area by arbitrary word counts, breaking genrb's 16-byte alignment of
URES_BINARYpayloads (CollationDataReadercasts them toint64_t*; 7 of 17 collation binaries landed misaligned — tolerated by x86-64/arm64, UB everywhere, and invisible to every behavior test) — the serializer now preserves each binary's alignment and asserts it; and the TOC reader copied name-pool strings into a fixed stack buffer with no bound — a corrupt package now fails the lookup instead of overrunning the stack. The rest is guards that turn silent drift into build failures: every named rewrite must actually fire, everykeep-raw.txtglob must still match something (this immediately exposed an entry dead since ICU 64), the Dockerfiles' category filter must match, removed items are probed by the test, and the Windows cross build now runs the package test host-natively against the emitted.dat.Front-coded pool strings (7). A tree's
pool.resholds the value strings shared by every bundle in that tree; the six pools ship raw (they are read on the locale lookup path, so compressing them would cost a per-process decode) and 55% of their bytes are prefixes shared with the lexicographic neighbor — redundancy zstd never sees. The packer (icu/frontcode.ts) rebuilds each pool as a verbatim region plus a front-coded region (blocks of 16, WTF-8 tails), renumbers every pool reference in every member bundle, and proves at build time that every member's resolved view is unchanged and every front-coded string round-trips through a reference decoder. The reader (icu/uresdata-frontcode.patch, ~150 lines) adds one branch to the only two functions that resolve pool string offsets: verbatim offsets take exactly today's path; higher offsets are ids, decoded one 16-string block at a time, on first access, into a side arena carried as zero-cost.bsssymbols insidelibicudata.aitself (materialized strings use the stock string-v2 encoding, so every downstream consumer is unchanged; ICU tools without the archive fall back to oneuprv_mallocper pool).The reader was reviewed adversarially before it shipped: the front-coded directories, varints, and arena slices are validated up front against the region the pool declares and against the archive-declared table sizes, so a corrupt, crafted, or version-skewed package fails the lookup (or the whole pool open) instead of writing anywhere — and the packer refuses to mark a pool formatVersion 4 unless it actually has a front-coded region.
Cost model, enforced rather than asserted: a traced Bun startup reads zero pool strings (JSC's boot never touches one), and the first use of every common Intl API reads 429, which are pinned verbatim (
icu/pin-strings-75.txt, ~6 KB; the hot set is CLDR text, soDockerfile.windows's ICU 73 has its own traced list) —icu/test-package.cppfails the image build if that phase materializes a single block, so the pin list cannot rot; it also asserts the front-coded path was exercised by the wide sweep and byte-compares three outputs that flow through front-coded strings. Cold strings (an app's first use of an uncommon locale) pay a one-time ≤16-string block decode (~1 µs) and are a cached pointer afterwards; the warm path is a pointer load. The side arena costs ~1 MB of address space and no file bytes; only touched pages become resident.Validation beyond the in-build proofs
libicuuc/libicui18n+ the final package: a 64-caseIntl/Segmenter/Collator/normalizecorpus is byte-identical to the unmodified build,test/js/web/intl+test/js/web/encodingpass (2,464 tests), and a traced startup materializes no front-coded blocks.Dockerfile.windowsrecipe — version-filtered removals, category filter, the whole packer with every in-build proof, and the host-native package gate — was replayed locally against the realicudt73l.datend to end. This is what caught thenfkc_scf.nrm/ ICU-73 incompatibility (commit 6) and, later, the need for a per-ICU-major pin list: the startup-phase materialization gate failed on ICU 73 because its CLDR text differs from the ICU-75-traced hot set — i.e. the gate caught exactly the class it exists for, in an image build.build_icustage ofDockerfile,Dockerfile.musl, andDockerfile.windowswas built locally with Docker end to end (packer + all proofs + the gate) before the final push.Not in this PR (deliberately)
.bssmaterialization arena, and the union-schema locale trees (~0.75 MB more): both require new code inres_getStringNoTrace— the hottest function in the locale subsystem — plus lazy materialization. They are a different risk class from the data-only edits here and should be their own PR with a startup-time gate.Bun picks this up on the next
WEBKIT_VERSIONbump after the prebuilts build.