Skip to content

Fix buffer overflow #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ybastide merged 4 commits into from
Jan 9, 2026
Merged

Fix buffer overflow #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5642b21
🐛 MaybeEscapePattern: overflow
ybastide Jan 8, 2026
8d31a62
⚡️ use ArrayPool<byte>.Shared
ybastide Jan 8, 2026
86f9141
✅ add tests suggested by Copilot 🤷
ybastide Jan 8, 2026
2acf20e
👷 update actions, .NET version
ybastide Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/dotnet.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ jobs:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v6
- name: Setup .NET
uses: actions/setup-dotnet@v3
uses: actions/setup-dotnet@v5
with:
dotnet-version: 9.0.x
dotnet-version: 10
- name: Restore dependencies
run: dotnet restore
- name: Build
Expand Down
20 changes: 13 additions & 7 deletions RobotsTxt/RobotsTxtParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
using System.Buffers;

namespace RobotsTxt;

public class RobotsTxtParser(byte[] robotsBody, IRobotsParseHandler handler)
Expand Down Expand Up @@ -74,8 +76,9 @@ private void ParseAndEmitLine(int currentLine, ReadOnlySpan<byte> line)
key.Parse(stringKey);
if (NeedEscapeValueForKey(key))
{
var escapedValue = MaybeEscapePattern(value);
var escapedValue = MaybeEscapePattern(value, out var dst);
EmitKeyValueToHandler(currentLine, key, escapedValue);
if (dst != null) ArrayPool<byte>.Shared.Return(dst);
}
else
{
Expand Down Expand Up @@ -107,7 +110,7 @@ private void EmitKeyValueToHandler(int currentLine, ParsedRobotsKey key, ReadOnl
}
}

public static ReadOnlySpan<byte> MaybeEscapePattern(ReadOnlySpan<byte> src)
public static ReadOnlySpan<byte> MaybeEscapePattern(ReadOnlySpan<byte> src, out byte[]? dst)
{
var numToEscape = 0;
var needCapitalize = false;
Expand All @@ -131,19 +134,22 @@ public static ReadOnlySpan<byte> MaybeEscapePattern(ReadOnlySpan<byte> src)

if (numToEscape == 0 && !needCapitalize)
{
dst = null;
return src;
}

var dst = new byte[numToEscape * 2 + src.Length];
dst = ArrayPool<byte>.Shared.Rent(numToEscape * 2 + src.Length);

var j = 0;
for (var i = 0; i < src.Length; i++)
{
var c = src[i];
if (c == '%' && i + 2 < src.Length && src[i + 1].IsXDigit() && src[i + 2].IsXDigit())
{
dst[j++] = src[i++];
dst[j++] = src[i++].ToUpper();
dst[j++] = src[i++].ToUpper();
dst[j++] = (byte)'%';
dst[j++] = src[i + 1].ToUpper();
dst[j++] = src[i + 2].ToUpper();
i += 2;
}
else if (c >= 0x80)
{
Expand All @@ -157,7 +163,7 @@ public static ReadOnlySpan<byte> MaybeEscapePattern(ReadOnlySpan<byte> src)
}
}

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

private static bool NeedEscapeValueForKey(ParsedRobotsKey key)
Expand Down
7 changes: 6 additions & 1 deletion TestRobotsTxt/GoogleTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Buffers;
using System.Diagnostics;
using System.Text;
using RobotsTxt;
Expand Down Expand Up @@ -1011,11 +1012,15 @@ public void TestGetPathParamsQuery(string url, string expected)
[InlineData("/a/b/c", "/a/b/c")]
[InlineData("á", "%C3%A1")]
[InlineData("%aa", "%AA")]
[InlineData("%ab%c", "%AB%c")]
[InlineData("test%", "test%")]
[InlineData("%a", "%a")]
public void TestMaybeEscapePattern(string url, string expected)
{
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.
Assert.Equal(expected, actual);
if (dst != null) ArrayPool<byte>.Shared.Return(dst);
}
}
}
6 changes: 5 additions & 1 deletion TestRobotsTxt/TestRobotsTxtParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
using System.Buffers;
using System.Text;

using RobotsTxt;

using Xunit;

namespace TestRobotsTxt
Expand Down Expand Up @@ -27,14 +30,14 @@
}

[Theory]
[InlineData("", false, null, null)]

Check warning on line 33 in TestRobotsTxt/TestRobotsTxtParser.cs

View workflow job for this annotation

GitHub Actions / build 

Null should not be used for type parameter 'expectedValue' of type 'string'. Use a non-null value, or convert the parameter to a nullable type. (https://xunit.net/xunit.analyzers/rules/xUnit1012)

Check warning on line 33 in TestRobotsTxt/TestRobotsTxtParser.cs

View workflow job for this annotation

GitHub Actions / build 

Null should not be used for type parameter 'expectedKey' of type 'string'. Use a non-null value, or convert the parameter to a nullable type. (https://xunit.net/xunit.analyzers/rules/xUnit1012)

Check warning on line 33 in TestRobotsTxt/TestRobotsTxtParser.cs

View workflow job for this annotation

GitHub Actions / build 

Null should not be used for type parameter 'expectedValue' of type 'string'. Use a non-null value, or convert the parameter to a nullable type. (https://xunit.net/xunit.analyzers/rules/xUnit1012)

Check warning on line 33 in TestRobotsTxt/TestRobotsTxtParser.cs

View workflow job for this annotation

GitHub Actions / build 

Null should not be used for type parameter 'expectedKey' of type 'string'. Use a non-null value, or convert the parameter to a nullable type. (https://xunit.net/xunit.analyzers/rules/xUnit1012)
[InlineData(":", false, "", null)]

Check warning on line 34 in TestRobotsTxt/TestRobotsTxtParser.cs

View workflow job for this annotation

GitHub Actions / build 

Null should not be used for type parameter 'expectedValue' of type 'string'. Use a non-null value, or convert the parameter to a nullable type. (https://xunit.net/xunit.analyzers/rules/xUnit1012)

Check warning on line 34 in TestRobotsTxt/TestRobotsTxtParser.cs

View workflow job for this annotation

GitHub Actions / build 

Null should not be used for type parameter 'expectedValue' of type 'string'. Use a non-null value, or convert the parameter to a nullable type. (https://xunit.net/xunit.analyzers/rules/xUnit1012)
[InlineData("abc:", true, "abc", "")]
[InlineData("allow: foo", true, "allow", "foo")]
[InlineData("allow:foo", true, "allow", "foo")]
[InlineData(" allow : foo ", true, "allow", "foo")]
[InlineData("#", false, null, null)]

Check warning on line 39 in TestRobotsTxt/TestRobotsTxtParser.cs

View workflow job for this annotation

GitHub Actions / build 

Null should not be used for type parameter 'expectedKey' of type 'string'. Use a non-null value, or convert the parameter to a nullable type. (https://xunit.net/xunit.analyzers/rules/xUnit1012)

Check warning on line 39 in TestRobotsTxt/TestRobotsTxtParser.cs

View workflow job for this annotation

GitHub Actions / build 

Null should not be used for type parameter 'expectedValue' of type 'string'. Use a non-null value, or convert the parameter to a nullable type. (https://xunit.net/xunit.analyzers/rules/xUnit1012)

Check warning on line 39 in TestRobotsTxt/TestRobotsTxtParser.cs

View workflow job for this annotation

GitHub Actions / build 

Null should not be used for type parameter 'expectedKey' of type 'string'. Use a non-null value, or convert the parameter to a nullable type. (https://xunit.net/xunit.analyzers/rules/xUnit1012)
[InlineData(" : #", false, "", null)]

Check warning on line 40 in TestRobotsTxt/TestRobotsTxtParser.cs

View workflow job for this annotation

GitHub Actions / build 

Null should not be used for type parameter 'expectedValue' of type 'string'. Use a non-null value, or convert the parameter to a nullable type. (https://xunit.net/xunit.analyzers/rules/xUnit1012)
[InlineData("allow: \t\tfoo\t# Bar", true, "allow", "foo")]
public void TestGetKeyAndValueFrom(string line, bool rc, string expectedKey, string expectedValue)
{
Expand All @@ -56,8 +59,9 @@
[InlineData("é", "%C3%A9")]
public void TestMaybeEscapePattern(string src, string expected)
{
var actual = RobotsTxtParser.MaybeEscapePattern(Encoding.UTF8.GetBytes(src));
var actual = RobotsTxtParser.MaybeEscapePattern(Encoding.UTF8.GetBytes(src), out var dst);
Assert.Equal(expected, Encoding.UTF8.GetString(actual.ToArray()));
if (dst != null) ArrayPool<byte>.Shared.Return(dst);
}
}
}