Merge pull request #926 from emersion/fix-x11-backend-memory-leaks

Fix some backend memory leaks

backend/headless/input_device.c

@@ -9,15 +9,7 @@
 #include "backend/headless.h"
 #include "util/signal.h"
 
-static void input_device_destroy(struct wlr_input_device *wlr_dev) {
-	struct wlr_headless_input_device *device =
-		(struct wlr_headless_input_device *)wlr_dev;
-	free(device);
-}
-
-static const struct wlr_input_device_impl input_device_impl = {
-	.destroy = input_device_destroy,
-};
+static const struct wlr_input_device_impl input_device_impl = { 0 };
 
 bool wlr_input_device_is_headless(struct wlr_input_device *wlr_dev) {
 	return wlr_dev->impl == &input_device_impl;

backend/libinput/keyboard.c

@@ -13,7 +13,8 @@ struct wlr_libinput_keyboard {
 };
 
 static void keyboard_set_leds(struct wlr_keyboard *wlr_kb, uint32_t leds) {
-	struct wlr_libinput_keyboard *wlr_libinput_kb = (struct wlr_libinput_keyboard *)wlr_kb;
+	struct wlr_libinput_keyboard *wlr_libinput_kb =
+		(struct wlr_libinput_keyboard *)wlr_kb;
 	libinput_device_led_update(wlr_libinput_kb->libinput_dev, leds);
 }
 
@@ -21,6 +22,7 @@ static void keyboard_destroy(struct wlr_keyboard *wlr_kb) {
 	struct wlr_libinput_keyboard *wlr_libinput_kb =
 		(struct wlr_libinput_keyboard *)wlr_kb;
 	libinput_device_unref(wlr_libinput_kb->libinput_dev);
+	free(wlr_libinput_kb);
 }
 
 struct wlr_keyboard_impl impl = {

backend/wayland/backend.c

@@ -176,7 +176,8 @@ static void handle_display_destroy(struct wl_listener *listener, void *data) {
 	backend_destroy(&backend->backend);
 }
 
-struct wlr_backend *wlr_wl_backend_create(struct wl_display *display, const char *remote) {
+struct wlr_backend *wlr_wl_backend_create(struct wl_display *display,
+		const char *remote) {
 	wlr_log(L_INFO, "Creating wayland backend");
 
 	struct wlr_wl_backend *backend = calloc(1, sizeof(struct wlr_wl_backend));
@@ -194,26 +195,40 @@ struct wlr_backend *wlr_wl_backend_create(struct wl_display *display, const char
 	backend->remote_display = wl_display_connect(remote);
 	if (!backend->remote_display) {
 		wlr_log_errno(L_ERROR, "Could not connect to remote display");
-		return false;
+		goto error_connect;
 	}
 
 	backend->registry = wl_display_get_registry(backend->remote_display);
 	if (backend->registry == NULL) {
 		wlr_log_errno(L_ERROR, "Could not obtain reference to remote registry");
-		return false;
+		goto error_registry;
 	}
 
-	wlr_egl_init(&backend->egl, EGL_PLATFORM_WAYLAND_EXT,
-		backend->remote_display, NULL, WL_SHM_FORMAT_ARGB8888);
+	if (!wlr_egl_init(&backend->egl, EGL_PLATFORM_WAYLAND_EXT,
+			backend->remote_display, NULL, WL_SHM_FORMAT_ARGB8888)) {
+		wlr_log(L_ERROR, "Could not initialize EGL");
+		goto error_egl;
+	}
 	wlr_egl_bind_display(&backend->egl, backend->local_display);
 
 	backend->renderer = wlr_gles2_renderer_create(&backend->egl);
 	if (backend->renderer == NULL) {
-		wlr_log_errno(L_ERROR, "Could not create renderer");
+		wlr_log(L_ERROR, "Could not create renderer");
+		goto error_renderer;
 	}
 
 	backend->local_display_destroy.notify = handle_display_destroy;
 	wl_display_add_destroy_listener(display, &backend->local_display_destroy);
 
 	return &backend->backend;
+
+error_renderer:
+	wlr_egl_finish(&backend->egl);
+error_egl:
+	wl_registry_destroy(backend->registry);
+error_registry:
+	wl_display_disconnect(backend->remote_display);
+error_connect:
+	free(backend);
+	return NULL;
 }

backend/x11/backend.c

@@ -223,14 +223,9 @@ static void backend_destroy(struct wlr_backend *backend) {
 
 	wlr_signal_emit_safe(&x11->pointer_dev.events.destroy, &x11->pointer_dev);
 	wlr_signal_emit_safe(&x11->keyboard_dev.events.destroy, &x11->keyboard_dev);
-	// TODO probably need to use wlr_keyboard_destroy, but the devices need to
-	// be malloced for that to work
-	if (x11->keyboard_dev.keyboard->keymap) {
-		xkb_keymap_unref(x11->keyboard_dev.keyboard->keymap);
-	}
-	if (x11->keyboard_dev.keyboard->xkb_state) {
-		xkb_state_unref(x11->keyboard_dev.keyboard->xkb_state);
-	}
+
+	wlr_input_device_destroy(&x11->keyboard_dev);
+	wlr_input_device_destroy(&x11->pointer_dev);
 
 	wlr_signal_emit_safe(&backend->events.destroy, backend);
 
@@ -322,12 +317,12 @@ struct wlr_backend *wlr_x11_backend_create(struct wl_display *display,
 
 	wlr_input_device_init(&x11->keyboard_dev, WLR_INPUT_DEVICE_KEYBOARD,
 		&input_device_impl, "X11 keyboard", 0, 0);
-	wlr_keyboard_init(&x11->keyboard, NULL);
+	wlr_keyboard_init(&x11->keyboard, &keyboard_impl);
 	x11->keyboard_dev.keyboard = &x11->keyboard;
 
 	wlr_input_device_init(&x11->pointer_dev, WLR_INPUT_DEVICE_POINTER,
 		&input_device_impl, "X11 pointer", 0, 0);
-	wlr_pointer_init(&x11->pointer, NULL);
+	wlr_pointer_init(&x11->pointer, &pointer_impl);
 	x11->pointer_dev.pointer = &x11->pointer;
 
 	x11->display_destroy.notify = handle_display_destroy;

backend/x11/input_device.c

@@ -136,7 +136,29 @@ void handle_x11_input_event(struct wlr_x11_backend *x11,
 	}
 }
 
-const struct wlr_input_device_impl input_device_impl = { 0 };
+static void input_device_destroy(struct wlr_input_device *wlr_device) {
+	// Don't free the input device, it's on the stack
+}
+
+const struct wlr_input_device_impl input_device_impl = {
+	.destroy = input_device_destroy,
+};
+
+static void keyboard_destroy(struct wlr_keyboard *wlr_keyboard) {
+	// Don't free the keyboard, it's on the stack
+}
+
+const struct wlr_keyboard_impl keyboard_impl = {
+	.destroy = keyboard_destroy,
+};
+
+static void pointer_destroy(struct wlr_pointer *wlr_pointer) {
+	// Don't free the pointer, it's on the stack
+}
+
+const struct wlr_pointer_impl pointer_impl = {
+	.destroy = pointer_destroy,
+};
 
 void update_x11_pointer_position(struct wlr_x11_output *output,
 		xcb_timestamp_t time) {

include/backend/x11.h

@@ -77,6 +77,8 @@ struct wlr_x11_output *get_x11_output_from_window_id(struct wlr_x11_backend *x11
 void get_x11_output_layout_box(struct wlr_x11_backend *backend,
 	struct wlr_box *box);
 
+extern const struct wlr_keyboard_impl keyboard_impl;
+extern const struct wlr_pointer_impl pointer_impl;
 extern const struct wlr_input_device_impl input_device_impl;
 
 void handle_x11_input_event(struct wlr_x11_backend *x11,

include/wlr/interfaces/wlr_keyboard.h

@@ -9,7 +9,8 @@ struct wlr_keyboard_impl {
 	void (*led_update)(struct wlr_keyboard *keyboard, uint32_t leds);
 };
 
-void wlr_keyboard_init(struct wlr_keyboard *keyboard, struct wlr_keyboard_impl *impl);
+void wlr_keyboard_init(struct wlr_keyboard *keyboard,
+		const struct wlr_keyboard_impl *impl);
 void wlr_keyboard_destroy(struct wlr_keyboard *keyboard);
 void wlr_keyboard_notify_key(struct wlr_keyboard *keyboard,
 		struct wlr_event_keyboard_key *event);

include/wlr/interfaces/wlr_pointer.h

@@ -8,7 +8,7 @@ struct wlr_pointer_impl {
 };
 
 void wlr_pointer_init(struct wlr_pointer *pointer,
-		struct wlr_pointer_impl *impl);
+		const struct wlr_pointer_impl *impl);
 void wlr_pointer_destroy(struct wlr_pointer *pointer);
 
 #endif

include/wlr/types/wlr_keyboard.h

@@ -40,8 +40,7 @@ struct wlr_keyboard_modifiers {
 };
 
 struct wlr_keyboard {
-	struct wlr_keyboard_impl *impl;
-	// TODO: Should this store key repeat info too?
+	const struct wlr_keyboard_impl *impl;
 
 	int keymap_fd;
 	size_t keymap_size;

include/wlr/types/wlr_pointer.h

@@ -8,7 +8,7 @@
 struct wlr_pointer_impl;
 
 struct wlr_pointer {
-	struct wlr_pointer_impl *impl;
+	const struct wlr_pointer_impl *impl;
 
 	struct {
 		struct wl_signal motion;

types/wlr_keyboard.c

@@ -138,7 +138,7 @@ void wlr_keyboard_notify_key(struct wlr_keyboard *keyboard,
 }
 
 void wlr_keyboard_init(struct wlr_keyboard *kb,
-		struct wlr_keyboard_impl *impl) {
+		const struct wlr_keyboard_impl *impl) {
 	kb->impl = impl;
 	wl_signal_init(&kb->events.key);
 	wl_signal_init(&kb->events.modifiers);
@@ -154,16 +154,16 @@ void wlr_keyboard_destroy(struct wlr_keyboard *kb) {
 	if (kb == NULL) {
 		return;
 	}
+	xkb_state_unref(kb->xkb_state);
+	xkb_keymap_unref(kb->keymap);
+	close(kb->keymap_fd);
 	if (kb->impl && kb->impl->destroy) {
 		kb->impl->destroy(kb);
 	} else {
 		wl_list_remove(&kb->events.key.listener_list);
-	}
-	xkb_state_unref(kb->xkb_state);
-	xkb_keymap_unref(kb->keymap);
-	close(kb->keymap_fd);
 		free(kb);
 	}
+}
 
 void wlr_keyboard_led_update(struct wlr_keyboard *kb, uint32_t leds) {
 	if (kb->impl && kb->impl->led_update) {

types/wlr_pointer.c

@@ -5,7 +5,7 @@
 #include <wlr/types/wlr_pointer.h>
 
 void wlr_pointer_init(struct wlr_pointer *pointer,
-		struct wlr_pointer_impl *impl) {
+		const struct wlr_pointer_impl *impl) {
 	pointer->impl = impl;
 	wl_signal_init(&pointer->events.motion);
 	wl_signal_init(&pointer->events.motion_absolute);