GNOME Bugzilla – Bug 776004
Remove pixdata loader
Last modified: 2017-07-03 12:14:14 UTC
The functions to parse it were deprecated in 2014: commit 48d76fb7f2d059013f5781b199245274998f05c9 Author: Bastien Nocera <hadess@hadess.net> Date: Tue Oct 21 19:24:11 2014 +0200 lib: Deprecate GdkPixdata GdkPixdata has a number of problems, including: - it doesn't use the original files, losing metadata - its RLE compression is ineffecient, even more so for lossy formats - the way it's implemented doesn't support data > 64 kB on Windows (See https://bugzilla.gnome.org/show_bug.cgi?id=603706) And the loader assumes that the data is 100% valid. But the was it was integrated into the loaders, it is possible to load external pixdata files, creating potential security issues.
The only worry I have about this is code (in particular under-maintained code) still using it. Not that we break the Inkscape build or Libreoffice - or worse, some small utilities that nobody is gonna rebuild for the next few months. Was using gdk-pixbuf-pixdata common back in the days? Other than that, I'm all for getting rid of it.
I haven't prepared a patch, but would propose to remove both the API and the loader for master, and remove the loader but stub out the API in older versions.
(In reply to Benjamin Otte (Company) from comment #1) > The only worry I have about this is code (in particular under-maintained > code) still using it. > Not that we break the Inkscape build or Libreoffice - or worse, some small > utilities that nobody is gonna rebuild for the next few months. > > Was using gdk-pixbuf-pixdata common back in the days? It's a 4 year old replacement for gdk-pixbuf-csource. It has been deprecated for 2 years.
I think we can't just remove the API because of API/ABI stability. Stubbing out should be fine. And if it's just 4 years old, I suppose pretty much nobody is using it, GResource is almost that old.
FTR: It'd be great to remove the code because it seems to be relatively fragile and prone to various vulnerabilities. I don't know anything about how other projects use it. Is there a clear description of how to use the new API?
Created attachment 341866 [details] [review] pixdata: Remove pixdata loader
Created attachment 341867 [details] [review] gdk-pixbuf: Don't try to load pixdata from GResource Remove _gdk_pixbuf_new_from_resource_try_mmap() so that GdkPixdata loading from internal resources is not attempted.
Created attachment 341868 [details] [review] gdk-pixbuf: Stub out GdkPixdata parsing And add a message that users will see when the loading fails.
Created attachment 341869 [details] [review] tests: Reverse pixdata tests As the loading is now disabled, we need to check for failure.
Created attachment 341872 [details] [review] tests: Reverse pixdata tests As the loading is now disabled, we need to check for failure.
Created attachment 341873 [details] [review] pixdata: Remove pixdata loader
Created attachment 341874 [details] [review] gdk-pixbuf: Don't try to load pixdata from GResource Remove _gdk_pixbuf_new_from_resource_try_mmap() so that GdkPixdata loading from internal resources is not attempted.
Created attachment 341875 [details] [review] gdk-pixbuf: Stub out GdkPixdata parsing And add a message that users will see when the loading fails.
Created attachment 341876 [details] [review] tests: Reverse pixdata tests As the loading is now disabled, we need to check for failure.
Created attachment 341878 [details] [review] tests: Add separate pixdata tests
Created attachment 341879 [details] [review] pixdata: Remove pixdata loader This won't stop pixdata-encoded data from being mmap'ed directly from the binary when using GResource, but will stop external files from being loaded, and thus block a possible attack vector. This also adjusts the new pixdata tests to make sure that external pixdata files fail to load.
I removed the loader, but the loading of GdkPixdata from GResource is still there. Attachment 341878 [details] pushed as 674227e - tests: Add separate pixdata tests Attachment 341879 [details] pushed as 349fab9 - pixdata: Remove pixdata loader
Just a heads up, Arch users are reporting this caused breakage on HexChat. It uses GResources with the to-pixdata option[1] and loads them with gdk_pixbuf_new_from_resource()[2] Anything i should change or is that a scenario you didn't intend to break? [1] - https://github.com/hexchat/hexchat/blob/master/data/hexchat.gresource.xml [2] - https://github.com/hexchat/hexchat/blob/master/src/fe-gtk/pixmaps.c#L100
(In reply to Patrick Griffis (tingping) from comment #18) > Just a heads up, Arch users are reporting this caused breakage on HexChat. > > It uses GResources with the to-pixdata option[1] and loads them with > gdk_pixbuf_new_from_resource()[2] > > Anything i should change or is that a scenario you didn't intend to break? > > [1] - > https://github.com/hexchat/hexchat/blob/master/data/hexchat.gresource.xml > [2] - > https://github.com/hexchat/hexchat/blob/master/src/fe-gtk/pixmaps.c#L100 Please file a new bug about this. You'll want to check whether _gdk_pixbuf_new_from_resource_try_mmap() is called and succeeds. It's currently tested and working in pixbuf-resource test in the test suite. Make sure that also passes.
(In reply to Patrick Griffis (tingping) from comment #18) > Just a heads up, Arch users are reporting this caused breakage on HexChat. > > It uses GResources with the to-pixdata option[1] and loads them with > gdk_pixbuf_new_from_resource()[2] > > Anything i should change or is that a scenario you didn't intend to break? > > [1] - > https://github.com/hexchat/hexchat/blob/master/data/hexchat.gresource.xml > [2] - > https://github.com/hexchat/hexchat/blob/master/src/fe-gtk/pixmaps.c#L100 I just had this problem with my current app. Removing the compressed attribute from the gresource file tags seems to have solved it. I never knew whether both compressed and to-pixdata made sense together, but I saw HexChat doing it and assumed that meant it was fine! Hope this info might be useful.
...and by "this problem", I mean: > 3 [...] Unrecognised image file format > terminate called after throwing an instance of 'Gdk::PixbufError' > Aborted where the 1st line is a diagnostic cout in the format "code() [...] what()" from the gdkmm wrapper for GdkPixbuf. So using both "compressed" and "to-pixdata" no longer works. Again, I don't know whether it ever made much sense. As a totally random guess, maybe compressed gresources had to be decompressed and then fed to the loader, hence why they no longer work after this.
(In reply to Bastien Nocera from comment #19) > (In reply to Patrick Griffis (tingping) from comment #18) > > Just a heads up, Arch users are reporting this caused breakage on HexChat. > > > > It uses GResources with the to-pixdata option[1] and loads them with > > gdk_pixbuf_new_from_resource()[2] > > > > Anything i should change or is that a scenario you didn't intend to break? > > > > [1] - > > https://github.com/hexchat/hexchat/blob/master/data/hexchat.gresource.xml > > [2] - > > https://github.com/hexchat/hexchat/blob/master/src/fe-gtk/pixmaps.c#L100 > > Please file a new bug about this. You'll want to check whether > _gdk_pixbuf_new_from_resource_try_mmap() is called and succeeds. It's > currently tested and working in pixbuf-resource test in the test suite. Make > sure that also passes. For reference of future readers, this was filed as https://bugzilla.gnome.org/show_bug.cgi?id=776105
Further regressions in bug 781583.