Skip to content

fix(bytesconv): detect integer overflow before multiplication in ParseUintBuf - #1538

Open
GerardGao wants to merge 1 commit into
cloudwego:mainfrom
GerardGao:fix/parseuint-overflow-detection
Open

fix(bytesconv): detect integer overflow before multiplication in ParseUintBuf#1538
GerardGao wants to merge 1 commit into
cloudwego:mainfrom
GerardGao:fix/parseuint-overflow-detection

Conversation

@GerardGao

Copy link
Copy Markdown

What this PR does

ParseUintBuf checks for overflow after the multiplication:

vNew := 10*v + int(k)
// Test for overflow.
if vNew < v {
    return -1, i, errTooLongInt
}

When 10*v itself overflows, the wrapped result can still be greater than v,
so the check silently passes and the function returns a wrapped value instead of
errTooLongInt.

This PR moves the check before the multiplication.

Reproduce

ParseUint accepts out-of-range input and returns a wrapped value:

input before after
21000000000000000000 2553255926290448384, err == nil errTooLongInt
25000000000000000000 6553255926290448384, err == nil errTooLongInt

Scanning all 90 twenty-digit inputs of the form dd000000000000000000, 18 of
them are accepted although they exceed math.MaxInt.

Note the inconsistency: 18446744073709551616 is correctly rejected today,
while the numerically smaller 21000000000000000000 is accepted.

Impact

ParseUintBuf backs protocol.ParseContentLength, so the value comes from an
untrusted request header. Parsing a real request through ReadHeader:

Content-Length: 25000000000000000000  =>  ContentLength() = 6553255926290448384, err = nil
Content-Length: 18446744073709551616  =>  rejected ("too long int")

ParseUint is also used for the Range header (pkg/app/fs.go) and the cookie
max-age attribute (pkg/protocol/cookie.go).

Notes

  • Uses math.MaxInt so the bound follows the platform word size; verified
    GOARCH=386 and GOARCH=arm still build.
  • Reuses the existing errTooLongInt; no new error value and no signature change.
  • The boundary value 9223372036854775807 is still accepted, covered by the
    existing TestParseUint.

Verification

  • Added the missed inputs to the existing TestParseUintError; they fail before
    this change and pass after it.
  • go test ./... passes.
  • gofmt -l and go vet are clean.

I could not open an issue first as suggested by CONTRIBUTING.md, because issue
creation is restricted in this repository, so the full reproduction is included
above. Happy to move this to an issue or another channel if you prefer.

@CLAassistant

CLAassistant commented Aug 24, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

…eUintBuf

ParseUintBuf checked for overflow after computing 10*v + k. When 10*v
itself overflows, the wrapped result can still be greater than v, so the
check silently passed and the function returned a wrapped value instead
of errTooLongInt.

Move the check before the multiplication so out-of-range input is
rejected consistently. Reuses the existing errTooLongInt and keeps the
function signature unchanged.

Add the previously missed inputs to TestParseUintError.
@GerardGao
GerardGao force-pushed the fix/parseuint-overflow-detection branch from c7e55c0 to a60cc56 Compare August 25, 2026 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants