From bfbb51f1d9e4e20d0667b28da5042abf080c323f Mon Sep 17 00:00:00 2001
From: Chris Down <chris@chrisdown.name>
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           | 22 ++++++++++++++++++----
 tests/x_integration_tests | 19 +++++++++++++++++++
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/src/clipmenud.c b/src/clipmenud.c
index 6cf3bfc..3816363 100644
--- a/src/clipmenud.c
+++ b/src/clipmenud.c
@@ -85,7 +85,20 @@ static char *get_clipboard_text(Atom clip_atom) {
         return NULL;
     }
 
-    return (char *)cur_text;
+    // Usually the lifecycle here would be ended by XFree(), but we need
+    // consistency with INCR handling, which uses malloc(), so we can always
+    // use free(). As such, move to the malloc() pool.
+    if (!cur_text) {
+        return NULL;
+    }
+
+    char *m_cur_text = malloc(nitems + 1);
+    expect(m_cur_text);
+    memcpy(m_cur_text, cur_text, nitems);
+    m_cur_text[nitems] = '\0';
+    XFree(cur_text);
+
+    return m_cur_text;
 }
 
 /**
@@ -243,7 +256,7 @@ static uint64_t store_clip(char *text) {
     }
 
     if (last_text) {
-        XFree(last_text);
+        free(last_text);
     }
     last_text = text;
     last_text_time = current_time;
@@ -263,7 +276,7 @@ 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';
@@ -280,6 +293,7 @@ static void incr_receive_finish(struct incr_transfer *it) {
         }
     } else {
         it_dbg(it, "Clipboard text is whitespace only, ignoring\n");
+        free(text);
     }
 
     free(it->data);
@@ -423,7 +437,7 @@ static int handle_property_notify(const XPropertyEvent *pe) {
             }
         } else {
             dbg("Clipboard text is whitespace only, ignoring\n");
-            XFree(text);
+            free(text);
         }
     }
 
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