Improve locking annotation somewhat

Unfortunately there's a good number of holes, which seem to be due
to how the PREfast analysis of the locking works. Someone familiar
with the mechanics may be able to do this with fewer suppressions.

lib/Common/Core/CriticalSection.h

@@ -4,6 +4,10 @@
 //-------------------------------------------------------------------------------------------------------
 #pragma once
 
+#ifdef _WIN32
+#include <suppress.h>
+#endif
+
 class CriticalSection
 #ifndef _WIN32
 : public CCLock
@@ -20,9 +24,9 @@ public:
         ::InitializeCriticalSectionAndSpinCount(&cs, spincount);
     }
     ~CriticalSection() { ::DeleteCriticalSection(&cs); }
-    BOOL TryEnter() { return ::TryEnterCriticalSection(&cs); }
-    void Enter() { ::EnterCriticalSection(&cs); }
-    void Leave() { ::LeaveCriticalSection(&cs); }
+    _Success_(return) BOOL _Acquires_lock_(this->cs) TryEnter() { return ::TryEnterCriticalSection(&cs); }
+    void _Acquires_lock_(this->cs) Enter() { ::EnterCriticalSection(&cs); }
+    void _Releases_lock_(this->cs) Leave() { ::LeaveCriticalSection(&cs); }
 #if DBG
     bool IsLocked() const { return cs.OwningThread == (HANDLE)::GetCurrentThreadId(); }
 #endif
@@ -37,19 +41,25 @@ class FakeCriticalSection
 public:
     FakeCriticalSection(DWORD spincount = 0) { /*do nothing*/spincount++; }
     ~FakeCriticalSection() {}
-    BOOL TryEnter() { return true; }
-    void Enter() {}
-    void Leave() {}
+#pragma prefast(suppress:__WARNING_FAILING_TO_ACQUIRE_MEDIUM_CONFIDENCE)
+    _Success_(return) BOOL _Acquires_lock_(this->cs) TryEnter() { return true; }
+#pragma prefast(suppress:__WARNING_FAILING_TO_ACQUIRE_MEDIUM_CONFIDENCE)
+    _Acquires_lock_(this->cs) void Enter() {}
+#pragma prefast(suppress:__WARNING_FAILING_TO_RELEASE_MEDIUM_CONFIDENCE)
+    _Releases_lock_(this->cs) void Leave() {}
 #if DBG
     bool IsLocked() const { return true; }
 #endif
+private:
+    // only used for prefast analysis
+    int cs;
 };
 
 class AutoCriticalSection
 {
 public:
-    AutoCriticalSection(CriticalSection * cs) : cs(cs) { cs->Enter(); }
-    ~AutoCriticalSection() { cs->Leave(); }
+    _Acquires_lock_(this->cs->cs) AutoCriticalSection(CriticalSection * cs) : cs(cs) { this->cs->Enter(); }
+    _Releases_lock_(this->cs->cs) ~AutoCriticalSection() { cs->Leave(); }
 private:
     CriticalSection * cs;
 };
@@ -57,19 +67,19 @@ private:
 class AutoOptionalCriticalSection
 {
 public:
-    AutoOptionalCriticalSection(CriticalSection * cs) : cs(cs)
+    _When_(this->cs != nullptr, _Acquires_lock_(this->cs->cs)) AutoOptionalCriticalSection(CriticalSection * cs) : cs(cs)
     {
-        if (cs)
+        if (this->cs)
         {
-            cs->Enter();
+            this->cs->Enter();
         }
     }
 
-    ~AutoOptionalCriticalSection()
+    _When_(this->cs != nullptr, _Releases_lock_(this->cs->cs)) ~AutoOptionalCriticalSection()
     {
-        if (cs)
+        if (this->cs)
         {
-            cs->Leave();
+            this->cs->Leave();
         }
     }
 
@@ -81,8 +91,8 @@ template <class SyncObject = FakeCriticalSection >
 class AutoRealOrFakeCriticalSection
 {
 public:
-    AutoRealOrFakeCriticalSection(SyncObject * cs) : cs(cs) { cs->Enter(); }
-    ~AutoRealOrFakeCriticalSection() { cs->Leave(); }
+    _Acquires_lock_(this->cs->cs) AutoRealOrFakeCriticalSection(SyncObject * cs) : cs(cs) { this->cs->Enter(); }
+    _Releases_lock_(this->cs->cs) ~AutoRealOrFakeCriticalSection() { this->cs->Leave(); }
 private:
     SyncObject * cs;
 };
@@ -91,19 +101,19 @@ template <class SyncObject = FakeCriticalSection >
 class AutoOptionalRealOrFakeCriticalSection
 {
 public:
-    AutoOptionalRealOrFakeCriticalSection(SyncObject * cs) : cs(cs)
+    _When_(this->cs != nullptr, _Acquires_lock_(this->cs->cs)) AutoOptionalRealOrFakeCriticalSection(SyncObject * cs) : cs(cs)
     {
-        if (cs)
+        if (this->cs)
         {
-            cs->Enter();
+            this->cs->Enter();
         }
     }
 
-    ~AutoOptionalRealOrFakeCriticalSection()
+    _When_(this->cs != nullptr, _Releases_lock_(this->cs->cs)) ~AutoOptionalRealOrFakeCriticalSection()
     {
-        if (cs)
+        if (this->cs)
         {
-            cs->Leave();
+            this->cs->Leave();
         }
     }
 

lib/Common/DataStructures/BaseDictionary.h

@@ -47,17 +47,24 @@ namespace JsUtil
     class NoResizeLock
     {
     public:
-        void BeginResize() {}
-        void EndResize() {}
+#pragma prefast(suppress:__WARNING_FAILING_TO_ACQUIRE_MEDIUM_CONFIDENCE)
+        void _Acquires_lock_(cs.cs) BeginResize() {}
+#pragma prefast(suppress:__WARNING_FAILING_TO_RELEASE_MEDIUM_CONFIDENCE)
+        void _Releases_lock_(cs.cs) EndResize() {}
+    private:
+        // For prefast analysis, we need to have a somewhat similar shape for both locks
+        struct {
+            int cs;
+        } cs;
     };
 
     class AsymetricResizeLock
     {
     public:
-        void BeginResize() { cs.Enter(); }
-        void EndResize() { cs.Leave(); }
-        void LockResize() { cs.Enter(); }
-        void UnlockResize() { cs.Leave(); }
+        void _Acquires_lock_(cs.cs) BeginResize() { cs.Enter(); }
+        void _Releases_lock_(cs.cs) EndResize() { cs.Leave(); }
+        void _Acquires_lock_(cs.cs) LockResize() { cs.Enter(); }
+        void _Releases_lock_(cs.cs) UnlockResize() { cs.Leave(); }
     private:
         CriticalSection cs;
     };
@@ -117,8 +124,10 @@ namespace JsUtil
         class AutoDoResize
         {
         public:
-            AutoDoResize(Lock& lock) : lock(lock) { lock.BeginResize(); };
-            ~AutoDoResize() { lock.EndResize(); };
+#pragma prefast(suppress:__WARNING_FAILING_TO_ACQUIRE_MEDIUM_CONFIDENCE)
+            _Acquires_lock_(this->lock.cs.cs) AutoDoResize(Lock& lock) : lock(lock) { this->lock.BeginResize(); };
+#pragma prefast(suppress:__WARNING_CALLER_FAILING_TO_HOLD_MEDIUM_CONFIDENCE)
+            _Releases_lock_(this->lock.cs.cs) ~AutoDoResize() { this->lock.EndResize(); };
         private:
             Lock& lock;
         };
@@ -671,12 +680,12 @@ namespace JsUtil
             DoCopy(other);
         }
 
-        void LockResize()
+        void _Acquires_lock_(this->cs.cs) LockResize()
         {
             __super::LockResize();
         }
 
-        void UnlockResize()
+        void _Releases_lock_(this->cs.cs) UnlockResize()
         {
             __super::UnlockResize();
         }
@@ -1573,12 +1582,12 @@ namespace JsUtil
             this->DoCopy(other);
         }
 
-        void LockResize()
+        void _Acquires_lock_(this->cs.cs) LockResize()
         {
             __super::LockResize();
         }
 
-        void UnlockResize()
+        void _Releases_lock_(this->cs.cs) UnlockResize()
         {
             __super::UnlockResize();
         }

lib/Common/Memory/LeakReport.cpp

@@ -19,6 +19,7 @@
 //  AU RecyclerWriteBarrierManager
 #pragma warning(disable:4075)       // initializers put in unrecognized initialization area on purpose
 #pragma init_seg(".CRT$XCAR")
+#pragma prefast(disable:__WARNING_CALLER_FAILING_TO_HOLD, "Not annotating this file for lock semantics due to poor accuracy and complicated conditions for some locks")
 
 CriticalSection LeakReport::s_cs;
 DWORD LeakReport::nestedSectionCount = 0;

lib/JITClient/JITManager.cpp

@@ -223,6 +223,9 @@ JITManager::IsOOPJITEnabled() const
     return m_oopJitEnabled;
 }
 
+#pragma prefast(push)
+#pragma prefast(disable:__WARNING_RELEASING_UNHELD_LOCK_MEDIUM_CONFIDENCE, "Lock is correctly acquired and released by RAII class AutoCriticalSection")
+#pragma prefast(disable:__WARNING_CALLER_FAILING_TO_HOLD, "Lock is correctly acquired and released by RAII class AutoCriticalSection")
 HRESULT
 JITManager::ConnectRpcServer(__in HANDLE jitProcessHandle, __in_opt void* serverSecurityDescriptor, __in UUID connectionUuid)
 {
@@ -260,6 +263,7 @@ FailureCleanup:
 
     return hr;
 }
+#pragma prefast(pop)
 
 HRESULT
 JITManager::Shutdown()

lib/Runtime/Library/AtomicsObject.cpp

@@ -220,6 +220,7 @@ namespace Js
 
             DWORD_PTR agent = (DWORD_PTR)scriptContext;
             Assert(sharedArrayBuffer->GetSharedContents()->IsValidAgent(agent));
+#pragma prefast(suppress:__WARNING_CALLER_FAILING_TO_HOLD, "This is a prefast false-positive caused by it being unable to identify that the critical section used here is the same as the one held by the AutoCriticalSection")
             awoken = waiterList->AddAndSuspendWaiter(agent, timeout);
             if (!awoken) 
             {

lib/Runtime/Library/SharedArrayBuffer.cpp

@@ -722,7 +722,7 @@ namespace Js
         return false;
     }
 
-    bool WaiterList::AddAndSuspendWaiter(DWORD_PTR waiter, uint32 timeout)
+    bool _Requires_lock_held_(csForAccess.cs) WaiterList::AddAndSuspendWaiter(DWORD_PTR waiter, uint32 timeout)
     {
 #ifdef _WIN32
         Assert(m_waiters != nullptr);

lib/Runtime/Library/SharedArrayBuffer.h

@@ -182,7 +182,7 @@ namespace Js
         WaiterList();
         void Cleanup();
 
-        bool AddAndSuspendWaiter(DWORD_PTR waiter, uint32 timeout);
+        bool _Requires_lock_held_(csForAccess.cs) AddAndSuspendWaiter(DWORD_PTR waiter, uint32 timeout);
         void RemoveWaiter(DWORD_PTR waiter);
         uint32 RemoveAndWakeWaiters(int32 count);
 
@@ -194,7 +194,7 @@ namespace Js
 
         Waiters * m_waiters;
 
-        // Below CS is used for synchronizig access in wait/wake API
+        // Below CS is used for synchronizing access in wait/wake API
         CriticalSection csForAccess;
     };
 }