GNOME Bugzilla – Bug 765094
Test case for cve-2015-4491 is not MALLOC_CHECK_=2 safe
Last modified: 2017-07-28 12:03:57 UTC
Created attachment 326078 [details] [review] skip-perturb-for-cve-2015-4491-original-test.patch Hello, This is similar to: https://bugzilla.gnome.org/show_bug.cgi?id=753908 https://bugzilla.gnome.org/show_bug.cgi?id=754387 But I am not sure. It appears that functions to load the image in cve-2015-4491 are not MALLOC_CHECK_2=2 safe, and thus for carefully randomly chosen MALLOC_PERTURB_ the test case runs out of memory. There is a bodge fix in the already - which skips the test upon function returns ENOMEM. However, what I am arguing here, the amount of memory on the system does not matter, as even on systems with a lot of memory all of it is exhausted and before that function can return ENOMEM oomkiller kills the whole test binary for me on s390x. Thus I conclude that gdk_pixbuf_new_from_resource_at_scale() is not MALLOC_PERTRUB_ safe. And instead of skipping the test all the time, it should be run without MALLOC_PERTURB_ or the gdk_pixbuf_new_from_resource_at_scale() should be fixed to support running under MALLOC_PERTURB_ I will attach my proposed patch, which temporarily turns off and restores MALLOC_PERTRUB_. This is against 2.32.2 Regards, Dimitri.
One can probably also remove the if (skip_if_insufficient_memory (&err)) return; with proposed patch applied.
For me the following values of MALLOC_PERTURB_ were good/bad: # for i in `seq 256`; do export MALLOC_PERTURB_=$i; .libs/lt-cve-2015-4491>/dev/null && echo $i: good || echo $i: bad; done 1: bad 2: good 3: good 4: bad 5: bad 6: good 7: good 8: good 9: good 10: bad 11: good 12: good 13: good 14: bad 15: good 16: good 17: good 18: good I gave up after that.
(In reply to Dimitri John Ledkov from comment #0) <snip> > Thus I conclude that gdk_pixbuf_new_from_resource_at_scale() is not > MALLOC_PERTRUB_ safe. And instead of skipping the test all the time, it > should be run without MALLOC_PERTURB_ or the > gdk_pixbuf_new_from_resource_at_scale() should be fixed to support running > under MALLOC_PERTURB_ The latter needs to happen.
The test fails because it can't allocate 5.7 gigs of RAM. With memory overcommit, and because the memory is never initialised, this would take a bit of time, but not that much. The way to fix this problem is to avoid the allocation in the first place, as the (calculated) rowstride times the height would overflow. We'd need a way to check the calculated rowstride: /* Always align rows to 32-bit boundaries */ rowstride = (width * channels + 3) & ~3; before allocating the pixbuf in src/io-bmp.c DecodeHeader()
*** Bug 778600 has been marked as a duplicate of this bug. ***
*** Bug 758104 has been marked as a duplicate of this bug. ***
Created attachment 356430 [details] [review] gdk-pixbuf: Add gdk_pixbuf_calculate_rowstride() To calculate the rowstride without allocating memory!
Created attachment 356431 [details] [review] bmp: Avoid allocating large buffers when not needed Avoid allocating nearly 6 gigs of data when parsing the CVE-2015-4491 test only to free it when we've calculated that the rowstride * height would overflow by calculating the rowstride before doing the allocation, using the new gdk_pixbuf_calculate_rowstride() helper.
Philip, if you want to check this for sanity. I probably forgot to update the API docs, and I would have preferred not making this public...
Review of attachment 356430 [details] [review]: ::: gdk-pixbuf/gdk-pixbuf.c @@ +453,3 @@ + + /* Overflow? */ + if (width > (G_MAXUINT - 3) / channels) Is G_MAXUINT intentional? I expected G_MAXINT due to the return type being a (signed) int. Previously, the result was assigned to "rowstride" which used to be unsigned int, i.e. l448. Now, however, the result seems to a signed int.
(In reply to Tobias Mueller from comment #10) > Review of attachment 356430 [details] [review] [review]: > > ::: gdk-pixbuf/gdk-pixbuf.c > @@ +453,3 @@ > + > + /* Overflow? */ > + if (width > (G_MAXUINT - 3) / channels) > > Is G_MAXUINT intentional? I expected G_MAXINT due to the return type being > a (signed) int. > > Previously, the result was assigned to "rowstride" which used to be unsigned > int, i.e. l448. Now, however, the result seems to a signed int. This should be MAXINT, yes. Introduced in https://bugzilla.gnome.org/show_bug.cgi?id=777315 by the person I added as a reviewer ;)
Created attachment 356467 [details] [review] gdk-pixbuf: Tighten rowstride overflow check The rowstride is stored as an int, and is an int in the public API. Making it an unsigned int for those calculations would increase the limit, which would obviously cause problems when the calculated value ends up between G_MAXUINT and G_MAXINT in the positives.
Created attachment 356468 [details] [review] gdk-pixbuf: Add gdk_pixbuf_calculate_rowstride() To calculate the rowstride without allocating memory!
Created attachment 356469 [details] [review] bmp: Avoid allocating large buffers when not needed Avoid allocating nearly 6 gigs of data when parsing the CVE-2015-4491 test only to free it when we've calculated that the rowstride * height would overflow by calculating the rowstride before doing the allocation, using the new gdk_pixbuf_calculate_rowstride() helper.
Comment on attachment 356467 [details] [review] gdk-pixbuf: Tighten rowstride overflow check Attachment 356467 [details] pushed as bb5492e - gdk-pixbuf: Tighten rowstride overflow check
Review of attachment 356468 [details] [review]: r+ with some nitpicks. ::: gdk-pixbuf/gdk-pixbuf.c @@ +435,3 @@ + * + * Return value: the rowstride for the given values, or -1 in case of error. + **/ Nitpick: */ rather than **/ (that’s the modern gtk-doc style). Also, add a `Since: 2.36` line.
Review of attachment 356469 [details] [review]: Nice.
Created attachment 356508 [details] [review] gdk-pixbuf: Add gdk_pixbuf_calculate_rowstride() To calculate the rowstride without allocating memory!
Attachment 356469 [details] pushed as a8c4ac9 - bmp: Avoid allocating large buffers when not needed Attachment 356508 [details] pushed as c1a9690 - gdk-pixbuf: Add gdk_pixbuf_calculate_rowstride()