After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 776695 - bmp: left shift by 24 bits does not always fit in an "int" (clrUsed)
bmp: left shift by 24 bits does not always fit in an "int" (clrUsed)
Status: RESOLVED OBSOLETE
Product: gdk-pixbuf
Classification: Platform
Component: loaders
git master
Other Linux
: Normal normal
: ---
Assigned To: gdk-pixbuf-maint
gdk-pixbuf-maint
Depends on:
Blocks:
 
 
Reported: 2017-01-01 23:28 UTC by Tobias Mueller
Modified: 2018-05-22 13:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test file (1.28 KB, patch)
2017-01-01 23:30 UTC, Tobias Mueller
none Details | Review
potential patch (2.43 KB, patch)
2017-01-01 23:34 UTC, Tobias Mueller
needs-work Details | Review
patch (2.22 KB, patch)
2017-03-20 10:24 UTC, Tobias Mueller
none Details | Review
patch (1.05 KB, patch)
2017-03-20 10:26 UTC, Tobias Mueller
none Details | Review
patch (2.81 KB, patch)
2017-03-20 10:32 UTC, Tobias Mueller
none Details | Review

Description Tobias Mueller 2017-01-01 23:28:11 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.
Comment 1 Tobias Mueller 2017-01-01 23:30:03 UTC
Created attachment 342704 [details] [review]
test file
Comment 2 Tobias Mueller 2017-01-01 23:34:26 UTC
Created attachment 342705 [details] [review]
potential patch

the file is now being discarded with:

error: BMP image has unsupported depth
Comment 3 Philip Withnall 2017-02-20 08:56:24 UTC
(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.
Comment 4 Philip Withnall 2017-02-20 09:01:25 UTC
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.)
Comment 5 Tobias Mueller 2017-03-19 17:54:27 UTC
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?
Comment 6 Tobias Mueller 2017-03-20 10:24:37 UTC
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.
Comment 7 Tobias Mueller 2017-03-20 10:26:07 UTC
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.
Comment 8 Tobias Mueller 2017-03-20 10:32:36 UTC
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.
Comment 9 Philip Withnall 2017-03-21 10:13:09 UTC
(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.
Comment 10 GNOME Infrastructure Team 2018-05-22 13:21:29 UTC
-- 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.