RenderKit / oidn

Intel® Open Image Denoise library
https://www.openimagedenoise.org/
Apache License 2.0
1.77k stars 164 forks source link

Undefined behavior reported in tza.cpp (misaligned memory read) #66

Closed GhisCo closed 4 years ago

GhisCo commented 4 years ago

Hi, At runtime, UB-sanitizer reported a "load of misaligned address 0x**** for type 'unsigned short', which requires 2 byte alignment" in tza.cpp on `(T*)ptr`:

  // Reads a value from a buffer (with bounds checking) and advances the pointer
  template<typename T>
  __forceinline T read(char*& ptr, char* end)
  {
    checkBounds(ptr, end, sizeof(T));
    const T value = *(T*)ptr;
    ptr += sizeof(T);
    return value;
  }

Indeed, if I understand https://stackoverflow.com/questions/47619944/load-of-misaligned-address-and-ubsan-finding correctly, the correct way to read the value would be to use memcpy, which is guaranteed by the standard to work even on misaligned addresses (because it must have the same effect as copying a series of individual bytes):

    T value;
    memcpy(&value, ptr, sizeof(T));

Regards,

atafra commented 4 years ago

Thanks! This currently works as expected on the platforms we support (only x86-64), but it's indeed non-standard, so I switched to memcpy. The generated code is still basically the same. The changes are in the devel branch.

GhisCo commented 4 years ago

Thanks for the prompt feedback & handling!