Use /etc/hosts for URLRequestTest.FileTest on Android

Use /etc/hosts for URLRequestTest.FileTest on Android

URLRequestTest.FileTest tries to read /self/proc/exe, which
Android's security model will prevent on User platform builds
running a Release apk.
This change uses /etc/hosts instead for Android to verify file
access.

BUG=598918
Committed: https://crrev.com/5a645820f0e600e96d3e96eb247ce62819156f89
Cr-Commit-Position: refs/heads/master@{#385817}

[kraush, 2016-03-30 01:30:27]

kraush@amazon.com changed reviewers:
+ mattm@chromium.org

[kraush, 2016-03-30 01:30:28]

Hi Matt,

Can you take a look at this proposed test change for Android?

Thanks!
Holger

[mattm, 2016-03-31 02:56:31]

mattm@chromium.org changed reviewers:
+ pauljensen@chromium.org

[mattm, 2016-03-31 02:56:31]

I'm not an android expert, but this seems weird. Paul, what do you think?

[pauljensen, 2016-03-31 03:59:59]

Ya, this seems weird. Kraush, can you further track down why it appears we're
not allowed to read file sizes?  I'd be very surprised if Android disallowed
stat().

[kraush, 2016-04-04 23:56:48]

On 2016/03/31 03:59:59, pauljensen wrote:
> Ya, this seems weird. Kraush, can you further track down why it appears we're
> not allowed to read file sizes?  I'd be very surprised if Android disallowed
> stat().

Sorry for the delayed response.
I just looked at this issue again, and it seems the commit message doesn't fit
what's actually happening.

The problem is that PathService::Get(base::FILE_EXE, &app_path) returns false on
Userbuild devices on a Chrome Release build (and thus app_path doesn't get set)
On a Debug Chrome build OR a UserDebug platform build, PathService::Get returns
true and sets the value to /system/bin/app_process32

I'll debug PathService::Get a little to see why this is happening, but this
might be intentional behavior.
I'm not sure why base::FILE_EXE was chosen for this test - there might be
another file with less strict access it could use.
I'll update as soon as I have more information.

[kraush, 2016-04-05 00:45:07]

On 2016/04/04 23:56:48, kraush wrote:
> On 2016/03/31 03:59:59, pauljensen wrote:
> > Ya, this seems weird. Kraush, can you further track down why it appears
we're
> > not allowed to read file sizes?  I'd be very surprised if Android disallowed
> > stat().
> 
> Sorry for the delayed response.
> I just looked at this issue again, and it seems the commit message doesn't fit
> what's actually happening.
> 
> The problem is that PathService::Get(base::FILE_EXE, &app_path) returns false
on
> Userbuild devices on a Chrome Release build (and thus app_path doesn't get
set)
> On a Debug Chrome build OR a UserDebug platform build, PathService::Get
returns
> true and sets the value to /system/bin/app_process32
> 
> I'll debug PathService::Get a little to see why this is happening, but this
> might be intentional behavior.
> I'm not sure why base::FILE_EXE was chosen for this test - there might be
> another file with less strict access it could use.
> I'll update as soon as I have more information.

This check fails because in base_paths_android.cc, readling /self/proc/exe
fails.
From how I understand Android's security model, this is expected behavior.

That being said, considering the fact that the test just looks for any file that
is guaranteed to exist, it seems feasible to just change the test to reference:
/system/bin/app_process on Android platforms (rather than using PathService)
app_process is guaranteed to exist and the test seems to succeed for me (even
when being spawned by app_process32)

Would you consider that a better solution to the problem?
Or should we consider the fact that base_paths_android.cc fails as a problem in
itself?

[pauljensen, 2016-04-07 12:02:57]

For Android how about we just change the test to use some known file, like
/etc/hosts for example?

[kraush, 2016-04-07 16:43:34]

On 2016/04/07 12:02:57, pauljensen wrote:
> For Android how about we just change the test to use some known file, like
> /etc/hosts for example?

Sounds like a good solution, thanks! :)
I'll go ahead and post a new diff pointing at /etc/hosts for Android.

[kraush, 2016-04-07 16:48:34]

Description was changed from

==========
Do not run URLRequestTest.FileTest on Release-User-combinations

This disables URLRequestTest.FileTest on Android user build
devices when running with a Release apk.
This avoids the issue of the test failing due to not being allowed
to read file sizes.

BUG=598918
==========

to

==========
Use /etc/hosts for URLRequestTest.FileTest on Android

URLRequestTest.FileTest tries to read /self/proc/exe, which
Android's security model will prevent on User platform builds
running a Release apk.
This change uses /etc/hosts instead for Android to verify file
access.

BUG=598918
==========

[kraush, 2016-04-07 16:49:07]

Updated to use /etc/hosts on Android platforms.
Also updated the commit message.

Please take another look :)

[pauljensen, 2016-04-07 17:18:08]

https://codereview.chromium.org/1839273002/diff/20001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1839273002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:844: PathService::Get(base::FILE_EXE,
&app_path);
this won't work...you've still got the line here

[kraush, 2016-04-07 17:21:31]

https://codereview.chromium.org/1839273002/diff/20001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1839273002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:844: PathService::Get(base::FILE_EXE,
&app_path);
On 2016/04/07 17:18:08, pauljensen wrote:
> this won't work...you've still got the line here

Argh, sorry.
I did run it and it did work, but that's only because PathService::Get fails and
never writes anything into &app_path

I'll remove the second invication to make it cleaner.

[kraush, 2016-04-07 17:35:09]

Sorry for the unnecessary duplicate invocation!
Removed in the latest patch set.

[pauljensen, 2016-04-07 17:58:31]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2016-04-07 17:58:31]

lgtm

[commit-bot: I haz the power, 2016-04-07 17:59:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1839273002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1839273002/40001

[commit-bot: I haz the power, 2016-04-07 18:35:12]

Description was changed from

==========
Use /etc/hosts for URLRequestTest.FileTest on Android

URLRequestTest.FileTest tries to read /self/proc/exe, which
Android's security model will prevent on User platform builds
running a Release apk.
This change uses /etc/hosts instead for Android to verify file
access.

BUG=598918
==========

to

==========
Use /etc/hosts for URLRequestTest.FileTest on Android

URLRequestTest.FileTest tries to read /self/proc/exe, which
Android's security model will prevent on User platform builds
running a Release apk.
This change uses /etc/hosts instead for Android to verify file
access.

BUG=598918
==========

[commit-bot: I haz the power, 2016-04-07 18:35:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-07 18:36:29]

Description was changed from

==========
Use /etc/hosts for URLRequestTest.FileTest on Android

URLRequestTest.FileTest tries to read /self/proc/exe, which
Android's security model will prevent on User platform builds
running a Release apk.
This change uses /etc/hosts instead for Android to verify file
access.

BUG=598918
==========

to

==========
Use /etc/hosts for URLRequestTest.FileTest on Android

URLRequestTest.FileTest tries to read /self/proc/exe, which
Android's security model will prevent on User platform builds
running a Release apk.
This change uses /etc/hosts instead for Android to verify file
access.

BUG=598918

Committed: https://crrev.com/5a645820f0e600e96d3e96eb247ce62819156f89
Cr-Commit-Position: refs/heads/master@{#385817}
==========

[commit-bot: I haz the power, 2016-04-07 18:36:30]

Patchset 3 (id:??) landed as
https://crrev.com/5a645820f0e600e96d3e96eb247ce62819156f89
Cr-Commit-Position: refs/heads/master@{#385817}