Created attachment 342703 [details] [review]
test file

Note that I haven't checked whether this does indeed cause a new test case to be run as I cannot run the vanilla test suite in first place.

Created attachment 343120 [details] [review]
WIP patch

I don't know how much of an issue GCC<5 breakage really is.
Also, I wonder whether GLib should provide something like in https://lwn.net/Articles/623368/.
I'm also not clear whether it's possible to catch this condition earlier, because the patch does not fully fix this issue. It crashes later during decompression, so the approach taken in this patch might be bad.

FWIW: Coverity identified this as 1388542 and complains about the height being tained, because it's provided by the user.

Review of attachment 343120 [details] [review]:

::: gdk-pixbuf/io-bmp.c
@@ +468,3 @@
+		unsigned header_height = State->Header.height;
+		size_t length;
+		if (__builtin_mul_overflow(rowstride, header_height, &length)) {

GLib has `g_uint_checked_mul()` (since 2.48; might need to bump the dependency) which would be a bit simpler to use here.

hm. In this case, I made "length" a size_t, because the later memset call wants "length" to be a size_t.  After I changed the __builtin_mul_overflow to g_uint_checked_mul, my compiler complained about the size_t not being a uint, as required by the glib macro:

io-bmp.c:482:27: warning: passing argument 1 of ‘_GLIB_CHECKED_MUL_U32’ from incompatible pointer type [-Wincompatible-pointer-types]
   if (g_uint_checked_mul (&length, rowstride, header_height)) {
                           ^
/usr/include/glib-2.0/glib/gtypes.h:394:27: note: in definition of macro ‘g_uint_checked_mul’
     _GLIB_CHECKED_MUL_U32(dest, a, b)
                           ^
/usr/include/glib-2.0/glib/gtypes.h:419:24: note: expected ‘guint32 * {aka unsigned int *}’ but argument is of type ‘size_t * {aka long unsigned int *}’


Should length not be a size_t?  Should we just make it an uint32?  Is there any existing glib macro to do the check for us here?

(In reply to Philip Withnall from comment #4)
> Review of attachment 343120 [details] [review] [review]:
> 
> ::: gdk-pixbuf/io-bmp.c
> @@ +468,3 @@
> +		unsigned header_height = State->Header.height;
> +		size_t length;
> +		if (__builtin_mul_overflow(rowstride, header_height, &length)) {
> 
> GLib has `g_uint_checked_mul()` (since 2.48; might need to bump the
> dependency) which would be a bit simpler to use here.

Was bumped for bug 777315.

Review of attachment 343120 [details] [review]:

::: gdk-pixbuf/io-bmp.c
@@ +458,3 @@
 			}
+
+		/* We are going to multi rowstride and heigth and use them

height.

@@ +463,3 @@
+		   with a negative number yields a negative number.
+		 */
+		g_assert (State->pixbuf->rowstride >= 0);

No assertion in a library!

@@ +465,3 @@
+		g_assert (State->pixbuf->rowstride >= 0);
+		g_assert (State->Header.height >= 0);
+		unsigned rowstride = State->pixbuf->rowstride;

Declaration should be at the top of the block.

@@ +484,3 @@
 		if (State->Compressed == BI_RLE4 || State->Compressed == BI_RLE8) {
+			memset (State->pixbuf->pixels, 0, length);
+			/* length = rowstride * height.  Hence

This comment explains the change from the old code to the new code, but means nothing with just the new code :)

I've moved the test file to tests/test-images/fail/

The files in randomly-modified are just randomly modified, they're not checked for validity. So you could end up modifying the broken image in such a way that it now works ;)

Created attachment 355543 [details] [review]
bmp: Prevent multiplication overflow in DecodeHeader

The multiplication can overflow as UBSan complained:

io-bmp.c:475:63: runtime error: signed integer overflow: 524672 * 4096
cannot be represented in type 'int'

Fix this by checking the header dimensions for sanity, and then checking
whether the rowstride * height multiplication does not overflow and fits
within the range.

Attachment 355543 [details] pushed as 4154d4f - bmp: Prevent multiplication overflow in DecodeHeader