Skip to content

fix: bound frame payload allocations - #5

Merged
kolkov merged 4 commits into
gogpu:mainfrom
besmpl:agent/bound-frame-payloads
Aug 12, 2026
Merged

fix: bound frame payload allocations#5
kolkov merged 4 commits into
gogpu:mainfrom
besmpl:agent/bound-frame-payloads

Conversation

@besmpl

@besmpl besmpl commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add one shared 64 MiB allocation limit for frame payloads
  • validate payload and decoded sizes before integer conversion or allocation while keeping wire-header decoding permissive
  • keep LZ4 fallback growth proportional to compressed input and reject inconsistent compressed metadata
  • remove allocation guards that are unreachable after the shared boundary checks
  • document the boundary and add transport, codec, server, and 32-bit portability coverage
  • keep the current golangci-lint output ordering configuration valid with the latest v2 parser

Why

Malformed or unsupported frame metadata should fail at the transport/decompression boundary rather than drive allocations from unchecked wire sizes. The codec also needs a proportional fallback ceiling so tiny invalid blocks cannot consume the full global budget.

Verification

  • go test -count=1 ./...
  • go test -race -count=1 ./...
  • go build ./...
  • go vet ./...
  • GOOS=linux GOARCH=386 go test -c for changed protocol/codec/socket packages
  • go test -coverprofile=coverage.out -covermode=atomic ./...: repository 94.6%; changed coverable statements 100%
  • exact 64 MiB boundary acceptance and over-limit rejection
  • valid near-maximum LZ4 expansion plus malformed-small-input regression
  • gofmt and git diff --check

Current CI status

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

@besmpl
besmpl marked this pull request as ready for review August 10, 2026 17:04
@besmpl
besmpl requested a review from kolkov as a code owner August 10, 2026 17:04

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

Addresses real unbounded-allocation vectors at three independent boundaries:

  1. Transport readconn.go did make([]byte, int(hdr.PayloadSize)) from untrusted uint32 wire data. Now validates against MaxPayloadSize (64 MiB) before conversion. 4K RGBA = ~31.6 MiB, so 64 MiB is a reasonable ceiling.

  2. LZ4 decode fallback — old code did make([]byte, len(src)*10) without overflow protection, then doubled up to a fixed 64 MB. New initialDecodeSize/maxDecodeSize helpers are overflow-safe and scale proportionally to input (maxDecodeRatio = 256), so a tiny malformed block can't force the full 64 MiB allocation.

  3. Server decompressiondecodePayload allocated make([]byte, hdr.UncompressedSize) directly from the wire. Now validates against MaxPayloadSize and rejects compressed payloads with zero declared uncompressed size.

Good architectural decision: MaxPayloadSize in protocol as the single source of truth, header decode stays permissive (full uint32 range), limits enforced at allocation boundaries.

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 fails on all three PRs with the same config issue:

jsonschema: "output" does not validate with "/properties/output/additionalProperties":
additional properties 'sort-results' not allowed

sort-results was deprecated in golangci-lint v1.63.0 and removed in v2 — results are now always sorted. The existing sort-order field in the config already controls the ordering.

Could you drop the sort-results: true addition from .golangci.yml in all three PRs?

@kolkov
kolkov merged commit bca729d into gogpu:main Aug 12, 2026
10 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