FalsePositive : Shifting 32-bit value

[Sander Bouwhuis]

I have the AddDataBlock function which is part of the implementation of the Whirlpool hash function.
CppCheck claims the following I changed the line numbers to match this comment:

Shifting 32-bit value by 520 bits is undefined behaviour. See condition at line 53.
line 55 -> Assuming that condition 'm_i32BufferBits==512' is not redundant
line 54 -> Assignment to 'm_i32BufferBits+=8-u8BufferRem'
line 53 -> Shift

Because u8BufferRem is an unsigned int of 8-bits and line 30 makes sure it can only have a value from 0 through 7, how can there possibly be a shift of 520 bits?!?

/*   1 */ // This function adds a block of data to the hash
/*   2 */ void CCrypto_Whirlpool::AddDataBlock(const uint8 *pu8Data, int32 i32DataLen)
/*   3 */ {
/*   4 */   /*
/*   5 */                       srcPos
/*   6 */                       |
/*   7 */                       +-------+-------+-------
/*   8 */                         ||||||||||||||||||||| source
/*   9 */                       +-------+-------+-------
/*  10 */   +-------+-------+-------+-------+-------+-------
/*  11 */   ||||||||||||||||||||||                           buffer
/*  12 */   +-------+-------+-------+-------+-------+-------
/*  13 */                   |
/*  14 */                   bufferPos
/*  15 */   */
/*  16 */ 
/*  17 */   try
/*  18 */   {
/*  19 */     CHECK_MEM_LEAKS
/*  20 */ 
/*  21 */     int32  i32SrcPos = 0, i32SrcGap;
/*  22 */     uint8  u8BufferRem;
/*  23 */     uint32 u32Byte;
/*  24 */ 
/*  25 */     // Convert the length in bytes to the length in bits
/*  26 */     i32DataLen *= 8;
/*  27 */ 
/*  28 */     // Initialize the data
/*  29 */     i32SrcGap    = (8 - (i32DataLen & 7)) & 7; // space on source[sourcePos]
/*  30 */     u8BufferRem = m_i32BufferBits & 7; // occupied bits on buffer[bufferPos]
/*  31 */ 
/*  32 */     {
/*  33 */       uint32 u32Carry = 0;
/*  34 */       uint64 u64Value = i32DataLen;
/*  35 */ 
/*  36 */       // Tally the length of the added data
/*  37 */       for(int32 i32=31;i32>=0 && (u32Carry!=0 || u64Value!= 0);i32--)
/*  38 */       {
/*  39 */         u32Carry += m_pu8BitLength[i32] + uint8(u64Value & 0xff);
/*  40 */         m_pu8BitLength[i32] = uint8(u32Carry & 0xff);
/*  41 */         u32Carry >>= 8;
/*  42 */         u64Value >>= 8;
/*  43 */       }
/*  44 */     }
/*  45 */ 
/*  46 */     // Process the source data in chunks of 8 bits
/*  47 */     while(i32DataLen > 8)
/*  48 */     {
/*  49 */       // Take a byte from the source (NOTE: at least pu8Data[i32SrcPos] and pu8Data[i32SrcPos+1] contain data)
/*  50 */       u32Byte = ((pu8Data[i32SrcPos] << i32SrcGap) & 0xff) | ((pu8Data[i32SrcPos + 1] & 0xff) >> (8 - i32SrcGap));
/*  51 */ 
/*  52 */       // Process the byte
/*  53 */       m_pu8Buffer[m_i32BufferPos++] |= uint8((u32Byte >> u8BufferRem) & 0xff);
/*  54 */       m_i32BufferBits += 8 - u8BufferRem; // bufferBits = 8*bufferPos;
/*  55 */       if(m_i32BufferBits == 512)
/*  56 */       {
/*  57 */         // Process the data block
/*  58 */         Transform();
/*  59 */ 
/*  60 */         // Reset the buffer
/*  61 */         m_i32BufferBits = 0;
/*  62 */         m_i32BufferPos  = 0;
/*  63 */       }
/*  64 */       m_pu8Buffer[m_i32BufferPos] = uint8((u32Byte << (8 - u8BufferRem)) & 0xff);
/*  65 */       m_i32BufferBits += u8BufferRem;
/*  66 */ 
/*  67 */       // Prepare for the next byte
/*  68 */       i32DataLen -= 8;
/*  69 */       i32SrcPos++;
/*  70 */     }
/*  71 */ 
/*  72 */     // Now there are at most 8 bits of source left, and it is in pu8Data[i32SrcPos]
/*  73 */     if(i32DataLen > 0)
/*  74 */     {
/*  75 */       // Take the last bits (NOTE: bits are left-justified on b)
/*  76 */       u32Byte = (pu8Data[i32SrcPos] << i32SrcGap) & 0xff;
/*  77 */ 
/*  78 */       // Process the remaining bits
/*  79 */       m_pu8Buffer[m_i32BufferPos] |= u32Byte >> u8BufferRem;
/*  80 */     }
/*  81 */     else
/*  82 */       u32Byte = 0;
/*  83 */ 
/*  84 */     if(u8BufferRem + i32DataLen < 8)
/*  85 */     {
/*  86 */       // All remaining data fits on m_pu8Buffer[m_i32BufferPos], and there still remains some space
/*  87 */       m_i32BufferBits += i32DataLen;
/*  88 */     }
/*  89 */     else
/*  90 */     {
/*  91 */       // m_pu8Buffer[m_i32BufferPos] is full
/*  92 */       m_i32BufferPos++;
/*  93 */       m_i32BufferBits += 8 - u8BufferRem; // bufferBits = 8*bufferPos;
/*  94 */       i32DataLen -= 8 - u8BufferRem;
/*  95 */ 
/*  96 */       // Now there are at most 8 bits of source left, and it is in pu8Data[i32SrcPos]
/*  97 */       if(m_i32BufferBits == 512)
/*  98 */       {
/*  99 */         // Process the data block
/* 100 */         Transform();
/* 101 */ 
/* 102 */         // Reset the buffer
/* 103 */         m_i32BufferBits = m_i32BufferPos = 0;
/* 104 */       }
/* 105 */       m_pu8Buffer[m_i32BufferPos] = uint8((u32Byte << (8 - u8BufferRem)) & 0xff);
/* 106 */       m_i32BufferBits += i32DataLen;
/* 107 */     }
/* 108 */   }
/* 109 */   catch(...) { OnException(m_pWstrDebugLog, __FILE__, __LINE__, __FUNCSIG__, __TIMESTAMP__, nullptr); }
/* 110 */ }

[Daniel Marjamäki]

I don't see it neither. Possibly it's a false positive. I can't reproduce with this code.

Can you try to reduce it? Try to remove various includes. And other functions. etc..

[Sander Bouwhuis]

Here is a shorter version which doesn't make any sense anymore, but still gives the same error:

void CCrypto_Whirlpool::CppCheckFunc(const uint8 *pu8Data, int32 i32DataLen)
{
  uint8 u8BufferRem = m_i32BufferBits & 7;

  while(i32DataLen > 8)
  {
    uint32 u32Byte = pu8Data[i32DataLen];

    m_pu8Buffer[m_i32BufferPos++] |= uint8((u32Byte >> u8BufferRem) & 0xff);
    m_i32BufferBits += 8 - u8BufferRem;
    if(m_i32BufferBits == 512)
      i32DataLen -= 8;
  }
}

[CHR]

Standalone example:

int32_t m_i32BufferBits;
uint8_t m_pu8Buffer[1024];
void CppCheckFunc(const uint8_t *pu8Data, int32_t i32DataLen)
{
  uint8_t u8BufferRem = m_i32BufferBits & 7;

  while(i32DataLen > 8)
  {
    uint32_t u32Byte = pu8Data[i32DataLen];

    m_pu8Buffer[m_i32BufferPos++] |= uint8_t((u32Byte >> u8BufferRem) & 0xff);
    m_i32BufferBits += 8 - u8BufferRem;
    if(m_i32BufferBits == 512)
      i32DataLen -= 8;
  }
}

Output:

bar.cpp:11:55: warning: Shifting 32-bit value by 520 bits is undefined behaviour. See condition at line 13. [shiftTooManyBits]
    m_pu8Buffer[m_i32BufferPos++] |= uint8_t((u32Byte >> u8BufferRem) & 0xff);
                                                      ^
bar.cpp:13:24: note: Assuming that condition 'm_i32BufferBits==512' is not redundant
    if(m_i32BufferBits == 512)
                       ^
bar.cpp:12:26: note: Assignment to 'm_i32BufferBits+=8-u8BufferRem'
    m_i32BufferBits += 8 - u8BufferRem;
                         ^
bar.cpp:11:55: note: Shift
    m_pu8Buffer[m_i32BufferPos++] |= uint8_t((u32Byte >> u8BufferRem) & 0xff);
                                                      ^
bar.cpp:3:0: style: The function 'CppCheckFunc' is never used. [unusedFunction]

^

[CHR]

I have created a ticket here: https://trac.cppcheck.net/ticket/10773