Skip to content

fix: bound WAV chunk parsing - #5

Merged
kolkov merged 3 commits into
gogpu:mainfrom
besmpl:agent/bound-wav-chunks
Aug 12, 2026
Merged

fix: bound WAV chunk parsing#5
kolkov merged 3 commits into
gogpu:mainfrom
besmpl:agent/bound-wav-chunks

Conversation

@besmpl

@besmpl besmpl commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • bound declared WAV chunk sizes by the bytes remaining before converting to int
  • keep chunk-end and word-padding arithmetic inside the input range
  • remove a now-unreachable post-bound guard
  • preserve the existing truncated-chunk behavior
  • add regressions for oversized fmt and data declarations plus odd-chunk padding

Why

WAV chunk sizes are unsigned 32-bit values. Converting them to int before checking the input boundary can wrap on 32-bit targets and produce invalid slice bounds. Comparing in a wider type first keeps parsing portable and bounded.

Verification

  • go test -count=1 ./...
  • go test -race -count=1 ./...
  • go build ./...
  • go vet ./...
  • CGO_ENABLED=0 GOOS=linux GOARCH=386 go build ./...
  • go test -coverprofile=coverage.out -covermode=atomic ./...: root package 93.2%; changed coverable statements 100%
  • structured malformed-input stress and valid odd/even chunk checks
  • gofmt and git diff --check

Current CI status

The upstream fork workflow for head 586e548 is awaiting maintainer approval in Actions run 31433847314. Until that approval, GitHub cannot publish the repository's Actions or Codecov checks/comments. Local changed-line coverage is 100%.

@besmpl
besmpl requested a review from kolkov as a code owner August 10, 2026 17:07
kolkov
kolkov previously approved these changes Aug 11, 2026

@kolkov kolkov 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.

Solid security fix. The int(uint32) conversion on 32-bit targets is a real bug — MaxUint32 wraps to -1, producing reversed slice bounds and a panic on any malformed WAV with a large chunk size.

The fix is correct: comparing in uint64 before converting to int keeps the declared size bounded by available bytes. The existing truncated-chunk behavior is preserved — an oversized data chunk still reads all available samples.

Good edge cases covered: oversized fmt chunk (panic regression), oversized data chunk (truncation), odd-chunk padding at EOF (no advance past input range). The existing unknown-chunk test fix (even → odd payload + explicit pad byte) correctly validates word-alignment.

Minor: the loop condition change (offset <= len(data)-8 vs offset+8 <= len(data)) is safe because the RIFF header check guarantees len(data) >= 12, so len(data)-8 >= 4 — no underflow.

LGTM, merging.

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@kolkov

kolkov commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

CI caught one gosec G115 issue:

wav.go:70:40: G115: integer overflow conversion int -> uint64 (gosec)
    if uint64(declaredChunkSize) < uint64(remaining) {

remaining is always non-negative here (the loop condition guarantees chunkDataStart <= len(data)), so this is a false positive — but CI won't pass without addressing it.

Simplest fix: since remaining is guaranteed non-negative, cast via uint first:

if uint64(declaredChunkSize) < uint64(uint(remaining)) {

Or use a guard for clarity:

if remaining >= 0 && uint64(declaredChunkSize) < uint64(remaining) {

@kolkov
kolkov merged commit ef3638b into gogpu:main Aug 12, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants