Skip to content

fix: Import math and validate request body size - #7535

Open
odaysec wants to merge 2 commits into
elastic:mainfrom
odaysec:patch-1
Open

fix: Import math and validate request body size#7535
odaysec wants to merge 2 commits into
elastic:mainfrom
odaysec:patch-1

Conversation

@odaysec

@odaysec odaysec commented Aug 1, 2026

Copy link
Copy Markdown

Add math import and check for bulk request body size. fix protect all allocation-size arithmetic by checking for overflow before computing/using capacities. For this specific issue, make Buf.grow compute a target capacity without overflowing, and panic with a clear message if growth cannot be represented as int.

Best targeted fix (no functional change beyond safety):

  • Edit internal/pkg/danger/buf.go, function grow.
  • Replace 2*cap(b.buf)+n with guarded arithmetic:
    • Validate n >= 0 (defensive, even though Grow checks).
    • Compute required capacity need := len(b.buf) + n with overflow check.
    • Compute doubled capacity from current cap with overflow check/saturation.
    • Choose newCap = max(need, doubledCap).
    • Allocate with make([]byte, len(b.buf), newCap).
  • Add math import for math.MaxInt.

Design Checklist

  • I have ensured my design is stateless and will work when multiple fleet-server instances are behind a load balancer.
  • I have or intend to scale test my changes, ensuring it will work reliably with 100K+ agents connected.
  • I have included fail safe mechanisms to limit the load on fleet-server: rate limiting, circuit breakers, caching, load shedding, etc.

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool

Related issues

Add math import and check for bulk request body size. fix protect all allocation-size arithmetic by checking for overflow before computing/using capacities. For this specific issue, make `Buf.grow` compute a target capacity without overflowing, and panic with a clear message if growth cannot be represented as `int`.

Best targeted fix (no functional change beyond safety):
- Edit `internal/pkg/danger/buf.go`, function `grow`.
- Replace `2*cap(b.buf)+n` with guarded arithmetic:
  - Validate `n >= 0` (defensive, even though `Grow` checks).
  - Compute required capacity `need := len(b.buf) + n` with overflow check.
  - Compute doubled capacity from current cap with overflow check/saturation.
  - Choose `newCap = max(need, doubledCap)`.
  - Allocate with `make([]byte, len(b.buf), newCap)`.
- Add `math` import for `math.MaxInt`.
@odaysec
odaysec requested a review from a team as a code owner August 1, 2026 22:37
@odaysec
odaysec requested review from blakerouse and lorienhu August 1, 2026 22:37
@mergify

mergify Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

This pull request does not have a backport label. Could you fix it @ekuboo100? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

Comment thread internal/pkg/danger/buf.go

// Serialize request
const kSlop = 64
if len(body) > math.MaxInt-kSlop {

@kruskall kruskall Aug 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you share what system you are running to have a byte slice of len(math.MaxInt) in memory ?

@ebeahan

ebeahan commented Aug 3, 2026

Copy link
Copy Markdown
Member

/test

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Buildkite failed in Run check-ci because mage check:ci detected generated code edits from go fix and the worktree was not clean. The failing file is internal/pkg/danger/buf.go, which go fix rewrites before Check.NoChanges fails.

Remediation

  • Run mage check:ci locally (or at minimum mage check:fix) and commit the resulting change in internal/pkg/danger/buf.go.
  • Re-run CI after pushing; this should clear the check-ci failure once the tree is clean after go fix.
Investigation details

Root Cause

check-ci runs Generate, Check.Imports, Check.Fix, Check.Headers, Check.Notice, then Check.NoChanges (magefile.go lines 625-627). Check.NoChanges fails if any file is modified after those steps, using git update-index --refresh and git diff-index --exit-code (magefile.go lines 609-615).

In this build, go fix rewrote internal/pkg/danger/buf.go from:

newCap := need
if doubled > newCap {
	newCap = doubled
}

to:

newCap := max(doubled, need)

That uncommitted rewrite caused Check.NoChanges to fail.

Evidence

diff --git a/internal/pkg/danger/buf.go b/internal/pkg/danger/buf.go
@@ -53,10 +53,7 @@ func (b *Buf) grow(n int) {
-	newCap := need
-	if doubled > newCap {
-		newCap = doubled
-	}
+	newCap := max(doubled, need)
internal/pkg/danger/buf.go: needs update
Error: git update-index failure: running "git update-index --refresh" failed with exit code 1

Verification

  • Not run in this workflow (read-only detective pass; no branch writes).

Follow-up

  • If this still reproduces after committing mage check:ci output, confirm the local Go version matches repository expectations (.go-version) and rerun mage check:ci before pushing.

What is this? | From workflow: PR Buildkite Detective

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

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.

3 participants