Opened 4 years ago

Closed 16 months ago

Last modified 16 months ago

#12056 closed defect (fixed)

wxBitmap::Load tries to read from file even if missing

Reported by: buddird Owned by: vadz
Priority: normal Milestone: 3.0.0
Component: GUI-all Version: stable-latest
Keywords: work-needed Cc:
Blocked By: Blocking:
Patch: yes

Description

When wxBitmap::Load is processing a file and the data is truncated, the function still tries to read the data. This patch makes the function return "false" if anything fails.

=== modified file 'src/common/imagbmp.cpp'
--- src/common/imagbmp.cpp	2010-02-09 19:39:34 +0000
+++ src/common/imagbmp.cpp	2010-02-14 10:30:14 +0000
@@ -522,6 +522,11 @@
             if (hasPalette)
             {
                 stream.Read(bbuf, 4);
+                if(!stream.IsOk())
+                {
+                    delete[] cmap;
+                    return false;
+                }
                 cmap[j].b = bbuf[0];
                 cmap[j].g = bbuf[1];
                 cmap[j].r = bbuf[2];
@@ -554,6 +559,11 @@
         {
             int bit = 0;
             stream.Read(dbuf, 4 * 3);
+            if(!stream.IsOk())
+            {
+                delete[] cmap;
+                return false;
+            }
             rmask = wxINT32_SWAP_ON_BE(dbuf[0]);
             gmask = wxINT32_SWAP_ON_BE(dbuf[1]);
             bmask = wxINT32_SWAP_ON_BE(dbuf[2]);
@@ -612,7 +622,15 @@
      * Reading the image data
      */
     if ( IsBmp )
+    {
         stream.SeekI(bmpOffset); // else icon, just carry on
+        if(!stream.IsOk())
+        {
+            delete[] cmap;
+            return false;
+        }
+    }
+
 
     unsigned char *data = ptr;
 
@@ -640,6 +658,11 @@
             {
                 linepos++;
                 aByte = stream.GetC();
+                if(!stream.IsOk())
+                {
+                    delete[] cmap;
+                    return false;
+                }
                 if ( bpp == 1 )
                 {
                     for (int bit = 0; bit < 8 && column < width; bit++)
@@ -658,6 +681,11 @@
                         wxUint8 first;
                         first = aByte;
                         aByte = stream.GetC();
+                        if(!stream.IsOk())
+                        {
+                            delete[] cmap;
+                            return false;
+                        }
                         if ( first == 0 )
                         {
                             if ( aByte == 0 )
@@ -673,9 +701,19 @@
                             else if ( aByte == 2 )
                             {
                                 aByte = stream.GetC();
+                                if(!stream.IsOk())
+                                {
+                                    delete[] cmap;
+                                    return false;
+                                }
                                 column += aByte;
                                 linepos = column * bpp / 4;
                                 aByte = stream.GetC();
+                                if(!stream.IsOk())
+                                {
+                                    delete[] cmap;
+                                    return false;
+                                }
                                 line -= aByte; // upside down
                             }
                             else
@@ -689,6 +727,11 @@
                                     {
                                         ++readBytes ;
                                         aByte = stream.GetC();
+                                        if(!stream.IsOk())
+                                        {
+                                            delete[] cmap;
+                                            return false;
+                                        }
                                         nibble[0] = (wxUint8)( (aByte & 0xF0) >> 4 ) ;
                                         nibble[1] = (wxUint8)( aByte & 0x0F ) ;
                                     }
@@ -700,8 +743,15 @@
                                         linepos++;
                                 }
                                 if ( readBytes & 0x01 )
+                                {
                                     aByte = stream.GetC();
-                            }
+                                     if(!stream.IsOk())
+                                    {
+                                        delete[] cmap;
+                                        return false;
+                                    }
+                                }
+                           }
                         }
                         else
                         {
@@ -741,6 +791,11 @@
                         unsigned char first;
                         first = aByte;
                         aByte = stream.GetC();
+                        if(!stream.IsOk())
+                        {
+                            delete[] cmap;
+                            return false;
+                        }
                         if ( first == 0 )
                         {
                             if ( aByte == 0 )
@@ -755,9 +810,19 @@
                             else if ( aByte == 2 )
                             {
                                 aByte = stream.GetC();
+                                if(!stream.IsOk())
+                                {
+                                    delete[] cmap;
+                                    return false;
+                                }
                                 column += aByte;
                                 linepos = column * bpp / 8;
                                 aByte = stream.GetC();
+                                if(!stream.IsOk())
+                                {
+                                    delete[] cmap;
+                                    return false;
+                                }
                                 line += aByte;
                             }
                             else
@@ -767,13 +832,25 @@
                                 {
                                     linepos++;
                                     aByte = stream.GetC();
+                                    if(!stream.IsOk())
+                                    {
+                                        delete[] cmap;
+                                        return false;
+                                    }
                                     ptr[poffset    ] = cmap[aByte].r;
                                     ptr[poffset + 1] = cmap[aByte].g;
                                     ptr[poffset + 2] = cmap[aByte].b;
                                     column++;
                                 }
                                 if ( absolute & 0x01 )
+                                {
                                     aByte = stream.GetC();
+                                    if(!stream.IsOk())
+                                    {
+                                        delete[] cmap;
+                                        return false;
+                                    }
+                                }
                             }
                         }
                         else
@@ -801,6 +878,11 @@
             else if ( bpp == 24 )
             {
                 stream.Read(bbuf, 3);
+                if(!stream.IsOk())
+                {
+                    delete[] cmap;
+                    return false;
+                }
                 linepos += 3;
                 ptr[poffset    ] = (unsigned char)bbuf[2];
                 ptr[poffset + 1] = (unsigned char)bbuf[1];
@@ -811,6 +893,11 @@
             {
                 unsigned char temp;
                 stream.Read(&aWord, 2);
+                if(!stream.IsOk())
+                {
+                    delete[] cmap;
+                    return false;
+                }
                 aWord = wxUINT16_SWAP_ON_BE(aWord);
                 linepos += 2;
                 /* use the masks and calculated amonut of shift
@@ -829,6 +916,11 @@
             {
                 unsigned char temp;
                 stream.Read(&aDword, 4);
+                if(!stream.IsOk())
+                {
+                    delete[] cmap;
+                    return false;
+                }
                 aDword = wxINT32_SWAP_ON_BE(aDword);
                 linepos += 4;
                 temp = (unsigned char)((aDword & rmask) >> rshift);
@@ -848,6 +940,11 @@
         while ( (linepos < linesize) && (comp != 1) && (comp != 2) )
         {
             stream.Read(&aByte, 1);
+            if(!stream.IsOk())
+            {
+                delete[] cmap;
+                return false;
+            }
             linepos += 1;
             if ( !stream )
                 break;
@@ -879,11 +976,23 @@
             offset = 0;
 
         stream.Read(bbuf, 2);
+        if(!stream.IsOk())
+        {
+            return false;
+        }
         stream.Read(dbuf, 16);
+        if(!stream.IsOk())
+        {
+            return false;
+        }
     }
     else
     {
         stream.Read(dbuf, 4);
+        if(!stream.IsOk())
+        {
+            return false;
+        }
     }
     #if 0 // unused
         wxInt32 size = wxINT32_SWAP_ON_BE(dbuf[0]);
@@ -891,6 +1000,10 @@
     offset = offset + wxINT32_SWAP_ON_BE(dbuf[2]);
 
     stream.Read(dbuf, 4 * 2);
+    if(!stream.IsOk())
+    {
+        return false;
+    }
     int width = wxINT32_SWAP_ON_BE((int)dbuf[0]);
     int height = wxINT32_SWAP_ON_BE((int)dbuf[1]);
     if ( !IsBmp)height = height  / 2; // for icons divide by 2
@@ -909,11 +1022,19 @@
     }
 
     stream.Read(&aWord, 2);
+    if(!stream.IsOk())
+    {
+        return false;
+    }
     /*
             TODO
             int planes = (int)wxUINT16_SWAP_ON_BE( aWord );
         */
     stream.Read(&aWord, 2);
+    if(!stream.IsOk())
+    {
+        return false;
+    }
     int bpp = wxUINT16_SWAP_ON_BE((int)aWord);
     if ( bpp != 1 && bpp != 4 && bpp != 8 && bpp != 16 && bpp != 24 && bpp != 32 )
     {
@@ -923,6 +1044,10 @@
     }
 
     stream.Read(dbuf, 4 * 4);
+    if(!stream.IsOk())
+    {
+        return false;
+    }
     int comp = wxINT32_SWAP_ON_BE((int)dbuf[0]);
     if ( comp != BI_RGB && comp != BI_RLE4 && comp != BI_RLE8 &&
          comp != BI_BITFIELDS )
@@ -933,6 +1058,10 @@
     }
 
     stream.Read(dbuf, 4 * 2);
+    if(!stream.IsOk())
+    {
+        return false;
+    }
     int ncolors = wxINT32_SWAP_ON_BE( (int)dbuf[0] );
     if (ncolors == 0)
         ncolors = 1 << bpp;

Attachments (2)

12056_fix_OnSysWrite.diff download (642 bytes) - added by catalin 17 months ago.
12056_imagbmp.cpp.diff download (21.7 KB) - added by catalin 16 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 4 years ago by vadz

  • Component changed from wxUniv (any port) to GUI-all
  • Patch unset
  • Status changed from new to confirmed
  • Version changed from 2.8.10 to 2.9-svn

The bug(s) are still there in 2.9 but the patch doesn't apply to the trunk sources. It would be nice if it could be updated (it should become simpler as well as delete [] cmap lines are not needed any more) and, most importantly, tested with 2.9.

TIA!

P.S. Please attach patch files instead of inlining them unless they're very simple and short.

Changed 17 months ago by catalin

comment:2 Changed 17 months ago by catalin

  • Patch set

Patch updated with checks for both reading from and writing to the stream.
Another issue was in wxCountingOutputStream::OnSysWrite(), fixed in a separate patch.
Tested with the wxW tests on MSW.

comment:3 follow-up: Changed 17 months ago by vadz

  • Milestone set to 3.0

Thanks, I didn't have time to really look at it yet, but one question already: why the change of the type of palette_size? Considering that size_t size is platform-dependent, it seems wrong to me to use it in this code. Why not wxUint32 or wxUint64 (if the latter is really needed)?

comment:4 in reply to: ↑ 3 Changed 17 months ago by catalin

Replying to vadz:

why the change of the type of palette_size? Considering that size_t size is platform-dependent, it seems wrong to me to use it in this code. Why not wxUint32 or wxUint64 (if the latter is really needed)?

One of the added checks is
if ( stream.Write(rgbquad, palette_size*4).LastWrite() != palette_size*4 )
LastWrite() returns size_t and it threw a warning about signed/unsigned comparison.
And if palette_size is the size of something (# of color map entries), it should definitely fit in a size_t type, shouldn't it? It could probably do with wxUint32 (also for i) but looked a bit more readable like this.

comment:5 follow-up: Changed 17 months ago by vadz

  • Keywords work-needed added

OK, let's make palette_size size_t, after looking at the code more closely I think it doesn't really matter anyhow. But actually there is a more important question, which might negate the need for its type change anyhow: why do we need to change all if ( !s.Write(buf, n) ) checks into if ( s.Write(buf, n) != n ) ones? I really hope this can be avoided, the current version is more clear and concise and less error-prone because you don't have to repeat the n. And I think it must work: if less than n bytes are written, an error must have occurred on the stream and its operator!() must return true. So why exactly is this needed and could we please avoid changing this?


I also think the changes to wxCountingOutputStream are incorrect... Of course, I also think this class is broken without these changes too. It seems there is some confusion between what m_lastcount is here, it's reused by this class to indicate the stream total size (see GetLength() implementation) but it shouldn't be.

I believe that if we apply your changes, we should also replace m_lastcount with m_currentPos in GetLength(). But to be totally correct it would seem that we need a new m_lastPos member that wouldn't be affected by OnSysSeek() and return m_lastPos from GetLength().

What do you think? Or am I missing something?

comment:6 in reply to: ↑ 5 ; follow-up: Changed 17 months ago by catalin

change all if ( !s.Write(buf, n) ) checks into if ( s.Write(buf, n).LastWrite() != n )

The docs of wxOutputStream::Write() say

Note that not all data can always be written so you must check the number of bytes really written to the stream using LastWrite() when this function returns.
In some cases (for example a write end of a pipe which is currently full) it is even possible that there is no errors and zero bytes have been written.

So is it enough to check that the stream IsOk()?


wxCountingOutputStream changes

Yes, I think a new m_lastPos member is needed, as described in comment:5
Should it be a separate ticket or just a separate patch?

comment:7 in reply to: ↑ 6 ; follow-up: Changed 16 months ago by vadz

Replying to catalin:

change all if ( !s.Write(buf, n) ) checks into if ( s.Write(buf, n).LastWrite() != n )

The docs of wxOutputStream::Write() say

Note that not all data can always be written so you must check the number of bytes really written to the stream using LastWrite() when this function returns.
In some cases (for example a write end of a pipe which is currently full) it is even possible that there is no errors and zero bytes have been written.

So is it enough to check that the stream IsOk()?

Maybe not... Looking at the existing code, some of it does assume that Write() always writes everything if it succeeds (see e.g. wxTempFileOutputStream::OnSysWrite()), but in principle it's indeed possible if we follow write(2) semantics.

So what about another idea: add bool wxOutputStream::WriteAll() which would call Write() in a loop until it either writes all the data or returns an error (or returns 0 written bytes which should probably be handled as an error too to avoid infinite loops)?


wxCountingOutputStream changes

Yes, I think a new m_lastPos member is needed, as described in comment:5
Should it be a separate ticket or just a separate patch?

I guess a separate ticket would be better as this one starts becoming rather long.

TIA!

Changed 16 months ago by catalin

comment:8 in reply to: ↑ 7 Changed 16 months ago by catalin

Replying to vadz:

So what about another idea: add bool wxOutputStream::WriteAll() which would call Write() in a loop until it either writes all the data or returns an error (or returns 0 written bytes which should probably be handled as an error too to avoid infinite loops)?

Done, patch updated.
Added also bool wxInputStream::ReadAll() with similar functionality.

comment:9 Changed 16 months ago by vadz

  • Owner set to vadz
  • Status changed from confirmed to accepted

Thanks!

I've changed it a bit more to make the code slightly more clear (IMHO) and got rid of size_t/int changes as they don't seem to be needed, will commit soon.

comment:10 Changed 16 months ago by VZ

(In [74034]) Add wxInputStream::ReadAll() and wxOutputStream::WriteAll().

Unlike Read() and Write(), these functions always transfer exactly the
specified number of bytes or fail.

See #12056.

comment:11 Changed 16 months ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [74035]) Make code reading BMP files more robust.

Check that we did correctly read the requested amount of data instead of
blindly assuming that the needed (from the point of view of BMP format
specification) number of bytes are always available -- this doesn't work so
well with corrupted or truncated files.

Closes #12056.

comment:12 Changed 16 months ago by vadz

Oops, my "improvements" actually broke everything because I didn't realize that m_lastcount was also modified inside Read(). Sorry about this, will fix in a moment.

comment:13 Changed 16 months ago by VZ

(In [74038]) Fix last count value after ReadAll() and WriteAll().

This corrects the bugs introduced when applying the patch adding these
functions in r74034: we can't simply use m_lastcount directly in them because
it's also modified by each call to Read() and Write(), so do use the temporary
variable.

See #12056.

Note: See TracTickets for help on using tickets.