[Pixman] Fwd: [PATCH] Avoid out-of-bounds read when accessing individual bytes from mask

[Heiko Lewin]

Hello!

You could merge
https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/40 while
you are at it. This is mostly minor fixes.

https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/44 was
discussed on this list a while ago. It seems Soren Sandman has done a
code-review on the patch.

A "hot topic" would be
https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/20 . I
would call this patch essential for using pixman on arm64. There were
concerns about this patch though. Siarhei Siamashka dropped a line or
two about it.

There are some more open merge requests which I have not pulled into my
fork and cannot say too much about. Maybe the best would be to go though
the list on gitlab.

Regards
Heiko

Am 07.05.2021 um 15:43 schrieb Adam Jackson:
> Ugh, my fault there. That explanation all makes a ton of sense, thanks
> for the patch! I've opened a merge request for it here:
>
> https://gitlab.freedesktop.org/pixman/pixman/-/merge_requests/45
>
> This is probably worth a minor release on its own, if anyone has a
> favorite bug or patch they want to see addressed now would be a good
> time to get it landed.
>
> - ajax
>
> On Tue, Mar 16, 2021 at 11:07 AM Jonathan Kew <jfkthame at gmail.com> wrote:
>> Added note: the use of a 4-byte memcpy() to read what should just be a
>> single byte dates from commit 32a55aa8acb4048720e18fbbeaa6c7b398b1a081.
>> Most of the changes in that commit -- for reading two- or four-byte
>> values -- were fine, but it inadvertently changed a few cases that were
>> reading a single byte and made them (over-)read 4 bytes instead.
>>
>> See for example the change at:
>> https://gitlab.freedesktop.org/pixman/pixman/-/commit/32a55aa8acb4048720e18fbbeaa6c7b398b1a081#c63c4dd467accbd4a6e99979df218fb52b8a85d4_4862_4865
>>
>> The presence of the (uint32_t) cast in the old code probably contributed
>> to the error by making it look misleadingly as though 4 bytes were
>> wanted. Some of the similar code sections use a uint8_t variable
>> instead, which avoids this misdirection, which is why I've favored that
>> option in this patch.
>>
>>
>> -------- Forwarded Message --------
>> Subject: [PATCH] Avoid out-of-bounds read when accessing individual
>> bytes from mask
>> Date: Tue, 16 Mar 2021 12:43:57 +0000
>> From: Jonathan Kew <jfkthame at gmail.com>
>> To: pixman at lists.freedesktop.org
>>
>> The attached patch fixes an out-of-bounds read error detected by ASAN.
>>
>> The important changes here are a handful of places where we replace
>>
>>                memcpy(&m, mask++, sizeof(uint32_t));
>>
>> or similar code with
>>
>>                uint8_t m = *mask++;
>>
>> because we're only supposed to be reading a single byte from *mask,
>> and accessing a 32-bit value may read out of bounds (besides that
>> it reads values we don't actually want; whether this matters would
>> depend exactly how the value in m is subsequently used).
>>
>> I've also changed a bunch of other places to use this same pattern
>> (a local 8-bit variable) when reading individual bytes from the mask;
>> the code was inconsistent about this, sometimes casting the byte to
>> a uint32_t instead. This makes no actual difference, it just seemed
>> better to use a consistent pattern throughout the file.
>>
>> _______________________________________________
>> Pixman mailing list
>> Pixman at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/pixman
> _______________________________________________
> Pixman mailing list
> Pixman at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman