Uh oh!
There was an error while loading. Please reload this page.
Improve PemEncoding.TryFind perf with degenerate inputs - #133137
Conversation
Rather than seeking for the correct PostEB, just ask if the next hyphen is the PostEB. If not, we can bypass the IsValidBase64 checks.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
There was a problem hiding this comment.
🟡 Changes recommended
The new tests have an off-by-one Range expectation for Location, and both PemEncoding implementations have a boundary check that can incorrectly accept a single trailing non-whitespace character after the PostEB.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates PemEncoding.TryFind to short-circuit scanning for the post-encapsulation boundary by first locating the next '-' and only doing more expensive work if that position begins a valid PostEB, and adds regression tests targeting pathological inputs with many BEGIN markers.
Changes:
- Optimize PostEB discovery in
PemEncoding.TryFindCore(both in-box and downlevel) to reduce work on degenerate inputs. - Add a large “many begins, one end” regression test in both
System.Security.CryptographyandMicrosoft.Bcl.Cryptographytest suites.
File summaries
| File | Description |
|---|---|
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/PemEncoding.cs | Reworks the PostEB search logic to check the next hyphen and skip expensive base64 validation in common-degenerate cases. |
| src/libraries/System.Security.Cryptography/tests/PemEncodingFindTests.cs | Adds a stress/regression test exercising many BEGIN headers before a single valid PEM. |
| src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/PemEncoding.cs | Mirrors the PostEB search optimization in the downlevel implementation. |
| src/libraries/Microsoft.Bcl.Cryptography/tests/PemEncodingFindTests.cs | Adds the corresponding downlevel regression test for the degenerate input scenario. |
Review details
Suppressed comments (2)
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/PemEncoding.cs:249
- Typo in comment: "alledged" should be "alleged".
{
// No matches can occur before the alledged post-EB suffix,
// so jump ahead.
src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/PemEncoding.cs:137
- The whitespace-after-PostEB check uses
pemEndIndex < pemData.Length - 1, which skips validating the last character when there is exactly one trailing character after the PostEB. UsepemEndIndex < pemData.Lengthso a single trailing non-whitespace character is rejected consistently.
// The PostEB must either end at the end of the string, or
// have at least one white space character after it.
if (pemEndIndex < pemData.Length - 1 &&
!IsWhiteSpaceCharacter(pemData[pemEndIndex]))
{
- Files reviewed: 4/4 changed files
- Comments generated: 6
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes subtle PEM boundary-search behavior in security-sensitive parsing logic and should get a careful human validation pass over edge cases and perf claims.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/libraries/Microsoft.Bcl.Cryptography/tests/PemEncodingFindTests.cs:473
- The loops that append repeated characters build a very large string (~4MB) one character/chunk at a time, which can make this unit test unnecessarily slow. StringBuilder has repeat-count overloads that avoid the per-iteration overhead while producing the same content.
This issue also appears on line 475 of the same file.
src/libraries/Microsoft.Bcl.Cryptography/tests/PemEncodingFindTests.cs:478
- Building the large base64 section via a loop of
Append("AAAA")does ~1M iterations for a 4MB payload and can slow the test suite. Prefer theAppend(char, int)overload to append the same number of 'A' characters in one call.
for (int i = 0; i < div4; i++)
{
builder.Append("AAAA");
}
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.
vcsjones
left a comment
There was a problem hiding this comment.
Looks good with a non-blocking suggestion.
There was a problem hiding this comment.
🟢 Approval recommended
The updated search strategy appears behavior-preserving for valid PEM while addressing the degenerate-input scenario, and the PR adds targeted regression coverage for the new logic.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
bartonjs
commented
Sep 4, 2026
/ba-g #131990 |
Uh oh!
There was an error while loading. Please reload this page.
bartonjs
commented
Sep 4, 2026
/backport to release/11.0 |
Started backporting to |
bartonjs
commented
Sep 4, 2026
/backport to release/10.0 |
Started backporting to |
#133249) Backport of #133137 to release/11.0 /cc @bartonjs ## Customer Impact - [X] Customer reported - [ ] Found internally When TryFind is given degenerate input, it wastes a lot of CPU time realizing that "-----" is not valid Base64. ## Regression - [ ] Yes - [X] No ## Testing Existing tests say that the new algorithm does not regress functionality, and the new test shows that degenerate inputs are handled well over 10x faster. ## Risk Low, due to test coverage. Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
Rather than seeking for the correct PostEB, just ask if the next hyphen is the PostEB. If not, we can bypass the IsValidBase64 checks.