After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 775896 - rgb888amsb() is broken on Big Endian, and crashes on Little Endian
rgb888amsb() is broken on Big Endian, and crashes on Little Endian
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: pixops
git master
Other Linux
: Normal normal
: ---
Assigned To: gdk-pixbuf-maint
gdk-pixbuf-maint
Depends on:
Blocks:
 
 
Reported: 2016-12-09 17:01 UTC by Max Staudt
Modified: 2017-09-22 09:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to fix rgb888amsb() (1.95 KB, patch)
2016-12-09 17:01 UTC, Max Staudt
none Details | Review
Minimal test case to use rgb888amsb() (1.76 KB, text/plain)
2016-12-13 16:28 UTC, Max Staudt
  Details

Description Max Staudt 2016-12-09 17:01:08 UTC
Created attachment 341686 [details] [review]
Patch to fix rgb888amsb()

On Little Endian, it writes memory out of bounds (i.e. when running
inside a little endian client, connecting to a big endian X server).

On Big Endian, it only processes the first 1/4 of an icon.

The effects can be seen when running IceWM. Running native on Big Endian
ppc64 with a Big Endian X11 server, the icon background was 3/4 random:

  https://bugzilla.suse.com/show_bug.cgi?id=929462

This match makes rgb888amsb() behave analogously to rgb888msb().
Comment 1 Bastien Nocera 2016-12-13 13:09:03 UTC
Can you please attach a test case for the problem as well? Something that doesn't involve running icewm would be nice.
Comment 2 Max Staudt 2016-12-13 16:28:38 UTC
Created attachment 341890 [details]
Minimal test case to use rgb888amsb()

Here is a test case, with instructions and explanations inside.

Note that you need to connect to a big-endian X server to reproduce this - such as an old-school PowerPC, or ppc64, but *not* on ppc64le.


I'm honestly surprised this test case is necessary, as the code in rgb888amsb() is clearly unfinished and will perform out-of-bounds writes in the #ifdef LITTLE case. I hope it helps in clarifying the problem though.
Comment 3 Bastien Nocera 2017-01-13 11:29:08 UTC
From the test case, there's at least one problem in the code. It's completely expected that uninitialised data is written, as the pixbuf was never initialised. You probably want to run gdk_pixbuf_fill() before showing it.

I've asked someone with access to PPC to integrate this into the tests and verify the patch.
Comment 4 Bastien Nocera 2017-01-16 18:13:32 UTC
(In reply to Max Staudt from comment #2)
<snip>
> I'm honestly surprised this test case is necessary, as the code in
> rgb888amsb() is clearly unfinished and will perform out-of-bounds writes in
> the #ifdef LITTLE case. I hope it helps in clarifying the problem though.

I couldn't see anything wrong, with your test case, on little endian (the out-of-bounds writes you mentioned). I'll be working to integrate your test case into the gdk-pixbuf test suite.
Comment 5 Max Staudt 2017-01-17 17:24:22 UTC
(In reply to Bastien Nocera from comment #4)
> I couldn't see anything wrong, with your test case, on little endian (the
> out-of-bounds writes you mentioned). I'll be working to integrate your test
> case into the gdk-pixbuf test suite.

To reproduce this, you need to run the X client on a little endian machine, and connect it to an X server which runs on a big endian machine.
Comment 6 Max Staudt 2017-01-17 17:30:12 UTC
In fact, this function rgb888amsb() is only used (as far as I can tell) when the X server is running on a big endian machine.

The way in which it misbehaves depends on whether the client (of which gdk-pixbuf is a part of) is running on a little endian machine, or on a big endian machine. On a little endian machine, you'll get out-of-bounds accesses, on big endian you'll get a pixmap that's 1/4 white and 3/4 uninitialized data.

What's really important is that the X server runs on a big endian machine, otherwise this code is never called.


Thanks a bunch for looking into this!
Comment 7 Bastien Nocera 2017-09-19 10:13:44 UTC
I never managed to get someone to test this out, so I'll take
your word for it that your patch fixes the problem. We might
be removing this code soon, and dependencies will have to start
using their own code for this instead.

This code was never fully supported as part of gdk-pixbuf (as
the path shows), and it's been on life support ever since.
Comment 8 Max Staudt 2017-09-22 09:20:40 UTC
Thanks for taking the fix!