Xorg/net wm pid #7445

thoughtjigs · 2018-06-13T18:09:58Z

While working on a personal X window manager project I discovered that minetest does not support most of the Extended Window Manager Hints spec. This includes things like _NET_WM_PID which allows the discovery of the process underlying a window.

Providing _NET_WM_PID will allow window managers to terminate an unresponsive minetest window.

There is actually quite a bit of work that could be done to improve how minetest handles the additional Xorg stuff that Irrlicht doesn't support but for a start getting support for _NET_WM_PID is probably the easiest to achieve.

I have also created a backport of these changes to the 14.7.1 branch if that will be useful.

--EDIT--
Editing to add that xprop can be used at the CLI to verify that _NET_WM_PID and WM_CLIENT_MACHINE have been set correctly.

…Hints

sfan5 · 2018-06-13T18:18:52Z

src/client/renderingengine.cpp

+
+	Atom NET_WM_PID = XInternAtom(x11_dpl,
+		"_NET_WM_PID",
+		false);


pretty sure this fits on a single line

Yup, probably left over from longer variable names I was using.

sfan5 · 2018-06-13T18:20:07Z

src/client/renderingengine.cpp

+			(unsigned char *) &pid,1);
+
+	// According to the Extended Window Manager Hints spec if _NET_WM_PID
+	// is set then WM_CLIENT_MACHINE must also be set.


and if it isn't? do other applications set this value?
it would be nice to avoid the getaddrinfo stuff below.

Good question. I took a look at the GTK/GDK libraries to see how they handle it and they don't even attempt to set WM_CLIENT_MACHINE even though they do set _NET_WM_PID. The reason for providing a FQDN for the the WM_CLIENT_MACHINE is for that uniquely X situation where the X server may actually reside on a different machine from the X client and so the PID would be meaningless to the window manager (assuming the window manager was not running on the same machine as minetest).

For minetest this is very likely not an issue as I am pretty sure that the performance of minetest over the network (i.e. thin client) would not be the best.

There are several options for dealing with this ... the code could attempt to discover if the server and client are on the same machine and if they are not then don't set the _NET_WM_PID. Alternatively _NET_WM_PID could be set in all cases and then just let the window manager deal with it. The former would be useful but problems would only arise in the extreme edge case described above.

Are you aware of any users running minetest in such a scenario?

Not aware of anyone using Minetest over the network and it's certainly not a configuration we explicitly support or recommend.
I'd definitely prefer omitting support for WM_CLIENT_MACHINE entirely.

Would you prefer blindly setting _NET_WM_PID or would it make sense to attempt a few sanity checks before hand to ensure that X is not running in an unsupported fashion? I could see the sanity checks as being generally useful and a warning could be issued in the logs indicating that the user was attempting to run minetest in an unsupported fashion.

Former, since that matches the behaviour of e.g. GTK as you've said.

sfan5 · 2018-06-13T18:23:11Z

src/client/renderingengine.cpp

+	// the results of gethostname to determine the length. 
+
+	size_t hostname_len=128;
+	char* hostname=0;


Yup, will fix

spaces around =

wrong pointer placement char *hostname

sfan5 · 2018-06-13T18:23:21Z

src/client/renderingengine.cpp

+	while (1) {
+		// (Re)allocate buffer of length hostname_len.
+		char* realloc_hostname=(char *) realloc(hostname,hostname_len);
+		if (realloc_hostname==0) {


Yup, will fix

sfan5 · 2018-06-13T18:24:38Z

src/client/renderingengine.cpp

+		if (gethostname(hostname,hostname_len-1)==0) {
+			size_t count=strlen(hostname);
+			if (count<hostname_len-2) {
+				// Break from loop if hostname definitely not truncated


instead of doing all this jazz to make sure it's 100% correct in all cases,
IMO in the sense of simplicity, just give gethostname 1024 bytes to work and hope it's enough

Yeah, I waffled back and forth between which solution was best. On the one hand I am pretty old school and prefer not to allocate memory where it is not needed but with modern day computers 1024 vs 128 is pretty insignificant.

An alternative would be to check to see if the hostname was longer than 1024 and just not set WM_CLIENT_MACHINE if it was.

A hostname longer than 1024 is extremely unlikely anyway.

I am fine with any of the options and only went with this option for efficiency and safety.

sfan5 · 2018-06-13T18:24:50Z

src/client/renderingengine.cpp

+
+	struct addrinfo hints={0};
+	hints.ai_family=AF_UNSPEC;
+	hints.ai_flags=AI_CANONNAME;


spacing around =

sfan5 · 2018-06-13T18:25:35Z

src/client/renderingengine.cpp

+			<< res->ai_canonname << "'"
+			<< std::endl;
+
+		// Copy canonical name to new string so that addrinfo can be freed


copying it to a new string does not mean you have to use a std::string*

Yeah, this could be reworked. I started using std::string before I discovered the info|verbose|error|etc...streams. I think it would be sufficient to just use a char array.

I noticed that there is no consensus in the code on whether to use wchar_t or char for the log streams. Are there plans to internationalize the log messages?

No, log messages should use char.

sfan5 · 2018-06-13T18:26:19Z

src/client/renderingengine.cpp

+	// gethostname and getaddrinfo do not support IDN (thank the 
+	// flying spaghetti monster) this means that fqdn will be represented
+	// in ASCII (any IDN names would be converted using punycode).
+	std::string *fqdn_ptr;          


you forgot to delete this one

Yup, will fix (or rework in light of no need for std::string)

nerzhul · 2018-06-13T20:00:23Z

src/client/renderingengine.cpp

+	// the results of gethostname to determine the length. 
+
+	size_t hostname_len=128;
+	char* hostname=0;


spaces around =

thoughtjigs · 2018-06-13T21:38:25Z

Updated the pull request with most of the requested changes.

I had a question about the use of std::string instead of std::string * for fqdn ... it occurs to me that not using a pointer and therefore new/delete that the client machines fqdn could potentially be present in memory after exiting the setXorgNetWMPID method. While it is unlikely that it won't get overwritten very soon after it does create a situation where code that should not have access to the users fqdn could maliciously obtain it. I am not sure if this includes lua scripts.

Is this a concern?

With regards to the code that gathers the hostname and fqdn I will defer to your collective judgement on the likely hood of minetest running on a thin client where minetest and the window manager are not on the same machine. I am comfortable only setting the _NET_WM_PID and either leaving the WM_CLIENT_MACHINE unset or only setting it to hostname.

As for discovery of the hostname I would prefer to use as little memory as possible but agree that creating a buffer with a size of 1024 will likely have no impact on modern day systems. As long as only 1023 bytes are offered to gethostname and the buffer is null terminated before calling gethostname there should be no issues.

sfan5 · 2018-06-13T21:43:54Z

could potentially be present in memory after exiting the setXorgNetWMPID method

Even if you don't use a pointer, instancing a std::string will allocate memory.
The C++ standard library does not zero freed memory anyway.

Is this a concern?

If Lua script had ways to access uninitialised stack or heap space, we would have far worse problems. So, no.

thoughtjigs · 2018-06-13T22:58:53Z

If Lua script had ways to access uninitialised stack or heap space, we would have far worse problems. So, no.

@sfan5 , that is a good point.

In general has there been any concern about timing/cross channel attacks (in light of meltdown/spectre) with regards to untrusted mods? Webbrowsers limited time resolution of javascript in order to protect against attacks coming from untrusted javascript. It might make sense to do something similar in lua mods ... if there is enough freedom there to pull off such an attack.

This is to be more consistent with how gtk/gdk handles it as well as take advantage of some of the improvements to the RenderingEngine class.

thoughtjigs · 2018-06-14T18:03:52Z

Took another look at how gtk/gdk handles things See the setup_toplevel_window method.

It looks like gdk is setting the WM_CLIENT_MACHINE via the XSetWMProperties function. I believe this sets it to the hostname (as that is what the ICCM requires).

I reworked my changes to more closely follow what the gdk does. I also tried to take advantage of some of the improvements to the RenderingEngine class code and began the process of isolating Irrlicht. Ideally I think it would be better to have a RenderingEngine class for each unique env so that details specific to those envs would be available directly as class members e.g. XDisplay or XWindow but that can be left to a later date as it would be a significant change.

I also went ahead and set the WM_CLIENT_LEADER property using the same pattern that the gdk uses.

Verified that XSetWMProperties uses gethostname (if available) to set WM_CLIENT_MACHINE so it will not be a fully qualified domain name. See https://github.com/mirror/libX11/blob/master/src/WMProps.c and https://github.com/mirror/libX11/blob/master/src/XlibInt.c. It also limits it to 255 bytes in length.

Moved verbose messaging to setupXorgTopLevelWindow method as Xorg messages should only occur when running in Xorg env.

sfan5 · 2018-06-15T16:41:08Z

src/client/renderingengine.cpp


+	Window x11_win = (Window)exposedData.OpenGLLinux.X11Window;


This one may not be necessary. I will try to compile it without it.

sfan5 · 2018-06-15T16:41:18Z

src/client/renderingengine.cpp

-	if (video_data.OpenGLLinux.X11Display == NULL)
+	const video::SExposedVideoData exposedData = driver->getExposedVideoData();
+
+	Display *x11_dpl = (Display *)exposedData.OpenGLLinux.X11Display;


is this cast required at all? otherwise reinterpret_cast

Seems to be. I get a compiler error when it not present. I will check and see what Irrlicht thinks it is.

Interesting ... returns it as a void * X11Display see: http://irrlicht.sourceforge.net/docu/structirr_1_1video_1_1_s_exposed_video_data.html

window is returned as an unsigned long which is probably ok as that is what it is under the hood in Xlib ... but it should probably be reinterpreted as a window type in case code further down expects it to be used in that way.

sfan5 · 2018-06-15T16:41:34Z

src/client/renderingengine.cpp

+	// method. But for now (as it would require some significant changes) 
+	// leave the code as is. 
+
+    // The following is borrowed from the above gdk source for setting top


should be tabs

Irrlicht returns the XDisplay as a void* and XWindow as an unsigned long so reinterpret those as the appropriate type. Also fixed a spaces for tab formating issue

SmallJoker

LGTM

Adding support for _NET_WM_PID as defined in Extended Window Manager Hints Move verbose messaging to setupXorgTopLevelWindow method as Xorg messages should only occur when running in Xorg env. Irrlicht returns the XDisplay as a void* and XWindow as an unsigned long so reinterpret those as the appropriate type. Also fixed a spaces for tab formating issue

thoughtjigs added 3 commits June 13, 2018 10:31

Adding support for _NET_WM_PID as defined in Extended Window Manager …

1457def

…Hints

Fix a few formating issues.

67eea85

Spell spaghetti correctly

ab45a94

SmallJoker added @ Startup / Config / Util Feature ✨ PRs that add or enhance a feature labels Jun 13, 2018

sfan5 requested changes Jun 13, 2018

View reviewed changes

nerzhul requested changes Jun 13, 2018

View reviewed changes

Requested changes from pull-request review

7fd8e11

Rework Xorg top level window property settings.

19f2c07

This is to be more consistent with how gtk/gdk handles it as well as take advantage of some of the improvements to the RenderingEngine class.

thoughtjigs added 2 commits June 14, 2018 11:31

Removed unecessary headers

2f05166

Updated to use C++ style casts and moved verbose messaging.

7aa2dc9

Moved verbose messaging to setupXorgTopLevelWindow method as Xorg messages should only occur when running in Xorg env.

sfan5 requested changes Jun 15, 2018

View reviewed changes

More cast changes

a4df01d

Irrlicht returns the XDisplay as a void* and XWindow as an unsigned long so reinterpret those as the appropriate type. Also fixed a spaces for tab formating issue

sfan5 approved these changes Jun 15, 2018

View reviewed changes

sfan5 added the One approval ✅ ◻️ label Jun 15, 2018

SmallJoker approved these changes Jun 17, 2018

View reviewed changes

SmallJoker added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Jun 17, 2018

SmallJoker merged commit 3d51607 into minetest:master Jun 17, 2018

thoughtjigs deleted the xorg/net_wm_pid branch June 17, 2018 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xorg/net wm pid #7445

Xorg/net wm pid #7445

thoughtjigs commented Jun 13, 2018 •

edited

sfan5 Jun 13, 2018

thoughtjigs Jun 13, 2018

sfan5 Jun 13, 2018

thoughtjigs Jun 13, 2018 •

edited

sfan5 Jun 13, 2018

thoughtjigs Jun 13, 2018

sfan5 Jun 14, 2018

sfan5 Jun 13, 2018

thoughtjigs Jun 13, 2018

nerzhul Jun 13, 2018

SmallJoker Jun 13, 2018

sfan5 Jun 13, 2018

thoughtjigs Jun 13, 2018

sfan5 Jun 13, 2018

thoughtjigs Jun 13, 2018

sfan5 Jun 13, 2018

thoughtjigs Jun 13, 2018

sfan5 Jun 13, 2018

thoughtjigs Jun 13, 2018

sfan5 Jun 13, 2018

sfan5 Jun 13, 2018

thoughtjigs Jun 13, 2018

nerzhul Jun 13, 2018

thoughtjigs commented Jun 13, 2018

sfan5 commented Jun 13, 2018

thoughtjigs commented Jun 13, 2018 •

edited

thoughtjigs commented Jun 14, 2018 •

edited

sfan5 Jun 15, 2018

thoughtjigs Jun 15, 2018

sfan5 Jun 15, 2018

thoughtjigs Jun 15, 2018

thoughtjigs Jun 15, 2018

sfan5 Jun 15, 2018

SmallJoker left a comment

Xorg/net wm pid #7445

Xorg/net wm pid #7445

Conversation

thoughtjigs commented Jun 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thoughtjigs Jun 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thoughtjigs commented Jun 13, 2018

sfan5 commented Jun 13, 2018

thoughtjigs commented Jun 13, 2018 • edited

thoughtjigs commented Jun 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmallJoker left a comment

Choose a reason for hiding this comment

thoughtjigs commented Jun 13, 2018 •

edited

thoughtjigs Jun 13, 2018 •

edited

thoughtjigs commented Jun 13, 2018 •

edited

thoughtjigs commented Jun 14, 2018 •

edited