Validate Bucket data in ShaderSourceBucket.

Validate Bucket data in ShaderSourceBucket.

As of now only the received bucket and
the size of bucket are validated. We
should check for validation of bucket
data before we start using it and report
failures.

BUG=240467
Committed: https://crrev.com/6df438ed2adaf24fa2f4a92d4f3863825247b910
Cr-Commit-Position: refs/heads/master@{#309882}

[sivag, 2014-12-24 10:16:01]

siva.gunturi@samsung.com changed reviewers:
+ sievers@chromium.org

[sivag, 2014-12-24 10:18:09]

siva.gunturi@samsung.com changed reviewers:
+ zmo@google.com

[sivag, 2014-12-24 13:31:12]

ptal..

[sivag, 2014-12-24 16:16:30]

siva.gunturi@samsung.com changed reviewers:
+ piman@chromium.org

[sivag, 2014-12-30 09:15:20]

siva.gunturi@samsung.com changed reviewers:
+ bajones@chromium.org

[Zhenyao Mo, 2014-12-31 00:13:46]

zmo@chromium.org changed reviewers:
+ zmo@chromium.org

[Zhenyao Mo, 2014-12-31 00:13:47]

https://codereview.chromium.org/822953002/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/822953002/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7094: if (!bucket->GetData(0,
bucket->size() - 1)) {
Good catch.  But instead of checking it here, it makes more sense to strengthen
the CommonDecoder::Bucket::SetSize to handle mem allocation failure, i.e., when
mem alloc fails, set size to 0.

[sivag, 2014-12-31 08:06:42]

siva.gunturi@samsung.com changed reviewers:
- zmo@google.com

[sivag, 2014-12-31 11:22:20]

ptal..

https://codereview.chromium.org/822953002/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/822953002/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7094: if (!bucket->GetData(0,
bucket->size() - 1)) {
On 2014/12/31 00:13:47, Zhenyao Mo wrote:
> Good catch.  But instead of checking it here, it makes more sense to
strengthen
> the CommonDecoder::Bucket::SetSize to handle mem allocation failure, i.e.,
when
> mem alloc fails, set size to 0.

Done.

[Zhenyao Mo, 2015-01-02 18:15:50]

LGTM

[sivag, 2015-01-04 02:42:24]

The CQ bit was checked by siva.gunturi@samsung.com

[commit-bot: I haz the power, 2015-01-04 02:43:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/822953002/100001

[commit-bot: I haz the power, 2015-01-04 03:37:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-04 03:38:23]

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

[piman, 2015-01-05 18:42:08]

On Tue, Dec 30, 2014 at 4:13 PM, <zmo@chromium.org> wrote:

>
> https://codereview.chromium.org/822953002/diff/60001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.org/822953002/diff/60001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode7094
> gpu/command_buffer/service/gles2_cmd_decoder.cc:7094: if
> (!bucket->GetData(0, bucket->size() - 1)) {
> Good catch.  But instead of checking it here, it makes more sense to
> strengthen the CommonDecoder::Bucket::SetSize to handle mem allocation
> failure, i.e., when mem alloc fails, set size to 0.
>

Mmh, allocations "can't" fail. new never returns NULL. In Chrome we
explicitly abort (crash) before returning from the allocator in case of
allocation failure.


>
> https://codereview.chromium.org/822953002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Zhenyao Mo, 2015-01-05 18:56:03]

On 2015/01/05 18:42:08, piman (Very slow to review) wrote:
> On Tue, Dec 30, 2014 at 4:13 PM, <mailto:zmo@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/822953002/diff/60001/gpu/
> > command_buffer/service/gles2_cmd_decoder.cc
> > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> >
> > https://codereview.chromium.org/822953002/diff/60001/gpu/
> > command_buffer/service/gles2_cmd_decoder.cc#newcode7094
> > gpu/command_buffer/service/gles2_cmd_decoder.cc:7094: if
> > (!bucket->GetData(0, bucket->size() - 1)) {
> > Good catch.  But instead of checking it here, it makes more sense to
> > strengthen the CommonDecoder::Bucket::SetSize to handle mem allocation
> > failure, i.e., when mem alloc fails, set size to 0.
> >
> 
> Mmh, allocations "can't" fail. new never returns NULL. In Chrome we
> explicitly abort (crash) before returning from the allocator in case of
> allocation failure.

Ah, I didn't know that.  So nothing needs to be done here actually.

@sikugu, sorry for the incorrect information.  Could you revert your CL?

> 
> 
> >
> > https://codereview.chromium.org/822953002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sivag, 2015-01-06 11:09:59]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/802323005/ by siva.gunturi@samsung.com.

The reason for reverting is: No need to check for memory allocation failure.
New never returns NULL. In Chrome we
explicitly abort (crash) before returning 
from the allocator in case of allocation failure..

[sivag, 2015-01-06 11:13:11]

On 2015/01/05 18:56:03, Zhenyao Mo wrote:
> On 2015/01/05 18:42:08, piman (Very slow to review) wrote:
> > On Tue, Dec 30, 2014 at 4:13 PM, <mailto:zmo@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.org/822953002/diff/60001/gpu/
> > > command_buffer/service/gles2_cmd_decoder.cc
> > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> > >
> > > https://codereview.chromium.org/822953002/diff/60001/gpu/
> > > command_buffer/service/gles2_cmd_decoder.cc#newcode7094
> > > gpu/command_buffer/service/gles2_cmd_decoder.cc:7094: if
> > > (!bucket->GetData(0, bucket->size() - 1)) {
> > > Good catch.  But instead of checking it here, it makes more sense to
> > > strengthen the CommonDecoder::Bucket::SetSize to handle mem allocation
> > > failure, i.e., when mem alloc fails, set size to 0.
> > >
> > 
> > Mmh, allocations "can't" fail. new never returns NULL. In Chrome we
> > explicitly abort (crash) before returning from the allocator in case of
> > allocation failure.
> 
> Ah, I didn't know that.  So nothing needs to be done here actually.
> 
> @sikugu, sorry for the incorrect information.  Could you revert your CL?
> 
> > 
> > 
> > >
> > > https://codereview.chromium.org/822953002/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

Done.