V610. Undefined behavior. Check the shift operator.


The analyzer has detected a shift operator that causes undefined behavior/unspecified behavior.

This is how the C++11 standard describes shift operators' work:

The shift operators << and >> group left-to-right.shift-expression << additive-expressionshift-expression >> additive-expression

The operands shall be of integral or unscoped enumeration type and integral promotions are performed.1. The type of the result is that of the promoted left operand. The behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand.2. The value of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are zero-filled. If E1 has an unsigned type, the value of the result is E1 * 2^E2, reduced modulo one more than the maximum value representable in the result type. Otherwise, if E1 has a signed type and non-negative value, and E1*2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined.3. The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined.

Let's give some code samples that cause undefined or unspecified behavior:

int A = 1;
int B;
B = A << -3; // undefined behavior
B = A << 100; // undefined behavior
B = -1 << 5; // undefined behavior
B = -1 >> 5; // unspecified behavior

These are, of course, simplified samples. In real applications, it's more complicated. Consider a sample taken from practice:

SZ_RESULT
SafeReadDirectUInt64(ISzInStream *inStream, UInt64 *value)
{
  int i;
  *value = 0;
  for (i = 0; i < 8; i++)
  {
    Byte b;
    RINOK(SafeReadDirectByte(inStream, &b));
    *value |= ((UInt32)b << (8 * i));
  }
  return SZ_OK;
}

The function tries to read a 64-bit value byte-by-byte. Unfortunately, it will fail if the number was larger than 0x00000000FFFFFFFF. Note the shift "(UInt32)b << (8 * i)". The size of the left operand is 32 bits. The shift takes from 0 to 56 bits. In practice, it will cause the high-order part of the 64-bit value to remain filled with zeroes. Theoretically, it is undefined behavior here and the result cannot be predicted.

This is the correct code:

*value |= ((UInt64)b << (8 * i));

To learn more on the issue we've discussed, please read the article "Wade not in unknown waters. Part three".

Let's examine the situation with the negative left operand in detail. Such a code usually seems to work correctly. You might think that although this is undefined behavior, all the compilers should handle the code in the same way. It's not so. It'd be more correct to say that most compilers do that in the same way. If you are concerned about code portability, you shouldn't use negative value shifts.

Here is an example to prove my words. You may get an unexpected result when using the GCC compiler for the MSP430 microprocessor. Such a situation is described here. Though the programmer blames the compiler, we in fact have that very case when the compiler acts in a different way than we're used to.

Nevertheless, we understand when programmers want the warning to be disabled for the cases when the left operand is negative. For this purpose, you may type in a special comment somewhere in the program text:

//-V610_LEFT_SIGN_OFF

This comment should be added into the header file included into all the other files. For example, such is the "stdafx.h" file. If you add this comment into a "*.cpp" file, it will affect only this particular file.



Do you make errors in the code?

Check your code
with PVS-Studio

Static code analysis
for C, C++ and C#

goto PVS-Studio;