Re-enable DrMemory test for ICOImageDecoderTests.parseAndDecodeByteByByte.

third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp

 /*
  * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 115 matching lines @@
     return (aEntryArea == bEntryArea) ? (a.m_bitCount > b.m_bitCount) : (aEntryArea > bEntryArea);
 }
 
 size_t ICOImageDecoder::decodeFrameCount()
 {
     decodeSize();
 
     // If decodeSize() fails, return the existing number of frames.  This way
     // if we get halfway through the image before decoding fails, we won't
     // suddenly start reporting that the image has zero frames.
     if (failed())

[aleksandar.stojiljkovic, 2016/06/20 23:30:24]

It relates to the part explained bellow and usage of m_data->size() here.
In case of m_data get truncated after successful call to decodeDirectory,
failed() returns false (as m_decodedOffset just makes decodeDirectory to skip
the computation).
 
I have considered another possibility, to call
if (failed() || m_data->size() < m_decodedOffset)
   return m_frameBufferCache.size()

here, but checking the same bellow where there is decodeDirectory call and check
if all data is received seemed more logical - after all decodeDirectory (even it
succeeded in past) should fail if allDataReceived=true but the data used to
calculate directory entries is truncated.

[Peter Kasting, 2016/06/20 23:35:47]

I still don't understand.  If we successfully decoded the directory, and we're
not doing any more decoding here, why should we force the decoder into the
failure state when the data is truncated after that?

If the data is truncated during the header, then some kind of call to decode
part of the header should fail.  If the data is truncated afterwards, then calls
to decode the image frames should fail.  But calls to decode the header, when
the header is complete, should succeed.

[aleksandar.stojiljkovic, 2016/06/20 23:58:39]

I am not sure when the situation could happen outside the tests.
In tests, we have this on multiple places:

1.decoder->setData(fulldata or )
2.decoder->decodeSize
3.decoder->setData(truncatedData)
4.decoder->decodeSize

2 would advance m_decodedOffset to the end of header.
4 would return results of 2, and not check if data is truncated.

Though it is truncated data, it is still fully received, and fully received
frame count that this method returns should be all frames. Thanks - changed my
mind about what makes sense here - sholdn't call setFailed() but check instead
if "allreceived" data got truncated and not return 0 frames but number of frames
in m_frameBufferCache.

[Peter Kasting, 2016/06/21 00:08:56]

It seems like if the test is calling setData() on the same decoder twice,
without resetting the decoder, and _shrinking_ the amount of data the decoder
gets the second time, that violates the way setData() should be used.

That should never happen during a normal run.  The amount of data we have for a
resource being decoded by the same instance of a decoder should never decrease.

[aleksandar.stojiljkovic, 2016/06/21 10:34:44]

You're right. This is not the fix.
After further code analysis, seems like DrMemory is making that decodeFrameCount
gets value from previously allocated and disposed decoder.
I did a placement new test for decoders, tried it on Linux and cannot reproduce.
Will try it on Windows too before setting up Drmemory to run the test locally.
Closing this.

         return m_frameBufferCache.size();
 
     // Length of sequence of completely received frames.
     for (size_t i = 0; i < m_dirEntries.size(); ++i) {
         const IconDirectoryEntry& dirEntry = m_dirEntries[i];
         if ((dirEntry.m_imageOffset + dirEntry.m_byteSize) > m_data->size())
             return i;
     }
     return m_dirEntries.size();
 }
 
 void ICOImageDecoder::setDataForPNGDecoderAtIndex(size_t index)
 {
     if (!m_pngDecoders[index])
         return;
 
     m_pngDecoders[index]->setData(m_data.get(), isAllDataReceived());
 }
 
 void ICOImageDecoder::decode(size_t index, bool onlySize)
 {
     if (failed())
         return;
 
     // Defensively clear the FastSharedBufferReader's cache, as another caller
     // may have called SharedBuffer::mergeSegmentsIntoBuffer().
     m_fastReader.clearCache();
 
-    // If we couldn't decode the image but we've received all the data, decoding
-    // has failed.
-    if ((!decodeDirectory() || (!onlySize && !decodeAtIndex(index))) && isAllDataReceived()) {
+    // If we couldn't decode the image or the data was truncated but we've
+    // received all the data, decoding has failed.
+    if ((!decodeDirectory() || m_data->size() < m_decodedOffset || (!onlySize && !decodeAtIndex(index))) && isAllDataReceived()) {

[Peter Kasting, 2016/06/20 23:08:03]

Why do we want this behavior?

If we're not trying to actually decode the missing data, it seems like decoding
shouldn't fail until we do.

[aleksandar.stojiljkovic, 2016/06/20 23:30:24]

I have put explanation above, in decodeFrameCount where decodeSize is called.

Related to decoding frames, agree - that's why the check on truncation here is
only about header space at the beginning of the file (m_decodedOffset) and not
checking frames data truncation.

         setFailed();
     // If we're done decoding this frame, we don't need the BMPImageReader or
     // PNGImageDecoder anymore.  (If we failed, these have already been
     // cleared.)
     } else if ((m_frameBufferCache.size() > index) && (m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete)) {
         m_bmpReaders[index].reset();
         m_pngDecoders[index].reset();
     }
 }
 
@@ skipping 147 matching lines @@
     ASSERT_WITH_SECURITY_IMPLICATION(index < m_dirEntries.size());
     const uint32_t imageOffset = m_dirEntries[index].m_imageOffset;
     if ((imageOffset > m_data->size()) || ((m_data->size() - imageOffset) < 4))
         return Unknown;
     char buffer[4];
     const char* data = m_fastReader.getConsecutiveData(imageOffset, 4, buffer);
     return strncmp(data, "\x89PNG", 4) ? BMP : PNG;
 }
 
 } // namespace blink