From bf7ef615a07a43a31ea37e5a5b8b68bbd5a5680b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Mon, 12 Aug 2024 16:37:43 +0200 Subject: [PATCH] Add memory barries to writes at the end of a lock There could have been a race condition on systems with weak memory ordering: Another thread can read the result value before we exit the critical section. Without a strong memory ordering, the pointer to the result value might get written before the value itself. The other thread might therefore read zeroed-out memory. --- src/Framework/Framework/Binding/DotvvmProperty.cs | 6 +++++- .../Framework/Binding/Expressions/BindingExpression.cs | 2 +- .../Compilation/ControlTree/DotvvmPropertyGroup.cs | 8 ++++++-- .../Framework/Compilation/ExtensionMethodsCache.cs | 1 - .../Compilation/Styles/ResolvedControlHelper.cs | 2 ++ .../Framework/Runtime/Caching/SimpleLruDictionary.cs | 10 ++++++++-- 6 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/Framework/Framework/Binding/DotvvmProperty.cs b/src/Framework/Framework/Binding/DotvvmProperty.cs index 8eb7261768..aaa21563e1 100644 --- a/src/Framework/Framework/Binding/DotvvmProperty.cs +++ b/src/Framework/Framework/Binding/DotvvmProperty.cs @@ -115,7 +115,11 @@ internal void AddUsedInCapability(DotvvmCapabilityProperty? p) if (p is object) lock(this) { - UsedInCapabilities = UsedInCapabilities.Add(p); + if (UsedInCapabilities.Contains(p)) return; + + var newArray = UsedInCapabilities.Add(p); + Thread.MemoryBarrier(); // make sure the array is complete before we let other threads use it lock-free + UsedInCapabilities = newArray; } } diff --git a/src/Framework/Framework/Binding/Expressions/BindingExpression.cs b/src/Framework/Framework/Binding/Expressions/BindingExpression.cs index c84e512c8b..3c38a0229c 100644 --- a/src/Framework/Framework/Binding/Expressions/BindingExpression.cs +++ b/src/Framework/Framework/Binding/Expressions/BindingExpression.cs @@ -260,7 +260,7 @@ protected void AddNullResolvers() - string? toStringValue; + volatile string? toStringValue; public override string ToString() { if (toStringValue is null) diff --git a/src/Framework/Framework/Compilation/ControlTree/DotvvmPropertyGroup.cs b/src/Framework/Framework/Compilation/ControlTree/DotvvmPropertyGroup.cs index 2fa03d4137..96c84f10cb 100644 --- a/src/Framework/Framework/Compilation/ControlTree/DotvvmPropertyGroup.cs +++ b/src/Framework/Framework/Compilation/ControlTree/DotvvmPropertyGroup.cs @@ -9,6 +9,7 @@ using DotVVM.Framework.Utils; using System.Runtime.CompilerServices; using System.Collections.Immutable; +using System.Threading; namespace DotVVM.Framework.Compilation.ControlTree { @@ -81,8 +82,11 @@ internal void AddUsedInCapability(DotvvmCapabilityProperty? p) if (p is object) lock(this) { - if (!UsedInCapabilities.Contains(p)) - UsedInCapabilities = UsedInCapabilities.Add(p); + if (UsedInCapabilities.Contains(p)) return; + + var newArray = UsedInCapabilities.Add(p); + Thread.MemoryBarrier(); + UsedInCapabilities = newArray; } } diff --git a/src/Framework/Framework/Compilation/ExtensionMethodsCache.cs b/src/Framework/Framework/Compilation/ExtensionMethodsCache.cs index f40859f1da..282304f212 100644 --- a/src/Framework/Framework/Compilation/ExtensionMethodsCache.cs +++ b/src/Framework/Framework/Compilation/ExtensionMethodsCache.cs @@ -38,7 +38,6 @@ public IEnumerable GetExtensionsForNamespaces(string[] @namespaces) // it's most likely the same namespaces, so it won't help at all - only run into lock contention in System.Reflection lock (methodsCache) { - results = namespaces.Select(x => methodsCache.GetValueOrDefault(x)).ToArray(); var missingNamespaces = namespaces.Where(x => !methodsCache.ContainsKey(x)).ToArray(); var createdNamespaces = CreateExtensionsForNamespaces(missingNamespaces); diff --git a/src/Framework/Framework/Compilation/Styles/ResolvedControlHelper.cs b/src/Framework/Framework/Compilation/Styles/ResolvedControlHelper.cs index 51669e764b..8eb3f9f034 100644 --- a/src/Framework/Framework/Compilation/Styles/ResolvedControlHelper.cs +++ b/src/Framework/Framework/Compilation/Styles/ResolvedControlHelper.cs @@ -19,6 +19,7 @@ using DotVVM.Framework.Compilation.ViewCompiler; using DotVVM.Framework.Runtime; using FastExpressionCompiler; +using System.Threading; namespace DotVVM.Framework.Compilation.Styles { @@ -417,6 +418,7 @@ void InitializeChildren(IDotvvmRequestContext? context) var services = context?.Services ?? this.services; Children.Add((DotvvmControl)ResolvedControl.ToRuntimeControl(services)); + Thread.MemoryBarrier(); // make sure write to Children is done before we let other threads read without lock initialized = true; } } diff --git a/src/Framework/Framework/Runtime/Caching/SimpleLruDictionary.cs b/src/Framework/Framework/Runtime/Caching/SimpleLruDictionary.cs index c227fefac6..5d7f39e5bb 100644 --- a/src/Framework/Framework/Runtime/Caching/SimpleLruDictionary.cs +++ b/src/Framework/Framework/Runtime/Caching/SimpleLruDictionary.cs @@ -20,9 +20,9 @@ public class SimpleLruDictionary { // concurrencyLevel: 1, we don't write in parallel anyway // new generation - private ConcurrentDictionary hot = new ConcurrentDictionary(concurrencyLevel: 1, capacity: 1); + private volatile ConcurrentDictionary hot = new ConcurrentDictionary(concurrencyLevel: 1, capacity: 1); // old generation - private ConcurrentDictionary cold = new ConcurrentDictionary(concurrencyLevel: 1, capacity: 1); + private volatile ConcurrentDictionary cold = new ConcurrentDictionary(concurrencyLevel: 1, capacity: 1); // free to take for GC. however, if the GC does not want to collect, we can still use it private readonly ConcurrentDictionary> dead = new ConcurrentDictionary>(concurrencyLevel: 1, capacity: 1); private TimeSpan lastCleanupTime = TimeSpan.MinValue; @@ -148,6 +148,12 @@ public bool Remove(TKey key, out TValue oldValue) oldValue = deadValue; r = true; } + if (hot.TryRemove(key, out hotValue)) + { + // hot again, it could have been added back in the meantime + oldValue = hotValue; + r = true; + } return r; } }