1

When running clang-tidy I get an warning when accessing an address of an array:

src/main.cpp:10:9: warning: Assigned value is garbage or undefined [clang-analyzer-core.uninitialized.Assign]
    sum += *(addr++);
        ^

The code is stripped down from a more complex example (full checksum implementation - taken from RFC1071 - and using a struct instead of the array):

#include <iostream>
#include <cstring>

static uint16_t csum(void* buffer, size_t size)
{
  uint32_t sum = 0;
  auto *addr = static_cast<uint16_t *>(buffer);
  while( size > 1 )  {
    /*  This is the inner loop */
    sum += *(addr++);
    size -= sizeof(*addr);
  }
  return static_cast<uint16_t>(~sum);
}

int main (int argc, char* argv[])
{
  uint32_t test[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
  //memset(&test, 0, sizeof(test));
  uint16_t sum = csum(test, sizeof(test));
  std::cout << argc << " " << argv[0] << " " << sum << " " << sizeof(test) << std::endl;
}

clang-tidy shows this value is garbage or undefined whenever the array is of type uint32_t or uint64_t (or using any custom packed struct) but not when being uint16_t or uint8_t - some alignment issue?

But at least for me more surprising - the warning is not shown with uint32_t, uint64_t or any custom packed struct when uncommenting the memset - why? Wasn't it proper initialized before? Do I hit some area, where C/C++ isn't defined properly?

Or will clang-tidy analyze every usage of csum - also those where the size of the buffer does not match the size variable?

Charly
  • 1,270
  • 19
  • 42
  • Alignment issues should occur the other way around, if you feed in 1-byte aligned pointers into the pointer cast to `uint16_t`. You have to do that by `memcpy` into a temporary - don't worry about performance, the compiler will eliminate it and read it as "oh, that data *may* be misaligned". – Ext3h Aug 11 '21 at 19:45
  • Even though if you did pass in something like `struct { uint32_t; uint8_t; }` the warning would be valid due to the undefined padding bytes. – Ext3h Aug 11 '21 at 19:47
  • I originally used a struct with only uint16_t and uint8_t and used ```__attribute__((packed))``` - and got the same warning. But maybe the attribute just not checked by clang-tidy. – Charly Aug 12 '21 at 05:17

1 Answers1

1

The warning is technically correct for everything which isn't a uint16_t.

You can't just reinterpret-cast a pointer to some typed memory to another incompatible type. The result of that is undefined behavior and outside of whatever guarantees the C++ standard gives you. The only unexpected case here is that you haven't gotten a warning for the uint_8 case, but I expect that's just a suppression to avoid false-positives in other use cases.

Why is that UB? Because you are making assumptions about memory layout beyond what's defined. Your code e.g. behaves entirely differently between little and big endian machines due to that cast.

You can get around that warning with the pattern:

uint8_t *src;
uint16_t tmp;
std::memcpy(&tmp, src, sizeof(tmp));
src += sizeof(tmp);

By which you don't perform any illegal casts, and actually claim all responsibility for the raw memory access.

Ext3h
  • 5,713
  • 17
  • 43
  • I changed the cast to a ```uint64_t``` (and changed the code accordingly). So there was no warning for ```uint8_t```, ```uint16_t```, ```uint32_t``` and ```uint64_t``` - but the ```__attribute__((packed))``` struct still did not work. Further more, when passing this struct by value (e.g. with a template method) the warning was gone - but passing it by ```const reference```, I got the same warning again. – Charly Aug 12 '21 at 05:20
  • Do you have an idea, why ```memset``` suppresses the warning? Regarding the endianess: The checksum is intended for structs which have content in network byte-order - but clang-tidy cannot assume that. – Charly Aug 12 '21 at 05:23
  • `memset` probably just gives a hint to clang that you have explicitly set the memory contents, so it can't argue that you can't know the memory layout of "0" in the more complex type during the reinterpret cast. – Ext3h Aug 12 '21 at 05:51
  • The warning being gone when changing the cast target to something bigger makes no sense though... – Ext3h Aug 12 '21 at 05:52