Skip to content

Limit SunRPC record fragments#2516

Open
MrAlias wants to merge 1 commit into
open-telemetry:mainfrom
MrAlias:sunrpc-parse-frag
Open

Limit SunRPC record fragments#2516
MrAlias wants to merge 1 commit into
open-telemetry:mainfrom
MrAlias:sunrpc-parse-frag

Conversation

@MrAlias

@MrAlias MrAlias commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Motivation

  • Prevent a CPU and allocation amplification vector in the SunRPC-over-TCP parser where many tiny fragments could force unbounded loop iterations and slice growth.
  • Bound parser work per record while preserving valid fragmented SunRPC records, including zero-byte record-marking fragments allowed by RFC 5531.

Description

  • Add a per-record fragment cap maxRecordFragments = 64 and wire it into the SunRPC reader to reject pathological records early in readRecord in pkg/internal/sunrpcparser/parser.go by returning ErrNotSunRPC when the cap is exceeded.
  • Accept empty non-final fragments within the fragment cap so legal RM records are still parsed while parser work remains bounded.
  • Add unit tests in pkg/internal/sunrpcparser/parser_test.go to cover a valid fragmented CALL, rejection of too many fragments, and acceptance of empty non-final fragments.
  • Strengthen the fragment-limit regression test so it uses zero-byte fragments followed by a valid final record, ensuring the failure is caused by the cap rather than an invalid reassembled payload.
  • Add the wrapTCPRecordFragments test helper for constructing multi-fragment records.

Testing

  • go test ./pkg/internal/sunrpcparser

@MrAlias MrAlias added this to the v0.10.0 milestone Jun 26, 2026
@MrAlias MrAlias requested a review from a team as a code owner June 26, 2026 16:24
@MrAlias MrAlias added bug Something isn't working go Related to Go code labels Jun 26, 2026
@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 69.18%. Comparing base (914ffa4) to head (972ef13).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pkg/internal/sunrpcparser/parser.go 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2516      +/-   ##
==========================================
- Coverage   69.24%   69.18%   -0.07%     
==========================================
  Files         345      345              
  Lines       46747    46803      +56     
==========================================
+ Hits        32372    32379       +7     
- Misses      12330    12394      +64     
+ Partials     2045     2030      -15     
Flag Coverage Δ
integration-test 51.36% <0.00%> (+0.44%) ⬆️
integration-test-arm 27.24% <62.50%> (-1.07%) ⬇️
integration-test-vm-5.15-lts ?
integration-test-vm-6.18-lts ?
k8s-integration-test 35.88% <0.00%> (+0.18%) ⬆️
oats-test 35.54% <0.00%> (+0.17%) ⬆️
unittests 63.38% <100.00%> (+0.06%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

LGTM! This looks like a good check to add. I don't know much about the SunRPC protocol to judge if 64 fragments is too many or too few, I think it's reasonable.

@grcevski grcevski requested a review from damemi June 26, 2026 18:56
@grcevski

Copy link
Copy Markdown
Contributor

I requested @damemi as a reviewer.

@MrAlias

MrAlias commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

I don't know much about the SunRPC protocol to judge if 64 fragments is too many or too few, I think it's reasonable.

Yeah, not sure either. It seemed reasonable for OBI's current use, but it is heuristic, not a SunRPC-standard value. RFC 5531 does not cap RM fragment count. For OBI’s current captured-buffer use, 64 is intentionally generous: generic TCP large buffers max out at 64 KiB, so this still allows roughly 1 KiB average fragments while bounding zero/tiny-fragment amplification. If we ever reuse this as a full-stream SunRPC parser, we may want a different limit or a configurable one. But seems reasonable for now.

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

Labels

bug Something isn't working go Related to Go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants