Skip to content

Conversation

@ybastide
Copy link
Collaborator

@ybastide ybastide commented Jan 8, 2026

Fix buffer overflow in MaybeEscapePattern (%[a-f] at pattern's end). Also replace new byte[] with ArrayPool<byte>.

@ybastide ybastide requested a review from Copilot January 8, 2026 11:23
Copy link

Copilot AI left a comment

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 addresses a buffer overflow vulnerability in the MaybeEscapePattern method and improves memory management by replacing heap allocations with ArrayPool<byte>.

Key Changes:

  • Fixed buffer overflow caused by incorrect index manipulation in the percent-encoding capitalization logic
  • Replaced new byte[] allocations with ArrayPool<byte>.Shared.Rent() for better memory efficiency
  • Updated method signature to return the rented array via an out parameter for proper cleanup

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
RobotsTxt/RobotsTxtParser.cs Fixed buffer overflow in percent-encoding logic and replaced direct byte array allocation with ArrayPool
TestRobotsTxt/TestRobotsTxtParser.cs Updated test to handle new ArrayPool-based method signature and added cleanup logic
TestRobotsTxt/GoogleTests.cs Updated test to handle new ArrayPool-based method signature, added cleanup logic, and added test case for the buffer overflow scenario

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

@ybastide ybastide requested a review from Copilot January 8, 2026 11:27
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


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

}

return dst;
return dst.AsSpan(0, j);
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

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

Returning a span over a rented array that will be returned to the pool creates a potential use-after-free scenario. The caller receives a span pointing to memory that may be reused once ArrayPool<byte>.Shared.Return(dst) is called. Consider either returning the array itself with the length, or copying the data to a new array before returning.

Suggested change
return dst.AsSpan(0, j);
var result = new byte[j];
System.Array.Copy(dst, 0, result, 0, j);
return result.AsSpan();

Copilot uses AI. Check for mistakes.
{
var actual =
Encoding.ASCII.GetString(RobotsTxtParser.MaybeEscapePattern(Encoding.UTF8.GetBytes(url)).ToArray());
Encoding.ASCII.GetString(RobotsTxtParser.MaybeEscapePattern(Encoding.UTF8.GetBytes(url), out var dst).ToArray());
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

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

Calling .ToArray() on the span creates a copy before the rented buffer is returned to the pool, but this pattern is fragile. If the .ToArray() call were removed or reordered, it would read from freed memory. Consider restructuring to make the dependency on copying before return more explicit.

Copilot uses AI. Check for mistakes.
@ybastide ybastide merged commit 97ba8a5 into main Jan 9, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants