1374557 - New OS X try failures with level 3 on task cluster tests

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Haik Aftandilian [:haik]]

With the changes to OS X automated tests to use task cluster, several tests are failing. Here's an example run enabling level3 including the fix for Bug 1334550.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=779a73db4cc282b66c829c330a07c892e539a0a4

The failures appear to be due to the tests being run out of the home directory. Before they were run out of /builds.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Spent a bit of time looking at it, moving the build to |/builds| leads to effectively the same problems as we see in bug 1357758. To solve this, we'll need to do essentially the same things as bug 1308400. Specifically the pieces we'll need are:

- |read_path_whitelist| from https://reviewboard.mozilla.org/r/133516/diff/2#index_header
- Setting |read_path_whitelist| with the test paths from https://reviewboard.mozilla.org/r/147690/diff/1#index_header

One thing that's going to be a little bit frustrating is that the macOS sandbox policy language does not have a way of passing arrays as a parameters or splitting a string (as far as google can tell at least, it's not like any of this is documented :-)). This means we'll probably have to resort to concatenating strings for each path we want to whitelist. Not the end of the world, just ugly.

Long term, I think we can remove all this manual whitelisting with bug 1136836 (as I mentioned on IRC a few weeks ago, I'm starting to wonder if we'd be better off remoting all of necko).

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Hah, actually I lied, there is  way to split a string |(split-string var ",")| so that reduces the ugliness of this a little bit. You end up with:

    (allow file-read* (apply subpath (split-string read-whitelist-paths ",")))

Not too horrible.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Will hopefully have a patch up for this tomorrow, will be a few grafts from :gcp's patch, so we'll have to figure out how to deconflict those.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1374557 - Part 1 - Add the ability to specify a list of paths to whitelist read access to in the macOS content sandbox;

Review commit: https://reviewboard.mozilla.org/r/151328/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151328/

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

Review commit: https://reviewboard.mozilla.org/r/151330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151330/

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Run the tests at level 3

Review commit: https://reviewboard.mozilla.org/r/151332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151332/

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879978 [details]
Bug 1374557 - Part 1 - Add the ability to specify a list of paths to whitelist read access to in the macOS content sandbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151328/diff/1-2/

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151330/diff/1-2/

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879980 [details]
Run the tests at level 3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151332/diff/1-2/

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151330/diff/2-3/

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879980 [details]
Run the tests at level 3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151332/diff/2-3/

Comment 12

[Haik Aftandilian [:haik]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

https://reviewboard.mozilla.org/r/151330/#review157880

This looks good, but if Mac will only require at most two directories to whitelist, I'd prefer to add two prefs specifically for Mac. That way we can avoid using a comma delimeter which will break for directories that include commas and we'll want to fix it eventually. Another way of encoding the pref that didn't depend on the comma-delimetering would also work.

::: testing/mozbase/mozprofile/mozprofile/profile.py:56
(Diff revision 3)
>          :param addons: String of one or list of addons to install
>          :param addon_manifests: Manifest for addons (see http://bit.ly/17jQ7i6)
>          :param preferences: Dictionary or class of preferences
>          :param locations: ServerLocations object
>          :param proxy: Setup a proxy
>          :param restore: Flag for removing all custom settings during cleanup

Document the whitelistpaths param.

::: testing/mozharness/configs/unittests/linux_unittest.py:120
(Diff revision 3)
>                  "--log-errorsummary=%(error_summary_file)s",
>                  "--use-test-media-devices",
>                  "--screenshot-on-fail",
>                  "--cleanup-crashes",
>                  "--marionette-startup-timeout=180",
> +                "--work-path=" + ABS_WORK_DIR,

We shouldn't include the Linux changes with this fix since we won't be testing it yet.

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879978 [details]
Bug 1374557 - Part 1 - Add the ability to specify a list of paths to whitelist read access to in the macOS content sandbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151328/diff/2-3/

Comment 14

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151330/diff/3-4/

Comment 15

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879980 [details]
Run the tests at level 3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151332/diff/3-4/

Comment 16

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879978 [details]
Bug 1374557 - Part 1 - Add the ability to specify a list of paths to whitelist read access to in the macOS content sandbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151328/diff/3-4/

Comment 17

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151330/diff/4-5/

Comment 18

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879980 [details]
Run the tests at level 3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151332/diff/4-5/

Comment 19

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

https://reviewboard.mozilla.org/r/151330/#review157880

Sounds good -- I'll do this once I've confirmed that tests are all green!

Comment 20

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879978 [details]
Bug 1374557 - Part 1 - Add the ability to specify a list of paths to whitelist read access to in the macOS content sandbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151328/diff/4-5/

Comment 21

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151330/diff/5-6/

Comment 22

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879980 [details]
Run the tests at level 3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151332/diff/5-6/

Comment 23

[Haik Aftandilian [:haik]]

Comment on attachment 8879978 [details]
Bug 1374557 - Part 1 - Add the ability to specify a list of paths to whitelist read access to in the macOS content sandbox;

https://reviewboard.mozilla.org/r/151328/#review158506

::: dom/ipc/ContentChild.cpp:1446
(Diff revision 5)
> -  } else {
> -    info.appDir.assign(appDir.get());
> +  info.appDir.assign(appDir.get());
> -  }
>    info.appTempDir.assign(tempDirPath.get());
>  
> +  nsAdoptingCString testingReadPath1 =

Please add a comment explaining what these are for. How we only expect those prefs to appear in profiles used for automated tests.

Comment 24

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879978 [details]
Bug 1374557 - Part 1 - Add the ability to specify a list of paths to whitelist read access to in the macOS content sandbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151328/diff/5-6/

Comment 25

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151330/diff/6-7/

Comment 26

[Alex Gaynor [:Alex_Gaynor]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9acaac80d45befe308091a0c0ee046c7e7fc62d1&group_state=expanded

Comment 27

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

https://reviewboard.mozilla.org/r/151330/#review158890

::: testing/mozbase/mozprofile/mozprofile/profile.py:118
(Diff revision 7)
>          prefs_js, user_js = self.permissions.network_prefs(self._proxy)
> +
> +        if self._whitelistpaths:
> +            # On macOS we don't want to support a generalized read whitelist,
> +            # and the macOS sandbox policy language doesn't have support for
> +            # lists, so we handle these specially.

I'm not sure that warrants a separate pref and/or the (fairly ugly) handling that follows here. You can split them up in the thing that reads this pref, right?

Also you need a proper peer to review changes to test scripts.

Comment 28

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

https://reviewboard.mozilla.org/r/151330/#review158890

> I'm not sure that warrants a separate pref and/or the (fairly ugly) handling that follows here. You can split them up in the thing that reads this pref, right?
> 
> Also you need a proper peer to review changes to test scripts.

Splitting on `,` is technically ambigious (if you have a `,` in the path), which :haik wanted to avoid.

Comment 29

[Haik Aftandilian [:haik]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

https://reviewboard.mozilla.org/r/151330/#review158906

r+ from me as long as we get build peer review.

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

https://reviewboard.mozilla.org/r/151330/#review158940

::: layout/tools/reftest/runreftest.py:306
(Diff revision 7)
>             '5.1' in platform.version() and options.e10s:
>              prefs['layers.acceleration.disabled'] = True
>  
> +        sandbox_whitelist_paths = [SCRIPT_DIRECTORY]
> +        try:
> +            if options.workPath:

do we need topsrcdir like we have for mochitest?

::: testing/mozbase/mozprofile/mozprofile/profile.py:131
(Diff revision 7)
> +                prefs_js.append((
> +                    "security.sandbox.content.mac.testing_read_path1",
> +                    self._whitelistpaths[0]
> +                ))
> +            else:
> +                prefs_js.append(("security.sandbox.content.read_path_whitelist",

do we need to do this for talos as well?

Comment 31

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8879979 [details]
Bug 1374557 - Part 2 - Use the new preference to whitelist paths for reading that are needed by tests;

https://reviewboard.mozilla.org/r/151330/#review158940

> do we need topsrcdir like we have for mochitest?

Nope! (Tests would have failed horribly if it was :-))

> do we need to do this for talos as well?

Great question. I don't _think_ so, unless talos is relying on being able to load `chrome://` URIs from various places on disk, it shouldn't be necessary.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74c02bb049fc
Part 1 - Add the ability to specify a list of paths to whitelist read access to in the macOS content sandbox; r=haik
https://hg.mozilla.org/integration/autoland/rev/b57aac875b65
Part 2 - Use the new preference to whitelist paths for reading that are needed by tests; r=gcp,haik,jmaher

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/74c02bb049fc
https://hg.mozilla.org/mozilla-central/rev/b57aac875b65