Re-enable DrMemory test for ICOImageDecoderTests.parseAndDecodeByteByByte.

ICOImageDecoder: fix decodeFrameCount when there is no enough data to parse directory entries.
Initializing struct members helps exposing the issue - otherwise, it was reproducible always only with placement new when buffer was zeroed before it.

Re-enable DrMemory test for ICOImageDecoderTests.parseAndDecodeByteByByte.

BUG=621488
Committed: https://crrev.com/4e621d9fc8a99383a04f5249f75f61a43cab2198
Cr-Commit-Position: refs/heads/master@{#402168}

[aleksandar.stojiljkovic, 2016-06-20 22:48:47]

Description was changed from

==========
ICOImageDecoder: fail size decode if data is fully received but truncated

Added unit test that triggers this after previous successful size decode.

Re-enable DrMemory test for ICOImageDecoderTests.parseAndDecodeByteByByte.

BUG=621488
==========

to

==========
ICOImageDecoder: fail size decode if data is fully received but truncated

Added unit test that triggers this after previous successful size decode.

Re-enable DrMemory test for ICOImageDecoderTests.parseAndDecodeByteByByte.

BUG=621488
==========

[aleksandar.stojiljkovic, 2016-06-20 22:48:48]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ benwells@chromium.org, pkasting@chromium.org, scroggo@google.com

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

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(right):

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:167:
if ((!decodeDirectory() || m_data->size() < m_decodedOffset || (!onlySize &&
!decodeAtIndex(index))) && isAllDataReceived()) {
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]

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(right):

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:136:
if (failed())
On 2016/06/20 23:08:03, Peter Kasting wrote:
> 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.

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.

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:167:
if ((!decodeDirectory() || m_data->size() < m_decodedOffset || (!onlySize &&
!decodeAtIndex(index))) && isAllDataReceived()) {
On 2016/06/20 23:08:03, Peter Kasting wrote:
> 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.

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.

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

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(right):

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:136:
if (failed())
On 2016/06/20 23:30:24, aleksandar.stojiljkovic wrote:
> On 2016/06/20 23:08:03, Peter Kasting wrote:
> > 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.
> 
> 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.

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:40]

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(right):

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:136:
if (failed())
On 2016/06/20 23:35:47, Peter Kasting wrote:
> On 2016/06/20 23:30:24, aleksandar.stojiljkovic wrote:
> > On 2016/06/20 23:08:03, Peter Kasting wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(right):

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:136:
if (failed())
On 2016/06/20 23:58:39, aleksandar.stojiljkovic wrote:
> On 2016/06/20 23:35:47, Peter Kasting wrote:
> > On 2016/06/20 23:30:24, aleksandar.stojiljkovic wrote:
> > > On 2016/06/20 23:08:03, Peter Kasting wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(right):

https://codereview.chromium.org/2081013002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:136:
if (failed())
On 2016/06/21 00:08:56, Peter Kasting wrote:
> On 2016/06/20 23:58:39, aleksandar.stojiljkovic wrote:
> > On 2016/06/20 23:35:47, Peter Kasting wrote:
> > > On 2016/06/20 23:30:24, aleksandar.stojiljkovic wrote:
> > > > On 2016/06/20 23:08:03, Peter Kasting wrote:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.

[aleksandar.stojiljkovic, 2016-06-22 14:33:14]

Description was changed from

==========
ICOImageDecoder: fail size decode if data is fully received but truncated

Added unit test that triggers this after previous successful size decode.

Re-enable DrMemory test for ICOImageDecoderTests.parseAndDecodeByteByByte.

BUG=621488
==========

to

==========
ICOImageDecoder: fix decodeFrameCount when there is no enough data to parse
directory entries.
Initializing struct members helps exposing the issue - otherwise, it was
reproducible always only with placement new when buffer was zeroed before it.

Re-enable DrMemory test for ICOImageDecoderTests.parseAndDecodeByteByByte.

BUG=621488
==========

[aleksandar.stojiljkovic, 2016-06-22 14:43:37]

It become easy to reproduce after implementing placement new test on Windows. 
After it, found the better way to reproduce it - to initializing DirectoryEntry
struct members to 0. That also reproduces it.
Removed the test and added the fix to ICOImageDecoder::decodeFrameCount.
Two trivial fixes in ICOImageDecoder.h.

[Peter Kasting, 2016-06-22 23:35:28]

https://codereview.chromium.org/2081013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(right):

https://codereview.chromium.org/2081013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:140:
if (m_decodedOffset < sizeOfDirectory + m_dirEntries.size() * sizeOfDirectory)
Is the problem here as follows?:

* Someone calls decodeFrameCount() after we've received enough data for the
initial directory, but not for the subsequent directory entries
* So |m_dirEntries| has been resized, but the individual entries are not
initialized
* This means their offsets and sizes are zero, and thus the loop below claims
we've received all of them
* As we start receiving more data, the frame count then recognizes that we don't
have all the necessary data, and decreases

If so, I don't think this is the right fix.  There are a lot of functions in
this file that expect that if m_dirEntries contains an entry, that entry's
details are set.

I think the best way to fix this is:

* Change processDirectory() to take an outparam, numDirectoryEntries, and set it
to the desired number of directory entries.  Remove the m_dirEntries.resize()
call from that function.
* Change processDirectoryEntries() to take this value as an argument, check
against it in the top conditional (instead of using m_dirEntries.size()), and if
the conditional succeeds, do the m_dirEntries.resize() call there.
* Change decodeDirectory() to something like this:

    size_t numDirectoryEntries = m_dirEntries.size();
    if ((m_decodedOffset < sizeOfDirectory) &&
!processDirectory(&numDirectoryEntries))
        ...

    return (m_decodedOffset >= (sizeOfDirectory + (numDirectoryEntries *
sizeOfDirEntry))) || processDirectoryEntries(numDirectoryEntries);

This ensures that m_dirEntries stays empty until we can properly set its
members.  This should avoid the need to add a null constructor for
IconDirectoryEntry as well, as we'll never allocate one of those unless we are
going to immediately set all its members or else setFailed().

[aleksandar.stojiljkovic, 2016-06-23 10:11:29]

Patchset 4. Thanks pkasting@.

https://codereview.chromium.org/2081013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(right):

https://codereview.chromium.org/2081013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:140:
if (m_decodedOffset < sizeOfDirectory + m_dirEntries.size() * sizeOfDirectory)
On 2016/06/22 23:35:28, Peter Kasting (OOO) wrote:
> Is the problem here as follows?:
> 
> * Someone calls decodeFrameCount() after we've received enough data for the
> initial directory, but not for the subsequent directory entries
> * So |m_dirEntries| has been resized, but the individual entries are not
> initialized
> * This means their offsets and sizes are zero, and thus the loop below claims
> we've received all of them
> * As we start receiving more data, the frame count then recognizes that we
don't
> have all the necessary data, and decreases
> 
> If so, I don't think this is the right fix.  There are a lot of functions in
> this file that expect that if m_dirEntries contains an entry, that entry's
> details are set.
> 
> I think the best way to fix this is:
> 
> * Change processDirectory() to take an outparam, numDirectoryEntries, and set
it
> to the desired number of directory entries.  Remove the m_dirEntries.resize()
> call from that function.
> * Change processDirectoryEntries() to take this value as an argument, check
> against it in the top conditional (instead of using m_dirEntries.size()), and
if
> the conditional succeeds, do the m_dirEntries.resize() call there.
> * Change decodeDirectory() to something like this:
> 
>     size_t numDirectoryEntries = m_dirEntries.size();
>     if ((m_decodedOffset < sizeOfDirectory) &&
> !processDirectory(&numDirectoryEntries))
>         ...
> 
>     return (m_decodedOffset >= (sizeOfDirectory + (numDirectoryEntries *
> sizeOfDirEntry))) || processDirectoryEntries(numDirectoryEntries);
> 
> This ensures that m_dirEntries stays empty until we can properly set its
> members.  This should avoid the need to add a null constructor for
> IconDirectoryEntry as well, as we'll never allocate one of those unless we are
> going to immediately set all its members or else setFailed().

Yes, that's the problem.
Used your approach with the change that i introduced member variable to hold
parsed m_dirEntriesCount before resizing m_dirEntries.
This way, we avoid multiple calls to readUint16 in processDirectory when
m_data->size( is between sizeOfDirectory (6) and (sizeOfDirectory +
m_dirEntriesCount * sizeOfDirEntry).
Thanks.

[aleksandar.stojiljkovic, 2016-06-24 20:56:25]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ scroggo@chromium.org
- scroggo@google.com

[aleksandar.stojiljkovic, 2016-06-24 20:56:48]

On 2016/06/23 10:11:29, aleksandar.stojiljkovic wrote:
> Patchset 4. Thanks pkasting@.
> 
>
https://codereview.chromium.org/2081013002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
> (right):
> 
>
https://codereview.chromium.org/2081013002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:140:
> if (m_decodedOffset < sizeOfDirectory + m_dirEntries.size() * sizeOfDirectory)
> On 2016/06/22 23:35:28, Peter Kasting (OOO) wrote:
> > Is the problem here as follows?:
> > 
> > * Someone calls decodeFrameCount() after we've received enough data for the
> > initial directory, but not for the subsequent directory entries
> > * So |m_dirEntries| has been resized, but the individual entries are not
> > initialized
> > * This means their offsets and sizes are zero, and thus the loop below
claims
> > we've received all of them
> > * As we start receiving more data, the frame count then recognizes that we
> don't
> > have all the necessary data, and decreases
> > 
> > If so, I don't think this is the right fix.  There are a lot of functions in
> > this file that expect that if m_dirEntries contains an entry, that entry's
> > details are set.
> > 
> > I think the best way to fix this is:
> > 
> > * Change processDirectory() to take an outparam, numDirectoryEntries, and
set
> it
> > to the desired number of directory entries.  Remove the
m_dirEntries.resize()
> > call from that function.
> > * Change processDirectoryEntries() to take this value as an argument, check
> > against it in the top conditional (instead of using m_dirEntries.size()),
and
> if
> > the conditional succeeds, do the m_dirEntries.resize() call there.
> > * Change decodeDirectory() to something like this:
> > 
> >     size_t numDirectoryEntries = m_dirEntries.size();
> >     if ((m_decodedOffset < sizeOfDirectory) &&
> > !processDirectory(&numDirectoryEntries))
> >         ...
> > 
> >     return (m_decodedOffset >= (sizeOfDirectory + (numDirectoryEntries *
> > sizeOfDirEntry))) || processDirectoryEntries(numDirectoryEntries);
> > 
> > This ensures that m_dirEntries stays empty until we can properly set its
> > members.  This should avoid the need to add a null constructor for
> > IconDirectoryEntry as well, as we'll never allocate one of those unless we
are
> > going to immediately set all its members or else setFailed().
> 
> Yes, that's the problem.
> Used your approach with the change that i introduced member variable to hold
> parsed m_dirEntriesCount before resizing m_dirEntries.
> This way, we avoid multiple calls to readUint16 in processDirectory when
> m_data->size( is between sizeOfDirectory (6) and (sizeOfDirectory +
> m_dirEntriesCount * sizeOfDirEntry).
> Thanks.

[aleksandar.stojiljkovic, 2016-06-24 21:00:24]

On 2016/06/23 10:11:29, aleksandar.stojiljkovic wrote:
> Patchset 4. Thanks pkasting@.
> 
>
https://codereview.chromium.org/2081013002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
> (right):
> 
>
https://codereview.chromium.org/2081013002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:140:
> if (m_decodedOffset < sizeOfDirectory + m_dirEntries.size() * sizeOfDirectory)
> On 2016/06/22 23:35:28, Peter Kasting (OOO) wrote:
> > Is the problem here as follows?:
> > 
> > * Someone calls decodeFrameCount() after we've received enough data for the
> > initial directory, but not for the subsequent directory entries
> > * So |m_dirEntries| has been resized, but the individual entries are not
> > initialized
> > * This means their offsets and sizes are zero, and thus the loop below
claims
> > we've received all of them
> > * As we start receiving more data, the frame count then recognizes that we
> don't
> > have all the necessary data, and decreases
> > 
> > If so, I don't think this is the right fix.  There are a lot of functions in
> > this file that expect that if m_dirEntries contains an entry, that entry's
> > details are set.
> > 
> > I think the best way to fix this is:
> > 
> > * Change processDirectory() to take an outparam, numDirectoryEntries, and
set
> it
> > to the desired number of directory entries.  Remove the
m_dirEntries.resize()
> > call from that function.
> > * Change processDirectoryEntries() to take this value as an argument, check
> > against it in the top conditional (instead of using m_dirEntries.size()),
and
> if
> > the conditional succeeds, do the m_dirEntries.resize() call there.
> > * Change decodeDirectory() to something like this:
> > 
> >     size_t numDirectoryEntries = m_dirEntries.size();
> >     if ((m_decodedOffset < sizeOfDirectory) &&
> > !processDirectory(&numDirectoryEntries))
> >         ...
> > 
> >     return (m_decodedOffset >= (sizeOfDirectory + (numDirectoryEntries *
> > sizeOfDirEntry))) || processDirectoryEntries(numDirectoryEntries);
> > 
> > This ensures that m_dirEntries stays empty until we can properly set its
> > members.  This should avoid the need to add a null constructor for
> > IconDirectoryEntry as well, as we'll never allocate one of those unless we
are
> > going to immediately set all its members or else setFailed().
> 
> Yes, that's the problem.
> Used your approach with the change that i introduced member variable to hold
> parsed m_dirEntriesCount before resizing m_dirEntries.
> This way, we avoid multiple calls to readUint16 in processDirectory when
> m_data->size( is between sizeOfDirectory (6) and (sizeOfDirectory +
> m_dirEntriesCount * sizeOfDirEntry).
> Thanks.

scroggo@,
As pkasting@ is OoO, I thought it is good idea to ask if you would have time to
check. It is a bug that could happen in run time.
Thanks.

[scroggo_chromium, 2016-06-24 21:36:24]

lgtm

https://codereview.chromium.org/2081013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.h
(right):

https://codereview.chromium.org/2081013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.h:44:
WTF_MAKE_NONCOPYABLE(ICOImageDecoder);
Weird that this wasn't here before...

[aleksandar.stojiljkovic, 2016-06-25 08:12:27]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[commit-bot: I haz the power, 2016-06-25 08:12:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-25 08:12:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-25 08:12:42]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[aleksandar.stojiljkovic, 2016-06-25 08:21:35]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-25 08:21:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleksandar.stojiljkovic, 2016-06-25 08:26:00]

On 2016/06/25 08:12:42, commit-bot: I haz the power wrote:
> No L-G-T-M from a valid reviewer yet. 
> CQ run can only be started by full committers or once the patch has
> received an L-G-T-M from a full committer.
> Even if an L-G-T-M may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Thanks Leon.
Seems that this would need to wait for pkasting@. You are in the OWNERS file for
decoders, gtest_exclude has '*' in OWNERS but it doesn't accept your review.

[commit-bot: I haz the power, 2016-06-25 10:14:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-25 10:14:27]

Dry run: This issue passed the CQ dry run.

[scroggo_chromium, 2016-06-27 12:34:58]

scroggo@chromium.org changed reviewers:
+ fmalita@chromium.org

[scroggo_chromium, 2016-06-27 12:34:59]

Florin, can you take a look at this CL? I gave it an LGTM, but it still needs
committer approval and I am not one yet.

[f(malita), 2016-06-27 12:52:00]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2016-06-27 12:52:00]

lgtm

[commit-bot: I haz the power, 2016-06-27 12:52:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 14:09:37]

Description was changed from

==========
ICOImageDecoder: fix decodeFrameCount when there is no enough data to parse
directory entries.
Initializing struct members helps exposing the issue - otherwise, it was
reproducible always only with placement new when buffer was zeroed before it.

Re-enable DrMemory test for ICOImageDecoderTests.parseAndDecodeByteByByte.

BUG=621488
==========

to

==========
ICOImageDecoder: fix decodeFrameCount when there is no enough data to parse
directory entries.
Initializing struct members helps exposing the issue - otherwise, it was
reproducible always only with placement new when buffer was zeroed before it.

Re-enable DrMemory test for ICOImageDecoderTests.parseAndDecodeByteByByte.

BUG=621488
==========

[commit-bot: I haz the power, 2016-06-27 14:09:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-27 14:12:10]

Description was changed from

==========
ICOImageDecoder: fix decodeFrameCount when there is no enough data to parse
directory entries.
Initializing struct members helps exposing the issue - otherwise, it was
reproducible always only with placement new when buffer was zeroed before it.

Re-enable DrMemory test for ICOImageDecoderTests.parseAndDecodeByteByByte.

BUG=621488
==========

to

==========
ICOImageDecoder: fix decodeFrameCount when there is no enough data to parse
directory entries.
Initializing struct members helps exposing the issue - otherwise, it was
reproducible always only with placement new when buffer was zeroed before it.

Re-enable DrMemory test for ICOImageDecoderTests.parseAndDecodeByteByByte.

BUG=621488

Committed: https://crrev.com/4e621d9fc8a99383a04f5249f75f61a43cab2198
Cr-Commit-Position: refs/heads/master@{#402168}
==========

[commit-bot: I haz the power, 2016-06-27 14:12:12]

Patchset 4 (id:??) landed as
https://crrev.com/4e621d9fc8a99383a04f5249f75f61a43cab2198
Cr-Commit-Position: refs/heads/master@{#402168}