From ba472fc3f07e0d44c8f856dda2869a7c6d2a1738 Mon Sep 17 00:00:00 2001 From: GrahamTheCoder Date: Sun, 10 Dec 2023 12:14:53 +0000 Subject: [PATCH] Avoid stack overflow in stack overflow prevention code - fixes #1047 --- CHANGELOG.md | 2 ++ CodeConverter/CSharp/ExpressionNodeVisitor.cs | 35 ++++++++++++------- .../ExpressionTests/BinaryExpressionTests.cs | 24 +++++++++++++ 3 files changed, 48 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc1ffe647..e412c83a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ### VB -> C# +* Avoid stack overflow in stack overflow prevention code [#1047](https://github.com/icsharpcode/CodeConverter/issues/1047) +* Convert optional DateTime parameters [#1056](https://github.com/icsharpcode/CodeConverter/issues/1056) * Convert optional parameters before ref parameters using attributes to avoid compile error [#1057](https://github.com/icsharpcode/CodeConverter/issues/1057) * Remove square brackets when escaping labels and identifiers [#1043](https://github.com/icsharpcode/CodeConverter/issues/1043) and [#1044](https://github.com/icsharpcode/CodeConverter/issues/1044) * Exit Property now returns value assigned to return variable [#1051](https://github.com/icsharpcode/CodeConverter/issues/1051) diff --git a/CodeConverter/CSharp/ExpressionNodeVisitor.cs b/CodeConverter/CSharp/ExpressionNodeVisitor.cs index 38f62a765..b24e7d320 100644 --- a/CodeConverter/CSharp/ExpressionNodeVisitor.cs +++ b/CodeConverter/CSharp/ExpressionNodeVisitor.cs @@ -2,6 +2,7 @@ using System.Data; using System.Globalization; using System.Linq.Expressions; +using System.Runtime.CompilerServices; using System.Xml.Linq; using ICSharpCode.CodeConverter.CSharp.Replacements; using ICSharpCode.CodeConverter.Util.FromRoslyn; @@ -845,30 +846,35 @@ private CSharpSyntaxNode ConvertAddressOf(VBSyntax.UnaryExpressionSyntax node, E return expr; } - public override async Task VisitBinaryExpression(VBasic.Syntax.BinaryExpressionSyntax node) + public override async Task VisitBinaryExpression(VBasic.Syntax.BinaryExpressionSyntax entryNode) { - // Manually walk tree bottom up to avoid stack overflow for deeply nested binary expressions like 3 + 4 + 5 + ... + // Walk down the syntax tree for deeply nested binary expressions to avoid stack overflow + // e.g. 3 + 4 + 5 + ... // Test "DeeplyNestedBinaryExpressionShouldNotStackOverflowAsync()" skipped because it's too slow - while (node.Left is VBSyntax.BinaryExpressionSyntax l) { - node = l; - } ExpressionSyntax csLhs = null; - for (; node is not null; node = BinaryParentWithLhs(node)) { - csLhs = (ExpressionSyntax) await ConvertBinaryExpressionAsync(node, csLhs); + int levelsToConvert = 0; + VBSyntax.BinaryExpressionSyntax currentNode = entryNode; + + // Walk down the nested levels to count them + for (var nextNode = entryNode; nextNode != null; currentNode = nextNode, nextNode = currentNode.Left as VBSyntax.BinaryExpressionSyntax, levelsToConvert++) { + // Don't go beyond a rewritten operator because that code has many paths that can call VisitBinaryExpression. Passing csLhs through all of that would harm the code quality more than it's worth to help that edge case. + if (await RewriteBinaryOperatorOrNullAsync(nextNode) is { } operatorNode) { + csLhs = operatorNode; + break; + } } - return csLhs; + // Walk back up the same levels converting as we go. + for (; levelsToConvert > 0; currentNode = currentNode!.Parent as VBSyntax.BinaryExpressionSyntax, levelsToConvert--) { + csLhs = (ExpressionSyntax)await ConvertBinaryExpressionAsync(currentNode, csLhs); + } - VBSyntax.BinaryExpressionSyntax BinaryParentWithLhs(SyntaxNode currentSyntaxNode) => currentSyntaxNode.Parent is VBSyntax.BinaryExpressionSyntax {Left: var l} p && l == currentSyntaxNode ? p : null; + return csLhs; } private async Task ConvertBinaryExpressionAsync(VBasic.Syntax.BinaryExpressionSyntax node, ExpressionSyntax lhs = null, ExpressionSyntax rhs = null) { - if (await _operatorConverter.ConvertRewrittenBinaryOperatorOrNullAsync(node, TriviaConvertingExpressionVisitor.IsWithinQuery) is { } operatorNode) { - return operatorNode; - } - lhs ??= await node.Left.AcceptAsync(TriviaConvertingExpressionVisitor); rhs ??= await node.Right.AcceptAsync(TriviaConvertingExpressionVisitor); @@ -926,6 +932,9 @@ private async Task ConvertBinaryExpressionAsync(VBasic.Syntax. return node.Parent.IsKind(VBasic.SyntaxKind.SimpleArgument) ? exp : exp.AddParens(); } + private async Task RewriteBinaryOperatorOrNullAsync(VBSyntax.BinaryExpressionSyntax node) => + await _operatorConverter.ConvertRewrittenBinaryOperatorOrNullAsync(node, TriviaConvertingExpressionVisitor.IsWithinQuery); + private async Task WithRemovedRedundantConversionOrNullAsync(VBSyntax.InvocationExpressionSyntax conversionNode, ISymbol invocationSymbol) { if (invocationSymbol?.ContainingNamespace.MetadataName != nameof(Microsoft.VisualBasic) || diff --git a/Tests/CSharp/ExpressionTests/BinaryExpressionTests.cs b/Tests/CSharp/ExpressionTests/BinaryExpressionTests.cs index bd9111220..9240c8f5e 100644 --- a/Tests/CSharp/ExpressionTests/BinaryExpressionTests.cs +++ b/Tests/CSharp/ExpressionTests/BinaryExpressionTests.cs @@ -724,6 +724,30 @@ public void Foo() }"); } + [Fact] + public async Task RewrittenObjectOperatorDoesNotStackOverflowAsync() + { + await TestConversionVisualBasicToCSharpAsync(@" +Sub S() + Dim a As Object + Dim b As Object + If (1=Integer.Parse(""1"") And b And a = 1) Then + Else If (a = 1 Or b Or a = 2 Or a = 3) Then + End If +End Sub", @" +public void S() +{ + var a = default(object); + var b = default(object); + if (Conversions.ToBoolean(Operators.AndObject(Operators.AndObject(1 == int.Parse(""1""), b), Operators.ConditionalCompareObjectEqual(a, 1, false)))) + { + } + else if (Conversions.ToBoolean(Operators.OrObject(Operators.OrObject(Operators.OrObject(Operators.ConditionalCompareObjectEqual(a, 1, false), b), Operators.ConditionalCompareObjectEqual(a, 2, false)), Operators.ConditionalCompareObjectEqual(a, 3, false)))) + { + } +}"); + } + [Fact(Skip = "Too slow")] public async Task DeeplyNestedBinaryExpressionShouldNotStackOverflowAsync() {