Skip to content

use std::format* instead of snprintf in xwayland_spawner.cpp #4793

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yotam-medini wants to merge 3 commits into
base: main
Choose a base branch
from
+80 −60
Open

use std::format* instead of snprintf in xwayland_spawner.cpp #4793

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ff9ba7
use std::format* instead of snprintf in xwayland_spawner.cpp
yotam-medini Mar 23, 2026
a413c84
save lockfile + x11_socket path as XWaylandSpawner data members
yotam-medini Mar 26, 2026
91ca948
iter_difference_t instead of size_t for format_to_n
yotam-medini Mar 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 65 additions & 59 deletions src/server/frontend_xwayland/xwayland_spawner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
#include <mir/fatal.h>
#include <mir/thread_name.h>

#include <array>
#include <format>
#include <string>
#include <fcntl.h>
#include <sys/socket.h>
#include <sys/un.h>
Expand All @@ -36,50 +39,8 @@ namespace md = mir::dispatch;

namespace
{
auto const x11_lock_fmt = "/tmp/.X%d-lock";
auto const x11_socket_fmt = "/tmp/.X11-unix/X%d";

// TODO this can be written with more modern c++
int create_lockfile(int xdisplay)
{
char lockfile[256];

snprintf(lockfile, sizeof lockfile, x11_lock_fmt, xdisplay);

mir::Fd const fd{open(lockfile, O_WRONLY | O_CLOEXEC | O_CREAT | O_EXCL, 0444)};
if (fd < 0)
return EEXIST;

// We format for consistency with the corresponding code in xorg-server (os/utils.c) which
// requires 11 characters. Vis:
//
// snprintf(pid_str, sizeof(pid_str), "%10lu\n", (unsigned long) getpid());
// if (write(lfd, pid_str, 11) != 11)
bool const success_writing_to_lock_file = dprintf(fd, "%10lu\n", (unsigned long) getpid()) == 11;

// Check if anyone else has created the socket (even though we have the lockfile)
char x11_socket[256];
snprintf(x11_socket, sizeof x11_socket, x11_socket_fmt, xdisplay);

if (!success_writing_to_lock_file || access(x11_socket, F_OK) == 0)
{
unlink(lockfile);
return EEXIST;
}

return 0;
}

auto choose_display() -> int
{
for (auto xdisplay = 0; xdisplay != 10000; ++xdisplay)
{
if (create_lockfile(xdisplay) == 0) return xdisplay;
}

mir::fatal_error("Cannot create X11 lockfile!");
return -1;
}
auto constexpr x11_lock_fmt = "/tmp/.X{}-lock";
auto constexpr x11_socket_fmt = "/tmp/.X11-unix/X{}";

auto create_socket(std::vector<mir::Fd>& fds, struct sockaddr_un *addr, size_t path_size)
{
Expand Down Expand Up @@ -135,17 +96,19 @@ auto create_socket(std::vector<mir::Fd>& fds, struct sockaddr_un *addr, size_t p
fds.push_back(fd);
}

auto create_sockets(int xdisplay) -> std::vector<mir::Fd>
auto create_sockets(const std::string &x11_socket) -> std::vector<mir::Fd>
{
std::vector<mir::Fd> result;
struct sockaddr_un addr{ .sun_family = AF_UNIX, .sun_path = { 0 } };
size_t path_size;

path_size = snprintf(addr.sun_path + 1, sizeof(addr.sun_path) - 1, x11_socket_fmt, xdisplay);
create_socket(result, &addr, path_size);

path_size = snprintf(addr.sun_path, sizeof(addr.sun_path), x11_socket_fmt, xdisplay);
create_socket(result, &addr, path_size);
auto name_create_socket = [&](char *bos, size_t limit)
{
auto eos = std::format_to_n(bos, limit, "{}", x11_socket).out;
Copy link
Copy Markdown
Contributor

@Saviq Saviq Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TiCS has this to say:

narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'iter_difference_t<char *>' (aka 'long') is implementation-defined.

Copy link
Copy Markdown
Contributor Author

@yotam-medini yotam-medini Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[How] can I run this TiCS locally ?

Copy link
Copy Markdown
Contributor

@Saviq Saviq Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, you can't, but that actually comes from clang-tidy. You should be able to reproduce this with the following CMake options:

-DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_CXX_CLANG_TIDY=clang-tidy

Copy link
Copy Markdown
Contributor Author

@yotam-medini yotam-medini Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some struggling with cmake, looking at output from: 'ps laxww' I got to the attached:
tidy.sh
Running it re-produced the error:

$ /tmp/tidy.sh
24 warnings generated.
/home/yotam/pub/mir/src/server/frontend_xwayland/xwayland_spawner.cpp:106:42: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'iter_difference_t<char *>' (aka 'long') is implementation-defined [cppcoreguidelines-narrowing-conversions]
  106 |         auto eos = std::format_to_n(bos, limit, "{}", x11_socket).out;
      |                                          ^

The commit:
91ca948 yotam.m iter_difference_t instead of size_t for format_to_n
fixes it.

*eos = '\0';
create_socket(result, &addr, eos - bos);
};
name_create_socket(addr.sun_path + 1, sizeof(addr.sun_path) - 2);
name_create_socket(addr.sun_path, sizeof(addr.sun_path) - 1);

return result;
}
Expand All @@ -163,9 +126,13 @@ auto create_dispatchers(
}
}

mf::XWaylandSpawner::XWaylandSpawner(std::function<void()> spawn)
: xdisplay{choose_display()},
fds{create_sockets(xdisplay)},
mf::XWaylandSpawner::XWaylandSpawner(
std::function<void()> spawn,
const XDisplayPaths &xp) :
xdisplay{xp.xdisplay},
lockfile{xp.lockfile},
x11_socket{xp.x11_socket},
fds{create_sockets(x11_socket)},
dispatcher{std::make_shared<md::MultiplexingDispatchable>()},
spawn_thread{std::make_unique<dispatch::ThreadedDispatcher>(
"Mir/X11 Spawner",
Expand Down Expand Up @@ -194,11 +161,50 @@ mf::XWaylandSpawner::XWaylandSpawner(std::function<void()> spawn)

mf::XWaylandSpawner::~XWaylandSpawner()
{
char path[256];
snprintf(path, sizeof path, x11_lock_fmt, xdisplay);
unlink(path);
snprintf(path, sizeof path, x11_socket_fmt, xdisplay);
unlink(path);
unlink(lockfile.c_str());
unlink(x11_socket.c_str());
}

int mf::XWaylandSpawner::create_lockfile(XDisplayPaths &xp)
{
xp.lockfile = std::format(x11_lock_fmt, xp.xdisplay);

mir::Fd const fd{open(xp.lockfile.c_str(), O_WRONLY | O_CLOEXEC | O_CREAT | O_EXCL, 0444)};
if (fd < 0)
{
return EEXIST;
}

// We format for consistency with the corresponding code in xorg-server (os/utils.c) which
// requires 11 characters. Vis:
//
// snprintf(pid_str, sizeof(pid_str), "%10lu\n", (unsigned long) getpid());
// if (write(lfd, pid_str, 11) != 11)
bool const success_writing_to_lock_file = dprintf(fd, "%10lu\n", (unsigned long) getpid()) == 11;

// Check if anyone else has created the socket (even though we have the lockfile)
xp.x11_socket = std::format(x11_socket_fmt, xp.xdisplay);

if (!success_writing_to_lock_file ||
access(xp.x11_socket.c_str(), F_OK) == 0)
{
unlink(xp.lockfile.c_str());
return EEXIST;
}

return 0;
}

auto mf::XWaylandSpawner::choose_display() -> XDisplayPaths
{
XDisplayPaths xp;
for (xp.xdisplay = 0; xp.xdisplay != 10000; ++xp.xdisplay)
{
if (create_lockfile(xp) == 0) return xp;
}

mir::fatal_error("Cannot create X11 lockfile!");
return XDisplayPaths();
}

auto mf::XWaylandSpawner::x11_display() const -> std::string
Expand Down
14 changes: 13 additions & 1 deletion src/server/frontend_xwayland/xwayland_spawner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ namespace frontend
class XWaylandSpawner
{
public:
XWaylandSpawner(std::function<void()> spawn);
XWaylandSpawner(std::function<void()> spawn) :
XWaylandSpawner(spawn, choose_display()) {}
~XWaylandSpawner();

XWaylandSpawner(XWaylandSpawner const&) = delete;
Expand All @@ -58,7 +59,18 @@ class XWaylandSpawner
static bool set_cloexec(mir::Fd const& fd, bool cloexec);

private:
struct XDisplayPaths
{
int xdisplay{-1};
std::string lockfile;
std::string x11_socket;
};
XWaylandSpawner(std::function<void()> spawn, const XDisplayPaths &xp);
static int create_lockfile(XDisplayPaths &xp);
static XDisplayPaths choose_display();
int const xdisplay;
std::string lockfile;
std::string x11_socket;
std::vector<Fd> const fds;
std::shared_ptr<dispatch::MultiplexingDispatchable> const dispatcher;
std::unique_ptr<dispatch::ThreadedDispatcher> const spawn_thread;
Expand Down
Loading