Skip to content

fix(stringbuilder): guard capacity growth against Int overflow - #3837

Closed
mizchi wants to merge 2 commits into
moonbitlang:mainfrom
mizchi:fix/stringbuilder-grow-overflow
Closed

mizchi wants to merge 2 commits into
moonbitlang:mainfrom
mizchi:fix/stringbuilder-grow-overflow

Conversation

@mizchi

@mizchi mizchi commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Guard StringBuilder capacity growth against Int overflow.
  • Add allocation-free whitebox coverage for normal growth and overflow boundaries.
  • Add a focused native growth benchmark.

Root cause

StringBuilder::grow_if_necessary repeatedly doubled the backing capacity. Once the capacity exceeded 2^30, Int overflow could make the loop cycle through non-positive values and never reach the requested length. The new private helper falls back to the exact requested capacity after an overflowing double.

This is the StringBuilder counterpart to #3822, but it changes the separate UTF-16 builder implementation.

Benchmark

StringBuilder growth from minimum capacity n=4096, lower is better.

  • Environment: Apple M5 (arm64), macOS Darwin 25.5.0, MoonBit 0.1.20260824
  • Command: moon bench --release --target native -p moonbitlang/core/builtin -f stringbuilder_bench_test.mbt -i 5
  • Three sequential runs: 15.17 µs, 14.56 µs, 14.82 µs
  • Mean across runs: 14.85 µs

Each run contains 10 benchmark samples.

Validation

  • moon info && moon fmt
  • moon test: 7546 passed, 0 failed
  • moon check
  • No generated interface or formatting diff

@mizchi
mizchi marked this pull request as ready for review July 20, 2026 11:20
Copilot AI review requested due to automatic review settings July 20, 2026 11:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the non-JS StringBuilder (UTF-16 buffer-backed implementation) against Int overflow during capacity doubling, preventing potential non-terminating growth loops for very large required sizes.

Changes:

  • Introduces a private stringbuilder_grow_capacity(current, required) helper that detects overflow during doubling and falls back to allocating exactly required.
  • Updates StringBuilder::grow_if_necessary to use the new helper instead of an unguarded doubling loop.
  • Adds allocation-free whitebox tests covering normal doubling behavior and overflow boundary cases, plus a focused benchmark for growth from the minimum capacity.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
builtin/stringbuilder_buffer.mbt Adds overflow-safe capacity growth helper and wires it into grow_if_necessary.
builtin/stringbuilder_grow_wbtest.mbt Adds whitebox tests validating normal doubling and overflow survival without allocating.
builtin/stringbuilder_bench_test.mbt Adds a benchmark targeting growth behavior starting from minimal initial capacity.
builtin/moon.pkg Excludes the new whitebox test from JS targets to match the non-JS implementation selection.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mizchi
mizchi force-pushed the fix/stringbuilder-grow-overflow branch from 545c6ce to 05f4de3 Compare July 27, 2026 09:37
@mizchi
mizchi force-pushed the fix/stringbuilder-grow-overflow branch from 05f4de3 to 2cab5a0 Compare August 24, 2026 15:41
@bobzhang

Copy link
Copy Markdown
Contributor

Thanks for this, and sorry it sat so long.

The overflow guard itself has since landed in b903d49 ("fix: make buffer growth overflow-safe"), which introduced stringbuilder_growth_capacity in builtin/stringbuilder_buffer.mbt with the same let next = space * 2; if next <= space { break required } shape, plus an abort for the case where a caller's len + n wraps. Regression tests came with it in builtin/stringbuilder_buffer_wbtest.mbt. The same commit superseded the buffer counterpart in #3317.

After your 2026-09-01 rebase this PR is down to stringbuilder_grow_wbtest.mbt, which duplicates that existing whitebox coverage, and one benchmark — so I'm closing it as superseded.

The benchmark is worth having on its own, though: every current StringBuilder bench passes a size_hint covering the whole workload, so none of them ever exercises the doubling path. A small PR with just the bench StringBuilder growth from minimum capacity n=4096 hunk would be welcome.

Heads-up that #3822 is in the same position — it still edits buffer/buffer.mbt, which b903d49 already rewrote.

@bobzhang bobzhang closed this Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants