Created attachment 15947 [details] Test jpegs The two files cookiemon.jpg and wookiemon.jpg in the attached tar file trigger allocation overflows on x86 and amd64. Valgrind says: [for wookiemon.jpg] ==4516== Invalid write of size 1 ==4516== at 0x445D8F8: (within /usr/lib/liboil-0.3.so.0.1.0) ==4516== by 0x80497FF: jpeg_decoder_decode_entropy_segment (jpeg.c:503) ==4516== by 0x8049DEB: jpeg_decoder_decode (jpeg.c:683) ==4516== by 0x804B1E1: jpeg_decode_argb (jpeg_rgb_decoder.c:58) ==4516== by 0x8048A51: main (load.c:46) ==4516== Address 0x632C490 is 0 bytes after a block of size 0 alloc'd ==4516== at 0x442438B: malloc (vg_replace_malloc.c:149) ==4516== by 0x8049084: jpeg_decoder_init_decoder (jpeg.c:192) ==4516== by 0x8049CD3: jpeg_decoder_decode (jpeg.c:654) ==4516== by 0x804B1E1: jpeg_decode_argb (jpeg_rgb_decoder.c:58) ==4516== by 0x8048A51: main (load.c:46) [for cookiemon.jpg] ==4520== Invalid write of size 4 ==4520== at 0x804B470: yuv_mux (jpeg_rgb_decoder.c:103) ==4520== by 0x804BDDF: get_argb_420 (jpeg_rgb_decoder.c:278) ==4520== by 0x804B329: jpeg_decoder_get_argb_image (jpeg_rgb_decoder.c:89) ==4520== by 0x804B217: jpeg_decode_argb (jpeg_rgb_decoder.c:63) ==4520== by 0x8048A51: main (load.c:46) ==4520== Address 0x78C57D80 is 0 bytes after a block of size 40 alloc'd ==4520== at 0x442438B: malloc (vg_replace_malloc.c:149) ==4520== by 0x804BB54: get_argb_420 (jpeg_rgb_decoder.c:253) ==4520== by 0x804B329: jpeg_decoder_get_argb_image (jpeg_rgb_decoder.c:89) ==4520== by 0x804B217: jpeg_decode_argb (jpeg_rgb_decoder.c:63) ==4520== by 0x8048A51: main (load.c:46)
Created attachment 18705 [details] [review] don't segfault with wookiemon.jpeg I think that when handling input using g_try_malloc is way better than segfaulting.
Created attachment 18706 [details] tentative of a test This is the test i'm trying against.
Created attachment 18720 [details] [review] validate jpeg image size in the right place This one fix both image loading for me.
Created attachment 18721 [details] test for cookiemon
patch applied in git.
Created attachment 18881 [details] avoid size validation Hi, I had some fun this weekend while looking at dsjpeg more closely. The proposed patch doesn't work 100% correctly, but does make crashing the decoder more difficult. Checking for multiplication overflow when computing a*b should either take the form of a test like (a*b)/b = a or explicitly checking the sizes of a and b to avoid overflow. The attached test case will crash due to a NULL pointer dereference (on 32 bit machines), or eventually due to an out of bounds write (on 64 bit machines.) On a 64 bit machine, the result in gdb is reproduced below. (Valgrind takes way too long as the case needs to trawl through a lot of memory before hitting the segfaulting overwrite.) Program received signal SIGSEGV, Segmentation fault. 0x00002b2c733b6000 in oil_test_new () from /usr/lib/liboil-0.3.so.0 (gdb) up #1 0x0000000000404962 in get_argb_420 (dec=0x507030) at jpeg_rgb_decoder.c:279 279 oil_colorspace_argb(argbp, tmp, jfif_matrix, dec->width); (gdb) p argb No symbol "argb" in current context. (gdb) p argbp $1 = (uint32_t *) 0x2b2d9e48a010 (gdb) p tmp $2 = (uint32_t *) 0x2b2cf39bf010
Created attachment 18883 [details] bypass check to hit another malloc This test case causes a malloc argument overflow later in the code causing malloc to return NULL and a subsequent NULL ptr deref on both x86 and x86_64.
Benjamin committed a different fix that is indeed more correct than mine. I've tested both your new attachments with latest git and both are handled correctly. If you have other images please try them with 0.8.0 or git.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.