|
| 1 | +--- |
| 2 | +description: "Learn more about: Warning C26837" |
| 3 | +title: Warning C26837 |
| 4 | +ms.date: 11/29/2023 |
| 5 | +f1_keywords: ["C26837", "INTERLOCKED_COMPARE_EXCHANGE_MISUSE", "__WARNING_INTERLOCKED_COMPARE_EXCHANGE_MISUSE"] |
| 6 | +helpviewer_keywords: ["C26837"] |
| 7 | +--- |
| 8 | +# Warning C26837 |
| 9 | + |
| 10 | +> Value for the comparand `comp` for function `func` has been loaded from the destination location `dest` through non-volatile read. |
| 11 | +
|
| 12 | +This rule was added in Visual Studio 2022 17.8. |
| 13 | + |
| 14 | +## Remarks |
| 15 | + |
| 16 | +The [`InterlockedCompareExchange`](/windows/win32/api/winnt/nf-winnt-interlockedcompareexchange) function, and its derivatives such as [`InterlockedCompareExchangePointer`](/windows/win32/api/winnt/nf-winnt-interlockedcompareexchangepointer), perform an atomic compare-and-exchange operation on the specified values. If the `Destination` value is equal to the `Comparand` value, the *exchange* value is stored in the address specified by `Destination`. Otherwise, no operation is performed. The `interlocked` functions provide a simple mechanism for synchronizing access to a variable that is shared by multiple threads. This function is atomic with respect to calls to other `interlocked` functions. Misuse of these functions can generate object code that behaves differently than you expect because optimization can change the behavior of the code in unexpected ways. |
| 17 | + |
| 18 | +Consider the following code: |
| 19 | + |
| 20 | +```cpp |
| 21 | +#include <Windows.h> |
| 22 | + |
| 23 | +bool TryLock(__int64* plock) |
| 24 | +{ |
| 25 | + __int64 lock = *plock; |
| 26 | + return (lock & 1) && |
| 27 | + _InterlockedCompareExchange64(plock, lock & ~1, lock) == lock; |
| 28 | +} |
| 29 | +``` |
| 30 | +
|
| 31 | +The intent of this code is: |
| 32 | +
|
| 33 | +1. Read the current value from the `plock` pointer. |
| 34 | +1. Check if this current value has the least significant bit set. |
| 35 | +1. If it does have least significant bit set, clear the bit while preserving the other bits of the current value. |
| 36 | +
|
| 37 | +To accomplish this, a copy of the current value is read from the`plock` pointer and saved to a stack variable `lock`. `lock` is used three times: |
| 38 | +
|
| 39 | +1. First, to check if the least-significant bit is set. |
| 40 | +1. Second, as the `Comparand` value to `InterlockedCompareExchange64`. |
| 41 | +1. Finally, in the comparison of the return value from `InterlockedCompareExchange64` |
| 42 | +
|
| 43 | +This assumes that the current value saved to the stack variable is read once at the start of the function and doesn't change. This is necessary because the current value is first checked before attempting the operation, then explicitly used as the `Comparand` in `InterlockedCompareExchange64`, and finally used to compare the return value from `InterlockedCompareExchange64`. |
| 44 | +
|
| 45 | +Unfortunately, the previous code can be compiled into assembly that behaves differently than from what you expect from the source code. Compile the previous code with the Microsoft Visual C++ (MSVC) compiler and the [`/O1`](../build/reference/o1-o2-minimize-size-maximize-speed.md) option and inspect the resultant assembly code to see how the value of the lock for each of the references to `lock` is obtained. The MSVC compiler version v19.37 produces assembly code that looks like: |
| 46 | +
|
| 47 | +```x86asm |
| 48 | +plock$ = 8 |
| 49 | +bool TryLock(__int64 *) PROC ; TryLock, COMDAT |
| 50 | + mov r8b, 1 |
| 51 | + test BYTE PTR [rcx], r8b |
| 52 | + je SHORT $LN3@TryLock |
| 53 | + mov rdx, QWORD PTR [rcx] |
| 54 | + mov rax, QWORD PTR [rcx] |
| 55 | + and rdx, -2 |
| 56 | + lock cmpxchg QWORD PTR [rcx], rdx |
| 57 | + je SHORT $LN4@TryLock |
| 58 | +$LN3@TryLock: |
| 59 | + xor r8b, r8b |
| 60 | +$LN4@TryLock: |
| 61 | + mov al, r8b |
| 62 | + ret 0 |
| 63 | +bool TryLock(__int64 *) ENDP ; TryLock |
| 64 | +``` |
| 65 | + |
| 66 | +`rcx` holds the value of the parameter `plock`. Rather than make a copy of the current value on the stack, the assembly code is re-reading the value from `plock` every time. This means the value could be different each time it's read. This invalidates the sanitization that the developer is performing. The value is re-read from `plock` after it's verified that it has its least-significant bit set. Because it's re-read after this validation is performed, the new value might no longer have the least-significant bit set. Under a race condition, this code might behave as if it successfully obtained the specified lock when it was already locked by another thread. |
| 67 | + |
| 68 | +The compiler is allowed to remove or add memory reads or writes as long as the behavior of the code isn't altered. To prevent the compiler from making such changes, force reads to be `volatile` when you read the value from memory and cache it in a variable. Objects that are declared as `volatile` aren't used in certain optimizations because their values can change at any time. The generated code always reads the current value of a `volatile` object when it's requested, even if a previous instruction asked for a value from the same object. The reverse also applies for the same reason. The value of the `volatile` object isn't read again unless requested. For more information about `volatile`, see [`volatile`](..\cpp\volatile-cpp.md). For example: |
| 69 | + |
| 70 | +```cpp |
| 71 | +#include <Windows.h> |
| 72 | + |
| 73 | +bool TryLock(__int64* plock) |
| 74 | +{ |
| 75 | + __int64 lock = *static_cast<volatile __int64*>(plock); |
| 76 | + return (lock & 1) && |
| 77 | + _InterlockedCompareExchange64(plock, lock & ~1, lock) == lock; |
| 78 | +} |
| 79 | +``` |
| 80 | +
|
| 81 | +Compile this code with same [`/O1`](../build/reference/o1-o2-minimize-size-maximize-speed.md) option as before. The generated assembly no longer reads `plock` for use of the cached value in `lock`. |
| 82 | +
|
| 83 | +For more examples of how the code can be fixed, see [Example](#example). |
| 84 | +
|
| 85 | +Code analysis name: `INTERLOCKED_COMPARE_EXCHANGE_MISUSE` |
| 86 | +
|
| 87 | +## Example |
| 88 | +
|
| 89 | +The compiler might optimize the following code to read `plock` multiple times instead of using the cached value in `lock`: |
| 90 | +
|
| 91 | +```cpp |
| 92 | +#include <Windows.h> |
| 93 | + |
| 94 | +bool TryLock(__int64* plock) |
| 95 | +{ |
| 96 | + __int64 lock = *plock; |
| 97 | + return (lock & 1) && |
| 98 | + _InterlockedCompareExchange64(plock, lock & ~1, lock) == lock; |
| 99 | +} |
| 100 | +``` |
| 101 | + |
| 102 | +To fix the problem, force reads to be `volatile` so that the compiler doesn't optimize code to read successively from the same memory unless explicitly instructed. This prevents the optimizer from introducing unexpected behavior. |
| 103 | + |
| 104 | +The first method to treat memory as `volatile` is to take the destination address as `volatile` pointer: |
| 105 | + |
| 106 | +```cpp |
| 107 | +#include <Windows.h> |
| 108 | + |
| 109 | +bool TryLock(volatile __int64* plock) |
| 110 | +{ |
| 111 | + __int64 lock = *plock; |
| 112 | + return (lock & 1) && |
| 113 | + _InterlockedCompareExchange64(plock, lock & ~1, lock) == lock; |
| 114 | +} |
| 115 | +``` |
| 116 | +
|
| 117 | +The second method is using `volatile` read from the destination address. There are a few different ways to do this: |
| 118 | +
|
| 119 | +- Casting the pointer to `volatile` pointer before dereferencing the pointer |
| 120 | +- Creating a `volatile` pointer from the provided pointer |
| 121 | +- Using `volatile` read helper functions. |
| 122 | +
|
| 123 | +For example: |
| 124 | +
|
| 125 | +```cpp |
| 126 | +#include <Windows.h> |
| 127 | + |
| 128 | +bool TryLock(__int64* plock) |
| 129 | +{ |
| 130 | + __int64 lock = ReadNoFence64(plock); |
| 131 | + return (lock & 1) && |
| 132 | + _InterlockedCompareExchange64(plock, lock & ~1, lock) == lock; |
| 133 | +} |
| 134 | +``` |
| 135 | + |
| 136 | +## Heuristics |
| 137 | + |
| 138 | +This rule is enforced by detecting if the value in the `Destination` of the `InterlockedCompareExchange` function, or any of its derivatives, is loaded through a non-`volatile` read, and then used as the `Comparand` value. However, it doesn't explicitly check if the loaded value is used to determine the *exchange* value. It assumes the *exchange* value is related to the `Comparand` value. |
| 139 | + |
| 140 | +## See also |
| 141 | + |
| 142 | +[`InterlockedCompareExchange` function (winnt.h)](/windows/win32/api/winnt/nf-winnt-interlockedcompareexchange)\ |
| 143 | +[`_InterlockedCompareExchange` intrinsic functions](../intrinsics/interlockedcompareexchange-intrinsic-functions.md) |
0 commit comments