188002 2018-07-25 07:15:33 -0700 [Flatpak] Pass more environment variables to sandbox 2018-07-25 10:00:27 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 cturner cturner aperez bugs-noreply commit-queue lforschler mcatanzaro webkit-bug-importer oldest_to_newest 1445014 0 cturner 2018-07-25 07:15:33 -0700 [Flatpak] Pass environment variables starting with WEBKIT to sandbox 1445015 1 345757 cturner 2018-07-25 07:22:41 -0700 Created attachment 345757 Patch 1445017 2 345757 aperez 2018-07-25 07:42:27 -0700 Comment on attachment 345757 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345757&action=review Reviewing informally... I have a comment about this, please read below. > Tools/flatpak/flatpakutils.py:683 > + if envvar.split("_")[0] in ("GST", "GTK", "G", "WEBKIT", "WEBKIT2") or \ I would also add variables with the WPE_ prefix. At least WPEBackend checks WPE_BACKEND_LIBRARY in developer mode builds, and WPEBackend-rdk also picks some variables from the environment like WPE_INIT_VIEW_{WIDTH,HEIGHT} and WPE_BCMRPI_{TOUCH,CURSOR}. 1445019 3 345759 cturner 2018-07-25 07:53:42 -0700 Created attachment 345759 Patch Thanks for the review Adrian, WPE_ prefixed variables are passed through as well now 1445020 4 mcatanzaro 2018-07-25 07:55:55 -0700 I think we should just pass through the entire environment. Look at https://trac.webkit.org/wiki/EnvironmentVariables. We also need at least GIGACAGE_ENABLED, JavaScriptCoreUseJIT, and Malloc. What still uses WEBKIT2? Any such environment variable should probably be renamed. 1445022 5 aperez 2018-07-25 08:25:19 -0700 (In reply to Michael Catanzaro from comment #4) > I think we should just pass through the entire environment. Look at > https://trac.webkit.org/wiki/EnvironmentVariables. We also need at least > GIGACAGE_ENABLED, JavaScriptCoreUseJIT, and Malloc. > > What still uses WEBKIT2? Any such environment variable should probably be > renamed. There's this one for pausing the Web process at launch time: % rg 'getenv.*WEBKIT2' Source/ Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp 49: if (g_getenv("WEBKIT2_PAUSE_WEB_PROCESS_ON_LAUNCH")) Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp 47: if (g_getenv("WEBKIT2_PAUSE_WEB_PROCESS_ON_LAUNCH")) % 1445024 6 cturner 2018-07-25 08:27:50 -0700 (In reply to Michael Catanzaro from comment #4) > I think we should just pass through the entire environment. Look at > https://trac.webkit.org/wiki/EnvironmentVariables. We also need at least > GIGACAGE_ENABLED, JavaScriptCoreUseJIT, and Malloc. My gut feeling is that passing the entire environment would not be a good idea, the sandbox sets up some environment variables of its own, and generally having uncontrolled environment pollution like that sounds a bit risky. I would prefer to just special case the WebKit environment variables as needed, even if there are a lot of special cases. > > What still uses WEBKIT2? Any such environment variable should probably be > renamed. WEBKIT2_PAUSE_WEB_PROCESS_ON_LAUNCH is the only one I can find. 1445025 7 mcatanzaro 2018-07-25 08:40:28 -0700 (In reply to Charlie Turner from comment #6) > My gut feeling is that passing the entire environment would not be a good > idea, the sandbox sets up some environment variables of its own, and > generally having uncontrolled environment pollution like that sounds a bit > risky. I would prefer to just special case the WebKit environment variables > as needed, even if there are a lot of special cases. OK (In reply to Charlie Turner from comment #6) > WEBKIT2_PAUSE_WEB_PROCESS_ON_LAUNCH is the only one I can find. We can consider in another bug whether it's better to rename, or if the cost of breaking old tutorials is too high. 1445036 8 345761 cturner 2018-07-25 09:01:38 -0700 Created attachment 345761 Patch 1445046 9 aperez 2018-07-25 09:13:40 -0700 FWIW, I agree with Charlie that it's better to pass a controlled and well-known set of environment variables. Otherwise we may run into the kind of weird situations which containers and sandboxing are supposed to fix. As a matter of fact, I would go as far as reporting which environment variables are being copied inside the sandbox before entering the Flatpak environment. That would help replicate the running conditions reported e.g. by other developers or build bots, for debugging. It can also help developers notice that they are unintentionally setting variables. 1445060 10 commit-queue 2018-07-25 09:58:36 -0700 The commit-queue encountered the following flaky tests while processing attachment 345761: fast/repaint/canvas-object-fit.html bug 188004 (author: simon.fraser@apple.com) The commit-queue is continuing to process your patch. 1445061 11 345761 commit-queue 2018-07-25 09:59:20 -0700 Comment on attachment 345761 Patch Clearing flags on attachment: 345761 Committed r234199: <https://trac.webkit.org/changeset/234199> 1445062 12 commit-queue 2018-07-25 09:59:21 -0700 All reviewed patches have been landed. Closing bug. 1445063 13 webkit-bug-importer 2018-07-25 10:00:27 -0700 <rdar://problem/42587131> 345757 2018-07-25 07:22:41 -0700 2018-07-25 07:53:39 -0700 Patch bug-188002-20180725152240.patch text/plain 1267 cturner U3VidmVyc2lvbiBSZXZpc2lvbjogMjM0MTkyCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggZDBlY2MyYTJiYWJkZWUwZDVhZjQzYTdmNTM5ZmEzNzI2 YWFkYTE3My4uMTkxN2IwY2EwYTBkZjViZGFiNzgzMjc4NWExZDAwN2MwNjNhYzc3MyAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEz IEBACisyMDE4LTA3LTI1ICBDaGFybGllIFR1cm5lciAgPGN0dXJuZXJAaWdhbGlhLmNvbT4KKwor ICAgICAgICBbRmxhdHBha10gUGFzcyBXRUJLSVQgZW52aXJvbm1lbnQgdmFyaWFibGVzIHRvIHNh bmRib3gKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4 ODAwMgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICog ZmxhdHBhay9mbGF0cGFrdXRpbHMucHk6CisgICAgICAgIChXZWJraXRGbGF0cGFrLnJ1bl9pbl9z YW5kYm94KToKKwogMjAxOC0wNy0yNCAgVGhvbWFzIERlbm5leSAgPHRkZW5uZXlAYXBwbGUuY29t PgogCiAgICAgICAgIFJlbW92ZSBzdXBwb3J0IGZvciB0aGUgZG91YmxlIHR5cGUgZnJvbSBXSExT TApkaWZmIC0tZ2l0IGEvVG9vbHMvZmxhdHBhay9mbGF0cGFrdXRpbHMucHkgYi9Ub29scy9mbGF0 cGFrL2ZsYXRwYWt1dGlscy5weQppbmRleCAwZTkwZGJmN2Q3NTNhNGE3ZDFhOTE1ZWQ4ZDBlN2Rl NWQzYjQ5OTlhLi5mZDI3NzI5MDg2OWNkNWQ0NmY4NTM4MjFmMjAxYzIwYWFhZmEyMjVhIDEwMDY0 NAotLS0gYS9Ub29scy9mbGF0cGFrL2ZsYXRwYWt1dGlscy5weQorKysgYi9Ub29scy9mbGF0cGFr L2ZsYXRwYWt1dGlscy5weQpAQCAtNjgwLDcgKzY4MCw3IEBAIGNsYXNzIFdlYmtpdEZsYXRwYWs6 CiAgICAgICAgICAgICB9CiAKICAgICAgICAgICAgIGZvciBlbnZ2YXIsIHZhbHVlIGluIG9zLmVu dmlyb24uaXRlbXMoKToKLSAgICAgICAgICAgICAgICBpZiBlbnZ2YXIuc3BsaXQoIl8iKVswXSBp biAoIkdTVCIsICJHVEsiLCAiRyIpIG9yIFwKKyAgICAgICAgICAgICAgICBpZiBlbnZ2YXIuc3Bs aXQoIl8iKVswXSBpbiAoIkdTVCIsICJHVEsiLCAiRyIsICJXRUJLSVQiLCAiV0VCS0lUMiIpIG9y IFwKICAgICAgICAgICAgICAgICAgICAgICAgIGVudnZhciBpbiBbIldBWUxBTkRfRElTUExBWSIs ICJESVNQTEFZIiwgIkxBTkciXToKICAgICAgICAgICAgICAgICAgICAgZm9yd2FyZGVkW2VudnZh cl0gPSB2YWx1ZQogCg== 345759 2018-07-25 07:53:42 -0700 2018-07-25 09:01:35 -0700 Patch bug-188002-20180725155341.patch text/plain 1272 cturner U3VidmVyc2lvbiBSZXZpc2lvbjogMjM0MTkyCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggZDBlY2MyYTJiYWJkZWUwZDVhZjQzYTdmNTM5ZmEzNzI2 YWFkYTE3My4uODg0ZjM4OTJiYjA4MjkyZjExMWMyMjY4Y2YyMjFlZjRmZmMzNTkyZiAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEz IEBACisyMDE4LTA3LTI1ICBDaGFybGllIFR1cm5lciAgPGN0dXJuZXJAaWdhbGlhLmNvbT4KKwor ICAgICAgICBbRmxhdHBha10gUGFzcyBtb3JlIGVudmlyb25tZW50IHZhcmlhYmxlcyB0byBzYW5k Ym94CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODgw MDIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIGZs YXRwYWsvZmxhdHBha3V0aWxzLnB5OgorICAgICAgICAoV2Via2l0RmxhdHBhay5ydW5faW5fc2Fu ZGJveCk6CisKIDIwMTgtMDctMjQgIFRob21hcyBEZW5uZXkgIDx0ZGVubmV5QGFwcGxlLmNvbT4K IAogICAgICAgICBSZW1vdmUgc3VwcG9ydCBmb3IgdGhlIGRvdWJsZSB0eXBlIGZyb20gV0hMU0wK ZGlmZiAtLWdpdCBhL1Rvb2xzL2ZsYXRwYWsvZmxhdHBha3V0aWxzLnB5IGIvVG9vbHMvZmxhdHBh ay9mbGF0cGFrdXRpbHMucHkKaW5kZXggMGU5MGRiZjdkNzUzYTRhN2QxYTkxNWVkOGQwZTdkZTVk M2I0OTk5YS4uNWViMDFiMGRkYzZjMTk2YTQ3MDExOTg4NmNhNzliYTM5NGQwNzg4OCAxMDA2NDQK LS0tIGEvVG9vbHMvZmxhdHBhay9mbGF0cGFrdXRpbHMucHkKKysrIGIvVG9vbHMvZmxhdHBhay9m bGF0cGFrdXRpbHMucHkKQEAgLTY4MCw3ICs2ODAsNyBAQCBjbGFzcyBXZWJraXRGbGF0cGFrOgog ICAgICAgICAgICAgfQogCiAgICAgICAgICAgICBmb3IgZW52dmFyLCB2YWx1ZSBpbiBvcy5lbnZp cm9uLml0ZW1zKCk6Ci0gICAgICAgICAgICAgICAgaWYgZW52dmFyLnNwbGl0KCJfIilbMF0gaW4g KCJHU1QiLCAiR1RLIiwgIkciKSBvciBcCisgICAgICAgICAgICAgICAgaWYgZW52dmFyLnNwbGl0 KCJfIilbMF0gaW4gKCJHU1QiLCAiR1RLIiwgIkciLCAiV0VCS0lUIiwgIldFQktJVDIiLCAiV1BF Iikgb3IgXAogICAgICAgICAgICAgICAgICAgICAgICAgZW52dmFyIGluIFsiV0FZTEFORF9ESVNQ TEFZIiwgIkRJU1BMQVkiLCAiTEFORyJdOgogICAgICAgICAgICAgICAgICAgICBmb3J3YXJkZWRb ZW52dmFyXSA9IHZhbHVlCiAK 345761 2018-07-25 09:01:38 -0700 2018-07-25 09:59:20 -0700 Patch bug-188002-20180725170136.patch text/plain 1905 cturner U3VidmVyc2lvbiBSZXZpc2lvbjogMjM0MTkyCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggZDBlY2MyYTJiYWJkZWUwZDVhZjQzYTdmNTM5ZmEzNzI2 YWFkYTE3My4uODg0ZjM4OTJiYjA4MjkyZjExMWMyMjY4Y2YyMjFlZjRmZmMzNTkyZiAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEz IEBACisyMDE4LTA3LTI1ICBDaGFybGllIFR1cm5lciAgPGN0dXJuZXJAaWdhbGlhLmNvbT4KKwor ICAgICAgICBbRmxhdHBha10gUGFzcyBtb3JlIGVudmlyb25tZW50IHZhcmlhYmxlcyB0byBzYW5k Ym94CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODgw MDIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIGZs YXRwYWsvZmxhdHBha3V0aWxzLnB5OgorICAgICAgICAoV2Via2l0RmxhdHBhay5ydW5faW5fc2Fu ZGJveCk6CisKIDIwMTgtMDctMjQgIFRob21hcyBEZW5uZXkgIDx0ZGVubmV5QGFwcGxlLmNvbT4K IAogICAgICAgICBSZW1vdmUgc3VwcG9ydCBmb3IgdGhlIGRvdWJsZSB0eXBlIGZyb20gV0hMU0wK ZGlmZiAtLWdpdCBhL1Rvb2xzL2ZsYXRwYWsvZmxhdHBha3V0aWxzLnB5IGIvVG9vbHMvZmxhdHBh ay9mbGF0cGFrdXRpbHMucHkKaW5kZXggMGU5MGRiZjdkNzUzYTRhN2QxYTkxNWVkOGQwZTdkZTVk M2I0OTk5YS4uOWFiYmU3YTFlODdjMDRhMGUzNzk2NWM3MmQ2ZWMwZTM1ZTg1ZTc4ZSAxMDA2NDQK LS0tIGEvVG9vbHMvZmxhdHBhay9mbGF0cGFrdXRpbHMucHkKKysrIGIvVG9vbHMvZmxhdHBhay9m bGF0cGFrdXRpbHMucHkKQEAgLTY3OSw5ICs2NzksMjcgQEAgY2xhc3MgV2Via2l0RmxhdHBhazoK ICAgICAgICAgICAgICAgICAiVEVTVF9SVU5ORVJfSU5KRUNURURfQlVORExFX0ZJTEVOQU1FIjog Ii9hcHAvd2Via2l0L2xpYi9saWJUZXN0UnVubmVySW5qZWN0ZWRCdW5kbGUuc28iLAogICAgICAg ICAgICAgfQogCisgICAgICAgICAgICBlbnZfdmFyX3ByZWZpeGVzX3RvX2tlZXAgPSBbCisgICAg ICAgICAgICAgICAgIkdTVCIsCisgICAgICAgICAgICAgICAgIkdUSyIsCisgICAgICAgICAgICAg ICAgIkciLAorICAgICAgICAgICAgICAgICJKU0MiLAorICAgICAgICAgICAgICAgICJXRUJLSVQi LAorICAgICAgICAgICAgICAgICJXRUJLSVQyIiwKKyAgICAgICAgICAgICAgICAiV1BFIiwKKyAg ICAgICAgICAgICAgICAiR0lHQUNBR0UiLAorICAgICAgICAgICAgXQorCisgICAgICAgICAgICBl bnZfdmFyc190b19rZWVwID0gWworICAgICAgICAgICAgICAgICJKYXZhU2NyaXB0Q29yZVVzZUpJ VCIsCisgICAgICAgICAgICAgICAgIk1hbGxvYyIsCisgICAgICAgICAgICAgICAgIldBWUxBTkRf RElTUExBWSIsCisgICAgICAgICAgICAgICAgIkRJU1BMQVkiLAorICAgICAgICAgICAgICAgICJM QU5HIiwKKyAgICAgICAgICAgIF0KKwogICAgICAgICAgICAgZm9yIGVudnZhciwgdmFsdWUgaW4g b3MuZW52aXJvbi5pdGVtcygpOgotICAgICAgICAgICAgICAgIGlmIGVudnZhci5zcGxpdCgiXyIp WzBdIGluICgiR1NUIiwgIkdUSyIsICJHIikgb3IgXAotICAgICAgICAgICAgICAgICAgICAgICAg ZW52dmFyIGluIFsiV0FZTEFORF9ESVNQTEFZIiwgIkRJU1BMQVkiLCAiTEFORyJdOgorICAgICAg ICAgICAgICAgIGlmIGVudnZhci5zcGxpdCgiXyIpWzBdIGluIGVudl92YXJfcHJlZml4ZXNfdG9f a2VlcCBvciBlbnZ2YXIgaW4gZW52X3ZhcnNfdG9fa2VlcDoKICAgICAgICAgICAgICAgICAgICAg Zm9yd2FyZGVkW2VudnZhcl0gPSB2YWx1ZQogCiAgICAgICAgICAgICBmb3IgZW52dmFyLCB2YWx1 ZSBpbiBmb3J3YXJkZWQuaXRlbXMoKToK