GNOME Bugzilla – Bug 777315
io-bmp: Add overflow checks to BMP image saver
Last modified: 2017-07-28 15:54:19 UTC
One main patch attached, and two smaller ones which I noticed while fixing it.
Created attachment 343538 [details] [review] gdk-pixbuf: Fix overflow check in gdk_pixbuf_new() The recommended way to do an overflow check is to check against the limit you have in mind, rather than doing the calculation and seeing if it failed.
Created attachment 343539 [details] [review] io-bmp: Add overflow checks to BMP image saver Return an error if the image width or height are too big to fit in the BMP size fields.
Created attachment 343540 [details] [review] thumbnailer: Add version checks around g_type_init() call g_type_init() was deprecated (made unnecessary) in GLib 2.36.0, so don’t call it if we are compiled against a new enough version of GLib.
Created attachment 343869 [details] [review] io-bmp: Add overflow checks to BMP image saver Return an error if the image width or height are too big to fit in the BMP size fields. This bumps our GLib dependency to 2.48.0, as it uses g_uint_checked_mul(). Coverity ID: 1388532
Review of attachment 343538 [details] [review]: > The recommended way to do an overflow check is to check against the > limit you have in mind, rather than doing the calculation and seeing if > it failed. FWIW, took me a while to realise that you're not checking against the limit per se, you're shifting one of the operands to the other side to check the limit in a way that can't overflow. Can you think of a better explanation?
Review of attachment 343540 [details] [review]: I've added this patch to gnome-thumbnailer-skeleton, can you please retitle this as "Update from gnome-thumbnailer-skeleton"?
Review of attachment 343869 [details] [review]: That looks good, but I'm not sure about bumping the glib dependency, making it harder to backport that fix. I'd say commit it, and add a mention to the commit message saying that backports, for platforms using gcc, should use __builtin_umul_overflow() directly.
Committed with the requested changes, which all make sense. Thanks. Attachment 343538 [details] pushed as d579de6 - gdk-pixbuf: Fix overflow check in gdk_pixbuf_new() Attachment 343869 [details] pushed as fe541ab - io-bmp: Add overflow checks to BMP image saver
hrm. I think I cannot review the patch anymore after it has been committed. I am reopening, because the check checks for GUINT_MAX, but the resulting rowstride is a (signed) gint. afl produced a file that upset pixbuf: ➜ mat>gdb --args /tmp//pixbuf-read /tmp/crash ... Type "apropos word" to search for commands related to "word"... Reading symbols from /tmp//pixbuf-read...done. (gdb) r Starting program: /tmp//pixbuf-read /tmp/crash [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". /tmp/crash (process:2180): GLib-GObject-WARNING **: value "-1073741816" of type 'gint' is invalid or out of range for property 'rowstride' of type 'gint' Program received signal SIGTRAP, Trace/breakpoint trap. 0x00007ffff7896a5b in g_logv () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 (gdb) bt
+ Trace 237152
colorspace=colorspace@entry=GDK_COLORSPACE_RGB, has_alpha=has_alpha@entry=0, bits_per_sample=<optimised out>, width=1073741826, height=1) at gdk-pixbuf.c:468 468 return gdk_pixbuf_new_from_data (buf, colorspace, has_alpha, bits_per_sample, (gdb) p rowstride $1 = 3221225480 (gdb) p channels $2 = 3 (gdb)
Created attachment 345995 [details] [review] patch with this patch, pixbuf-read does not complain anymore. I hope it doesn't break other stuff.
Created attachment 345996 [details] [review] test file
Review of attachment 345995 [details] [review]: Cripes, good catch. ::: gdk-pixbuf/gdk-pixbuf.c @@ +455,3 @@ channels = has_alpha ? 4 : 3; + /* Overflow of rowstride (signed int property of PixBuf)? */ I’d leave it as “Overflow of rowstride?” and change the declaration above to `int rowstride` so it matches the declaration of gdk_pixbuf_new_from_data(), which we pass it to.
Review of attachment 345995 [details] [review]: I did something similar in bb5492e3360eec136525677072ca292ec94f7193 which was attached to https://bugzilla.gnome.org/show_bug.cgi?id=765094
Comment on attachment 345996 [details] [review] test file Committed with a different commit message.
That's errorint out as expected now, closing.