Skip to content
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.

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

X11 display error sanitizting #385

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cwendling
Copy link
Member

This may or may not fix #384.

@lukefromdc
Copy link
Member

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
Copy link
Member Author

@lukefromdc me neither, this was solely based on backtraces and comments from @ilya-fedin, and after the work on mate-desktop/mate-panel#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 #384 (comment) for some more details.

@cwendling
Copy link
Member Author

@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.

@@ -139,8 +139,9 @@ send_selection_notify (MsdClipboardManager *manager,
notify.property = success ? manager->priv->property : None;
notify.time = manager->priv->time;

display = gdk_display_get_default ();
gdk_x11_display_error_trap_push (display);
display = gdk_x11_lookup_xdisplay (manager->priv->display);

Choose a reason for hiding this comment

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

This does not change anything. priv->display is gdk_display_get_default():

static void
msd_clipboard_manager_init (MsdClipboardManager *manager)
{
manager->priv = msd_clipboard_manager_get_instance_private (manager);
manager->priv->display = GDK_DISPLAY_XDISPLAY (gdk_display_get_default ());
}

Copy link
Member Author

Choose a reason for hiding this comment

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

As said, most of it probably don't change anything as there probably is only one display anyway. Yet, gdk_display_get_default()'s return value can theoretically change over time, so this could not be the same. I don't think it ever really happens in most real situations, though.

Choose a reason for hiding this comment

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

For that to happen someone would need to call gdk_display_open to create new display and close default display. Clipboard code is not doing that... Using GtkInspector you can get second display, but that won't change default display.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the code, the simplest way would be calling gdk_display_manager_set_default_display(), but that's not something any more likely to happen.

@ilya-fedin
Copy link

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link

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

@cwendling cwendling force-pushed the x11-display-error-sanitizting branch from 96e1560 to ab48cca Compare May 18, 2022 10:12
@cwendling
Copy link
Member Author

@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
Copy link

Any example where you think error traps should be added?

@cwendling
Copy link
Member Author

@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
Copy link

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
Copy link

ilya-fedin commented May 19, 2022

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

@cwendling
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mate-settings-daemon crashes due to misbehavior in Qt's clipboard implementation
4 participants