Skip to content

Commit 3ba6e06

Browse files
authored
Prevent stack overflow in use-stable-resource-identifiers linter (#10001)
* Bail walking symbol values when a cycle is detected * Update cyclic symbol checker not to emit cyclic error message on references that don't participate in the cycle itself.
1 parent 87663da commit 3ba6e06

File tree

5 files changed

+34
-11
lines changed

5 files changed

+34
-11
lines changed

src/Bicep.Core.IntegrationTests/ScenarioTests.cs

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
using System.Diagnostics.CodeAnalysis;
77
using System.Linq;
88
using System.Security.Cryptography;
9-
using Bicep.Core.CodeAction;
109
using Bicep.Core.Diagnostics;
1110
using Bicep.Core.Resources;
1211
using Bicep.Core.TypeSystem;
@@ -1894,7 +1893,7 @@ public void Test_Issue2494()
18941893
{
18951894
("BCP080", DiagnosticLevel.Error, "The expression is involved in a cycle (\"nameCopy\" -> \"name\")."),
18961895
("BCP080", DiagnosticLevel.Error, "The expression is involved in a cycle (\"name\" -> \"nameCopy\")."),
1897-
("BCP080", DiagnosticLevel.Error, "The expression is involved in a cycle (\"name\" -> \"nameCopy\").")
1896+
("BCP062", DiagnosticLevel.Error, "The referenced declaration with name \"name\" is not valid.")
18981897
});
18991898
}
19001899

@@ -4346,8 +4345,28 @@ param CertificateSubjects certificateMapping[]
43464345

43474346
result.Should().GenerateATemplate();
43484347

4349-
var evaluated = TemplateEvaluator.Evaluate(result.Template, parameters);
4348+
var evaluated = TemplateEvaluator.Evaluate(result.Template, parameters);
43504349
evaluated.Should().HaveValueAtPath("$.outputs['vaultId'].value", "/subscriptions/mySub/resourceGroups/myRg/providers/Microsoft.KeyVault/vaults/myKv");
43514350
}
4351+
4352+
// https://github.com/Azure/bicep/issues/9978
4353+
[TestMethod]
4354+
public void Test_Issue9978()
4355+
{
4356+
var result = CompilationHelper.Compile(@"
4357+
param foo string = guid(foo)
4358+
4359+
#disable-next-line no-unused-existing-resources
4360+
resource asdf 'Microsoft.Storage/storageAccounts@2022-09-01' existing = {
4361+
name: foo
4362+
}
4363+
");
4364+
4365+
result.Should().HaveDiagnostics(new[]
4366+
{
4367+
("BCP079", DiagnosticLevel.Error, "This expression is referencing its own declaration, which is not allowed."),
4368+
("BCP062", DiagnosticLevel.Error, "The referenced declaration with name \"foo\" is not valid."),
4369+
});
4370+
}
43524371
}
43534372
}

src/Bicep.Core.Samples/Files/InvalidCycles_CRLF/main.diagnostics.bicep

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ var two = one.f
5252
// #completionTest(17) -> empty
5353
var twotwo = one.
5454
//@[04:10) [no-unused-vars (Warning)] Variable "twotwo" is declared but never used. (CodeDescription: bicep core(https://aka.ms/bicep/linter/no-unused-vars)) |twotwo|
55-
//@[13:16) [BCP080 (Error)] The expression is involved in a cycle ("one" -> "two"). (CodeDescription: none) |one|
55+
//@[13:16) [BCP062 (Error)] The referenced declaration with name "one" is not valid. (CodeDescription: none) |one|
5656
//@[17:17) [BCP020 (Error)] Expected a function or property name at this location. (CodeDescription: none) ||
5757

5858
// resource completion cycles

src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/UseResourceIdFunctionsRuleTests.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1047,7 +1047,7 @@ param idValue1 string
10471047
"[2:26] The expression is involved in a cycle (\"v3\" -> \"v2\" -> \"v1\").",
10481048
"[3:26] The expression is involved in a cycle (\"v1\" -> \"v3\" -> \"v2\").",
10491049
"[4:26] The expression is involved in a cycle (\"v2\" -> \"v1\" -> \"v3\").",
1050-
"[9:25] The expression is involved in a cycle (\"v3\" -> \"v2\" -> \"v1\").",
1050+
"[9:25] The referenced declaration with name \"v3\" is not valid.",
10511051
},
10521052
DisplayName = "resolved variable cycle should not hang")]
10531053
[DataRow(@"

src/Bicep.Core/Analyzers/Linter/Rules/UseStableResourceIdentifiersRule.cs

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ public override void VisitVariableAccessSyntax(VariableAccessSyntax syntax)
7474
{
7575
switch (model.GetSymbolInfo(syntax))
7676
{
77+
case Symbol symbol when pathSegments.Contains(symbol):
78+
// Symbol cycles are reported on elsewhere. As far as this visitor is concerned, a cycle does not introduce nondeterminism.
79+
break;
7780
case ParameterSymbol @parameter:
7881
if (@parameter.DeclaringParameter.Modifier is ParameterDefaultValueSyntax defaultValueSyntax)
7982
{
@@ -83,12 +86,6 @@ public override void VisitVariableAccessSyntax(VariableAccessSyntax syntax)
8386
}
8487
break;
8588
case VariableSymbol @variable:
86-
// Variable cycles are reported on elsewhere. As far as this visitor is concerned, a cycle does not introduce nondeterminism.
87-
if (pathSegments.Contains(@variable))
88-
{
89-
return;
90-
}
91-
9289
pathSegments.AddLast(@variable);
9390
@variable.DeclaringVariable.Value.Accept(this);
9491
pathSegments.RemoveLast();

src/Bicep.Core/TypeSystem/TypeAssignmentVisitor.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,13 @@ private void AssignTypeWithDiagnostics(SyntaxBase syntax, Func<IDiagnosticWriter
111111
if (this.binder.TryGetCycle(declaredSymbol) is { } cycle)
112112
{
113113
// there's a cycle. stop visiting now or we never will!
114+
115+
if (!cycle.Any(symbol => this.binder.IsDescendant(syntax, symbol.DeclaringSyntax)))
116+
{
117+
// the supplied syntax is not part of the cycle, just a reference to the cyclic symbol.
118+
return ErrorType.Create(DiagnosticBuilder.ForPosition(syntax).ReferencedSymbolHasErrors(declaredSymbol.Name));
119+
}
120+
114121
if (cycle.Length == 1)
115122
{
116123
return ErrorType.Create(DiagnosticBuilder.ForPosition(syntax).CyclicExpressionSelfReference());

0 commit comments

Comments
 (0)