Changeset 61619 in webkit


Ignore:
Timestamp:
Jun 22, 2010 11:02:14 AM (14 years ago)
Author:
pkasting@chromium.org
Message:

Override setFailed() in each image decoder to clean up any temporary
objects.
​https://bugs.webkit.org/show_bug.cgi?id=35411

Reviewed by Adam Barth.

In a few cases, we need to be careful to avoid deleting objects until
after they're no longer needed. These cases usually mean some jumping
through hoops, to the detriment of code simplicity.

No layout tests because this does not change the visible output of
decoding in any way.

  • platform/image-decoders/ImageDecoder.h:

(WebCore::ImageDecoder::setData):

  • platform/image-decoders/bmp/BMPImageDecoder.cpp:

(WebCore::BMPImageDecoder::setFailed):
(WebCore::BMPImageDecoder::decode):

  • platform/image-decoders/bmp/BMPImageDecoder.h:
  • platform/image-decoders/bmp/BMPImageReader.cpp:

(WebCore::BMPImageReader::decodeBMP):
(WebCore::BMPImageReader::readInfoHeaderSize):
(WebCore::BMPImageReader::processInfoHeader):
(WebCore::BMPImageReader::readInfoHeader):
(WebCore::BMPImageReader::processBitmasks):
(WebCore::BMPImageReader::processColorTable):
(WebCore::BMPImageReader::processRLEData):
(WebCore::BMPImageReader::processNonRLEData):

  • platform/image-decoders/bmp/BMPImageReader.h:

(WebCore::BMPImageReader::):

  • platform/image-decoders/gif/GIFImageDecoder.cpp:

(WebCore::GIFImageDecoder::setFailed):
(WebCore::GIFImageDecoder::decode):

  • platform/image-decoders/gif/GIFImageDecoder.h:
  • platform/image-decoders/ico/ICOImageDecoder.cpp:

(WebCore::ICOImageDecoder::setFailed):
(WebCore::ICOImageDecoder::decode):

  • platform/image-decoders/ico/ICOImageDecoder.h:
  • platform/image-decoders/jpeg/JPEGImageDecoder.cpp:

(WebCore::JPEGImageReader::decode):
(WebCore::JPEGImageDecoder::setFailed):
(WebCore::JPEGImageDecoder::decode):

  • platform/image-decoders/jpeg/JPEGImageDecoder.h:
  • platform/image-decoders/png/PNGImageDecoder.cpp:

(WebCore::PNGImageReader::decode):
(WebCore::PNGImageDecoder::PNGImageDecoder):
(WebCore::PNGImageDecoder::setFailed):
(WebCore::PNGImageDecoder::headerAvailable):
(WebCore::PNGImageDecoder::decode):

  • platform/image-decoders/png/PNGImageDecoder.h:
Location:
trunk/WebCore
Files:
14 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebCore/ChangeLog

    r61618 r61619  
     12010-06-22  Peter Kasting  <pkasting@google.com>
     2
     3        Reviewed by Adam Barth.
     4
     5        Override setFailed() in each image decoder to clean up any temporary
     6        objects.
     7        https://bugs.webkit.org/show_bug.cgi?id=35411
     8       
     9        In a few cases, we need to be careful to avoid deleting objects until
     10        after they're no longer needed.  These cases usually mean some jumping
     11        through hoops, to the detriment of code simplicity.
     12
     13        No layout tests because this does not change the visible output of
     14        decoding in any way.
     15
     16        * platform/image-decoders/ImageDecoder.h:
     17        (WebCore::ImageDecoder::setData):
     18        * platform/image-decoders/bmp/BMPImageDecoder.cpp:
     19        (WebCore::BMPImageDecoder::setFailed):
     20        (WebCore::BMPImageDecoder::decode):
     21        * platform/image-decoders/bmp/BMPImageDecoder.h:
     22        * platform/image-decoders/bmp/BMPImageReader.cpp:
     23        (WebCore::BMPImageReader::decodeBMP):
     24        (WebCore::BMPImageReader::readInfoHeaderSize):
     25        (WebCore::BMPImageReader::processInfoHeader):
     26        (WebCore::BMPImageReader::readInfoHeader):
     27        (WebCore::BMPImageReader::processBitmasks):
     28        (WebCore::BMPImageReader::processColorTable):
     29        (WebCore::BMPImageReader::processRLEData):
     30        (WebCore::BMPImageReader::processNonRLEData):
     31        * platform/image-decoders/bmp/BMPImageReader.h:
     32        (WebCore::BMPImageReader::):
     33        * platform/image-decoders/gif/GIFImageDecoder.cpp:
     34        (WebCore::GIFImageDecoder::setFailed):
     35        (WebCore::GIFImageDecoder::decode):
     36        * platform/image-decoders/gif/GIFImageDecoder.h:
     37        * platform/image-decoders/ico/ICOImageDecoder.cpp:
     38        (WebCore::ICOImageDecoder::setFailed):
     39        (WebCore::ICOImageDecoder::decode):
     40        * platform/image-decoders/ico/ICOImageDecoder.h:
     41        * platform/image-decoders/jpeg/JPEGImageDecoder.cpp:
     42        (WebCore::JPEGImageReader::decode):
     43        (WebCore::JPEGImageDecoder::setFailed):
     44        (WebCore::JPEGImageDecoder::decode):
     45        * platform/image-decoders/jpeg/JPEGImageDecoder.h:
     46        * platform/image-decoders/png/PNGImageDecoder.cpp:
     47        (WebCore::PNGImageReader::decode):
     48        (WebCore::PNGImageDecoder::PNGImageDecoder):
     49        (WebCore::PNGImageDecoder::setFailed):
     50        (WebCore::PNGImageDecoder::headerAvailable):
     51        (WebCore::PNGImageDecoder::decode):
     52        * platform/image-decoders/png/PNGImageDecoder.h:
     53
    1542010-06-04  Dimitri Glazkov  <dglazkov@chromium.org>
    255

  • trunk/WebCore/platform/image-decoders/ImageDecoder.h

    r61534 r61619  
    230230        virtual void setData(SharedBuffer* data, bool allDataReceived)
    231231        {
    232             if (failed())
     232            if (m_failed)
    233233                return;
    234234            m_data = data;
    … …  
    299299        // Sets the "decode failure" flag.  For caller convenience (since so
    300300        // many callers want to return false after calling this), returns false
    301         // to enable easy tailcalling.
     301        // to enable easy tailcalling.  Subclasses may override this to also
     302        // clean up any local data.
    302303        virtual bool setFailed()
    303304        {

  • trunk/WebCore/platform/image-decoders/bmp/BMPImageDecoder.cpp

    r55687 r61619  
    7878}
    7979
     80bool BMPImageDecoder::setFailed()
     81{
     82    m_reader.clear();
     83    return ImageDecoder::setFailed();
     84}
     85
    8086void BMPImageDecoder::decode(bool onlySize)
    8187{
    … …  
    8793    if (!decodeHelper(onlySize) && isAllDataReceived())
    8894        setFailed();
     95    // If we're done decoding the image, we don't need the BMPImageReader
     96    // anymore.  (If we failed, |m_reader| has already been cleared.)
     97    else if (!m_frameBufferCache.isEmpty() && (m_frameBufferCache.first().status() == RGBA32Buffer::FrameComplete))
     98        m_reader.clear();
    8999}
    90100

  • trunk/WebCore/platform/image-decoders/bmp/BMPImageDecoder.h

    r54823 r61619  
    4747        virtual bool isSizeAvailable();
    4848        virtual RGBA32Buffer* frameBufferAtIndex(size_t index);
     49        // CAUTION: setFailed() deletes |m_reader|.  Be careful to avoid
     50        // accessing deleted memory, especially when calling this from inside
     51        // BMPImageReader!
     52        virtual bool setFailed();
    4953
    5054    private:

  • trunk/WebCore/platform/image-decoders/bmp/BMPImageReader.cpp

    r54823 r61619  
    8080    if (m_buffer->status() == RGBA32Buffer::FrameEmpty) {
    8181        if (!m_buffer->setSize(m_parent->size().width(), m_parent->size().height()))
    82             return setFailed(); // Unable to allocate.
     82            return m_parent->setFailed(); // Unable to allocate.
    8383        m_buffer->setStatus(RGBA32Buffer::FramePartial);
    8484        // setSize() calls eraseARGB(), which resets the alpha flag, so we force
    … …  
    9696    // Decode the data.
    9797    if ((m_andMaskState != Decoding) && !pastEndOfImage(0)) {
    98         if ((m_infoHeader.biCompression == RLE4) || (m_infoHeader.biCompression == RLE8) || (m_infoHeader.biCompression == RLE24)) {
    99             if (!processRLEData())
    100                 return false;
    101         } else if (!processNonRLEData(false, 0))
     98        if ((m_infoHeader.biCompression != RLE4) && (m_infoHeader.biCompression != RLE8) && (m_infoHeader.biCompression != RLE24)) {
     99            const ProcessingResult result = processNonRLEData(false, 0);
     100            if (result != Success)
     101                return (result == Failure) ? m_parent->setFailed() : false;
     102        } else if (!processRLEData())
    102103            return false;
    103104    }
    … …  
    115116        m_andMaskState = Decoding;
    116117    }
    117     if ((m_andMaskState == Decoding) && !processNonRLEData(false, 0))
    118         return false;
     118    if (m_andMaskState == Decoding) {
     119        const ProcessingResult result = processNonRLEData(false, 0);
     120        if (result != Success)
     121            return (result == Failure) ? m_parent->setFailed() : false;
     122    }
    119123
    120124    // Done!
    … …  
    137141    // image data.
    138142    if (((m_headerOffset + m_infoHeader.biSize) < m_headerOffset) || (m_imgDataOffset && (m_imgDataOffset < (m_headerOffset + m_infoHeader.biSize))))
    139         return setFailed();
     143        return m_parent->setFailed();
    140144
    141145    // See if this is a header size we understand:
    … …  
    150154        m_isOS22x = true;
    151155    else
    152         return setFailed();
     156        return m_parent->setFailed();
    153157
    154158    return true;
    … …  
    165169    // Sanity-check header values.
    166170    if (!isInfoHeaderValid())
    167         return setFailed();
     171        return m_parent->setFailed();
    168172
    169173    // Set our size.
    170174    if (!m_parent->setSize(m_infoHeader.biWidth, m_infoHeader.biHeight))
    171         return setFailed();
     175        return m_parent->setFailed();
    172176
    173177    // For paletted images, bitmaps can set biClrUsed to 0 to mean "all
    … …  
    229233            m_isOS22x = true;
    230234        } else if (biCompression > 5)
    231             return setFailed(); // Some type we don't understand.
     235            return m_parent->setFailed(); // Some type we don't understand.
    232236        else
    233237            m_infoHeader.biCompression = static_cast<CompressionType>(biCompression);
    … …  
    396400        static const size_t SIZEOF_BITMASKS = 12;
    397401        if (((m_headerOffset + m_infoHeader.biSize + SIZEOF_BITMASKS) < (m_headerOffset + m_infoHeader.biSize)) || (m_imgDataOffset && (m_imgDataOffset < (m_headerOffset + m_infoHeader.biSize + SIZEOF_BITMASKS))))
    398             return setFailed();
     402            return m_parent->setFailed();
    399403
    400404        // Read bitmasks.
    … …  
    436440        for (int j = 0; j < i; ++j) {
    437441            if (tempMask & m_bitMasks[j])
    438                 return setFailed();
     442                return m_parent->setFailed();
    439443        }
    440444
    … …  
    449453        // Make sure bitmask is contiguous.
    450454        if (tempMask)
    451             return setFailed();
     455            return m_parent->setFailed();
    452456
    453457        // Since RGBABuffer tops out at 8 bits per channel, adjust the shift
    … …  
    468472    // Fail if we don't have enough file space for the color table.
    469473    if (((m_headerOffset + m_infoHeader.biSize + m_tableSizeInBytes) < (m_headerOffset + m_infoHeader.biSize)) || (m_imgDataOffset && (m_imgDataOffset < (m_headerOffset + m_infoHeader.biSize + m_tableSizeInBytes))))
    470         return setFailed();
     474        return m_parent->setFailed();
    471475
    472476    // Read color table.
    … …  
    530534        const uint8_t code = m_data->data()[m_decodedOffset + 1];
    531535        if ((count || (code != 1)) && pastEndOfImage(0))
    532             return setFailed();
     536            return m_parent->setFailed();
    533537
    534538        // Decode.
    535         if (count == 0) {
     539        if (!count) {
    536540            switch (code) {
    537541            case 0:  // Magic token: EOL
    … …  
    563567                    m_buffer->setHasAlpha(true);
    564568                if (((m_coord.x() + dx) > m_parent->size().width()) || pastEndOfImage(dy))
    565                     return setFailed();
     569                    return m_parent->setFailed();
    566570
    567571                // Skip intervening pixels.
    … …  
    572576            }
    573577
    574             default: // Absolute mode
     578            default: { // Absolute mode
    575579                // |code| pixels specified as in BI_RGB, zero-padded at the end
    576580                // to a multiple of 16 bits.
    … …  
    579583                // the escape bytes and then reset if decoding failed.
    580584                m_decodedOffset += 2;
    581                 if (!processNonRLEData(true, code)) {
     585                const ProcessingResult result = processNonRLEData(true, code);
     586                if (result == Failure)
     587                    return m_parent->setFailed();
     588                if (result == InsufficientData) {
    582589                    m_decodedOffset -= 2;
    583590                    return false;
    584591                }
    585592                break;
     593            }
    586594            }
    587595        } else {  // Encoded mode
    … …  
    609617                }
    610618                if ((colorIndexes[0] >= m_infoHeader.biClrUsed) || (colorIndexes[1] >= m_infoHeader.biClrUsed))
    611                     return setFailed();
     619                    return m_parent->setFailed();
    612620                for (int which = 0; m_coord.x() < endX; ) {
    613621                    setI(colorIndexes[which]);
    … …  
    621629}
    622630
    623 bool BMPImageReader::processNonRLEData(bool inRLE, int numPixels)
     631BMPImageReader::ProcessingResult BMPImageReader::processNonRLEData(bool inRLE, int numPixels)
    624632{
    625633    if (m_decodedOffset > m_data->size())
    626         return false;
     634        return InsufficientData;
    627635
    628636    if (!inRLE)
    … …  
    632640    const int endX = m_coord.x() + numPixels;
    633641    if (endX > m_parent->size().width())
    634         return setFailed();
     642        return Failure;
    635643
    636644    // Determine how many bytes of data the requested number of pixels
    … …  
    649657        // Bail if we don't have enough data for the desired number of pixels.
    650658        if ((m_data->size() - m_decodedOffset) < paddedNumBytes)
    651             return false;
     659            return InsufficientData;
    652660
    653661        if (m_infoHeader.biBitCount < 16) {
    … …  
    673681                    } else {
    674682                        if (colorIndex >= m_infoHeader.biClrUsed)
    675                             return setFailed();
     683                            return Failure;
    676684                        setI(colorIndex);
    677685                    }
    … …  
    714722        m_decodedOffset += paddedNumBytes;
    715723        if (inRLE)
    716             return true;
     724            return Success;
    717725        moveBufferToNextRow();
    718726    }
    719727
    720728    // Finished decoding whole image.
    721     return true;
     729    return Success;
    722730}
    723731
    … …  
    727735}
    728736
    729 bool BMPImageReader::setFailed()
    730 {
    731     m_parent->setFailed();
    732     m_colorTable.clear();
    733     return false;
    734 }
    735 
    736737} // namespace WebCore

  • trunk/WebCore/platform/image-decoders/bmp/BMPImageReader.h

    r54823 r61619  
    9898            Decoding,
    9999        };
     100        enum ProcessingResult {
     101            Success,
     102            Failure,
     103            InsufficientData,
     104        };
    100105
    101106        // These are based on the Windows BITMAPINFOHEADER and RGBTRIPLE
    … …  
    162167        // Processes a set of non-RLE-compressed pixels.  Two cases:
    163168        //   * inRLE = true: the data is inside an RLE-encoded bitmap.  Tries to
    164         //     process |numPixels| pixels on the current row; returns true on
    165         //     success.
     169        //     process |numPixels| pixels on the current row.
    166170        //   * inRLE = false: the data is inside a non-RLE-encoded bitmap.
    167171        //     |numPixels| is ignored.  Expects |m_coord| to point at the
    168172        //     beginning of the next row to be decoded.  Tries to process as
    169         //     many complete rows as possible.  Returns true if the whole image
    170         //     was decoded.
    171         bool processNonRLEData(bool inRLE, int numPixels);
     173        //     many complete rows as possible.  Returns InsufficientData if
     174        //     there wasn't enough data to decode the whole image.
     175        //
     176        // This function returns a ProcessingResult instead of a bool so that it
     177        // can avoid calling m_parent->setFailed(), which could lead to memory
     178        // corruption since that will delete |this| but some callers still want
     179        // to access member variables after this returns.
     180        ProcessingResult processNonRLEData(bool inRLE, int numPixels);
    172181
    173182        // Returns true if the current y-coordinate plus |numRows| would be past
    … …  
    261270        // depending on the value of |m_isTopDown|.
    262271        void moveBufferToNextRow();
    263 
    264         // Sets the "decode failure" flag and clears any local storage.  For
    265         // caller convenience (since so many callers want to return false after
    266         // calling this), returns false to enable easy tailcalling.
    267         bool setFailed();
    268272
    269273        // The decoder that owns us.

  • trunk/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp

    r60375 r61619  
    120120}
    121121
     122bool GIFImageDecoder::setFailed()
     123{
     124    m_reader.clear();
     125    return ImageDecoder::setFailed();
     126}
     127
    122128void GIFImageDecoder::clearFrameBufferCache(size_t clearBeforeFrame)
    123129{
    … …  
    301307    if (!m_reader->read((const unsigned char*)m_data->data() + m_readOffset, m_data->size() - m_readOffset, query, haltAtFrame) && isAllDataReceived())
    302308        setFailed();
    303 
    304     if (failed())
    305         m_reader.clear();
    306309}
    307310

  • trunk/WebCore/platform/image-decoders/gif/GIFImageDecoder.h

    r58589 r61619  
    5050        virtual int repetitionCount() const;
    5151        virtual RGBA32Buffer* frameBufferAtIndex(size_t index);
     52        // CAUTION: setFailed() deletes |m_reader|.  Be careful to avoid
     53        // accessing deleted memory, especially when calling this from inside
     54        // GIFImageReader!
     55        virtual bool setFailed();
    5256        virtual void clearFrameBufferCache(size_t clearBeforeFrame);
    5357

  • trunk/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp

    r56007 r61619  
    117117}
    118118
     119bool ICOImageDecoder::setFailed()
     120{
     121    m_bmpReaders.clear();
     122    m_pngDecoders.clear();
     123    return ImageDecoder::setFailed();
     124}
     125
    119126// static
    120127bool ICOImageDecoder::compareEntries(const IconDirectoryEntry& a, const IconDirectoryEntry& b)
    … …  
    148155    if ((!decodeDirectory() || (!onlySize && !decodeAtIndex(index))) && isAllDataReceived())
    149156        setFailed();
     157    // If we're done decoding this frame, we don't need the BMPImageReader or
     158    // PNGImageDecoder anymore.  (If we failed, these have already been
     159    // cleared.)
     160    else if ((m_frameBufferCache.size() > index) && (m_frameBufferCache[index].status() == RGBA32Buffer::FrameComplete)) {
     161        m_bmpReaders[index].clear();
     162        m_pngDecoders[index].clear();
     163    }
    150164}
    151165

  • trunk/WebCore/platform/image-decoders/ico/ICOImageDecoder.h

    r54978 r61619  
    5353        virtual size_t frameCount();
    5454        virtual RGBA32Buffer* frameBufferAtIndex(size_t);
     55        // CAUTION: setFailed() deletes all readers and decoders.  Be careful to
     56        // avoid accessing deleted memory, especially when calling this from
     57        // inside BMPImageReader!
     58        virtual bool setFailed();
    5559
    5660    private:

  • trunk/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp

    r58589 r61619  
    168168       
    169169        // We need to do the setjmp here. Otherwise bad things will happen
    170         if (setjmp(m_err.setjmp_buffer)) {
    171             close();
     170        if (setjmp(m_err.setjmp_buffer))
    172171            return m_decoder->setFailed();
    173         }
    174172
    175173        switch (m_state) {
    … …  
    305303            // Finish decompression.
    306304            return jpeg_finish_decompress(&m_info);
    307        
     305
    308306        case JPEG_ERROR:
    309307            // We can get here if the constructor failed.
    … …  
    401399        decode(false);
    402400    return &frame;
     401}
     402
     403bool JPEGImageDecoder::setFailed()
     404{
     405    m_reader.clear();
     406    return ImageDecoder::setFailed();
    403407}
    404408
    … …  
    483487    if (!m_reader->decode(m_data->buffer(), onlySize) && isAllDataReceived())
    484488        setFailed();
    485 
    486     if (failed() || (!m_frameBufferCache.isEmpty() && m_frameBufferCache[0].status() == RGBA32Buffer::FrameComplete))
     489    // If we're done decoding the image, we don't need the JPEGImageReader
     490    // anymore.  (If we failed, |m_reader| has already been cleared.)
     491    else if (!m_frameBufferCache.isEmpty() && (m_frameBufferCache[0].status() == RGBA32Buffer::FrameComplete))
    487492        m_reader.clear();
    488493}

  • trunk/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.h

    r58589 r61619  
    4747        virtual RGBA32Buffer* frameBufferAtIndex(size_t index);
    4848        virtual bool supportsAlpha() const { return false; }
     49        // CAUTION: setFailed() deletes |m_reader|.  Be careful to avoid
     50        // accessing deleted memory, especially when calling this from inside
     51        // JPEGImageReader!
     52        virtual bool setFailed();
    4953
    5054        bool outputScanlines();

  • trunk/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp

    r58589 r61619  
    126126
    127127        // We need to do the setjmp here. Otherwise bad things will happen.
    128         if (setjmp(m_png->jmpbuf)) {
    129             close();
     128        if (setjmp(m_png->jmpbuf))
    130129            return decoder->setFailed();
    131         }
    132130
    133131        const char* segment;
    … …  
    167165
    168166PNGImageDecoder::PNGImageDecoder()
     167    : m_doNothingOnFailure(false)
    169168{
    170169}
    … …  
    203202        decode(false);
    204203    return &frame;
     204}
     205
     206bool PNGImageDecoder::setFailed()
     207{
     208    if (m_doNothingOnFailure)
     209        return false;
     210    m_reader.clear();
     211    return ImageDecoder::setFailed();
    205212}
    206213
    … …  
    218225    }
    219226   
    220     // We can fill in the size now that the header is available.
    221     if (!setSize(width, height)) {
     227    // We can fill in the size now that the header is available.  Avoid memory
     228    // corruption issues by neutering setFailed() during this call; if we don't
     229    // do this, failures will cause |m_reader| to be deleted, and our jmpbuf
     230    // will cease to exist.  Note that we'll still properly set the failure flag
     231    // in this case as soon as we longjmp().
     232    m_doNothingOnFailure = true;
     233    bool result = setSize(width, height);
     234    m_doNothingOnFailure = false;
     235    if (!result) {
    222236        longjmp(png->jmpbuf, 1);
    223237        return;
    … …  
    374388    if (!m_reader->decode(*m_data, onlySize) && isAllDataReceived())
    375389        setFailed();
    376    
    377     if (failed() || isComplete())
     390    // If we're done decoding the image, we don't need the PNGImageReader
     391    // anymore.  (If we failed, |m_reader| has already been cleared.)
     392    else if (isComplete())
    378393        m_reader.clear();
    379394}

  • trunk/WebCore/platform/image-decoders/png/PNGImageDecoder.h

    r58589 r61619  
    4545        virtual bool setSize(unsigned width, unsigned height);
    4646        virtual RGBA32Buffer* frameBufferAtIndex(size_t index);
     47        // CAUTION: setFailed() deletes |m_reader|.  Be careful to avoid
     48        // accessing deleted memory, especially when calling this from inside
     49        // PNGImageReader!
     50        virtual bool setFailed();
    4751
    4852        // Callbacks from libpng
    … …  
    6367
    6468        OwnPtr<PNGImageReader> m_reader;
     69        bool m_doNothingOnFailure;
    6570    };
    6671
Note: See TracChangeset for help on using the changeset viewer.