Skip to content

oggopus: clamp the end-trim keep in int64 before converting to int (32-bit hardening) #89

Description

@tphakala

In oggopus/decoder.go Decoder.fill(), the granule end-trim computes keep := max(int(d.limit-d.delivered), 0) and then slices samples[:keep*d.info.Channels]. The int64 delta d.limit - d.delivered is converted to int before the clamp.

In normal decoding this is safe. The clamp only runs under d.limit >= 0 && d.delivered+perChan > d.limit, and delivered never exceeds limit (once end-trim fires, delivered is pinned to limit), so the delta is in [0, perChan) where perChan is a single Opus frame (at most 5760 samples per channel). That fits int32 on every platform, and on 64-bit int is 64-bit anyway, so there is no way to reach a bad value.

The theoretical gap is 32-bit only (GOARCH=386/arm). The total-sample limit is discovered lazily (it stays -1 until totalSamples() returns). If a malformed or adversarial stream withholds the limit until after more than ~2^31 samples per channel have already been delivered, and the eventually-discovered limit is smaller than delivered, then d.limit - d.delivered is a large negative int64 whose int() conversion can wrap to a positive int on a 32-bit build. That defeats the max(..., 0) floor and yields an oversized slice bound in samples[:keep*d.info.Channels], i.e. a panic while decoding untrusted input.

This predates the modernize change in #88: that PR only replaced an equivalent if keep < 0 { keep = 0 } clamp with max() and did not alter the conversion order. The observation was raised by Copilot on #88.

Suggested fix: clamp in int64 and cap to perChan before converting to int, for example:

keep := int(min(max(d.limit-d.delivered, 0), int64(perChan)))

Severity is low (32-bit targets only, and only under a specifically malformed late-limit stream), but go-opus decodes untrusted Ogg Opus input, so not panicking on a malformed stream regardless of word size is the right property.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions