From 6b78783cb7463ae61624c74e4024b035982ba083 Mon Sep 17 00:00:00 2001
From: Steven Kirk <grokys@users.noreply.github.com>
Date: Wed, 25 Sep 2024 16:45:07 +0200
Subject: [PATCH] Prevent stack overflow in two-way bindings. (#17073)

* Add failing test for #16746

* Always read the value from the target object.

The value must be read from the target object instead of using the value from the event because the value may have changed again between the time the event was raised and now: if that occurs in a two-way binding then we end up with a stack overflow.
---
 .../Data/Core/BindingExpression.cs            |  6 +-
 .../Data/BindingTests.cs                      | 57 +++++++++++++++++++
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/src/Avalonia.Base/Data/Core/BindingExpression.cs b/src/Avalonia.Base/Data/Core/BindingExpression.cs
index abf6f6cb887..635fa3e0960 100644
--- a/src/Avalonia.Base/Data/Core/BindingExpression.cs
+++ b/src/Avalonia.Base/Data/Core/BindingExpression.cs
@@ -512,8 +512,10 @@ private void OnTargetPropertyChanged(object? sender, AvaloniaPropertyChangedEven
         Debug.Assert(_mode is BindingMode.TwoWay or BindingMode.OneWayToSource);
         Debug.Assert(UpdateSourceTrigger is UpdateSourceTrigger.PropertyChanged);
 
-        if (e.Property == TargetProperty)
-            WriteValueToSource(e.NewValue);
+        // The value must be read from the target object instead of using the value from the event
+        // because the value may have changed again between the time the event was raised and now.
+        if (e.Property == TargetProperty && TryGetTarget(out var target))
+            WriteValueToSource(target.GetValue(TargetProperty));
     }
 
     private object? ConvertFallback(object? fallback, string fallbackName)
diff --git a/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs b/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs
index d4a799da6fc..9fb52fee388 100644
--- a/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs
+++ b/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs
@@ -667,6 +667,31 @@ public void OneWayToSource_Binding_Does_Not_Override_TwoWay_Binding()
             Assert.Equal("TwoWay", source.Foo);
         }
 
+        [Fact]
+        public void Target_Undoing_Property_Change_During_TwoWay_Binding_Does_Not_Cause_StackOverflow()
+        {
+            var source = new TestStackOverflowViewModel { BoolValue = true };
+            var target = new TwoWayBindingTest();
+
+            source.ResetSetterInvokedCount();
+
+            // The AlwaysFalse property is set to false in the PropertyChanged callback. Ensure
+            // that binding it to an initial `true` value with a two-way binding does not cause a
+            // stack overflow.
+            target.Bind(
+                TwoWayBindingTest.AlwaysFalseProperty,
+                new Binding(nameof(TestStackOverflowViewModel.BoolValue))
+                {
+                    Mode = BindingMode.TwoWay,
+                });
+
+            target.DataContext = source;
+
+            Assert.Equal(1, source.SetterInvokedCount);
+            Assert.False(source.BoolValue);
+            Assert.False(target.AlwaysFalse);
+        }
+
         private class StyledPropertyClass : AvaloniaObject
         {
             public static readonly StyledProperty<double> DoubleValueProperty =
@@ -725,10 +750,24 @@ private class TestStackOverflowViewModel : INotifyPropertyChanged
 
             public const int MaxInvokedCount = 1000;
 
+            private bool _boolValue;
             private double _value;
 
             public event PropertyChangedEventHandler PropertyChanged;
 
+            public bool BoolValue
+            {
+                get => _boolValue;
+                set
+                {
+                    if (_boolValue != value)
+                    {
+                        _boolValue = value;
+                        SetterInvokedCount++;
+                        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(BoolValue)));
+                    }
+                } }
+
             public double Value
             {
                 get => _value;
@@ -754,20 +793,38 @@ public double Value
                     }
                 }
             }
+
+            public void ResetSetterInvokedCount() => SetterInvokedCount = 0;
         }
 
         private class TwoWayBindingTest : Control
         {
+            public static readonly StyledProperty<bool> AlwaysFalseProperty =
+                AvaloniaProperty.Register<StyledPropertyClass, bool>(nameof(AlwaysFalse));
             public static readonly StyledProperty<string> TwoWayProperty =
                 AvaloniaProperty.Register<TwoWayBindingTest, string>(
                     "TwoWay",
                     defaultBindingMode: BindingMode.TwoWay);
 
+            public bool AlwaysFalse
+            {
+                get => GetValue(AlwaysFalseProperty);
+                set => SetValue(AlwaysFalseProperty, value);
+            }
+
             public string TwoWay
             {
                 get => GetValue(TwoWayProperty);
                 set => SetValue(TwoWayProperty, value);
             }
+
+            protected override void OnPropertyChanged(AvaloniaPropertyChangedEventArgs change)
+            {
+                base.OnPropertyChanged(change);
+
+                if (change.Property == AlwaysFalseProperty)
+                    SetCurrentValue(AlwaysFalseProperty, false);
+            }
         }
 
         public class Source : INotifyPropertyChanged