GNOME Bugzilla – Bug 776695
bmp: left shift by 24 bits does not always fit in an "int" (clrUsed)
Last modified: 2018-05-22 13:21:29 UTC
io-bmp.c:335:28: runtime error: left shift of 222 by 24 places cannot be represented in type 'int' io-bmp.c:337:26: runtime error: shift exponent 32 is too large for 32-bit type 'int' (process:20881): GLib-GObject-WARNING **: value "-2147352580" of type 'gint' is invalid or out of range for property 'rowstride' of type 'gint' fish: “./tests/pixbuf-read /tmp/comput…” terminated by signal SIGTRAP (Trace or breakpoint trap) We can see the offending shift here: if (State->Header.size == 12) clrUsed = 1 << State->Header.depth; else clrUsed = (BIH[35] << 24) + (BIH[34] << 16) + (BIH[33] << 8) + (BIH[32]); if (clrUsed > (1U << State->Header.depth)) { g_set_error_literal (error The values get promoted to (signed) ints. If you shift by 24 digits and the high bit is set, then you overflow the range of the signed int. So the BIH[35], which gets promoted to an int, should be marked as unsigned before shifting. But also, n_colors is assigned the value of clrUsed later and n_colors seems to be an unsigned type: struct headerpair { guint32 size; gint32 width; gint32 height; guint depth; guint Negative; /* Negative = 1 -> top down BMP, Negative = 0 -> bottom up BMP */ guint n_colors; }; So I guess that clrUsed should also be unsigned. That would allow a shift of 1 by up to 31 digits. We cannot shift a 1 by 32 places in a regular 32bit int, being it unsigned or not. So I think the pragmatic approach is to check for the depth being strictly less than 31. I don't know whether this breaks some images. I guess it doesn't, though. This seems to also be CID 1388521.
Created attachment 342704 [details] [review] test file
Created attachment 342705 [details] [review] potential patch the file is now being discarded with: error: BMP image has unsupported depth
(In reply to Tobias Mueller from comment #1) > Created attachment 342704 [details] [review] [review] > test file I would squash the test file into the main patch, since it’s essentially a regression test for the bug you’re fixing.
Review of attachment 342705 [details] [review]: The commit message should also mention the (unsigned) promotions. Wikipedia says that 32b colour depths are allowed: https://en.wikipedia.org/wiki/BMP_file_format#cite_ref-bmp_1-2 Have you tested this with various (valid) bitmaps? ::: gdk-pixbuf/io-bmp.c @@ +322,3 @@ + /* We exclude 32 as valid because we use the value to + left shift a 32 bit wide int, i.e. clrUsed, and it + would not be possible do that with "32". */ Please put a ‘*’ at the start of each new line in a block comment. (See further up the file for examples.)
Thanks for the comments. I will address them. I haven't tested the patch, mainly because I don't know how. The test suite doesn't work for me as mentioned in bug 778600. Reg. the valid 32bit deep colours: It's fine to shift the "1" by "31" bit to get 2^32. But shifting by 32 places, as is currently done, is not. Maybe the right fix is to clrUsed = 1U << (State->Header.depth - 1) After checking for Header.depth being >= 1, of course. What do you think?
Created attachment 348304 [details] [review] patch This patch cares about BIH[35] and the typed of clrUsed. It shouldn't change the behaviour except for removing undefined behaviour.
Created attachment 348305 [details] [review] patch this patches lsb_32 which also suffers from undefined shifting. It is not included in the other patch for aesthetic reasons.
Created attachment 348306 [details] [review] patch This patch should be more controversial. It may be entirely wrong, so handle with care. The central change is to shift by one digit less than before, i.e. 1U << (State->Header.depth - 1); The question should probably be if anything breaks. And I can't answer that question. The clrUsed goes into n_colors which is used to allocate and manipulate memory, so I have my doubts whether this is working as intended.
(In reply to Tobias Mueller from comment #5) > Thanks for the comments. I will address them. > > I haven't tested the patch, mainly because I don't know how. The test suite > doesn't work for me as mentioned in bug 778600. You could temporarily disable the cve-2015-4491 test. > Reg. the valid 32bit deep colours: It's fine to shift the "1" by "31" bit to > get 2^32. But shifting by 32 places, as is currently done, is not. Maybe > the right fix is to > > clrUsed = 1U << (State->Header.depth - 1) > > After checking for Header.depth being >= 1, of course. > What do you think? I think I don’t know enough about the BMP standard. Look this up in the standards and work out what the correct behaviour is based on what they say, I guess.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gdk-pixbuf/issues/59.