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 <[email protected]> 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 <[email protected]> > To: [email protected] > > 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 > [email protected] > https://lists.freedesktop.org/mailman/listinfo/pixman _______________________________________________ Pixman mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/pixman
