aseprite / aseprite/tga

Unaligned access with potential undefined behavior (with fix included)

Open
#4 1 comment 0 reactions 1 assignee Claimed by @dacap View on GitHub
Dominant language
C++
Stars
34
Forks
13
PR merge metrics
No merged PRs in 30d

Description

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
T getPixel() {
T value = *((T*)m_ptr);
advance();
return value;
}
```

With this:

```
template
T getPixel() {
T value = alignedAccess();
advance();
return value;
}

template
T alignedAccess() const {
static_assert(std::is_unsigned::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(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(&buf[0]);
}(),
*(reinterpret_cast(m_ptr - alignment_remainder))
};
return values[static_cast(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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.