From 67c7faf279523cbe882fefed04e08ecdd16886dd Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Tue, 28 Sep 2010 13:29:21 +0200 Subject: [PATCH] [asp.net] Get rid of a possible (although unlikely) race condition when acquiring locks The patch removes a small race condition where a boolean flag is set after acquiring a lock to indicate to code executing in the finally {} block that it should release the lock. The boolean variable is now removed and the lock is released unconditionally. It carries a potential to throw an exception when the lock is not held, but it's better than to fail to release it and lead the application to a deadlock. --- .../System.Web/System.Web.Caching/Cache.cs | 103 ++++++++---------- .../CacheItemPriorityQueue.cs | 20 ++-- .../System.Web.Compilation/BuildManager.cs | 27 ++--- .../WebConfigurationManager.cs | 22 +--- 4 files changed, 67 insertions(+), 105 deletions(-) diff --git a/mcs/class/System.Web/System.Web.Caching/Cache.cs b/mcs/class/System.Web/System.Web.Caching/Cache.cs index 37462aedcd2..abbd2d01dd8 100644 --- a/mcs/class/System.Web/System.Web.Caching/Cache.cs +++ b/mcs/class/System.Web/System.Web.Caching/Cache.cs @@ -42,6 +42,21 @@ namespace System.Web.Caching { public static readonly DateTime NoAbsoluteExpiration = DateTime.MaxValue; public static readonly TimeSpan NoSlidingExpiration = TimeSpan.Zero; + + // cacheLock will be released in the code below without checking whether it was + // actually acquired. The API doesn't offer a reliable way to check whether the lock + // is being held by the current thread and since Mono does't implement CER + // (Constrained Execution Regions - + // http://msdn.microsoft.com/en-us/library/ms228973.aspx) currently, we have no + // reliable way of recording the information that the lock has been successfully + // acquired. + // It can happen that a Thread.Abort occurs while acquiring the lock and the lock + // isn't actually held. In this case the attempt to release a lock will throw an + // exception. It's better than a race of setting a boolean flag after acquiring the + // lock and then relying upon it here to release it - that may cause a deadlock + // should we fail to release the lock which was successfully acquired but + // Thread.Abort happened right after that during the stloc instruction to set the + // boolean flag. Once CERs are supported we can use the boolean flag reliably. ReaderWriterLockSlim cacheLock; Dictionary cache; CacheItemPriorityQueue timedItems; @@ -153,19 +168,17 @@ namespace System.Web.Caching { if (key == null) throw new ArgumentNullException ("key"); - - bool locked = false; + try { cacheLock.EnterWriteLock (); - locked = true; CacheItem it = GetCacheItem (key); if (it != null) return it.Value; Insert (key, value, dependencies, absoluteExpiration, slidingExpiration, priority, onRemoveCallback, null, false); } finally { - if (locked) - cacheLock.ExitWriteLock (); + // See comment at the top of the file, above cacheLock declaration + cacheLock.ExitWriteLock (); } return null; @@ -173,10 +186,8 @@ namespace System.Web.Caching public object Get (string key) { - bool locked = false; try { cacheLock.EnterUpgradeableReadLock (); - locked = true; CacheItem it = GetCacheItem (key); if (it == null) return null; @@ -187,6 +198,7 @@ namespace System.Web.Caching if (!NeedsUpdate (it, CacheItemUpdateReason.DependencyChanged, false)) Remove (it.Key, CacheItemRemovedReason.DependencyChanged, false, true); } finally { + // See comment at the top of the file, above cacheLock declaration cacheLock.ExitWriteLock (); } @@ -212,6 +224,7 @@ namespace System.Web.Caching if (!NeedsUpdate (it, CacheItemUpdateReason.Expired, false)) Remove (key, CacheItemRemovedReason.Expired, false, true); } finally { + // See comment at the top of the file, above cacheLock declaration cacheLock.ExitWriteLock (); } @@ -221,9 +234,8 @@ namespace System.Web.Caching return it.Value; } finally { - if (locked) { - cacheLock.ExitUpgradeableReadLock (); - } + // See comment at the top of the file, above cacheLock declaration + cacheLock.ExitUpgradeableReadLock (); } } @@ -282,20 +294,17 @@ namespace System.Web.Caching internal void SetItemTimeout (string key, DateTime absoluteExpiration, TimeSpan slidingExpiration, bool doLock) { - CacheItem ci = null; - bool locked = false; - + CacheItem ci = null; try { - if (doLock) { + if (doLock) cacheLock.EnterWriteLock (); - locked = true; - } ci = GetCacheItem (key); if (ci != null) SetItemTimeout (ci, absoluteExpiration, slidingExpiration, ci.OnRemoveCallback, null, key, false); } finally { - if (locked) { + if (doLock) { + // See comment at the top of the file, above cacheLock declaration cacheLock.ExitWriteLock (); } } @@ -317,12 +326,9 @@ namespace System.Web.Caching ci.OnRemoveCallback = onRemoveCallback; ci.OnUpdateCallback = onUpdateCallback; - bool locked = false; try { - if (doLock) { + if (doLock) cacheLock.EnterWriteLock (); - locked = true; - } if (ci.Timer != null) { ci.Timer.Dispose (); @@ -336,7 +342,8 @@ namespace System.Web.Caching if (!disableExpiration && ci.AbsoluteExpiration != NoAbsoluteExpiration) EnqueueTimedItem (ci); } finally { - if (locked) { + if (doLock) { + // See comment at the top of the file, above cacheLock declaration cacheLock.ExitWriteLock (); } } @@ -376,16 +383,14 @@ namespace System.Web.Caching object Remove (string key, CacheItemRemovedReason reason, bool doLock, bool invokeCallback) { CacheItem it = null; - bool locked = false; try { - if (doLock) { + if (doLock) cacheLock.EnterWriteLock (); - locked = true; - } it = RemoveCacheItem (key); } finally { - if (locked) { + if (doLock) { + // See comment at the top of the file, above cacheLock declaration cacheLock.ExitWriteLock (); } } @@ -424,10 +429,8 @@ namespace System.Web.Caching internal void InvokePrivateCallbacks () { CacheItemRemovedReason reason = CacheItemRemovedReason.Removed; - bool locked = false; try { cacheLock.EnterReadLock (); - locked = true; foreach (string key in cache.Keys) { CacheItem item = GetCacheItem (key); if (item.Disabled) @@ -442,25 +445,21 @@ namespace System.Web.Caching } } } finally { - if (locked) { - cacheLock.ExitReadLock (); - } + // See comment at the top of the file, above cacheLock declaration + cacheLock.ExitReadLock (); } } public IDictionaryEnumerator GetEnumerator () { ArrayList list = new ArrayList (); - bool locked = false; try { cacheLock.EnterReadLock (); - locked = true; foreach (CacheItem it in cache.Values) list.Add (it); } finally { - if (locked) { - cacheLock.ExitReadLock (); - } + // See comment at the top of the file, above cacheLock declaration + cacheLock.ExitReadLock (); } return new CacheItemEnumerator (list); @@ -478,13 +477,9 @@ namespace System.Web.Caching bool NeedsUpdate (CacheItem item, CacheItemUpdateReason reason, bool needLock) { - bool locked = false; - try { - if (needLock) { + if (needLock) cacheLock.EnterWriteLock (); - locked = true; - } if (item == null || item.OnUpdateCallback == null) return false; @@ -525,8 +520,10 @@ namespace System.Web.Caching } catch (Exception) { return false; } finally { - if (locked) + if (needLock) { + // See comment at the top of the file, above cacheLock declaration cacheLock.ExitWriteLock (); + } } } @@ -534,11 +531,9 @@ namespace System.Web.Caching { DateTime now = DateTime.Now; CacheItem item = timedItems.Peek (); - bool locked = false; try { cacheLock.EnterWriteLock (); - locked = true; while (item != null) { if (!item.Disabled && item.ExpiresAt > now.Ticks) @@ -554,8 +549,8 @@ namespace System.Web.Caching item = timedItems.Peek (); } } finally { - if (locked) - cacheLock.ExitWriteLock (); + // See comment at the top of the file, above cacheLock declaration + cacheLock.ExitWriteLock (); } if (item != null) { @@ -574,10 +569,8 @@ namespace System.Web.Caching internal void CheckDependencies () { IList list; - bool locked = false; try { cacheLock.EnterWriteLock (); - locked = true; list = new List (); foreach (CacheItem it in cache.Values) list.Add (it); @@ -587,18 +580,15 @@ namespace System.Web.Caching Remove (it.Key, CacheItemRemovedReason.DependencyChanged, false, true); } } finally { - if (locked) { - cacheLock.ExitWriteLock (); - } + // See comment at the top of the file, above cacheLock declaration + cacheLock.ExitWriteLock (); } } internal DateTime GetKeyLastChange (string key) { - bool locked = false; try { cacheLock.EnterReadLock (); - locked = true; CacheItem it = GetCacheItem (key); if (it == null) @@ -606,9 +596,8 @@ namespace System.Web.Caching return it.LastChange; } finally { - if (locked) { - cacheLock.ExitReadLock (); - } + // See comment at the top of the file, above cacheLock declaration + cacheLock.ExitReadLock (); } } diff --git a/mcs/class/System.Web/System.Web.Caching/CacheItemPriorityQueue.cs b/mcs/class/System.Web/System.Web.Caching/CacheItemPriorityQueue.cs index 67a7205941c..918f3521cd0 100644 --- a/mcs/class/System.Web/System.Web.Caching/CacheItemPriorityQueue.cs +++ b/mcs/class/System.Web/System.Web.Caching/CacheItemPriorityQueue.cs @@ -41,6 +41,8 @@ namespace System.Web.Caching CacheItem[] heap; int heapSize = 0; int heapCount = 0; + + // See comment for the cacheLock field at top of System.Web.Caching/Cache.cs ReaderWriterLockSlim queueLock; public int Count { @@ -94,20 +96,18 @@ namespace System.Web.Caching if (item == null) return; - bool locked = false; CacheItem[] heap; try { queueLock.EnterWriteLock (); - locked = true; heap = GetHeapWithGrow (); heap [heapCount++] = item; BubbleUp (heap); AddSequenceEntry (item, EDSequenceEntryType.Enqueue); } finally { - if (locked) - queueLock.ExitWriteLock (); + // See comment at the top of the file, above queueLock declaration + queueLock.ExitWriteLock (); } } @@ -115,12 +115,10 @@ namespace System.Web.Caching { CacheItem ret = null; CacheItem[] heap; - bool locked = false; int index; try { queueLock.EnterWriteLock (); - locked = true; heap = GetHeapWithShrink (); if (heap == null || heapCount == 0) return null; @@ -136,19 +134,17 @@ namespace System.Web.Caching AddSequenceEntry (ret, EDSequenceEntryType.Dequeue); return ret; } finally { - if (locked) - queueLock.ExitWriteLock (); + // See comment at the top of the file, above queueLock declaration + queueLock.ExitWriteLock (); } } public CacheItem Peek () { - bool locked = false; CacheItem ret; try { queueLock.EnterReadLock (); - locked = true; if (heap == null || heapCount == 0) return null; @@ -157,8 +153,8 @@ namespace System.Web.Caching return ret; } finally { - if (locked) - queueLock.ExitReadLock (); + // See comment at the top of the file, above queueLock declaration + queueLock.ExitReadLock (); } } diff --git a/mcs/class/System.Web/System.Web.Compilation/BuildManager.cs b/mcs/class/System.Web/System.Web.Compilation/BuildManager.cs index 0e9dbdcd6db..445754eb4ca 100644 --- a/mcs/class/System.Web/System.Web.Compilation/BuildManager.cs +++ b/mcs/class/System.Web/System.Web.Compilation/BuildManager.cs @@ -94,6 +94,7 @@ namespace System.Web.Compilation // This is here _only_ for the purpose of unit tests! internal static bool suppressDebugModeMessages; + // See comment for the cacheLock field at top of System.Web.Caching/Cache.cs #if SYSTEMCORE_DEP static ReaderWriterLockSlim buildCacheLock; #else @@ -817,14 +818,12 @@ namespace System.Web.Compilation // to the assembly builder and, in effect, no assembly is produced but there are STILL types that need // to be added to the cache. Assembly compiledAssembly = results != null ? results.CompiledAssembly : null; - bool locked = false; try { #if SYSTEMCORE_DEP buildCacheLock.EnterWriteLock (); #else buildCacheLock.AcquireWriterLock (-1); #endif - locked = true; if (compiledAssembly != null) referencedAssemblies.Add (compiledAssembly); @@ -835,13 +834,11 @@ namespace System.Web.Compilation StoreInCache (bp, compiledAssembly, results); } } finally { - if (locked) { #if SYSTEMCORE_DEP - buildCacheLock.ExitWriteLock (); + buildCacheLock.ExitWriteLock (); #else - buildCacheLock.ReleaseWriterLock (); + buildCacheLock.ReleaseWriterLock (); #endif - } } } @@ -883,24 +880,19 @@ namespace System.Web.Compilation #endif static BuildManagerCacheItem GetCachedItem (string vp) { - bool locked = false; - try { #if SYSTEMCORE_DEP buildCacheLock.EnterReadLock (); #else buildCacheLock.AcquireReaderLock (-1); #endif - locked = true; return GetCachedItemNoLock (vp); } finally { - if (locked) { #if SYSTEMCORE_DEP - buildCacheLock.ExitReadLock (); + buildCacheLock.ExitReadLock (); #else - buildCacheLock.ReleaseReaderLock (); + buildCacheLock.ReleaseReaderLock (); #endif - } } } @@ -1290,27 +1282,22 @@ namespace System.Web.Compilation else return; - bool locked = false; try { #if SYSTEMCORE_DEP buildCacheLock.EnterWriteLock (); #else buildCacheLock.AcquireWriterLock (-1); #endif - locked = true; - if (HasCachedItemNoLock (virtualPath)) { buildCache [virtualPath] = null; OnEntryRemoved (virtualPath); } } finally { - if (locked) { #if SYSTEMCORE_DEP - buildCacheLock.ExitWriteLock (); + buildCacheLock.ExitWriteLock (); #else - buildCacheLock.ReleaseWriterLock (); + buildCacheLock.ReleaseWriterLock (); #endif - } } } diff --git a/mcs/class/System.Web/System.Web.Configuration_2.0/WebConfigurationManager.cs b/mcs/class/System.Web/System.Web.Configuration_2.0/WebConfigurationManager.cs index 00d9f8ec880..de0790a4acf 100644 --- a/mcs/class/System.Web/System.Web.Configuration_2.0/WebConfigurationManager.cs +++ b/mcs/class/System.Web/System.Web.Configuration_2.0/WebConfigurationManager.cs @@ -67,6 +67,8 @@ namespace System.Web.Configuration { static readonly char[] pathTrimChars = { '/' }; static readonly object suppressAppReloadLock = new object (); static readonly object saveLocationsCacheLock = new object (); + + // See comment for the cacheLock field at top of System.Web.Caching/Cache.cs static readonly ReaderWriterLockSlim sectionCacheLock; #if !TARGET_J2EE @@ -225,14 +227,11 @@ namespace System.Web.Configuration { static void ConfigurationSaveHandler (_Configuration sender, ConfigurationSaveEventArgs args) { - bool locked = false; try { sectionCacheLock.EnterWriteLock (); - locked = true; sectionCache.Clear (); } finally { - if (locked) - sectionCacheLock.ExitWriteLock (); + sectionCacheLock.ExitWriteLock (); } lock (suppressAppReloadLock) { @@ -442,7 +441,6 @@ namespace System.Web.Configuration { int cacheKey; bool pathPresent = !String.IsNullOrEmpty (path); string locationPath = null; - bool locked = false; if (pathPresent) locationPath = "location_" + path; @@ -453,7 +451,6 @@ namespace System.Web.Configuration { try { sectionCacheLock.EnterReadLock (); - locked = true; object o; if (pathPresent) { @@ -469,8 +466,7 @@ namespace System.Web.Configuration { if (sectionCache.TryGetValue (baseCacheKey, out o)) return o; } finally { - if (locked) - sectionCacheLock.ExitReadLock (); + sectionCacheLock.ExitReadLock (); } string cachePath = null; @@ -687,27 +683,21 @@ namespace System.Web.Configuration { static void AddSectionToCache (int key, object section) { object cachedSection; - bool locked = false; try { sectionCacheLock.EnterUpgradeableReadLock (); - locked = true; if (sectionCache.TryGetValue (key, out cachedSection) && cachedSection != null) return; - bool innerLocked = false; try { sectionCacheLock.EnterWriteLock (); - innerLocked = true; sectionCache.Add (key, section); } finally { - if (innerLocked) - sectionCacheLock.ExitWriteLock (); + sectionCacheLock.ExitWriteLock (); } } finally { - if (locked) - sectionCacheLock.ExitUpgradeableReadLock (); + sectionCacheLock.ExitUpgradeableReadLock (); } } -- GitLab