Skip to content

clipmenud: Prevent race in disable/enable by checking X timestamps #254

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

Merged
cdown merged 1 commit into from
May 29, 2025
Merged

clipmenud: Prevent race in disable/enable by checking X timestamps #254

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
49 changes: 47 additions & 2 deletions src/clipmenud.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,15 @@ static Window win;
static int enabled = 1;
static int sig_fd;

static Atom timestamp_atom;
static Atom incr_atom;
static struct incr_transfer *it_list;

static struct cm_selections sels[CM_SEL_MAX];

static Time last_disable_time = 0;
static Time last_enable_time = 0;

enum clip_text_source {
CLIP_TEXT_SOURCE_X,
CLIP_TEXT_SOURCE_MALLOC,
Expand All @@ -60,6 +64,25 @@ static void free_clip_text(struct clip_text *ct) {
ct->source = CLIP_TEXT_SOURCE_INVALID;
}

/**
* Get the current X server time by triggering a PropertyNotify.
*/
static Time get_current_server_time(void) {
Copy link
Owner Author

@cdown cdown May 28, 2025

Choose a reason for hiding this comment

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

@N-R-K do you know a better way to do this? i wondered if there was some extension that can help here, but it seems not?

Copy link
Contributor

@N-R-K N-R-K May 29, 2025

Choose a reason for hiding this comment

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

i wondered if there was some extension that can help here, but it seems not?

@cdown Well, yes, I do happen to have a bit of esoteric knowledge in this matter. Roughly, you'd need to do the following:

  1. XSyncQueryExtension() + XSyncInitialize() at startup for initialization.
  2. Then use XSyncListSystemCounters() to get a list of available counters.
  3. Iterate over them to find "SERVERTIME".
  4. Now you should be able to use XSyncQueryCounter() to query the server time with the counter id found via step 3.

But I'm not entirely sure if this is worth the trouble. Can't client set arbitrary timestamps anyways? Though I guess from a security perspective it makes sense to completely eliminate any chances of storing a sensitive clip that should've been discarded.

Copy link
Owner Author

@cdown cdown May 29, 2025

Choose a reason for hiding this comment

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

Thank you! I guessed you might have some knowledge here :-)

Okay, I guess we'll stick with the PropertyNotify method for now since we don't need the XSync guarantees (and it's less code).

XEvent ev;
XChangeProperty(dpy, win, timestamp_atom, XA_INTEGER, 32, PropModeReplace,
NULL, 0);
XSync(dpy, False);

while (1) {
XNextEvent(dpy, &ev);
if (ev.type == PropertyNotify && ev.xproperty.atom == timestamp_atom) {
XDeleteProperty(dpy, win, timestamp_atom);
return ev.xproperty.time;
}
XPutBackEvent(dpy, &ev);
}
}

/**
* Check if a text s1 is a possible partial of s2.
*
Expand Down Expand Up @@ -175,12 +198,25 @@ static void handle_signalfd_event(void) {
si.ssi_pid);
switch (si.ssi_signo) {
case SIGUSR1:
// If we're already disabled, we need to keep the original
// timestamp to properly filter all events that were queued during
// any part of the disabled period.
if (enabled) {
last_disable_time = get_current_server_time();
}
enabled = 0;
dbg("Clipboard collection disabled by signal\n");
dbg("Clipboard collection disabled by signal at time %lu\n",
(unsigned long)last_disable_time);
break;
case SIGUSR2:
// If we're already enabled, we need to keep the original timestamp
// so we don't mistakenly filter out valid messages.
if (!enabled) {
last_enable_time = get_current_server_time();
}
enabled = 1;
dbg("Clipboard collection enabled by signal\n");
dbg("Clipboard collection enabled by signal at time %lu\n",
(unsigned long)last_enable_time);
break;
}
write_status();
Expand All @@ -191,6 +227,14 @@ static void handle_signalfd_event(void) {
* desired property type.
*/
static void handle_xfixes_selection_notify(XFixesSelectionNotifyEvent *se) {
if (last_disable_time > 0 && se->timestamp >= last_disable_time &&
se->timestamp < last_enable_time) {
dbg("Ignoring selection event from disabled period (event time: %lu, disabled: %lu, enabled: %lu)\n",
(unsigned long)se->timestamp, (unsigned long)last_disable_time,
(unsigned long)last_enable_time);
return;
}

enum selection_type sel =
selection_atom_to_selection_type(se->selection, sels);
if (sel == CM_SEL_INVALID) {
Expand Down Expand Up @@ -613,6 +657,7 @@ int main(int argc, char *argv[]) {
setup_selections(dpy, sels);

incr_atom = XInternAtom(dpy, "INCR", False);
timestamp_atom = XInternAtom(dpy, "CLIPMENUD_TIMESTAMP", False);

sigset_t mask;
sigemptyset(&mask);
Expand Down