From 8aef5e968437fc4ff42acbdeb4df91abcbc9b288 Mon Sep 17 00:00:00 2001 From: Chris Down Date: Sat, 14 Dec 2024 01:36:05 +0000 Subject: [PATCH] clipmenud: Ensure consistent memory management for clipboard text Previously, clipboard text was sometimes freed with XFree() and sometimes with free(), potentially causing double frees when using the INCR protocol for large clippings. Convert the XGetWindowProperty()-allocated memory to a malloc()-allocated buffer immediately in get_clipboard_text(), and always free it with free(). Fixes #241. --- src/clipmenud.c | 83 ++++++++++++++++++++++++++++----------- tests/x_integration_tests | 19 +++++++++ 2 files changed, 78 insertions(+), 24 deletions(-) diff --git a/src/clipmenud.c b/src/clipmenud.c index 6cf3bfc..08e0785 100644 --- a/src/clipmenud.c +++ b/src/clipmenud.c @@ -34,6 +34,32 @@ static struct incr_transfer *it_list; static struct cm_selections sels[CM_SEL_MAX]; +enum clip_text_source { + CLIP_TEXT_SOURCE_X, + CLIP_TEXT_SOURCE_MALLOC, + CLIP_TEXT_SOURCE_INVALID +}; + +struct clip_text { + char *data; + enum clip_text_source source; +}; + +static void free_clip_text(struct clip_text *ct) { + expect(ct->source != CLIP_TEXT_SOURCE_INVALID); + + if (ct->data) { + if (ct->source == CLIP_TEXT_SOURCE_X) { + XFree(ct->data); + } else { + free(ct->data); + } + ct->data = NULL; + } + + ct->source = CLIP_TEXT_SOURCE_INVALID; +} + /** * Check if a text s1 is a possible partial of s2. * @@ -66,7 +92,8 @@ static bool is_possible_partial(const char *s1, const char *s2) { * happen a conversion must have been performed in an earlier iteration with * XConvertSelection. */ -static char *get_clipboard_text(Atom clip_atom) { +static struct clip_text get_clipboard_text(Atom clip_atom) { + struct clip_text ct = {NULL, CLIP_TEXT_SOURCE_X}; unsigned char *cur_text; Atom actual_type; int actual_format; @@ -76,16 +103,18 @@ static char *get_clipboard_text(Atom clip_atom) { AnyPropertyType, &actual_type, &actual_format, &nitems, &bytes_after, &cur_text); if (res != Success) { - return NULL; + return ct; } if (actual_type == incr_atom) { dbg("Unexpected INCR transfer detected\n"); XFree(cur_text); - return NULL; + return ct; } - return (char *)cur_text; + ct.data = (char *)cur_text; + + return ct; } /** @@ -226,28 +255,31 @@ static void maybe_trim(void) { * Store the clipboard text. If the text is a possible partial of the last clip * and it was received shortly afterwards, replace instead of adding. */ -static uint64_t store_clip(char *text) { - static char *last_text = NULL; +static uint64_t store_clip(struct clip_text *ct) { + static struct clip_text last_text = {NULL, CLIP_TEXT_SOURCE_MALLOC}; static time_t last_text_time; dbg("Clipboard text is considered salient, storing\n"); time_t current_time = time(NULL); uint64_t hash; - if (last_text && + + if (last_text.data && difftime(current_time, last_text_time) <= PARTIAL_MAX_SECS && - is_possible_partial(last_text, text)) { + is_possible_partial(last_text.data, ct->data)) { dbg("Possible partial of last clip, replacing\n"); - expect(cs_replace(&cs, CS_ITER_NEWEST_FIRST, 0, text, &hash) == 0); + expect(cs_replace(&cs, CS_ITER_NEWEST_FIRST, 0, ct->data, &hash) == 0); } else { - expect(cs_add(&cs, text, &hash) == 0); + expect(cs_add(&cs, ct->data, &hash) == 0); } - if (last_text) { - XFree(last_text); - } - last_text = text; + free_clip_text(&last_text); + last_text = *ct; last_text_time = current_time; + // The caller no longer owns this data. + ct->data = NULL; + ct->source = CLIP_TEXT_SOURCE_INVALID; + return hash; } @@ -263,23 +295,26 @@ static void incr_receive_finish(struct incr_transfer *it) { } it_dbg(it, "Finished (bytes buffered: %zu)\n", it->data_size); - _drop_(free) char *text = malloc(it->data_size + 1); + char *text = malloc(it->data_size + 1); expect(text); memcpy(text, it->data, it->data_size); text[it->data_size] = '\0'; + struct clip_text ct = {text, CLIP_TEXT_SOURCE_MALLOC}; + char line[CS_SNIP_LINE_SIZE]; - first_line(text, line); + first_line(ct.data, line); it_dbg(it, "First line: %s\n", line); - if (is_salient_text(text)) { - uint64_t hash = store_clip(text); + if (is_salient_text(ct.data)) { + uint64_t hash = store_clip(&ct); maybe_trim(); if (cfg.owned_selections[sel].active && cfg.own_clipboard) { run_clipserve(hash); } } else { it_dbg(it, "Clipboard text is whitespace only, ignoring\n"); + free_clip_text(&ct); } free(it->data); @@ -399,17 +434,17 @@ static int handle_property_notify(const XPropertyEvent *pe) { // store_clip will take care of freeing this later when it's gone from // last_text. - char *text = get_clipboard_text(pe->atom); - if (!text) { + struct clip_text ct = get_clipboard_text(pe->atom); + if (!ct.data) { dbg("Failed to get clipboard text\n"); return -EINVAL; } char line[CS_SNIP_LINE_SIZE]; - first_line(text, line); + first_line(ct.data, line); dbg("First line: %s\n", line); - if (is_salient_text(text)) { - uint64_t hash = store_clip(text); + if (is_salient_text(ct.data)) { + uint64_t hash = store_clip(&ct); maybe_trim(); /* We only own CLIPBOARD because otherwise the behaviour is wonky: * @@ -423,7 +458,7 @@ static int handle_property_notify(const XPropertyEvent *pe) { } } else { dbg("Clipboard text is whitespace only, ignoring\n"); - XFree(text); + free_clip_text(&ct); } } diff --git a/tests/x_integration_tests b/tests/x_integration_tests index 43ffc75..9ca3525 100755 --- a/tests/x_integration_tests +++ b/tests/x_integration_tests @@ -195,6 +195,25 @@ len_after=$(xsel -po | wc -c) # Make sure we got the whole thing (( len_before == len_after )) +# Issue #241. Put a large clipboard selection via INCR, then put something else +# small immediately after. +xsel -bc +settle +check_nr_clips 5 + +# Large selection (INCR) +printf '%.0sa' {1..4001} | xsel -b +long_settle + +check_nr_clips 6 + +# After large selection is stored, put something small that's a possible # +# partial (non-INCR) +printf a | xsel -b +settle + +check_nr_clips 6 + if (( _UNSHARED )); then umount -l /tmp fi