Skip to content

Fix DnD memory safety issues, protocol output bugs, and undefined behavior #9869

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
kovidgoyal merged 2 commits into from
Apr 14, 2026
+142 −20
Merged

Fix DnD memory safety issues, protocol output bugs, and undefined behavior #9869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8e2b31
Fix memory safety issues, protocol bugs, and add tests for DnD implem…
Copilot Apr 14, 2026
48523ef
Add null check for strdup in dnd_test_drag_notify
Copilot Apr 14, 2026
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
83 changes: 63 additions & 20 deletions kitty/dnd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,8 @@ queue_payload_to_child(id_type id, uint32_t client_id, PendingData *pending, con
ensure_space_for(pending, items, PendingEntry, pending->count + 1, capacity, 32, true);
char *buf = malloc(header_sz + data_sz - offset);
if (!buf) fatal("Out of memory");
memcpy(buf, header, header_sz); memcpy(buf + header_sz, data, data_sz - offset);
memcpy(buf, header, header_sz);
if (data_sz - offset) memcpy(buf + header_sz, data, data_sz - offset);
PendingEntry *e = &pending->items[pending->count++];
e->buf = buf; e->header_sz = header_sz; e->data_sz = data_sz - offset;
e->as_base64 = as_base64; e->client_id = client_id;
Expand Down Expand Up @@ -429,8 +430,9 @@ drop_register_window(Window *w, const uint8_t *payload, size_t payload_sz, bool
if (!payload || !payload_sz) return;
size_t sz = w->drop.registered_mimes ? strlen(w->drop.registered_mimes) : 0;
if (sz + payload_sz > MIME_LIST_SIZE_CAP) return;
w->drop.registered_mimes = realloc(w->drop.registered_mimes, sz + payload_sz + 1);
if (w->drop.registered_mimes) {
char *tmp = realloc(w->drop.registered_mimes, sz + payload_sz + 1);
if (tmp) {
w->drop.registered_mimes = tmp;
memcpy(w->drop.registered_mimes + sz, payload, payload_sz);
sz += payload_sz;
w->drop.registered_mimes[sz] = 0;
Expand Down Expand Up @@ -628,9 +630,9 @@ drop_dispatch_data(Window *w, const char *mime, const char *data, ssize_t sz) {
queue_payload_to_child(w->id, w->drop.client_id, &w->drop.pending, buf, header_size, sz ? data : NULL, sz, true);
if (is_uri_list) {
w->drop.uri_list_sz += sz;
w->drop.uri_list = realloc(w->drop.uri_list, w->drop.uri_list_sz);
if (w->drop.uri_list) memcpy(w->drop.uri_list + w->drop.uri_list_sz - sz, data, sz);
else w->drop.uri_list_sz = 0;
char *tmp = realloc(w->drop.uri_list, w->drop.uri_list_sz);
if (tmp) { w->drop.uri_list = tmp; memcpy(w->drop.uri_list + w->drop.uri_list_sz - sz, data, sz); }
else { free(w->drop.uri_list); w->drop.uri_list = NULL; w->drop.uri_list_sz = 0; }
}
if (sz == 0) { drop_pop_request(w); drop_process_queue(w); }
}
Expand Down Expand Up @@ -1195,6 +1197,7 @@ drag_free_remote_item(DragRemoteItem *x) {
if (x->top_level_parent_dir_fd_plus_one) safe_close(x->top_level_parent_dir_fd_plus_one-1, __FILE__, __LINE__);
if (x->children) {
for (size_t i = 0; i < x->children_sz; i++) drag_free_remote_item(x->children + i);
free(x->children);
}
zero_at_ptr(x);
}
Expand Down Expand Up @@ -1230,6 +1233,7 @@ drag_free_offer(Window *w) {
if (ds.images[i].data) free(ds.images[i].data);
zero_at_ptr(ds.images + i);
}
free_pending(&ds.pending);
ds.allowed_operations = 0;
ds.state = DRAG_SOURCE_NONE;
ds.pre_sent_total_sz = 0;
Expand Down Expand Up @@ -1275,8 +1279,9 @@ drag_add_mimes(Window *w, int allowed_operations, uint32_t client_id, const char
ds.client_id = client_id;
size_t new_sz = ds.bufsz + sz;
if (new_sz > MIME_LIST_SIZE_CAP) abrt(EFBIG);
ds.mimes_buf = realloc(ds.mimes_buf, ds.bufsz + sz + 1);
if (!ds.mimes_buf) abrt(ENOMEM);
char *tmp = realloc(ds.mimes_buf, ds.bufsz + sz + 1);
if (!tmp) abrt(ENOMEM);
ds.mimes_buf = tmp;
memcpy(ds.mimes_buf + ds.bufsz, data, sz);
ds.bufsz = new_sz;
ds.mimes_buf[ds.bufsz] = 0;
Expand Down Expand Up @@ -1315,8 +1320,9 @@ drag_add_pre_sent_data(Window *w, unsigned idx, const uint8_t *payload, size_t s
}
if (item.data_capacity < sz + item.data_size) {
size_t newcap = MAX(item.data_capacity * 2, sz + item.data_size);
item.optional_data = realloc(item.optional_data, newcap);
if (!item.optional_data) abrt(ENOMEM);
uint8_t *tmp = realloc(item.optional_data, newcap);
if (!tmp) abrt(ENOMEM);
item.optional_data = tmp;
item.data_capacity = newcap;
}
size_t outlen = item.data_capacity - item.data_size;
Expand All @@ -1343,8 +1349,9 @@ drag_add_image(Window *w, unsigned idx, int fmt, int width, int height, const ui
}
if (img.capacity < sz + img.sz) {
size_t newcap = MAX(img.capacity * 2, sz + img.sz);
img.data = realloc(img.data, newcap);
if (!img.data) abrt(ENOMEM);
uint8_t *tmp = realloc(img.data, newcap);
if (!tmp) abrt(ENOMEM);
img.data = tmp;
img.capacity = newcap;
}
size_t outlen = img.capacity - img.sz;
Expand All @@ -1362,7 +1369,7 @@ static bool
expand_rgb_data(Window *w, size_t idx) {
#define fail(code) { cancel_drag(w, code); return false; }
if (img.sz != (size_t)img.width * (size_t)img.height * 3) fail(EINVAL);
const size_t sz = img.width * img.height * 4;
const size_t sz = (size_t)img.width * (size_t)img.height * 4u;
RAII_ALLOC(uint8_t, expanded, malloc(sz));
if (!expanded) fail(ENOMEM);
memset(expanded, 0xff, sz);
Expand Down Expand Up @@ -1434,24 +1441,24 @@ void
drag_notify(Window *w, DragNotifyType type) {
if (ds.state < DRAG_SOURCE_STARTED) return;
char buf[128];
size_t sz = snprintf(buf, sizeof(buf), "t=e:x=%d", type + 1);
size_t sz = snprintf(buf, sizeof(buf), "\x1b]%d;t=e:x=%d", DND_CODE, type + 1);
switch(type) {
case DRAG_NOTIFY_ACCEPTED:
for (size_t i = 0; i < ds.num_mimes; i++) {
if (strcmp(ds.items[i].mime_type, global_state.drag_source.accepted_mime_type) == 0) {
sz += snprintf(buf + sz, sizeof(buf) - sz, "y=%zu", i); break;
sz += snprintf(buf + sz, sizeof(buf) - sz, ":y=%zu", i); break;
}
} break;
case DRAG_NOTIFY_ACTION_CHANGED:
switch (global_state.drag_source.action) {
case GLFW_DRAG_OPERATION_MOVE:
sz += snprintf(buf + sz, sizeof(buf) - sz, "o=2"); break;
sz += snprintf(buf + sz, sizeof(buf) - sz, ":o=2"); break;
default:
sz += snprintf(buf + sz, sizeof(buf) - sz, "o=1"); break;
sz += snprintf(buf + sz, sizeof(buf) - sz, ":o=1"); break;
} break;
case DRAG_NOTIFY_DROPPED: ds.state = DRAG_SOURCE_DROPPED; break;
case DRAG_NOTIFY_FINISHED:
sz += snprintf(buf + sz, sizeof(buf) - sz, "y=%d", global_state.drag_source.was_canceled ? 1 : 0); break;
sz += snprintf(buf + sz, sizeof(buf) - sz, ":y=%d", global_state.drag_source.was_canceled ? 1 : 0); break;
}
queue_payload_to_child(w->id, w->drag_source.client_id, &w->drag_source.pending, buf, sz, NULL, 0, false);
if (type == DRAG_NOTIFY_FINISHED) drag_free_offer(w);
Expand Down Expand Up @@ -1758,8 +1765,9 @@ add_payload(Window *w, DragRemoteItem *ri, bool has_more, const uint8_t *payload
if (ri->data_sz + payload_sz > ri->data_capacity) {
size_t cap = MAX(ri->data_capacity * 2, ri->data_sz + payload_sz + 4096);
if (cap > PRESENT_DATA_CAP) abrt(EMFILE);
ri->data = realloc(ri->data, cap);
if (!ri->data) abrt(ENOMEM);
uint8_t *tmp = realloc(ri->data, cap);
if (!tmp) abrt(ENOMEM);
ri->data = tmp;
ri->data_capacity = cap;
}
size_t outlen = ri->data_capacity - ri->data_sz;
Expand All @@ -1777,6 +1785,13 @@ add_payload(Window *w, DragRemoteItem *ri, bool has_more, const uint8_t *payload
ri->fd_plus_one = 0;
break;
case 1:
// Ensure room for the null terminator needed by symlinkat
if (ri->data_sz >= ri->data_capacity) {
uint8_t *tmp = realloc(ri->data, ri->data_sz + 1);
if (!tmp) abrt(ENOMEM);
ri->data = tmp;
ri->data_capacity = ri->data_sz + 1;
}
ri->data[ri->data_sz] = 0;
if (symlinkat((char*)ri->data, dirfd, ri->dir_entry_name) != 0) abrt(errno);
break;
Expand Down Expand Up @@ -1823,6 +1838,7 @@ toplevel_data_for_drag(
int fd = safe_openat(mi.base_dir_fd_plus_one - 1, path, O_RDONLY | O_DIRECTORY, 0);
if (fd < 0) abrt(errno);
ri->top_level_parent_dir_fd_plus_one = fd + 1;
free(mi.uri_list[uri_item_idx]);
mi.uri_list[uri_item_idx] = as_file_url(mi.base_dir_for_remote_items, path, ri->dir_entry_name);
}
add_payload(w, ri, has_more, payload, payload_sz, ri->top_level_parent_dir_fd_plus_one - 1);
Expand Down Expand Up @@ -2115,6 +2131,32 @@ dnd_test_request_drag_data(PyObject *self UNUSED, PyObject *args) {
Py_RETURN_NONE;
}

static PyObject *
dnd_test_drag_notify(PyObject *self UNUSED, PyObject *args) {
// Call drag_notify with a specific type for testing the protocol output.
// type: 0=ACCEPTED, 1=ACTION_CHANGED, 2=DROPPED, 3=FINISHED
// accepted_mime: the MIME type to set in global_state.drag_source.accepted_mime_type (for ACCEPTED)
// action: the action to set in global_state.drag_source.action (for ACTION_CHANGED)
// was_canceled: whether the drag was canceled (for FINISHED)
unsigned long long window_id;
int type;
const char *accepted_mime = NULL;
int action = 0, was_canceled = 0;
if (!PyArg_ParseTuple(args, "Ki|sii", &window_id, &type, &accepted_mime, &action, &was_canceled)) return NULL;
Window *w = window_for_window_id((id_type)window_id);
if (!w) { PyErr_SetString(PyExc_ValueError, "Window not found"); return NULL; }
if (type < 0 || type > 3) { PyErr_SetString(PyExc_ValueError, "Invalid type"); return NULL; }
if (accepted_mime && *accepted_mime) {
free(global_state.drag_source.accepted_mime_type);
global_state.drag_source.accepted_mime_type = strdup(accepted_mime);
if (!global_state.drag_source.accepted_mime_type) { PyErr_NoMemory(); return NULL; }
}
global_state.drag_source.action = action;
global_state.drag_source.was_canceled = was_canceled;
drag_notify(w, (DragNotifyType)type);
Py_RETURN_NONE;
}

static PyMethodDef dnd_methods[] = {
{"dnd_set_test_write_func", (PyCFunction)py_dnd_set_test_write_func, METH_VARARGS, ""},
METHODB(dnd_test_create_fake_window, METH_NOARGS),
Expand All @@ -2124,6 +2166,7 @@ static PyMethodDef dnd_methods[] = {
METHODB(dnd_test_fake_drop_data, METH_VARARGS),
METHODB(dnd_test_force_drag_dropped, METH_VARARGS),
METHODB(dnd_test_request_drag_data, METH_VARARGS),
METHODB(dnd_test_drag_notify, METH_VARARGS),
{NULL, NULL, 0, NULL}
};

Expand Down
79 changes: 79 additions & 0 deletions kitty_tests/dnd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
dnd_set_test_write_func,
dnd_test_cleanup_fake_window,
dnd_test_create_fake_window,
dnd_test_drag_notify,
dnd_test_fake_drop_data,
dnd_test_fake_drop_event,
dnd_test_force_drag_dropped,
Expand Down Expand Up @@ -2989,3 +2990,81 @@ def test_dos_mime_list_size_cap_drop_target(self) -> None:
events = self._get_events(cap, wid)
# Should get a move event
self.assertTrue(len(events) >= 1, events)

def test_drag_notify_colon_separators(self) -> None:
"""drag_notify output has proper colon separators between metadata keys."""
with dnd_test_window() as (osw, wid, screen, cap):
self._setup_drag_offer(screen, wid, cap, 'text/plain text/html')
dnd_test_force_drag_dropped(wid)
# DRAG_NOTIFY_ACCEPTED (type=0) should produce t=e:x=1:y=<idx>
dnd_test_drag_notify(wid, 0, 'text/html')
events = self._get_events(cap, wid)
self.assertEqual(len(events), 1, events)
self.ae(events[0]['type'], 'e')
# Verify proper key formatting with colons
self.ae(events[0]['meta'].get('x'), '1')
self.ae(events[0]['meta'].get('y'), '1') # text/html is index 1

def test_drag_notify_action_changed_colon_separator(self) -> None:
"""drag_notify ACTION_CHANGED output has proper colon separators."""
from kitty.fast_data_types import GLFW_DRAG_OPERATION_MOVE
with dnd_test_window() as (osw, wid, screen, cap):
self._setup_drag_offer(screen, wid, cap, 'text/plain')
dnd_test_force_drag_dropped(wid)
# DRAG_NOTIFY_ACTION_CHANGED (type=1) with MOVE action
dnd_test_drag_notify(wid, 1, '', GLFW_DRAG_OPERATION_MOVE)
events = self._get_events(cap, wid)
self.assertEqual(len(events), 1, events)
self.ae(events[0]['type'], 'e')
self.ae(events[0]['meta'].get('x'), '2') # ACTION_CHANGED = type+1 = 2
self.ae(events[0]['meta'].get('o'), '2') # MOVE = o=2

def test_drag_notify_finished_colon_separator(self) -> None:
"""drag_notify FINISHED output has proper colon separators."""
with dnd_test_window() as (osw, wid, screen, cap):
self._setup_drag_offer(screen, wid, cap, 'text/plain')
dnd_test_force_drag_dropped(wid)
# DRAG_NOTIFY_FINISHED (type=3) with was_canceled=0
dnd_test_drag_notify(wid, 3, '', 0, 0)
events = self._get_events(cap, wid)
self.assertEqual(len(events), 1, events)
self.ae(events[0]['type'], 'e')
self.ae(events[0]['meta'].get('x'), '4') # FINISHED = type+1 = 4
self.ae(events[0]['meta'].get('y'), '0') # was_canceled = 0

def test_remote_drag_children_freed_on_cleanup(self) -> None:
"""Remote drag with directories properly frees the children array on cleanup."""
uri_list = b'file:///home/user/mydir\r\n'
with dnd_test_window() as (osw, wid, screen, cap):
self._setup_remote_drag(screen, wid, cap, uri_list)
# Create a directory entry (X=2 means directory handle=2)
dir_listing = standard_b64encode(b'file1.txt\x00subdir').decode()
parse_bytes(screen, client_remote_file(1, dir_listing, item_type=2))
self._assert_no_output(cap, wid)
# Finish the directory entry
parse_bytes(screen, client_remote_file(1, '', item_type=2))
self._assert_no_output(cap, wid)
# Now send file data for the first child (entry_num=1, Y=handle)
file_data = standard_b64encode(b'hello').decode()
parse_bytes(screen, client_remote_file(1, file_data, item_type=0, parent_handle=2, entry_num=1))
parse_bytes(screen, client_remote_file(1, '', item_type=0, parent_handle=2, entry_num=1))
self._assert_no_output(cap, wid)
# Cleanup happens when context manager exits - no crash means children are freed

def test_remote_drag_uri_replaced_without_leak(self) -> None:
"""Remote drag replaces URI string without leaking the original."""
uri_list = b'file:///home/user/hello.txt\r\n'
file_content = b'test content'
with dnd_test_window() as (osw, wid, screen, cap):
self._setup_remote_drag(screen, wid, cap, uri_list)
b64 = standard_b64encode(file_content).decode()
# Send file data - this replaces the URI string in the uri_list
parse_bytes(screen, client_remote_file(1, b64, item_type=0))
self._assert_no_output(cap, wid)
# End of data for this file
parse_bytes(screen, client_remote_file(1, '', item_type=0))
self._assert_no_output(cap, wid)
# Completion signal
parse_bytes(screen, client_remote_file_finish())
self._assert_no_output(cap, wid)
# No crash or leak - cleanup happens in context manager exit
Loading