Pepper: Refactor PNaCl OpenManifestEntry logic.

Pepper: Refactor PNaCl OpenManifestEntry logic.

This change moves some more OpenManifestEntry logic out of the trusted plugin.
This code specifically looks up file descriptors for support files for helper
processes (llc, ld) for the PNaCl translator.

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

[bbudge, 2014-05-30 17:57:56]

https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:530: // strip component
scheme.
Since we're in chrome now, can you use GURL to parse URLs now?

[teravest, 2014-05-30 18:09:58]

On Fri, May 30, 2014 at 11:57 AM,  <bbudge@chromium.org> wrote:
>
>
https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
> File components/nacl/renderer/ppb_nacl_private_impl.cc (right):
>
>
https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:530: // strip
> component scheme.
> Since we're in chrome now, can you use GURL to parse URLs now?

I don't think using GURL would help much; I think the parsing there
would just split off the scheme for us, but we'd still have to parse
off the leading "pnacl-translator" in the path.

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

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

[bbudge, 2014-05-30 18:31:49]

On 2014/05/30 18:09:58, teravest wrote:
> On Fri, May 30, 2014 at 11:57 AM,  <mailto:bbudge@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
> > File components/nacl/renderer/ppb_nacl_private_impl.cc (right):
> >
> >
>
https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
> > components/nacl/renderer/ppb_nacl_private_impl.cc:530: // strip
> > component scheme.
> > Since we're in chrome now, can you use GURL to parse URLs now?
> 
> I don't think using GURL would help much; I think the parsing there
> would just split off the scheme for us, but we'd still have to parse
> off the leading "pnacl-translator" in the path.
> 
> >
> > https://codereview.chromium.org/307933005/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

It's not so much to help as to protect against exploits. URL parsing is
notoriously easy to get wrong. If these URLs are completely trusted, then it's
fine as is, but it might still be worthwhile if will need to do more URL
processing in this file.

[teravest, 2014-05-30 19:54:00]

On Fri, May 30, 2014 at 12:31 PM,  <bbudge@chromium.org> wrote:
> On 2014/05/30 18:09:58, teravest wrote:
>
>> On Fri, May 30, 2014 at 11:57 AM,  <mailto:bbudge@chromium.org> wrote:
>> >
>> >
>
>
>
https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
>>
>> > File components/nacl/renderer/ppb_nacl_private_impl.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
>>
>> > components/nacl/renderer/ppb_nacl_private_impl.cc:530: // strip
>> > component scheme.
>> > Since we're in chrome now, can you use GURL to parse URLs now?
>
>
>> I don't think using GURL would help much; I think the parsing there
>> would just split off the scheme for us, but we'd still have to parse
>> off the leading "pnacl-translator" in the path.
>
>
>> >
>> > https://codereview.chromium.org/307933005/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
> It's not so much to help as to protect against exploits. URL parsing is
> notoriously easy to get wrong. If these URLs are completely trusted, then
> it's
> fine as is, but it might still be worthwhile if will need to do more URL
> processing in this file.

The URLs are trusted (you can see where they're generated in
ManifestResolveKey).
I intend to get rid of this prefix and make them look less like URLs
eventually, joining the logic for ManifestResolveKey and DownloadFile,
but I'm not that far yet.

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

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

[bbudge, 2014-05-30 20:04:56]

https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:534: // Use
white-listed-chars.
If we're generating these, why do we have to do this?

[teravest, 2014-05-30 20:09:05]

https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:534: // Use
white-listed-chars.
On 2014/05/30 20:04:57, bbudge wrote:
> If we're generating these, why do we have to do this?

We depend on this translation happening for some files
(like "chrome://pnacl-translator/pnacl.json"). Presumably "llc" and "ld" could
make open_resource() calls that would have unsafe file paths? But we do the
appending for the "chrome://pnacl-translator" prefix, which is provided here.

My point is that this isn't a "real" URL.

[bbudge, 2014-05-30 20:17:12]

LGTM

https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/307933005/diff/20001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:534: // Use
white-listed-chars.
On 2014/05/30 20:09:05, teravest wrote:
> On 2014/05/30 20:04:57, bbudge wrote:
> > If we're generating these, why do we have to do this?
> 
> We depend on this translation happening for some files
> (like "chrome://pnacl-translator/pnacl.json"). Presumably "llc" and "ld" could
> make open_resource() calls that would have unsafe file paths? But we do the
> appending for the "chrome://pnacl-translator" prefix, which is provided here.
> 
> My point is that this isn't a "real" URL.

Could you add a short comment to that effect here, i.e. why replace unsafe
chars?
Also, above where you use std::string to remove the prefix, why that is safe.
Most chrome reviewers would want you to use GURL for this, so explain why it's
OK here.

[teravest, 2014-05-30 20:22:14]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-05-30 20:26:04]

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

[commit-bot: I haz the power, 2014-05-31 00:28:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-31 01:59:33]

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

[commit-bot: I haz the power, 2014-05-31 01:59:33]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[teravest, 2014-06-02 14:29:30]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-06-02 14:30:18]

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

[commit-bot: I haz the power, 2014-06-02 14:31:50]

Change committed as 274233