Our fork/exec spawning is unsafe

[RAOF]

exec_wayland() is called after fork():

mir/src/server/frontend_xwayland/xwayland_server.cpp

Lines 53 to 98 in 953de04

	void exec_xwayland(
	mf::XWaylandSpawner const& spawner,
	std::string const& xwayland_path,
	mir::Fd wayland_client_fd,
	mir::Fd x11_wm_server_fd,
	float scale)
	{
	mf::XWaylandSpawner::set_cloexec(wayland_client_fd, false);
	mf::XWaylandSpawner::set_cloexec(x11_wm_server_fd, false);
	setenv("WAYLAND_SOCKET", std::to_string(wayland_client_fd).c_str(), 1);
	auto const x11_wm_server = std::to_string(x11_wm_server_fd);
	auto const dsp_str = spawner.x11_display();
	// This DPI only effects some apps (most app care about GDK_SCALE and other environment variables)
	unsigned const dpi = scale * 96;
	auto const dpi_str = std::to_string(dpi);
	std::vector<char const*> args =
	{
	xwayland_path.c_str(),
	dsp_str.c_str(),
	"-rootless",
	"-dpi",
	dpi_str.c_str(),
	"-wm", x11_wm_server.c_str(),
	"-terminate",
	};
	for (auto const& fd : spawner.socket_fds())
	{
	mf::XWaylandSpawner::set_cloexec(fd, false);
	args.push_back("-listen");
	// strdup() may look like a leak, but execvp() will trash all resource management
	args.push_back(strdup(std::to_string(fd).c_str()));
	}
	if (auto const xwayland_option = getenv("MIR_XWAYLAND_OPTION"))
	args.push_back(xwayland_option);
	args.push_back(nullptr);
	execvp(xwayland_path.c_str(), const_cast<char* const*>(args.data()));
	perror(("Failed to execute Xwayland binary: xwayland_path='" + xwayland_path + "'").c_str());
	}

Unfortunately, man 2 fork says:

After a fork() in a multithreaded program, the child can safely call only async-signal-safe functions (see signal-safety(7)) until such time as it calls execve(2).

We should instead do as much setup as possible in the parent, and then call fork(). (Amusingly, even execvp is not async-signal-safe 🤦‍♀️ )

[AlanGriffiths]

Amusingly, even execvp is not async-signal-safe

That probably doesn't matter: we should use execve() directly (we default xwayland-path to "/usr/bin/Xwayland", which doesn't need PATH handling)

[RAOF]

Oops! When reviewing #3497 I also see that miral::launch_app_env suffers from the same problems.

[AlanGriffiths]

in #3588 @RAOF asked:

This is a partial fix for #3494, right? It's not a full fix for that?

True. There's still...

mir/src/miral/launch_app.cpp

Lines 110 to 116 in ecc4899

	std::vector<char const*> exec_args;
	for (auto const& arg : app)
	exec_args.push_back(arg.c_str());
	exec_args.push_back(nullptr);

...that clearly needs fixing to avoid allocations.

We also need to deal with the {un,}setenv() calls. By my reading these are not "async-signal-safe". I think we should be using execvpe().