Locking SAL with Visual Studio

Question

I'm trying to add locking SAL statements to find/prevent incorrect locking in my application. Like missing locking calls and mismatched locking calls. I'm getting warnings I don't understand. I've put them as comments in the example below. I turned on code analysis and set Microsoft All Rules. I'm using Visual Studio 2019 16.11.9. I've also tried Visual Studio 2022 17.0.5 and get the same results.

I've tried _Success_ in place of _When_. I've tried returning bool instead of BOOL and _Success_(return) vs _Success_(return != 0).

Some research I've read:

https://learn.microsoft.com/en-us/cpp/code-quality/c26135?view=msvc-170

_When_(return != 0, _Acquires_lock_(p->cs))
int TryEnter(DATA* p)
{
    if (p->state != 0)
    {
        EnterCriticalSection(&p->cs);
        return p->state;
    }
    return 0;
}

The former article is from 2021 so is probably more up to date than the later one which is for Visual Studio 2015. The VS2015 example doesn't work as is; it would seem the _Success_ annotation has been changed as it no longer takes two parameters.

https://learn.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2015/code-quality/best-practices-and-examples-sal?view=vs-2015

// Incorrect  
_Success_(return == TRUE, _Acquires_lock_(*lpCriticalSection))  
BOOL WINAPI TryEnterCriticalSection(  
  _Inout_ LPCRITICAL_SECTION lpCriticalSection  
);  
  
// Correct  
_Success_(return != 0, _Acquires_lock_(*lpCriticalSection))  
BOOL WINAPI TryEnterCriticalSection(  
  _Inout_ LPCRITICAL_SECTION lpCriticalSection  
);

My test code:

#define _WIN32_WINNT _WIN32_WINNT_WIN7

#include <windows.h>
#include <tchar.h>

// https://learn.microsoft.com/en-us/cpp/code-quality/c26135?view=msvc-170

class CLock {
public:

    CLock() noexcept : m_hMutex(CreateMutex(nullptr,FALSE,nullptr)){
    }

    ~CLock(){

        CloseHandle(m_hMutex);

        m_hMutex = nullptr;

    }

    _When_(return != 0, _Acquires_lock_(this->m_hMutex)) BOOL Lock() noexcept {

        // warning C26135: Missing annotation _Acquires_lock_(this->m_hMutex) at function 'CLock::Lock'.
        // I don't understand this warning, I have the annotation?
        return (WaitForSingleObject(m_hMutex,INFINITE) == WAIT_OBJECT_0);

    }

    _When_(return != 0, _Releases_lock_(this->m_hMutex)) BOOL Unlock() noexcept {

        // warning C26135 : Missing annotation _Releases_lock_(this->m_hMutex) at function 'CLock::Unlock'.
        // I don't understand this warning, I have the annotation?
        return ReleaseMutex(m_hMutex);

    }

private:

    // Ensure no copies can be made
    CLock(const CLock &lCopy) = delete;
    CLock(const CLock &&lMove) = delete;
    const CLock &operator=(const CLock &lCopy) = delete;
    const CLock &&operator=(const CLock &&lMove) = delete;

    _Has_lock_kind_(_Lock_kind_mutex_) HANDLE m_hMutex;

};

class ThreadedThingy {
public:

    ThreadedThingy() noexcept : m_iThingy(0){
    }

    VOID TestBadNotLocked() noexcept {

        // warning C26130: Missing annotation _Requires_lock_held_(this->m_lLock) or _No_competing_thread_
        //                 at function 'ThreadedThingy::TestBadNotLocked'. Otherwise it could be a race
        //                 condition. Variable 'this->m_iThingy' should be protected by lock 'this->m_lLock'.
        // This should be a warning since the lock isn't held
        m_iThingy = 10;

    }

    VOID TestBadNotUnlocked() noexcept {

        if (m_lLock.Lock()){

            // warning C26165: Possibly failing to release lock '(&this->m_lLock)->m_hMutex' in function
            //                'ThreadedThingy::TestBadNotUnlocked'.
            // This should be a warning since lock isn't unlocked

            // warning C26130: Missing annotation _Requires_lock_held_(this->m_lLock) or _No_competing_thread_
            //                 at function 'ThreadedThingy::TestBadNotUnlocked'. Otherwise it could be a race
            //                 condition. Variable 'this->m_iThingy' should be protected by lock 'this->m_lLock'.
            // I don't understand this warning, the lock is held
            m_iThingy = 20;

        }

    }

    VOID TestGood() noexcept {

        if (m_lLock.Lock()){

            // warning C26130: Missing annotation _Requires_lock_held_(this->m_lLock) or _No_competing_thread_ at
            //                 function 'ThreadedThingy::TestGood'. Otherwise it could be a race condition.
            //                 Variable 'this->m_iThingy' should be protected by lock 'this->m_lLock'.
            // I don't understand this warning, the lock is held
            m_iThingy = 20;

            // warning C26165: Possibly failing to release lock '(&this->m_lLock)->m_hMutex' in function
            //                 'ThreadedThingy::TestGood'.
            // I kind of understand this warning because Unlock can't gaurentee the lock is released
            m_lLock.Unlock();

        }

    }

private:

    _Has_lock_kind_(_Lock_kind_mutex_) CLock m_lLock;

    _Guarded_by_(this->m_lLock) INT m_iThingy;

};

INT _tmain() noexcept {

    ThreadedThingy ttTest;

    ttTest.TestBadNotLocked();

    ttTest.TestBadNotUnlocked();

    ttTest.TestGood();

    return 0;

}

Thanks!

-Daniel

score 0 · Accepted Answer · answered Mar 11 '22 at 06:39

I opened a ticket with Microsoft to get an answer to this. I learned three things. First, when WaitForSingleObject returns WAIT_ABANDONED, it means the lock is now owned, similar to when it returns WAIT_OBJECT_0 but is not signaled. So, you have to check both return values, otherwise it says you don't have the correct annotation. Second, the static code analysis tools assumes ReleaseMutex always releases the lock. So, my _When_ annotation on Unlock() is unnecessary/wrong. Lastly, there is another annotation needed to link the mutex lock to the CLock object. This annotation is _Post_same_lock_. Google doesn't have much on this annotation, just header files. The working code:

#define _WIN32_WINNT _WIN32_WINNT_WIN7

#include <windows.h>
#include <tchar.h>

class CLock {
public:

    CLock() noexcept : m_hMutex(CreateMutex(nullptr,FALSE,nullptr)){
    }

    ~CLock(){

        CloseHandle(m_hMutex);

        m_hMutex = nullptr;

    }

    _When_(return != 0, _Acquires_lock_(this->m_hMutex) _Post_same_lock_(*this, this->m_hMutex)) BOOL Lock() noexcept {

        const DWORD dwWait = WaitForSingleObject(m_hMutex, INFINITE);

        // You have to do more than this to handle WAIT_ABANDONED_0
        // This example is only about SAL, not how to corretly handle WAIT_ABANDONED_0

        return ((dwWait == WAIT_OBJECT_0) || (dwWait == WAIT_ABANDONED_0));

    }

    _Releases_lock_(this->m_hMutex) VOID Unlock() noexcept {

        ReleaseMutex(m_hMutex);

    }

private:

    // Ensure no copies can be made
    CLock(const CLock &lCopy) = delete;
    CLock(const CLock &&lMove) = delete;
    const CLock &operator=(const CLock &lCopy) = delete;
    const CLock &&operator=(const CLock &&lMove) = delete;

    _Has_lock_kind_(_Lock_kind_mutex_) HANDLE m_hMutex;

};

class ThreadedThingy {
public:

    ThreadedThingy() noexcept : m_iThingy(0){
    }

    VOID TestBadNotLocked() noexcept {

        // warning C26130: Missing annotation _Requires_lock_held_(this->m_lLock) or _No_competing_thread_
        //                 at function 'ThreadedThingy::TestBadNotLocked'. Otherwise it could be a race
        //                 condition. Variable 'this->m_iThingy' should be protected by lock 'this->m_lLock'.
        // This should be a warning since the lock isn't held
        m_iThingy = 10;

    }

    VOID TestBadNotUnlocked() noexcept {

        if (m_lLock.Lock()){

            // warning C26165: Possibly failing to release lock '(&this->m_lLock)->m_hMutex' in function
            //                'ThreadedThingy::TestBadNotUnlocked'.
            // This should be a warning since lock isn't unlocked

            m_iThingy = 20;

        }

    }

    VOID TestGood() noexcept {

        if (m_lLock.Lock()){

            m_iThingy = 20;

            m_lLock.Unlock();

        }

    }

private:

    CLock m_lLock;

    _Guarded_by_(this->m_lLock) INT m_iThingy;

};

INT _tmain() noexcept {

    ThreadedThingy ttTest;

    ttTest.TestBadNotLocked();

    ttTest.TestBadNotUnlocked();

    ttTest.TestGood();

    return 0;

}

Locking SAL with Visual Studio

1 Answers1