From a0e57260ad7db8ef55189ff5ff7156a3834b6fab Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Thu, 10 Jun 2021 18:36:36 +0200 Subject: [PATCH] fix: do not propagate GDK_BACKEND env variable to subproc fixes https://github.com/electron/electron/issues/28436 --- .circleci/build_config.yml | 2 +- .circleci/config.yml | 2 +- shell/browser/electron_browser_main_parts.cc | 16 ++++ shell/browser/electron_browser_main_parts.h | 5 ++ shell/common/platform_util_linux.cc | 13 ++++ spec-main/chromium-spec.ts | 73 +++++++++++++++++++ .../api/gdk-backend-check/after-test.sh | 14 ++++ .../api/gdk-backend-check/before-test.sh | 31 ++++++++ spec/fixtures/api/gdk-backend-check/groot | 2 + .../api/gdk-backend-check/groot.desktop | 5 ++ spec/fixtures/api/gdk-backend-check/groot.xml | 7 ++ .../api/gdk-backend-check/hello.groot | 1 + spec/fixtures/api/gdk-backend-check/main.js | 7 ++ 13 files changed, 176 insertions(+), 2 deletions(-) create mode 100755 spec/fixtures/api/gdk-backend-check/after-test.sh create mode 100755 spec/fixtures/api/gdk-backend-check/before-test.sh create mode 100755 spec/fixtures/api/gdk-backend-check/groot create mode 100644 spec/fixtures/api/gdk-backend-check/groot.desktop create mode 100644 spec/fixtures/api/gdk-backend-check/groot.xml create mode 100755 spec/fixtures/api/gdk-backend-check/hello.groot create mode 100644 spec/fixtures/api/gdk-backend-check/main.js diff --git a/.circleci/build_config.yml b/.circleci/build_config.yml index 17129a46072e6..943938783d408 100644 --- a/.circleci/build_config.yml +++ b/.circleci/build_config.yml @@ -45,7 +45,7 @@ executors: type: enum enum: ["medium", "xlarge", "2xlarge+"] docker: - - image: ghcr.io/electron/build:e6bebd08a51a0d78ec23e5b3fd7e7c0846412328 + - image: ghcr.io/electron/build@sha256:93975f5641bff21cd9d92da65dbfdfdeee9199ef4ad5cfc4b86af38437ca0e10 resource_class: << parameters.size >> macos: diff --git a/.circleci/config.yml b/.circleci/config.yml index 344b434438134..927d08d7d4885 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -61,7 +61,7 @@ executors: type: enum enum: ["medium", "xlarge", "2xlarge+"] docker: - - image: ghcr.io/electron/build:27db4a3e3512bfd2e47f58cea69922da0835f1d9 + - image: ghcr.io/electron/build@sha256:93975f5641bff21cd9d92da65dbfdfdeee9199ef4ad5cfc4b86af38437ca0e10 resource_class: << parameters.size >> # List of always run steps diff --git a/shell/browser/electron_browser_main_parts.cc b/shell/browser/electron_browser_main_parts.cc index 9a15edbdfddbe..aa09794450134 100644 --- a/shell/browser/electron_browser_main_parts.cc +++ b/shell/browser/electron_browser_main_parts.cc @@ -361,8 +361,24 @@ void ElectronBrowserMainParts::PostDestroyThreads() { fake_browser_process_->PostDestroyThreads(); } +#if BUILDFLAG(IS_LINUX) +// static +absl::optional& ElectronBrowserMainParts::GetGDKBackend() { + static absl::optional gdk_backend; + return gdk_backend; +} +#endif + void ElectronBrowserMainParts::ToolkitInitialized() { #if BUILDFLAG(IS_LINUX) + // This is set by Chromium here: + // https://chromium-review.googlesource.com/c/chromium/src/+/2586184 + // and can detrimentally affect external app behaviors, so we want to + // check if the user has set it so we can use it later. + std::string backend; + if (base::Environment::Create()->GetVar("GDK_BACKEND", &backend)) + GetGDKBackend() = backend; + auto linux_ui = BuildGtkUi(); linux_ui->Initialize(); DCHECK(ui::LinuxInputMethodContextFactory::instance()); diff --git a/shell/browser/electron_browser_main_parts.h b/shell/browser/electron_browser_main_parts.h index d8e49529e5641..d085e4934ba40 100644 --- a/shell/browser/electron_browser_main_parts.h +++ b/shell/browser/electron_browser_main_parts.h @@ -102,6 +102,11 @@ class ElectronBrowserMainParts : public content::BrowserMainParts { Browser* browser() { return browser_.get(); } BrowserProcessImpl* browser_process() { return fake_browser_process_.get(); } +#if BUILDFLAG(IS_LINUX) + // Used by platform_util to set GDK_BACKEND. + static absl::optional& GetGDKBackend(); +#endif + protected: // content::BrowserMainParts: int PreEarlyInitialization() override; diff --git a/shell/common/platform_util_linux.cc b/shell/common/platform_util_linux.cc index 0d86d7cf1c28a..3da4f4fb70e5e 100644 --- a/shell/common/platform_util_linux.cc +++ b/shell/common/platform_util_linux.cc @@ -26,6 +26,7 @@ #include "dbus/bus.h" #include "dbus/message.h" #include "dbus/object_proxy.h" +#include "shell/browser/electron_browser_main_parts.h" #include "shell/common/platform_util_internal.h" #include "third_party/abseil-cpp/absl/types/optional.h" #include "ui/gtk/gtk_util.h" @@ -275,6 +276,18 @@ bool XDGUtil(const std::vector& argv, // bring up a new terminal if necessary. See "man mailcap". options.environment["MM_NOTTTY"] = "1"; + // If the user sets a GDK_BACKEND value of their own, use that, + // otherwise unset it because Chromium is setting GDK_BACKEND + // during GTK initialization and we want to respect user preference. + // Setting values in EnvironmentMap to an empty-string + // will make sure that they get unset from the environment via + // AlterEnvironment(). + const absl::optional& gdk_backend = + electron::ElectronBrowserMainParts::GetGDKBackend(); + options.environment["GDK_BACKEND"] = gdk_backend.has_value() + ? gdk_backend.value().c_str() + : base::NativeEnvironmentString(); + base::Process process = base::LaunchProcess(argv, options); if (!process.IsValid()) return false; diff --git a/spec-main/chromium-spec.ts b/spec-main/chromium-spec.ts index f2df2aa34c94f..2d0e819b64e64 100644 --- a/spec-main/chromium-spec.ts +++ b/spec-main/chromium-spec.ts @@ -13,6 +13,7 @@ import { promisify } from 'util'; import { ifit, ifdescribe, delay, defer } from './spec-helpers'; import { AddressInfo } from 'net'; import { PipeTransport } from './pipe-transport'; +const timersPromises = require('timers/promises'); const features = process._linkedBinding('electron_common_features'); @@ -364,6 +365,78 @@ describe('web security', () => { }); }); +ifdescribe(process.platform === 'linux' && !process.arch.includes('arm'))('subprocesses', () => { + let appProcess: ChildProcess.ChildProcessWithoutNullStreams | undefined; + const appPath = path.join(fixturesPath, 'api', 'gdk-backend-check'); + beforeEach(() => { + ChildProcess.execFileSync(path.join(appPath, 'before-test.sh'), { cwd: appPath, encoding: 'utf8' }); + }); + afterEach(() => { + ChildProcess.execFileSync(path.join(appPath, 'after-test.sh'), { cwd: appPath, encoding: 'utf8' }); + if (appProcess && !appProcess.killed) { + appProcess.kill(); + appProcess = undefined; + } + }); + + it('does not propagate GDK_BACKEND', async () => { + appProcess = ChildProcess.spawn(process.execPath, [path.join(appPath, 'main.js')], { env: { ...process.env, GDK_BACKEND: '' } }); + + let output = ''; + appProcess.stdout.on('data', (data) => { output += data; }); + let stderr = ''; + appProcess.stderr.on('data', (data) => { stderr += data; }); + + const [code, signal] = await emittedOnce(appProcess, 'exit'); + if (code !== 0) { + throw new Error(`Process exited with code "${code}" signal "${signal}" output "${output}" stderr "${stderr}"`); + } + + for await (const startTime of timersPromises.setInterval(10, Date.now())) { + if (fs.existsSync('/tmp/groot-says')) { + let gdkBackend = fs.readFileSync('/tmp/groot-says', 'utf8'); + gdkBackend = gdkBackend.trim(); + + expect(gdkBackend).to.be.empty(); + break; + } + const now = Date.now(); + if ((now - startTime) > 5000) { + throw new Error('The gdk backend test failed timing out on waiting for the file'); + } + } + }); + + it('successfully honors GDK_BACKEND set in the subproc', async () => { + appProcess = ChildProcess.spawn(process.execPath, [path.join(appPath, 'main.js')], { env: { ...process.env, GDK_BACKEND: 'groot' } }); + + let output = ''; + appProcess.stdout.on('data', (data) => { output += data; }); + let stderr = ''; + appProcess.stderr.on('data', (data) => { stderr += data; }); + + const [code, signal] = await emittedOnce(appProcess, 'exit'); + if (code !== 0) { + throw new Error(`Process exited with code "${code}" signal "${signal}" output "${output}" stderr "${stderr}"`); + } + + for await (const startTime of timersPromises.setInterval(10, Date.now())) { + if (fs.existsSync('/tmp/groot-says')) { + let gdkBackend = fs.readFileSync('/tmp/groot-says', 'utf8'); + gdkBackend = gdkBackend.trim(); + + expect(gdkBackend).to.not.be.empty(); + expect(gdkBackend).to.equal('groot'); + break; + } + const now = Date.now(); + if ((now - startTime) > 5000) { + throw new Error('The gdk backend test failed timing out on waiting for the file'); + } + } + }); +}); + describe('command line switches', () => { let appProcess: ChildProcess.ChildProcessWithoutNullStreams | undefined; afterEach(() => { diff --git a/spec/fixtures/api/gdk-backend-check/after-test.sh b/spec/fixtures/api/gdk-backend-check/after-test.sh new file mode 100755 index 0000000000000..c0827b18ca531 --- /dev/null +++ b/spec/fixtures/api/gdk-backend-check/after-test.sh @@ -0,0 +1,14 @@ +#!/bin/sh + +set -o errexit +set -o nounset + +rm -f /tmp/groot-says +rm -f /tmp/groot +rm -f ~/.local/share/applications/groot.desktop + +update-desktop-database ~/.local/share/applications + +rm -f ~/.local/share/mime/packages/groot.xml + +update-mime-database ~/.local/share/mime diff --git a/spec/fixtures/api/gdk-backend-check/before-test.sh b/spec/fixtures/api/gdk-backend-check/before-test.sh new file mode 100755 index 0000000000000..6e84edb4ed3dd --- /dev/null +++ b/spec/fixtures/api/gdk-backend-check/before-test.sh @@ -0,0 +1,31 @@ +#!/bin/sh + +set -o errexit +set -o nounset + +cleanup () { + exec "./after-test.sh" +} + +trap cleanup HUP INT + +# Install the application Groot +cp ./groot /tmp/ + +# Make sure the applications directory exists. +mkdir -p ~/.local/share/applications + +# Register Groot as desktop application +cp ./groot.desktop ~/.local/share/applications/ + +# Update database of desktop entries +update-desktop-database ~/.local/share/applications + +# Make sure the packages directory exists. +mkdir -p ~/.local/share/mime/packages + +# Associate 'application/x-groot' mime type with '*.groot' files +cp ./groot.xml ~/.local/share/mime/packages/ + +# update the MIME database +update-mime-database ~/.local/share/mime diff --git a/spec/fixtures/api/gdk-backend-check/groot b/spec/fixtures/api/gdk-backend-check/groot new file mode 100755 index 0000000000000..ebd5bcdc0f81e --- /dev/null +++ b/spec/fixtures/api/gdk-backend-check/groot @@ -0,0 +1,2 @@ +#!/bin/sh +echo $GDK_BACKEND > /tmp/groot-says diff --git a/spec/fixtures/api/gdk-backend-check/groot.desktop b/spec/fixtures/api/gdk-backend-check/groot.desktop new file mode 100644 index 0000000000000..e56f1012e07c3 --- /dev/null +++ b/spec/fixtures/api/gdk-backend-check/groot.desktop @@ -0,0 +1,5 @@ +[Desktop Entry] +Type=Application +Exec=/tmp/groot +Name=Groot +MimeType=application/x-groot diff --git a/spec/fixtures/api/gdk-backend-check/groot.xml b/spec/fixtures/api/gdk-backend-check/groot.xml new file mode 100644 index 0000000000000..44c6230d478a4 --- /dev/null +++ b/spec/fixtures/api/gdk-backend-check/groot.xml @@ -0,0 +1,7 @@ + + + + Groot File + + + diff --git a/spec/fixtures/api/gdk-backend-check/hello.groot b/spec/fixtures/api/gdk-backend-check/hello.groot new file mode 100755 index 0000000000000..1a2485251c33a --- /dev/null +++ b/spec/fixtures/api/gdk-backend-check/hello.groot @@ -0,0 +1 @@ +#!/bin/sh diff --git a/spec/fixtures/api/gdk-backend-check/main.js b/spec/fixtures/api/gdk-backend-check/main.js new file mode 100644 index 0000000000000..467b1504208f4 --- /dev/null +++ b/spec/fixtures/api/gdk-backend-check/main.js @@ -0,0 +1,7 @@ +const { app, shell } = require('electron'); +const path = require('path'); + +app.whenReady().then(async () => { + await shell.openPath(path.join(__dirname, 'hello.groot')); + app.quit(); +});