Pepper: Fix crash on allocation failure.

Pepper: Fix crash on allocation failure.

GetPNaClResourceInfo() currently performs a 1 megabyte allocation. Though any
memory allocation can fail, this one is more likely due to its size.

This change mitigates this problem in two ways. If memory allocation fails, we
report an error and return early instead of using memory.

Additionally, this change checks the size of the file before performing a
memory allocation. This should lead to smaller allocations and more successful
plugin loads.

BUG=370965
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269791

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269948

[bbudge, 2014-05-08 00:15:59]

https://codereview.chromium.org/274673002/diff/1/components/nacl/renderer/ppb...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/274673002/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:1148: if (file_info.size > 1
<< 20) {
The old behavior seems bad since it would only do a partial read if the resource
was bigger than the limit, and if the memory was successfully allocated there
wouldn't be any error notification.

This seems much better, but why not just try to allocate the whole file size,
and return failure only when the allocation failed (when buffer.get() == NULL)?

[teravest, 2014-05-08 03:44:55]

On Wed, May 7, 2014 at 6:15 PM,  <bbudge@chromium.org> wrote:
>
>
https://codereview.chromium.org/274673002/diff/1/components/nacl/renderer/ppb...
> File components/nacl/renderer/ppb_nacl_private_impl.cc (right):
>
>
https://codereview.chromium.org/274673002/diff/1/components/nacl/renderer/ppb...
> components/nacl/renderer/ppb_nacl_private_impl.cc:1148: if
> (file_info.size > 1 << 20) {
> The old behavior seems bad since it would only do a partial read if the
> resource was bigger than the limit, and if the memory was successfully
> allocated there wouldn't be any error notification.
>
> This seems much better, but why not just try to allocate the whole file
> size, and return failure only when the allocation failed (when
> buffer.get() == NULL)?

It's an attempt to keep the allocation within a "reasonable" limit. We
already have a 1M limit for manifests, and it seemed reasonable to
preserve it here. However, I'd be fine relaxing it and removing this
size cap if you'd prefer.

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

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

[teravest, 2014-05-09 00:14:05]

ping?

[bbudge, 2014-05-09 13:29:46]

LGTM

[teravest, 2014-05-12 14:27:11]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-05-12 14:27:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/274673002/1

[commit-bot: I haz the power, 2014-05-12 15:58:27]

Change committed as 269791

[teravest, 2014-05-12 21:19:21]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-05-12 21:21:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/274673002/20001

[commit-bot: I haz the power, 2014-05-13 01:07:07]

Change committed as 269948