Skip to content
Permalink
Browse files
[GTK][WPE] Crash at WebKit::bindA11y() in WebKitGTK 2.35.2
https://bugs.webkit.org/show_bug.cgi?id=236144

Patch by Carlos Garcia Campos <cgarcia@igalia.com> on 2022-02-08
Reviewed by Adrian Perez de Castro.

ATSPI stopped using an abstract socket but we are still assuming the a11y bus address is an abstract socket,
even when the path is not really used after all. However, just handling the case of the socket being a normal
unix socket is not enough, because the actual socket path is not mounted in the xdg-dbus-proxy sandbox, so it
fails to connect to the original a11y bus.

* UIProcess/Launcher/ProcessLauncher.h: Rename extraWebProcessSandboxPaths as extraSandboxPaths.
* UIProcess/Launcher/glib/BubblewrapLauncher.cpp:
(WebKit::XDGDBusProxyLauncher::setAddress): Remove the DBusAddressType parameter. Check the address looks valid
and just store the result of dbusAddressToPath() in m_path.
(WebKit::XDGDBusProxyLauncher::launch): Do not require m_path to be non-null and add it to the sanbox.
(WebKit::XDGDBusProxyLauncher::dbusAddressToPath): Only return the path for normal unix sockets.
(WebKit::bindDBusSession): Remove the DBusAddressType parameter passed to dbusAddressToPath().
(WebKit::bindA11y): Check proxy path is not null before setting a11y bus address in display.
(WebKit::addExtraPaths): Helper to add extra paths to the sandbox.
(WebKit::bubblewrapSpawn): Call addExtraPaths() for WebProcess and DBusProxy types.
* UIProcess/Launcher/glib/FlatpakLauncher.cpp:
(WebKit::flatpakSpawn): Use extraSandboxPaths.
* UIProcess/glib/WebProcessProxyGLib.cpp:
(WebKit::WebProcessProxy::platformGetLaunchOptions): Ditto.


Canonical link: https://commits.webkit.org/246957@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@289369 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
carlosgcampos authored and webkit-commit-queue committed Feb 8, 2022
1 parent 53496bc commit 925802d0c7fa49e69a0edf33e8ab4c904c6b95a1
Showing 5 changed files with 60 additions and 35 deletions.
@@ -1,3 +1,30 @@
2022-02-08 Carlos Garcia Campos <cgarcia@igalia.com>

[GTK][WPE] Crash at WebKit::bindA11y() in WebKitGTK 2.35.2
https://bugs.webkit.org/show_bug.cgi?id=236144

Reviewed by Adrian Perez de Castro.

ATSPI stopped using an abstract socket but we are still assuming the a11y bus address is an abstract socket,
even when the path is not really used after all. However, just handling the case of the socket being a normal
unix socket is not enough, because the actual socket path is not mounted in the xdg-dbus-proxy sandbox, so it
fails to connect to the original a11y bus.

* UIProcess/Launcher/ProcessLauncher.h: Rename extraWebProcessSandboxPaths as extraSandboxPaths.
* UIProcess/Launcher/glib/BubblewrapLauncher.cpp:
(WebKit::XDGDBusProxyLauncher::setAddress): Remove the DBusAddressType parameter. Check the address looks valid
and just store the result of dbusAddressToPath() in m_path.
(WebKit::XDGDBusProxyLauncher::launch): Do not require m_path to be non-null and add it to the sanbox.
(WebKit::XDGDBusProxyLauncher::dbusAddressToPath): Only return the path for normal unix sockets.
(WebKit::bindDBusSession): Remove the DBusAddressType parameter passed to dbusAddressToPath().
(WebKit::bindA11y): Check proxy path is not null before setting a11y bus address in display.
(WebKit::addExtraPaths): Helper to add extra paths to the sandbox.
(WebKit::bubblewrapSpawn): Call addExtraPaths() for WebProcess and DBusProxy types.
* UIProcess/Launcher/glib/FlatpakLauncher.cpp:
(WebKit::flatpakSpawn): Use extraSandboxPaths.
* UIProcess/glib/WebProcessProxyGLib.cpp:
(WebKit::WebProcessProxy::platformGetLaunchOptions): Ditto.

2022-02-08 Philippe Normand <pnormand@igalia.com>

[GStreamer] Test webkit/WebKitWebView/display-usermedia-permission-request times out
@@ -91,7 +91,7 @@ class ProcessLauncher : public ThreadSafeRefCounted<ProcessLauncher>, public Can
CString customWebContentServiceBundleIdentifier;

#if PLATFORM(GTK) || PLATFORM(WPE)
HashMap<CString, SandboxPermission> extraWebProcessSandboxPaths;
HashMap<CString, SandboxPermission> extraSandboxPaths;
#if ENABLE(DEVELOPER_MODE)
String processCmdPrefix;
#endif
@@ -165,24 +165,18 @@ static int createFlatpakInfo()
return createSealedMemFdWithData("flatpak-info", data->get(), size);
}

enum class DBusAddressType {
Normal,
Abstract,
};

class XDGDBusProxyLauncher {
public:
void setAddress(const char* dbusAddress, DBusAddressType addressType)
void setAddress(const char* dbusAddress)
{
CString dbusPath = dbusAddressToPath(dbusAddress, addressType);
if (dbusPath.isNull())
if (!dbusAddress || !g_str_has_prefix(dbusAddress, "unix:"))
return;

GUniquePtr<char> appRunDir(g_build_filename(g_get_user_runtime_dir(), BASE_DIRECTORY, nullptr));
m_proxyPath = makeProxyPath(appRunDir.get());

m_socket = dbusAddress;
m_path = WTFMove(dbusPath);
m_path = dbusAddressToPath(dbusAddress);
}

bool isRunning() const { return m_isRunning; };
@@ -199,7 +193,7 @@ class XDGDBusProxyLauncher {
{
RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!isRunning());

if (m_socket.isNull() || m_path.isNull() || m_proxyPath.isNull())
if (m_socket.isNull() || m_proxyPath.isNull())
return;

int syncFds[2];
@@ -260,6 +254,8 @@ class XDGDBusProxyLauncher {

ProcessLauncher::LaunchOptions launchOptions;
launchOptions.processType = ProcessLauncher::ProcessType::DBusProxy;
if (!m_path.isNull())
launchOptions.extraSandboxPaths.add(m_path, SandboxPermission::ReadOnly);
GRefPtr<GSubprocess> process = bubblewrapSpawn(launcher.get(), launchOptions, argv, &error.outPtr());
if (!process.get())
g_error("Failed to start dbus proxy: %s", error->message);
@@ -292,30 +288,24 @@ class XDGDBusProxyLauncher {
return CString(proxySocketTemplate.get());
};

static CString dbusAddressToPath(const char* address, DBusAddressType addressType = DBusAddressType::Normal)
static CString dbusAddressToPath(const char* address)
{
if (!address)
return { };

if (!g_str_has_prefix(address, "unix:"))
return { };

const char* path = strstr(address, addressType == DBusAddressType::Abstract ? "abstract=" : "path=");
const char* path = strstr(address, "path=");
if (!path)
return { };

path += strlen(addressType == DBusAddressType::Abstract ? "abstract=" : "path=");
path += strlen("path=");
const char* pathEnd = path;
while (*pathEnd && *pathEnd != ',')
pathEnd++;

return CString(path, pathEnd - path);
}
}

CString m_socket;
CString m_path;
CString m_proxyPath;
bool m_isRunning;
bool m_isRunning { false };
Vector<CString> m_permissions;
};

@@ -343,9 +333,9 @@ static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bind
static void bindDBusSession(Vector<CString>& args, XDGDBusProxyLauncher& proxy)
{
if (!proxy.isRunning())
proxy.setAddress(g_getenv("DBUS_SESSION_BUS_ADDRESS"), DBusAddressType::Normal);
proxy.setAddress(g_getenv("DBUS_SESSION_BUS_ADDRESS"));

if (proxy.proxyPath().data()) {
if (!proxy.proxyPath().isNull() && !proxy.path().isNull()) {
args.appendVector(Vector<CString>({
"--bind", proxy.proxyPath(), proxy.path(),
}));
@@ -514,9 +504,10 @@ static void bindA11y(Vector<CString>& args)
} else {
GUniqueOutPtr<char> a11yAddress;
g_variant_get(g_dbus_message_get_body(reply.get()), "(s)", &a11yAddress.outPtr());
proxy.setAddress(a11yAddress.get(), DBusAddressType::Abstract);
proxy.setAddress(a11yAddress.get());
#if USE(ATSPI)
PlatformDisplay::sharedDisplay().setAccessibilityBusAddress(makeString("unix:path=", proxy.proxyPath().data()));
if (!proxy.proxyPath().isNull())
PlatformDisplay::sharedDisplay().setAccessibilityBusAddress(makeString("unix:path=", proxy.proxyPath().data()));
#endif
}
}
@@ -535,7 +526,7 @@ static void bindA11y(Vector<CString>& args)
proxy.launch(!g_strcmp0(g_getenv("WEBKIT_ENABLE_A11Y_DBUS_PROXY_LOGGING"), "1"));
}

if (proxy.proxyPath().data()) {
if (!proxy.proxyPath().isNull()) {
GUniquePtr<char> proxyAddress(g_strdup_printf("unix:path=%s", proxy.proxyPath().data()));
args.appendVector(Vector<CString>({
"--ro-bind", proxy.proxyPath(), proxy.proxyPath(),
@@ -816,6 +807,16 @@ static bool shouldUnshareNetwork(ProcessLauncher::ProcessType processType)
return true;
}

static void addExtraPaths(const HashMap<CString, SandboxPermission>& paths, Vector<CString>& args)
{
for (const auto& pathAndPermission : paths) {
args.appendVector(Vector<CString>({
pathAndPermission.value == SandboxPermission::ReadOnly ? "--ro-bind-try": "--bind-try",
pathAndPermission.key, pathAndPermission.key
}));
}
}

GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const ProcessLauncher::LaunchOptions& launchOptions, char** argv, GError **error)
{
ASSERT(launcher);
@@ -880,6 +881,8 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
// is where we mount our proxy socket.
"--bind", runDir, runDir,
}));

addExtraPaths(launchOptions.extraSandboxPaths, sandboxArgs);
}

if (shouldUnshareNetwork(launchOptions.processType))
@@ -928,12 +931,7 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
bindX11(sandboxArgs);
#endif

for (const auto& pathAndPermission : launchOptions.extraWebProcessSandboxPaths) {
sandboxArgs.appendVector(Vector<CString>({
pathAndPermission.value == SandboxPermission::ReadOnly ? "--ro-bind-try": "--bind-try",
pathAndPermission.key, pathAndPermission.key
}));
}
addExtraPaths(launchOptions.extraSandboxPaths, sandboxArgs);

Vector<String> extraPaths = { "applicationCacheDirectory", "mediaKeysDirectory", "waylandSocket", "webSQLDatabaseDirectory" };
for (const auto& path : extraPaths) {
@@ -59,7 +59,7 @@ GRefPtr<GSubprocess> flatpakSpawn(GSubprocessLauncher* launcher, const WebKit::P
"--sandbox-flag=allow-dbus", // Note that this only allows portals and $appid.Sandbox.* access
}));

for (const auto& pathAndPermission : launchOptions.extraWebProcessSandboxPaths) {
for (const auto& pathAndPermission : launchOptions.extraSandboxPaths) {
const char* formatString = pathAndPermission.value == SandboxPermission::ReadOnly ? "--sandbox-expose-path-ro=%s": "--sandbox-expose-path=%s";
GUniquePtr<gchar> pathArg(g_strdup_printf(formatString, pathAndPermission.key.data()));
flatpakArgs.append(pathArg.get());
@@ -53,7 +53,7 @@ void WebProcessProxy::platformGetLaunchOptions(ProcessLauncher::LaunchOptions& l
launchOptions.extraInitializationData.set("mediaKeysDirectory", dataStore->resolvedMediaKeysDirectory());
launchOptions.extraInitializationData.set("applicationCacheDirectory", dataStore->resolvedApplicationCacheDirectory());

launchOptions.extraWebProcessSandboxPaths = m_processPool->sandboxPaths();
launchOptions.extraSandboxPaths = m_processPool->sandboxPaths();
}
}

0 comments on commit 925802d

Please sign in to comment.