From f826037acc82288386eb49106b14893b5273ba84 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Sat, 23 Aug 2025 15:30:38 +0200 Subject: [PATCH] Protect IsInst against multi-step inlining -- we can only allow `Box` as the top-level argument, not anywhere within the argument tree. --- .../IL/Instructions/ILInstruction.cs | 4 ++++ ICSharpCode.Decompiler/IL/Instructions/IsInst.cs | 10 +++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/ICSharpCode.Decompiler/IL/Instructions/ILInstruction.cs b/ICSharpCode.Decompiler/IL/Instructions/ILInstruction.cs index d3f32df2e..03c8ee9d8 100644 --- a/ICSharpCode.Decompiler/IL/Instructions/ILInstruction.cs +++ b/ICSharpCode.Decompiler/IL/Instructions/ILInstruction.cs @@ -958,6 +958,10 @@ namespace ICSharpCode.Decompiler.IL /// Unlike `CanInlineIntoSlot`, this is not about descendants of the slot, only about /// whether SetChild(childIndex, newChild) is valid. /// (i.e. this controls whether FindLoadInNext may return the specified slot as a final result) + /// + /// Warning: after newChild is inlined, other nodes may be inlined into newChild's sub-instructions + /// without asking this function again. This means this function is not suitable for protecting + /// a slot from having side effects, use `CanInlineIntoSlot` for that. /// internal virtual bool SatisfiesSlotRestrictionForInlining(int childIndex, ILInstruction newChild) { diff --git a/ICSharpCode.Decompiler/IL/Instructions/IsInst.cs b/ICSharpCode.Decompiler/IL/Instructions/IsInst.cs index 714bfefbf..b06cf5d28 100644 --- a/ICSharpCode.Decompiler/IL/Instructions/IsInst.cs +++ b/ICSharpCode.Decompiler/IL/Instructions/IsInst.cs @@ -25,10 +25,10 @@ namespace ICSharpCode.Decompiler.IL; partial class IsInst { - internal override bool SatisfiesSlotRestrictionForInlining(int childIndex, ILInstruction newChild) + internal override bool CanInlineIntoSlot(int childIndex, ILInstruction newChild) { Debug.Assert(childIndex == 0); - Debug.Assert(base.SatisfiesSlotRestrictionForInlining(childIndex, newChild)); + Debug.Assert(base.CanInlineIntoSlot(childIndex, newChild)); if (this.Type.IsReferenceType == true) { return true; // reference-type isinst is always supported @@ -37,12 +37,16 @@ partial class IsInst { return true; // emulated via "expr is T ? (T)expr : null" } - else if (newChild is Box box && SemanticHelper.IsPure(box.Argument.Flags)) + else if (newChild is Box box && SemanticHelper.IsPure(box.Argument.Flags) && this.Argument.Children.Count == 0) { // Also emulated via "expr is T ? (T)expr : null". // This duplicates the boxing side-effect, but that's harmless as one of the boxes is only // used in the `expr is T` type test where the object identity can never be observed. // This appears as part of C# pattern matching, inlining early makes those code patterns easier to detect. + + // We can only do this if the Box appears directly top-level in the IsInst, we cannot inline Box instructions + // deeper into our Argument subtree. So restricts to the case were the previous argument has no children + // (which means inlining can only replace the argument, not insert within it). return true; } if (this.Parent is UnboxAny unboxAny && ExpressionBuilder.IsUnboxAnyWithIsInst(unboxAny, this.Type))