Skip to content

Fixed extmap parsing and integer overflows#250

Open
sirzooro wants to merge 1 commit into
pion:mainfrom
sirzooro:extmap_intovl
Open

Fixed extmap parsing and integer overflows#250
sirzooro wants to merge 1 commit into
pion:mainfrom
sirzooro:extmap_intovl

Conversation

@sirzooro
Copy link
Copy Markdown
Contributor

This PR addresses several bugs in the SDP parser:

  • Integer overflow in readUint64Field and parseTimeUnits — values exceeding the type's range previously wrapped around silently; they now return a syntaxError or errSDPInvalidValue.
  • ExtMap parsing logic — the err check after strconv.ParseInt was performed after the range check, which could mask parse errors; the order is now correct. Additionally, the upper bound of valid extmap keys was incorrectly 246 (the error message already said 256); corrected to 256.

Changes

base_lexer.go — overflow-safe readUint64Field

Replaced the verbose per-digit switch/case with a compact digit-range check and an explicit overflow guard before each multiply-add step:

if i > (math.MaxUint64-digit)/10 {
    return i, l.syntaxError()
}
i = i*10 + digit

Previously, values larger than math.MaxUint64 (e.g. 2^64) would silently wrap around to 0. Now a syntaxError is returned instead.

base_lexer_test.go — uint64 overflow test

Added a test that verifies readUint64Field returns a syntaxError for the value 18446744073709551616 (2^64), which previously wrapped to 0.

extmap.go — two bug fixes in ExtMap.Unmarshal

  1. Order of checks: The range check on value was performed before the strconv.ParseInt error check, meaning a parse failure could silently fall through to the range check. The two checks are now in the correct order: parse error first, range check second.

  2. Correct upper bound: The upper bound was 246 even though the error message stated 1-256. Corrected to 256.

extmap_test.go — boundary value tests for ExtMap.Unmarshal

Added a table-driven test covering all four boundary cases:

  • Value 0 → rejected (below lower bound)
  • Value 1 → accepted (lower bound)
  • Value 256 → accepted (upper bound)
  • Value 257 → rejected (above upper bound)

unmarshal.go — overflow-safe parseTimeUnits

Added a pre-multiplication overflow guard when applying the time-unit multiplier k (seconds/minutes/hours/days):

if k > 1 && (num > math.MaxInt64/k || num < math.MinInt64/k) {
    return 0, fmt.Errorf("%w `%v`", errSDPInvalidValue, value)
}

Without this check, an input such as 9223372036854775807h would overflow int64 silently.

unmarshal_test.goparseTimeUnits overflow tests

Added TestParseTimeUnits_Overflow covering:

  • Positive overflow with the h (hours) multiplier
  • Negative overflow with the h multiplier
  • A non-overflowing value at the boundary with the d (days) multiplier

@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.41%. Comparing base (1d03772) to head (02f1a05).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #250      +/-   ##
==========================================
- Coverage   97.43%   97.41%   -0.03%     
==========================================
  Files          12       12              
  Lines        1405     1390      -15     
==========================================
- Hits         1369     1354      -15     
  Misses         19       19              
  Partials       17       17              
Flag Coverage Δ
go 97.41% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sirzooro sirzooro requested a review from JoTurk May 22, 2026 15:01
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.

1 participant