From 15719eedd67254091cffe994d0ee7257e8618c6d Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Sun, 12 Jul 2026 20:39:22 +0200 Subject: [PATCH] Reject negative char index in the length-and-char string switch MatchSwitchOnCharBlock's case 2 and default paths guarded against a negative character index, but case 1 (a bare switch on get_Chars) did not. Crafted IL whose get_Chars/get_Item index is negative - a value no compiler emits, but valid IL - therefore reached the pattern reconstruction unchecked. For a length-1 group this silently miscompiled the switch (it rebuilds the string switch from the char labels without using the index), turning IL that reads s[-1] into `switch (s)`; for longer strings it threw IndexOutOfRangeException and aborted the method. Move the check into MatchGetChars so all three call sites reject a negative index by construction, and drop the two now-redundant guards. Same class of unvalidated-integer robustness issue as #3878, in a different switch-on-string pattern. Assisted-by: Claude:claude-opus-4-8:Claude Code --- .../ILPrettyTestRunner.cs | 6 ++ .../SwitchOnStringNegativeCharIndex.cs | 36 +++++++ .../SwitchOnStringNegativeCharIndex.il | 94 +++++++++++++++++++ .../IL/Transforms/SwitchOnStringTransform.cs | 8 +- 4 files changed, 138 insertions(+), 6 deletions(-) create mode 100644 ICSharpCode.Decompiler.Tests/TestCases/ILPretty/SwitchOnStringNegativeCharIndex.cs create mode 100644 ICSharpCode.Decompiler.Tests/TestCases/ILPretty/SwitchOnStringNegativeCharIndex.il diff --git a/ICSharpCode.Decompiler.Tests/ILPrettyTestRunner.cs b/ICSharpCode.Decompiler.Tests/ILPrettyTestRunner.cs index f4d097e69..4fd52fa0a 100644 --- a/ICSharpCode.Decompiler.Tests/ILPrettyTestRunner.cs +++ b/ICSharpCode.Decompiler.Tests/ILPrettyTestRunner.cs @@ -267,6 +267,12 @@ namespace ICSharpCode.Decompiler.Tests await Run(); } + [Test] + public async Task SwitchOnStringNegativeCharIndex() + { + await Run(); + } + [Test] public async Task ConstantBlobs() { diff --git a/ICSharpCode.Decompiler.Tests/TestCases/ILPretty/SwitchOnStringNegativeCharIndex.cs b/ICSharpCode.Decompiler.Tests/TestCases/ILPretty/SwitchOnStringNegativeCharIndex.cs new file mode 100644 index 000000000..a6c3aa025 --- /dev/null +++ b/ICSharpCode.Decompiler.Tests/TestCases/ILPretty/SwitchOnStringNegativeCharIndex.cs @@ -0,0 +1,36 @@ +namespace ICSharpCode.Decompiler.Tests.TestCases.ILPretty +{ + internal class SwitchOnStringNegativeCharIndex + { + public static int M(string s) + { + if (s != null) + { + int length = s.Length; + if (length == 1) + { + switch (s[-1]) + { + case 'a': + return 1; + case 'b': + return 2; + case 'c': + return 3; + case 'd': + return 4; + case 'e': + return 5; + case 'f': + return 6; + case 'g': + return 7; + case 'h': + return 8; + } + } + } + return 0; + } + } +} diff --git a/ICSharpCode.Decompiler.Tests/TestCases/ILPretty/SwitchOnStringNegativeCharIndex.il b/ICSharpCode.Decompiler.Tests/TestCases/ILPretty/SwitchOnStringNegativeCharIndex.il new file mode 100644 index 000000000..2dfa80fd0 --- /dev/null +++ b/ICSharpCode.Decompiler.Tests/TestCases/ILPretty/SwitchOnStringNegativeCharIndex.il @@ -0,0 +1,94 @@ +// Regression fixture: real Roslyn length-1 "switch on string" codegen (switch on s[0]) with the +// get_Chars index edited to -1 - a value no compiler emits. Without a negative-index guard the +// transform silently miscompiles this to `switch (s)` even though the IL reads s[-1]; the fixed +// transform declines and preserves the raw `switch (s[-1])`. + +.assembly extern mscorlib +{ + .publickeytoken = (B7 7A 5C 56 19 34 E0 89 ) + .ver 4:0:0:0 +} +.assembly SwitchOnStringNegativeCharIndex +{ + .hash algorithm 0x00008004 + .ver 1:0:0:0 +} +.module SwitchOnStringNegativeCharIndex.dll + +.class private auto ansi beforefieldinit ICSharpCode.Decompiler.Tests.TestCases.ILPretty.SwitchOnStringNegativeCharIndex + extends [mscorlib]System.Object +{ + .method public hidebysig static int32 M(string s) cil managed + { + + .maxstack 2 + .locals init (int32 V_0, + char V_1) + IL_0000: ldarg.0 + IL_0001: brfalse.s IL_0051 + + IL_0003: ldarg.0 + IL_0004: call instance int32 [mscorlib]System.String::get_Length() + IL_0009: stloc.0 + IL_000a: ldloc.0 + IL_000b: ldc.i4.1 + IL_000c: bne.un.s IL_0051 + + IL_000e: ldarg.0 + IL_000f: ldc.i4.m1 + IL_0010: call instance char [mscorlib]System.String::get_Chars(int32) + IL_0015: stloc.1 + IL_0016: ldloc.1 + IL_0017: ldc.i4.s 97 + IL_0019: sub + IL_001a: switch ( + IL_0041, + IL_0043, + IL_0045, + IL_0047, + IL_0049, + IL_004b, + IL_004d, + IL_004f) + IL_003f: br.s IL_0051 + + IL_0041: ldc.i4.1 + IL_0042: ret + + IL_0043: ldc.i4.2 + IL_0044: ret + + IL_0045: ldc.i4.3 + IL_0046: ret + + IL_0047: ldc.i4.4 + IL_0048: ret + + IL_0049: ldc.i4.5 + IL_004a: ret + + IL_004b: ldc.i4.6 + IL_004c: ret + + IL_004d: ldc.i4.7 + IL_004e: ret + + IL_004f: ldc.i4.8 + IL_0050: ret + + IL_0051: ldc.i4.0 + IL_0052: ret + } // end of method M + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void [mscorlib]System.Object::.ctor() + IL_0006: ret + } // end of method .ctor + +} // end of class SwitchOnStringNegativeCharIndex + diff --git a/ICSharpCode.Decompiler/IL/Transforms/SwitchOnStringTransform.cs b/ICSharpCode.Decompiler/IL/Transforms/SwitchOnStringTransform.cs index df75cb5e4..9e650100a 100644 --- a/ICSharpCode.Decompiler/IL/Transforms/SwitchOnStringTransform.cs +++ b/ICSharpCode.Decompiler/IL/Transforms/SwitchOnStringTransform.cs @@ -1354,7 +1354,7 @@ namespace ICSharpCode.Decompiler.IL.Transforms || call.Method.FullNameIs("System.Span", "get_Item")) && call.Arguments.Count == 2 && call.Arguments[0].MatchLdLoca(switchValueVar) - && call.Arguments[1].MatchLdcI4(out index); + && call.Arguments[1].MatchLdcI4(out index) && index >= 0; } else { @@ -1362,7 +1362,7 @@ namespace ICSharpCode.Decompiler.IL.Transforms && call.Method.FullNameIs("System.String", "get_Chars") && call.Arguments.Count == 2 && call.Arguments[0].MatchLdLoc(switchValueVar) - && call.Arguments[1].MatchLdcI4(out index); + && call.Arguments[1].MatchLdcI4(out index) && index >= 0; } } @@ -1390,8 +1390,6 @@ namespace ICSharpCode.Decompiler.IL.Transforms return false; if (!MatchGetChars(getCharsCall, switchValueVar, out index)) return false; - if (index < 0) - return false; @switch = block.Instructions[1] as SwitchInstruction; if (@switch == null) return false; @@ -1409,8 +1407,6 @@ namespace ICSharpCode.Decompiler.IL.Transforms return false; if (!MatchGetChars(getCharsCall, switchValueVar, out index)) return false; - if (index < 0) - return false; if (analysis.SwitchVariable != charTempVar) return false; sections = analysis.Sections;