fix: make buffer growth overflow-safe - #3944
Merged
Merged
Conversation
Collaborator
Coverage Report for CI Build 5639Coverage decreased (-0.03%) to 90.434%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
peter-jerry-ye
marked this pull request as ready for review
July 28, 2026 07:11
peter-jerry-ye
force-pushed
the
codex/fix-growth-overflow
branch
from
July 28, 2026 07:59
7e10027 to
af2d7d8
Compare
peter-jerry-ye
enabled auto-merge (rebase)
July 28, 2026 07:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
grow_if_necessarycall inBufferand the non-JSStringBuilderimplementation with an inline capacity fast path and a private slow growth pathrequired >= leninvariant, without imposing a backend-specific maximumWhy
The old doubling loops could overflow before reaching a representable requested size. Depending on the backend, that could loop indefinitely or pass an invalid capacity to allocation. The common append path also paid for a function call whose capacity check could be kept directly at the write site.
Correctness
Append sizes are non-negative, so a computed
requiredsmaller than the current logical length proves that the size calculation wrapped. Those cases enter the slow path and panic before allocation. For a representablerequired, growth doubles while the next capacity remains representable; if doubling wraps, it usesrequiredexactly and leaves backend allocation limits to the runtime.The boundary tests exercise the capacity calculation without attempting huge allocations. No public API changes.
This supersedes #3317, #3822, and #3837.
Performance
Initial native release comparison against
main, before the final fixed-size fast-path refinement:Buffer::write_byte, preallocated, n=1,000,000Buffer::write_uint64_le, preallocated, n=125,000Buffer::write_leb128, preallocated, n=100,000StringBuilder::write_string, n=4,096StringBuilder::write_view, n=4,096StringBuilder::write_stringview, n=4,096StringBuilder::write_charASCII, n=4,096StringBuilder::write_charnon-BMP, n=4,096StringBuilder::resetwith capacity reuse, n=4,096The final revision uses the invariant
0 <= len <= capacityto compare fixed-size appends against remaining capacity without evaluatinglen + Non the common path. Compared with the preceding PR revision, using the same runtime for each pair:StringBuilder::write_charASCII, n=4,096Buffer::write_byte, preallocated, n=65,536Buffer::write_uint64_le, preallocated, n=8,192An earlier JS comparison accidentally mixed runtimes; those numbers were discarded. The generated JavaScript for the
StringBuildercase was byte-identical.The full PR measured about 3.2% higher throughput than
mainin themoonbitlang/asyncHTTP server benchmark. The incremental effect of the final refinement could not be isolated reliably because the unchanged baseline varied from 54k to 86k requests/second under system load, so no additional HTTP percentage is claimed.