Skip to content

Commit 5ec43e0

Browse files
github-actions[bot]tarekghlewing
authored
[release/9.0] Fix stack overflow in the configuration source gen (#106668)
* Fix StackOverFlow in the Logging source gen * Address the feedback * Avoid static field --------- Co-authored-by: Tarek Mahmoud Sayed <tarekms@microsoft.com> Co-authored-by: Larry Ewing <lewing@microsoft.com>
1 parent 4bd35a1 commit 5ec43e0

File tree

4 files changed

+141
-14
lines changed

4 files changed

+141
-14
lines changed

‎src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs

+44-12
Original file line numberDiff line numberDiff line change
@@ -552,7 +552,9 @@ private bool IsAssignableTo(ITypeSymbol source, ITypeSymbol dest)
552552
return conversion.IsReference && conversion.IsImplicit;
553553
}
554554

555-
private bool IsUnsupportedType(ITypeSymbol type)
555+
private HashSet<ITypeSymbol>? _visitedTypes = new(SymbolEqualityComparer.Default);
556+
557+
private bool IsUnsupportedType(ITypeSymbol type, HashSet<ITypeSymbol>? visitedTypes = null)
556558
{
557559
if (type.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T)
558560
{
@@ -569,25 +571,55 @@ private bool IsUnsupportedType(ITypeSymbol type)
569571
return true;
570572
}
571573

572-
if (type is IArrayTypeSymbol arrayTypeSymbol)
574+
if (visitedTypes?.Contains(type) is true)
573575
{
574-
return arrayTypeSymbol.Rank > 1 || IsUnsupportedType(arrayTypeSymbol.ElementType);
576+
// avoid infinite recursion in nested types like
577+
// public record RecursiveType
578+
// {
579+
// public TreeElement? Tree { get; set; }
580+
// }
581+
// public sealed class TreeElement : Dictionary<string, TreeElement>;
582+
//
583+
// return false for the second call. The type will continue be checked in the first call anyway.
584+
return false;
575585
}
576586

577-
if (IsCollection(type))
587+
IArrayTypeSymbol? arrayTypeSymbol = type as IArrayTypeSymbol;
588+
if (arrayTypeSymbol is null)
578589
{
579-
INamedTypeSymbol collectionType = (INamedTypeSymbol)type;
580-
581-
if (IsCandidateDictionary(collectionType, out ITypeSymbol? keyType, out ITypeSymbol? elementType))
582-
{
583-
return IsUnsupportedType(keyType) || IsUnsupportedType(elementType);
584-
}
585-
else if (TryGetElementType(collectionType, out elementType))
590+
if (!IsCollection(type))
586591
{
587-
return IsUnsupportedType(elementType);
592+
return false;
588593
}
589594
}
590595

596+
if (visitedTypes is null)
597+
{
598+
visitedTypes = _visitedTypes;
599+
visitedTypes.Clear();
600+
}
601+
602+
visitedTypes.Add(type);
603+
604+
if (arrayTypeSymbol is not null)
605+
{
606+
return arrayTypeSymbol.Rank > 1 || IsUnsupportedType(arrayTypeSymbol.ElementType, visitedTypes);
607+
}
608+
609+
Debug.Assert(IsCollection(type));
610+
611+
INamedTypeSymbol collectionType = (INamedTypeSymbol)type;
612+
613+
if (IsCandidateDictionary(collectionType, out ITypeSymbol? keyType, out ITypeSymbol? elementType))
614+
{
615+
return IsUnsupportedType(keyType, visitedTypes) || IsUnsupportedType(elementType, visitedTypes);
616+
}
617+
618+
if (TryGetElementType(collectionType, out elementType))
619+
{
620+
return IsUnsupportedType(elementType, visitedTypes);
621+
}
622+
591623
return false;
592624
}
593625

‎src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/BindingHelperInfo.cs

+7-2
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,9 @@ bool TryRegisterCore()
131131
}
132132
case DictionarySpec dictionarySpec:
133133
{
134+
// Base case to avoid stack overflow for recursive object graphs.
135+
_seenTransitiveTypes.Add(typeRef, true);
136+
134137
bool shouldRegister = _typeIndex.CanBindTo(typeRef) &&
135138
TryRegisterTransitiveTypesForMethodGen(dictionarySpec.KeyTypeRef) &&
136139
TryRegisterTransitiveTypesForMethodGen(dictionarySpec.ElementTypeRef) &&
@@ -145,6 +148,9 @@ bool TryRegisterCore()
145148
}
146149
case CollectionSpec collectionSpec:
147150
{
151+
// Base case to avoid stack overflow for recursive object graphs.
152+
_seenTransitiveTypes.Add(typeRef, true);
153+
148154
if (_typeIndex.GetTypeSpec(collectionSpec.ElementTypeRef) is ComplexTypeSpec)
149155
{
150156
_namespaces.Add("System.Linq");
@@ -157,8 +163,7 @@ bool TryRegisterCore()
157163
{
158164
// Base case to avoid stack overflow for recursive object graphs.
159165
// Register all object types for gen; we need to throw runtime exceptions in some cases.
160-
bool shouldRegister = true;
161-
_seenTransitiveTypes.Add(typeRef, shouldRegister);
166+
_seenTransitiveTypes.Add(typeRef, true);
162167

163168
// List<string> is used in generated code as a temp holder for formatting
164169
// an error for config properties that don't map to object properties.

‎src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs

+9
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,15 @@ public string Color
189189
}
190190
}
191191

192+
public sealed class TreeElement : Dictionary<string, TreeElement>;
193+
194+
public record TypeWithRecursionThroughCollections
195+
{
196+
public TreeElement? Tree { get; set; }
197+
public TreeElement?[]? Flat { get; set; }
198+
public List<TreeElement>? List { get; set; }
199+
}
200+
192201
public record RecordWithArrayParameter(string[] Array);
193202

194203
public readonly record struct ReadonlyRecordStructTypeOptions(string Color, int Length);

‎src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs

+81
Original file line numberDiff line numberDiff line change
@@ -1544,6 +1544,87 @@ public void CanBindOnParametersAndProperties_RecordWithArrayConstructorParameter
15441544
Assert.Equal(new string[] { "a", "b", "c" }, options.Array);
15451545
}
15461546

1547+
/// <summary>
1548+
/// Test binding to recursive types using Dictionary or Collections.
1549+
/// This ensure no stack overflow will occur during the compilation through the source gen or at runtime.
1550+
/// </summary>
1551+
[Fact]
1552+
public void BindToRecursiveTypesTest()
1553+
{
1554+
string jsonConfig = @"{
1555+
""Tree"": {
1556+
""Branch1"": {
1557+
""Leaf1"": {},
1558+
""Leaf2"": {}
1559+
},
1560+
""Branch2"": {
1561+
""Leaf3"": {}
1562+
}
1563+
},
1564+
""Flat"": [
1565+
{
1566+
""Element1"": {
1567+
""SubElement1"": {}
1568+
}
1569+
},
1570+
{
1571+
""Element2"": {
1572+
""SubElement2"": {}
1573+
}
1574+
},
1575+
{
1576+
""Element3"": {}
1577+
}
1578+
],
1579+
""List"": [
1580+
{
1581+
""Item1"": {
1582+
""NestedItem1"": {}
1583+
}
1584+
},
1585+
{
1586+
""Item2"": {}
1587+
},
1588+
]
1589+
}";
1590+
1591+
var configuration = new ConfigurationBuilder()
1592+
.AddJsonStream(new System.IO.MemoryStream(System.Text.Encoding.UTF8.GetBytes(jsonConfig)))
1593+
.Build();
1594+
1595+
var instance = new TypeWithRecursionThroughCollections();
1596+
configuration.Bind(instance);
1597+
1598+
// Validate the dictionary
1599+
Assert.NotNull(instance.Tree);
1600+
Assert.Equal(2, instance.Tree.Count);
1601+
Assert.NotNull(instance.Tree["Branch1"]);
1602+
Assert.Equal(2, instance.Tree["Branch1"].Count);
1603+
Assert.Equal(["Leaf1", "Leaf2"], instance.Tree["Branch1"].Keys);
1604+
Assert.Equal(["Leaf3"], instance.Tree["Branch2"].Keys);
1605+
1606+
// Validate the array
1607+
Assert.NotNull(instance.Flat);
1608+
Assert.Equal(3, instance.Flat.Length);
1609+
Assert.Equal(["Element1"], instance.Flat[0].Keys);
1610+
Assert.Equal(["Element2"], instance.Flat[1].Keys);
1611+
Assert.Equal(["Element3"], instance.Flat[2].Keys);
1612+
Assert.Equal(1, instance.Flat[0].Values.Count);
1613+
Assert.Equal(["SubElement1"], instance.Flat[0].Values.ToArray()[0].Keys);
1614+
Assert.Equal(1, instance.Flat[1].Values.Count);
1615+
Assert.Equal(["SubElement2"], instance.Flat[1].Values.ToArray()[0].Keys);
1616+
Assert.Equal(1, instance.Flat[2].Values.Count);
1617+
1618+
// Validate the List
1619+
Assert.NotNull(instance.Flat);
1620+
Assert.Equal(2, instance.List.Count);
1621+
Assert.Equal(["Item1"], instance.List[0].Keys);
1622+
Assert.Equal(["Item2"], instance.List[1].Keys);
1623+
Assert.Equal(1, instance.List[0].Values.Count);
1624+
Assert.Equal(["NestedItem1"], instance.List[0].Values.ToArray()[0].Keys);
1625+
Assert.Equal(1, instance.List[1].Values.Count);
1626+
}
1627+
15471628
[Fact]
15481629
public void CanBindReadonlyRecordStructOptions()
15491630
{

0 commit comments

Comments
 (0)