aseprite / tga

C++ library to read/write Truevision TGA/TARGA files
MIT License
23 stars 9 forks source link

Unaligned access with potential undefined behavior (with fix included) #4

Open germandiagogomez opened 3 months ago

germandiagogomez commented 3 months ago

Hello there.

I am using aseprite tga to convert files to tga for RmlUi consumption. When passing UB sanitizer I spot a potential bug (that probably does not manifest in x86, since unaligned access, even if slow, is allowed, but could in ARM). Since I was experimenting random times at which my file would not be saved, I suspect it could be related even in x86?

Anyway, the relevant line is here:

https://github.com/aseprite/tga/blob/cd3420c3a5797f1200fd1e1970ca29598f2c2436/tga.h#L195

This access is potentially misaligned. This code corrects it via a branchless implementation.

Since accessing *((T*)m_ptr) is potentially unaligned access and hence, illegal, a trick comes into play: every time we access the first result, the second result will be calculated aligned (with an incorrect value) but since the second result is not needed in that case, no undefined behavior access is done at all in that case. When the second var is accessed, then the access is aligned and legal (and correct).

template<typename T>
    T getPixel() {
      T value = *((T*)m_ptr);
      advance();
      return value;
    }

With this:

    template<typename T>
    T getPixel() {
      T value = alignedAccess<T>();
      advance();
      return value;
    }

    template<typename T>
    T alignedAccess() const {
      static_assert(std::is_unsigned<T>::value, "T is not unsigned");
      static_assert(alignof(std::uint8_t) == 1, "std::uint8_t does not have 1 byte alignment");
      constexpr auto t_alignment = alignof(T);
      auto alignment_remainder = reinterpret_cast<std::uintptr_t>(m_ptr) % t_alignment;
      T values[2] = {
          [&] {
            std::uint8_t buf[sizeof(T)];
            std::copy(m_ptr, m_ptr + sizeof(T), &buf[0]);
            return *reinterpret_cast<T*>(&buf[0]);
          }(),
          *(reinterpret_cast<T*>(m_ptr - alignment_remainder)) 
      };
      return values[static_cast<bool>(alignment_remainder == 0)];
    }

This would remove the unaligned access. Of course, equivalent and/or better optimized version can probably be done, but this one seems reasonable to me.

dacap commented 1 month ago

Hi @germandiagogomez, from what you've commented, it looks like the misalignment can happen only if the memory assigned to the Image::pixels member is misaligned and the Image::rowstride contains an invalid/unaligned value. Did you try allocating aligned memory? (_aligned_malloc / aligned_alloc)