search

mate-desktop / mate-settings-daemon

MATE settings daemon
https://mate-desktop.org
GNU General Public License v2.0
44 stars 46 forks source link

X11 display error sanitizting #385

Open cwendling opened 2 years ago

cwendling commented 2 years ago

This may or may not fix #384.

lukefromdc commented 2 years ago

I've never had this issue(cannot duplicate it), so it would be difficult for me to test this as a fix. I use Kdenlive (a QT 5 program) a lot, and routinely use the clipboard to input filenames for render jobs with never a problem

cwendling commented 2 years ago

@lukefromdc me neither, this was solely based on backtraces and comments from @ilya-fedin, and after the work on https://github.com/mate-desktop/mate-panel/pull/1308. I'm not entirely convinced the changes I propose here do help much, but I believe they are still good (given I didn't make any mistake) as they sanitize the code to use the display the code actually wants to trap errors on (although I would think it'll always be the same anyway). See https://github.com/mate-desktop/mate-settings-daemon/issues/384#issuecomment-1109634694 for some more details.

cwendling commented 2 years ago

@lukefromdc one thing we can verify though is that my changes do not make anything worse, and wait for feedback from @ilya-fedin on whether it solves his issues or not.

ilya-fedin commented 2 years ago

The reason I don't answer so long is I've hit a crash even with that patch, unfortunately. Then I tried to set GDK_SYNCHRONIZE=1 as the output in journalctl suggests and haven't hit a crash yet. I don't know though whether this is due to this variable or I just haven't copy-pasted anything that would trigger the bug.

cwendling commented 2 years ago

The reason I don't answer so long is I've hit a crash even with that patch, unfortunately.

OK then this means this patchset is not helpful, which is valuable info nonetheless :)

cwendling commented 2 years ago

OK, given @ilya-fedin's feedback, this patchset is not very interesting, apart from its potential better dealing with multiple displays but as discussed it's unlikely to ever actually matter anyway. The only bit I think could be of real interest is the unmatched push/pop pair I fixed here, unless it was intentional for some reason that eludes me.

@muktupavels if you're willing to review this a tad deeper I'd be happy to undo any change you deem unneeded (like the default display we were discussing). Otherwise, I'll just abandon this PR which doesn't fix what it vaguely hoped to.

muktupavels commented 2 years ago

Leave only change that makes sure push is always followed by pop.

cwendling commented 2 years ago

@muktupavels here we go.

FWIW cursory looking at this file there are actually numerous potentially failing X calls unchecked, so maybe this should be dealt with as well?

muktupavels commented 2 years ago

Any example where you think error traps should be added?

cwendling commented 2 years ago

@muktupavels As said somewhere before I'm not too knowledgeable about X11 APIs, but I was thinking about several XConvertSelection() and XChangeProperty() calls, like e.g. further down in convert_clipboard_manager(). Basically I'd guess they'd be safer around all possibly failing X calls, but I guess we can expect some never to actually fail short of a disconnection; so maybe I'm just showing my lack of familiarity with this :)

muktupavels commented 2 years ago

Check when/why function can fail...

For XConvertSelection https://tronche.com/gui/x/xlib/window-information/XConvertSelection.html:

BadAtom | A value for an Atom argument does not name a defined Atom.
BadWindow | A value for a Window argument does not name a defined Window.

I would expect that XA_* atoms always exists / is defined. manager->priv->window window is created by clipboard and destroyed only when plugin is stopped. So you should not be worried that window might suddenly become invalid...

ilya-fedin commented 2 years ago

Literally any X call that operates on the requestor can fail with BadWindow...

cwendling commented 2 years ago

@muktupavels yeah OK, the ones purely on the internal window could be deemed safe (we'd likely have other issues if it wasn't), if it doesn't include BadAlloc (and even then, GLib aborting on memory error probably suggests BadAlloc might often not be out primary concern). But as @ilya-fedin said, whenever there's something we don't manage ourselves, we have no way to know whether it's (still) valid, and that seems to apply to the 2 XChangeProperty() calls in that very convert_clipboard_manager(), or several places like convert_clipboard(), etc.

@ilya-fedin BTW if you finally can reproduce your issue, you could try guarding pretty much every call with a pair of gdk_x11_display_error_trap_push()/gdk_x11_display_error_trap_pop_ignored() and see if it helps -- or possibly even better, log the failures to see what was the cause.