From dd5276b373be15930ddfd6867a8d9222b8f75841 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 17 Mar 2026 02:51:01 +0100 Subject: [PATCH 1/5] Remove unsafe code from StringParser Replace pointer-based parsing with safe indexing and for loops in ParseNextInt32, ParseNextInt64, ParseNextUInt32, ParseNextUInt64. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Common/src/System/IO/StringParser.cs | 128 +++++++----------- 1 file changed, 52 insertions(+), 76 deletions(-) diff --git a/src/libraries/Common/src/System/IO/StringParser.cs b/src/libraries/Common/src/System/IO/StringParser.cs index 941e78ac52239b..c2af18a166da07 100644 --- a/src/libraries/Common/src/System/IO/StringParser.cs +++ b/src/libraries/Common/src/System/IO/StringParser.cs @@ -134,44 +134,37 @@ public string ExtractCurrent() } /// Moves to the next component and parses it as an Int32. - public unsafe int ParseNextInt32() + public int ParseNextInt32() { MoveNextOrFail(); + if (_startIndex == _endIndex) + { + ThrowForInvalidData(); + } + bool negative = false; int result = 0; + int i = _startIndex; - fixed (char* bufferPtr = _buffer) + if (_buffer[i] == '-') { - char* p = bufferPtr + _startIndex; - char* end = bufferPtr + _endIndex; - - if (p == end) + negative = true; + i++; + if (i == _endIndex) { ThrowForInvalidData(); } + } - if (*p == '-') - { - negative = true; - p++; - if (p == end) - { - ThrowForInvalidData(); - } - } - - while (p != end) + for (; i < _endIndex; i++) + { + int d = _buffer[i] - '0'; + if (d < 0 || d > 9) { - int d = *p - '0'; - if (d < 0 || d > 9) - { - ThrowForInvalidData(); - } - result = negative ? checked((result * 10) - d) : checked((result * 10) + d); - - p++; + ThrowForInvalidData(); } + result = negative ? checked((result * 10) - d) : checked((result * 10) + d); } Debug.Assert(result == int.Parse(ExtractCurrent()), "Expected manually parsed result to match Parse result"); @@ -179,44 +172,37 @@ public unsafe int ParseNextInt32() } /// Moves to the next component and parses it as an Int64. - public unsafe long ParseNextInt64() + public long ParseNextInt64() { MoveNextOrFail(); + if (_startIndex == _endIndex) + { + ThrowForInvalidData(); + } + bool negative = false; long result = 0; + int i = _startIndex; - fixed (char* bufferPtr = _buffer) + if (_buffer[i] == '-') { - char* p = bufferPtr + _startIndex; - char* end = bufferPtr + _endIndex; - - if (p == end) + negative = true; + i++; + if (i == _endIndex) { ThrowForInvalidData(); } + } - if (*p == '-') - { - negative = true; - p++; - if (p == end) - { - ThrowForInvalidData(); - } - } - - while (p != end) + for (; i < _endIndex; i++) + { + int d = _buffer[i] - '0'; + if (d < 0 || d > 9) { - int d = *p - '0'; - if (d < 0 || d > 9) - { - ThrowForInvalidData(); - } - result = negative ? checked((result * 10) - d) : checked((result * 10) + d); - - p++; + ThrowForInvalidData(); } + result = negative ? checked((result * 10) - d) : checked((result * 10) + d); } Debug.Assert(result == long.Parse(ExtractCurrent()), "Expected manually parsed result to match Parse result"); @@ -224,7 +210,7 @@ public unsafe long ParseNextInt64() } /// Moves to the next component and parses it as a UInt32. - public unsafe uint ParseNextUInt32() + public uint ParseNextUInt32() { MoveNextOrFail(); if (_startIndex == _endIndex) @@ -233,21 +219,14 @@ public unsafe uint ParseNextUInt32() } uint result = 0; - fixed (char* bufferPtr = _buffer) + for (int i = _startIndex; i < _endIndex; i++) { - char* p = bufferPtr + _startIndex; - char* end = bufferPtr + _endIndex; - while (p != end) + int d = _buffer[i] - '0'; + if (d < 0 || d > 9) { - int d = *p - '0'; - if (d < 0 || d > 9) - { - ThrowForInvalidData(); - } - result = (uint)checked((result * 10) + d); - - p++; + ThrowForInvalidData(); } + result = (uint)checked((result * 10) + d); } Debug.Assert(result == uint.Parse(ExtractCurrent()), "Expected manually parsed result to match Parse result"); @@ -255,26 +234,23 @@ public unsafe uint ParseNextUInt32() } /// Moves to the next component and parses it as a UInt64. - public unsafe ulong ParseNextUInt64() + public ulong ParseNextUInt64() { MoveNextOrFail(); + if (_startIndex == _endIndex) + { + ThrowForInvalidData(); + } ulong result = 0; - fixed (char* bufferPtr = _buffer) + for (int i = _startIndex; i < _endIndex; i++) { - char* p = bufferPtr + _startIndex; - char* end = bufferPtr + _endIndex; - while (p != end) + int d = _buffer[i] - '0'; + if (d < 0 || d > 9) { - int d = *p - '0'; - if (d < 0 || d > 9) - { - ThrowForInvalidData(); - } - result = checked((result * 10ul) + (ulong)d); - - p++; + ThrowForInvalidData(); } + result = checked((result * 10ul) + (ulong)d); } Debug.Assert(result == ulong.Parse(ExtractCurrent()), "Expected manually parsed result to match Parse result"); From 4fe274fc1eec273cc68b7af26fc7be8d1f204ee9 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 17 Mar 2026 02:57:56 +0100 Subject: [PATCH 2/5] Hoist fields to locals in StringParser parse methods Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Common/src/System/IO/StringParser.cs | 54 ++++++++++++------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/src/libraries/Common/src/System/IO/StringParser.cs b/src/libraries/Common/src/System/IO/StringParser.cs index c2af18a166da07..5de666c94dc802 100644 --- a/src/libraries/Common/src/System/IO/StringParser.cs +++ b/src/libraries/Common/src/System/IO/StringParser.cs @@ -138,28 +138,32 @@ public int ParseNextInt32() { MoveNextOrFail(); - if (_startIndex == _endIndex) + int startIndex = _startIndex; + int endIndex = _endIndex; + string buffer = _buffer; + + if (startIndex == endIndex) { ThrowForInvalidData(); } bool negative = false; int result = 0; - int i = _startIndex; + int i = startIndex; - if (_buffer[i] == '-') + if (buffer[i] == '-') { negative = true; i++; - if (i == _endIndex) + if (i == endIndex) { ThrowForInvalidData(); } } - for (; i < _endIndex; i++) + for (; i < endIndex; i++) { - int d = _buffer[i] - '0'; + int d = buffer[i] - '0'; if (d < 0 || d > 9) { ThrowForInvalidData(); @@ -176,28 +180,32 @@ public long ParseNextInt64() { MoveNextOrFail(); - if (_startIndex == _endIndex) + int startIndex = _startIndex; + int endIndex = _endIndex; + string buffer = _buffer; + + if (startIndex == endIndex) { ThrowForInvalidData(); } bool negative = false; long result = 0; - int i = _startIndex; + int i = startIndex; - if (_buffer[i] == '-') + if (buffer[i] == '-') { negative = true; i++; - if (i == _endIndex) + if (i == endIndex) { ThrowForInvalidData(); } } - for (; i < _endIndex; i++) + for (; i < endIndex; i++) { - int d = _buffer[i] - '0'; + int d = buffer[i] - '0'; if (d < 0 || d > 9) { ThrowForInvalidData(); @@ -213,15 +221,20 @@ public long ParseNextInt64() public uint ParseNextUInt32() { MoveNextOrFail(); - if (_startIndex == _endIndex) + + int startIndex = _startIndex; + int endIndex = _endIndex; + string buffer = _buffer; + + if (startIndex == endIndex) { ThrowForInvalidData(); } uint result = 0; - for (int i = _startIndex; i < _endIndex; i++) + for (int i = startIndex; i < endIndex; i++) { - int d = _buffer[i] - '0'; + int d = buffer[i] - '0'; if (d < 0 || d > 9) { ThrowForInvalidData(); @@ -237,15 +250,20 @@ public uint ParseNextUInt32() public ulong ParseNextUInt64() { MoveNextOrFail(); - if (_startIndex == _endIndex) + + int startIndex = _startIndex; + int endIndex = _endIndex; + string buffer = _buffer; + + if (startIndex == endIndex) { ThrowForInvalidData(); } ulong result = 0; - for (int i = _startIndex; i < _endIndex; i++) + for (int i = startIndex; i < endIndex; i++) { - int d = _buffer[i] - '0'; + int d = buffer[i] - '0'; if (d < 0 || d > 9) { ThrowForInvalidData(); From b74e4575d77eda88f5b6ea41662cfd93bc273de0 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 17 Mar 2026 03:01:14 +0100 Subject: [PATCH 3/5] Address PR feedback: use ReadOnlySpan for bounds-check elimination and add empty-component tests - Use AsSpan slice in all parse methods so JIT can eliminate bounds checks - Add Theory test covering empty component -> InvalidDataException for all numeric parse methods Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Common/src/System/IO/StringParser.cs | 52 +++++++++---------- .../Tests/System/IO/StringParserTests.cs | 27 ++++++++++ 2 files changed, 51 insertions(+), 28 deletions(-) diff --git a/src/libraries/Common/src/System/IO/StringParser.cs b/src/libraries/Common/src/System/IO/StringParser.cs index 5de666c94dc802..df68525bb40a89 100644 --- a/src/libraries/Common/src/System/IO/StringParser.cs +++ b/src/libraries/Common/src/System/IO/StringParser.cs @@ -140,30 +140,30 @@ public int ParseNextInt32() int startIndex = _startIndex; int endIndex = _endIndex; - string buffer = _buffer; + ReadOnlySpan span = _buffer.AsSpan(startIndex, endIndex - startIndex); - if (startIndex == endIndex) + if (span.IsEmpty) { ThrowForInvalidData(); } bool negative = false; int result = 0; - int i = startIndex; + int i = 0; - if (buffer[i] == '-') + if (span[0] == '-') { negative = true; - i++; - if (i == endIndex) + i = 1; + if (i == span.Length) { ThrowForInvalidData(); } } - for (; i < endIndex; i++) + for (; i < span.Length; i++) { - int d = buffer[i] - '0'; + int d = span[i] - '0'; if (d < 0 || d > 9) { ThrowForInvalidData(); @@ -182,30 +182,30 @@ public long ParseNextInt64() int startIndex = _startIndex; int endIndex = _endIndex; - string buffer = _buffer; + ReadOnlySpan span = _buffer.AsSpan(startIndex, endIndex - startIndex); - if (startIndex == endIndex) + if (span.IsEmpty) { ThrowForInvalidData(); } bool negative = false; long result = 0; - int i = startIndex; + int i = 0; - if (buffer[i] == '-') + if (span[0] == '-') { negative = true; - i++; - if (i == endIndex) + i = 1; + if (i == span.Length) { ThrowForInvalidData(); } } - for (; i < endIndex; i++) + for (; i < span.Length; i++) { - int d = buffer[i] - '0'; + int d = span[i] - '0'; if (d < 0 || d > 9) { ThrowForInvalidData(); @@ -222,19 +222,17 @@ public uint ParseNextUInt32() { MoveNextOrFail(); - int startIndex = _startIndex; - int endIndex = _endIndex; - string buffer = _buffer; + ReadOnlySpan span = _buffer.AsSpan(_startIndex, _endIndex - _startIndex); - if (startIndex == endIndex) + if (span.IsEmpty) { ThrowForInvalidData(); } uint result = 0; - for (int i = startIndex; i < endIndex; i++) + for (int i = 0; i < span.Length; i++) { - int d = buffer[i] - '0'; + int d = span[i] - '0'; if (d < 0 || d > 9) { ThrowForInvalidData(); @@ -251,19 +249,17 @@ public ulong ParseNextUInt64() { MoveNextOrFail(); - int startIndex = _startIndex; - int endIndex = _endIndex; - string buffer = _buffer; + ReadOnlySpan span = _buffer.AsSpan(_startIndex, _endIndex - _startIndex); - if (startIndex == endIndex) + if (span.IsEmpty) { ThrowForInvalidData(); } ulong result = 0; - for (int i = startIndex; i < endIndex; i++) + for (int i = 0; i < span.Length; i++) { - int d = buffer[i] - '0'; + int d = span[i] - '0'; if (d < 0 || d > 9) { ThrowForInvalidData(); diff --git a/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs b/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs index 410d7f1f82a46c..09e7ff8c3fb439 100644 --- a/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs +++ b/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs @@ -109,6 +109,33 @@ public void TestOverflowFromNumericParsing() Assert.Throws(() => sp.ParseNextUInt64()); } + [Theory] + [InlineData(",,", "ParseNextInt32")] + [InlineData(",,", "ParseNextInt64")] + [InlineData(",,", "ParseNextUInt32")] + [InlineData(",,", "ParseNextUInt64")] + public void TestEmptyComponentThrowsInvalidData(string buffer, string method) + { + StringParser sp = new StringParser(buffer, ','); + sp.MoveNextOrFail(); + + switch (method) + { + case "ParseNextInt32": + Assert.Throws(() => sp.ParseNextInt32()); + break; + case "ParseNextInt64": + Assert.Throws(() => sp.ParseNextInt64()); + break; + case "ParseNextUInt32": + Assert.Throws(() => sp.ParseNextUInt32()); + break; + case "ParseNextUInt64": + Assert.Throws(() => sp.ParseNextUInt64()); + break; + } + } + [Fact] public void TestParseNextChar() { From a74125c67ad3988b066ffa6ef2033e7168e71e04 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 17 Mar 2026 03:15:17 +0100 Subject: [PATCH 4/5] Fix empty-component test: remove extra MoveNextOrFail, add default branch Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Common/tests/Tests/System/IO/StringParserTests.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs b/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs index 09e7ff8c3fb439..81d1d8f25c2a24 100644 --- a/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs +++ b/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs @@ -117,7 +117,6 @@ public void TestOverflowFromNumericParsing() public void TestEmptyComponentThrowsInvalidData(string buffer, string method) { StringParser sp = new StringParser(buffer, ','); - sp.MoveNextOrFail(); switch (method) { @@ -133,6 +132,9 @@ public void TestEmptyComponentThrowsInvalidData(string buffer, string method) case "ParseNextUInt64": Assert.Throws(() => sp.ParseNextUInt64()); break; + default: + Assert.Fail($"Unknown parse method: {method}"); + break; } } From c5b7d9434cf330a2e8d679d1e63a0c747d849e28 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 17 Mar 2026 15:12:38 +0100 Subject: [PATCH 5/5] Update StringParserTests.cs --- .../Tests/System/IO/StringParserTests.cs | 29 ------------------- 1 file changed, 29 deletions(-) diff --git a/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs b/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs index 81d1d8f25c2a24..410d7f1f82a46c 100644 --- a/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs +++ b/src/libraries/Common/tests/Tests/System/IO/StringParserTests.cs @@ -109,35 +109,6 @@ public void TestOverflowFromNumericParsing() Assert.Throws(() => sp.ParseNextUInt64()); } - [Theory] - [InlineData(",,", "ParseNextInt32")] - [InlineData(",,", "ParseNextInt64")] - [InlineData(",,", "ParseNextUInt32")] - [InlineData(",,", "ParseNextUInt64")] - public void TestEmptyComponentThrowsInvalidData(string buffer, string method) - { - StringParser sp = new StringParser(buffer, ','); - - switch (method) - { - case "ParseNextInt32": - Assert.Throws(() => sp.ParseNextInt32()); - break; - case "ParseNextInt64": - Assert.Throws(() => sp.ParseNextInt64()); - break; - case "ParseNextUInt32": - Assert.Throws(() => sp.ParseNextUInt32()); - break; - case "ParseNextUInt64": - Assert.Throws(() => sp.ParseNextUInt64()); - break; - default: - Assert.Fail($"Unknown parse method: {method}"); - break; - } - } - [Fact] public void TestParseNextChar() {