Skip to content

Avoid argv allocation size overflow - #429

Merged
Quim Muntal (qmuntal) merged 1 commit into
mainfrom
fix-localshell-argv-overflow
Jul 6, 2026
Merged

Quim Muntal (qmuntal) merged 1 commit into
mainfrom
fix-localshell-argv-overflow

Conversation

@qmuntal

@qmuntal Quim Muntal (qmuntal) commented Jul 6, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Replace the overflow-prone len(extra)+len(suffix) capacity calculation in shell argv construction with slices.Concat(extra, suffix).
  • Preserve the existing copy semantics while avoiding unchecked integer overflow in allocation size computation.

Fixes Size computation for allocation may overflow · Code scanning alert #1 · microsoft/agent-framework-go.

Validation

  • go test -count=1 ./tool/shelltool

@qmuntal
Quim Muntal (qmuntal) requested a review from a team as a code owner July 6, 2026 14:04
Copilot AI review requested due to automatic review settings July 6, 2026 14:04

Copilot AI 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.

Pull request overview

This PR hardens shell argv construction in shelltool by removing an integer-overflow-prone capacity calculation while keeping the “copy-on-combine” semantics (i.e., not aliasing the input slices).

Changes:

  • Replaces the manual make(..., len(extra)+len(suffix)) capacity computation with a slices-based approach to avoid overflow in allocation size math.
  • Simplifies combineArgv implementation used by both stateless and persistent shell argv construction.
Show a summary per file
File Description
tool/shelltool/localshell.go Updates argv combination to avoid allocation-size overflow during shell command invocation setup.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread tool/shelltool/localshell.go
@github-actions

This comment has been minimized.

@qmuntal
Quim Muntal (qmuntal) force-pushed the fix-localshell-argv-overflow branch from 71da681 to 6b85ec1 Compare July 6, 2026 14:11
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Cross-Repo Parity Review — PR #429

Result: Out of scope — no parity issues.

This PR only modifies the unexported combineArgv helper inside tool/shelltool/localshell.go, replacing a manual slice-append pattern with slices.Concat to fix a potential integer overflow in the capacity calculation. There are no changes to exported Go types, option structs, public methods, or observable runtime behavior.

No upstream .NET or Python parity check is required.

Generated by Go API Consistency Review Agent for #429 · 28.2 AIC · ⌖ 12.8 AIC · ⊞ 27.1K · ◷

@qmuntal
Quim Muntal (qmuntal) added this pull request to the merge queue Jul 6, 2026
Merged via the queue into main with commit 3bf1114 Jul 6, 2026
19 checks passed
@qmuntal
Quim Muntal (qmuntal) deleted the fix-localshell-argv-overflow branch July 6, 2026 14:21
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