Infinite spin wait in CWGXBitmapLockState::LockRead()

[DavidYawCSpeed]

Description

In method CWGXBitmapLockState::LockRead(), the exit-on-other-lock-active is implemented incorrectly.

lockCount = m_lockState & (~lockWrite); // lockWrite == 0x80000000

LONG incCount = lockCount + 1;
// Check for locked for write and also extreme case of too many simultaneous read requests.
if (incCount & lockWrite)
{
    TraceTag((tagMILWarning, "CWGXBitmapLockState::LockRead failed -- there is already an outstanding write lock or too many reads."));
    IFC(WINCODEC_ERR_ALREADYLOCKED);
}

original = InterlockedCompareExchange(&m_lockState, incCount, lockCount);

This is incorrect. Because the high bit is cleared when reading the lock, the "LockRead failed -- outstanding write lock" will never happen. Then, because lockCount is not the same value as m_lockState, the compare exchange will fail, and it will loop. If there is an outstanding write lock, this will loop forever. (In my application, this ended up deadlocking with the UI thread.)

Solution: Don't clear the high bit.

lockCount = m_lockState; // <--- No clearing high bit.

LONG incCount = lockCount + 1;
// Check for locked for write and also extreme case of too many simultaneous read requests.
if (incCount & lockWrite)
{
    TraceTag((tagMILWarning, "CWGXBitmapLockState::LockRead failed -- there is already an outstanding write lock or too many reads."));
    IFC(WINCODEC_ERR_ALREADYLOCKED);
}

original = InterlockedCompareExchange(&m_lockState, incCount, lockCount);

Reproduction Steps

Sorry, I don't have a demo application that exhibits this all the time.

I was able to get a deadlock between the render thread and the UI thread: The render thread was stuck in this method, the UI thread was in DUCE.Channel.SyncFlush() --> CMilChannel::SyncFlush() --> CMilConnection::SynchronizeChannel(hChannel), waiting on a synchronization object that presumably would have been signaled from the render thread.

Expected behavior

As the comments in the method say, I expect it to exit with an error if a write lock is active.

Actual behavior

It doesn't.

Regression?

No response

Known Workarounds

No response

Impact

No response

Configuration

.Net version: seen in .Net 7 and .Net 8. Also present in main branch in this repo.

Other information

No response

[miloush]

If you don't clear the bit, then the count would be wrong... a better fix would be to do if (m_lockState & lockWrite), wouldn't it?

[DavidYawCSpeed]

Since m_lockState is being used with atomic updates, I think the idea is to only read m_lockState once, and then use InterlockedWhatever to update it.

m_lockState will be one of three things:

• Zero: Not currently locked.
• 0x8000000: Locked for writing.
• Some small number: Locked for reading, that many times.

If it's currently 0x8000000, then incCount will be 0x80000001, which will properly trigger the "already locked" error return. m_lockState will never be incremented to 0x80000001; incCount will never be 0x80000002.

If it's zero or a small number, then incCount will be a small number, and it won't error return.

If somehow we end up with 2 billion simultaneous read locks, then m_LockState will be 0x7FFFFFFF, incCount will be 0x8000000, and the "too many reads" half of the error return will happen.

But more importantly, it absolutely must change to this:

lockCount = m_lockState; // no manipulation during the read

...

original = InterlockedCompareExchange(&m_lockState, whatever, lockCount);
                                                              ^^^^^^^^^ not manipulated

If lockCount is not equal to the old value of m_lockState, then InterlockedCompareExchange will always fail, which is what's causing the endless loop that I've been seeing.