From f0d455f088510bf8a79aaccb2c67fc2a926b5b1a Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 09:59:44 +0900 Subject: [PATCH 01/13] drm backend: overflow fixes These operations are done in 32-bit arithmetics before being casted to 64-bit, thus can overflow before the cast. Casting early fixes the issue. Found through static analysis --- backend/drm/atomic.c | 4 ++-- backend/drm/drm.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/drm/atomic.c b/backend/drm/atomic.c index acc56e65d..41b6424c8 100644 --- a/backend/drm/atomic.c +++ b/backend/drm/atomic.c @@ -83,8 +83,8 @@ static void set_plane_props(struct atomic *atom, struct wlr_drm_plane *plane, // The src_* properties are in 16.16 fixed point atomic_add(atom, id, props->src_x, 0); atomic_add(atom, id, props->src_y, 0); - atomic_add(atom, id, props->src_w, plane->surf.width << 16); - atomic_add(atom, id, props->src_h, plane->surf.height << 16); + atomic_add(atom, id, props->src_w, (uint64_t)plane->surf.width << 16); + atomic_add(atom, id, props->src_h, (uint64_t)plane->surf.height << 16); atomic_add(atom, id, props->crtc_w, plane->surf.width); atomic_add(atom, id, props->crtc_h, plane->surf.height); atomic_add(atom, id, props->fb_id, fb_id); diff --git a/backend/drm/drm.c b/backend/drm/drm.c index c5db480e7..f4a971a24 100644 --- a/backend/drm/drm.c +++ b/backend/drm/drm.c @@ -973,7 +973,7 @@ int handle_drm_event(int fd, uint32_t mask, void *data) { } void restore_drm_outputs(struct wlr_drm_backend *drm) { - uint64_t to_close = (1 << wl_list_length(&drm->outputs)) - 1; + uint64_t to_close = (1L << wl_list_length(&drm->outputs)) - 1; struct wlr_drm_connector *conn; wl_list_for_each(conn, &drm->outputs, link) { From 4f7b1382d4ceb1ed308563809d485ea6c047f077 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 10:03:26 +0900 Subject: [PATCH 02/13] wayland backend seat: fix NULL output check The test was done after dereferencing output in pointer_handle_enter, just move it up one line. No reason pointer_handle_leave would not need the check if enter needs it, add it there. Found through static analysis. --- backend/wayland/wl_seat.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/backend/wayland/wl_seat.c b/backend/wayland/wl_seat.c index 8ed614094..d5001a513 100644 --- a/backend/wayland/wl_seat.c +++ b/backend/wayland/wl_seat.c @@ -38,10 +38,8 @@ static void pointer_handle_enter(void *data, struct wl_pointer *wl_pointer, } struct wlr_wl_output *output = wl_surface_get_user_data(surface); + assert(output); struct wlr_wl_pointer *pointer = output_get_pointer(output); - if (output == NULL) { - return; - } output->enter_serial = serial; backend->current_pointer = pointer; @@ -56,6 +54,7 @@ static void pointer_handle_leave(void *data, struct wl_pointer *wl_pointer, } struct wlr_wl_output *output = wl_surface_get_user_data(surface); + assert(output); output->enter_serial = 0; if (backend->current_pointer == NULL || From bcc2c64c1e1a4562699a94deb6f9d57e1e072ed8 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 10:17:36 +0900 Subject: [PATCH 03/13] x11 backend init: fix leak on failed XOpenDisplay Found through static analysis --- backend/x11/backend.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/backend/x11/backend.c b/backend/x11/backend.c index d4793b9c2..e35cbed7c 100644 --- a/backend/x11/backend.c +++ b/backend/x11/backend.c @@ -245,13 +245,13 @@ struct wlr_backend *wlr_x11_backend_create(struct wl_display *display, x11->xlib_conn = XOpenDisplay(x11_display); if (!x11->xlib_conn) { wlr_log(L_ERROR, "Failed to open X connection"); - return NULL; + goto error_x11; } x11->xcb_conn = XGetXCBConnection(x11->xlib_conn); if (!x11->xcb_conn || xcb_connection_has_error(x11->xcb_conn)) { wlr_log(L_ERROR, "Failed to open xcb connection"); - goto error_x11; + goto error_display; } XSetEventQueueOwner(x11->xlib_conn, XCBOwnsEventQueue); @@ -262,7 +262,7 @@ struct wlr_backend *wlr_x11_backend_create(struct wl_display *display, x11->event_source = wl_event_loop_add_fd(ev, fd, events, x11_event, x11); if (!x11->event_source) { wlr_log(L_ERROR, "Could not create event source"); - goto error_x11; + goto error_display; } x11->screen = xcb_setup_roots_iterator(xcb_get_setup(x11->xcb_conn)).data; @@ -291,8 +291,9 @@ struct wlr_backend *wlr_x11_backend_create(struct wl_display *display, error_event: wl_event_source_remove(x11->event_source); -error_x11: +error_display: XCloseDisplay(x11->xlib_conn); +error_x11: free(x11); return NULL; } From 1e17f4deb6e73880fe135662a483a4fb0af690c7 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 10:18:58 +0900 Subject: [PATCH 04/13] rootston: fix leak in handle_layer_shell_surface Found through static analysis --- rootston/layer_shell.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/rootston/layer_shell.c b/rootston/layer_shell.c index db0aeb596..2adf11a5a 100644 --- a/rootston/layer_shell.c +++ b/rootston/layer_shell.c @@ -381,12 +381,6 @@ void handle_layer_shell_surface(struct wl_listener *listener, void *data) { layer_surface->client_pending.margin.bottom, layer_surface->client_pending.margin.left); - struct roots_layer_surface *roots_surface = - calloc(1, sizeof(struct roots_layer_surface)); - if (!roots_surface) { - return; - } - if (!layer_surface->output) { struct roots_input *input = desktop->server->input; struct roots_seat *seat = input_last_active_seat(input); @@ -409,6 +403,12 @@ void handle_layer_shell_surface(struct wl_listener *listener, void *data) { } } + struct roots_layer_surface *roots_surface = + calloc(1, sizeof(struct roots_layer_surface)); + if (!roots_surface) { + return; + } + roots_surface->surface_commit.notify = handle_surface_commit; wl_signal_add(&layer_surface->surface->events.commit, &roots_surface->surface_commit); From 266898ca1f1eeabaaff3f951a17e612147153ce5 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 10:28:41 +0900 Subject: [PATCH 05/13] direct session backend: fix closing -1 on error Found through static analysis --- backend/session/direct-ipc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/backend/session/direct-ipc.c b/backend/session/direct-ipc.c index f8ba07f71..2dd777c87 100644 --- a/backend/session/direct-ipc.c +++ b/backend/session/direct-ipc.c @@ -159,7 +159,9 @@ static void communicate(int sock) { } error: send_msg(sock, ret ? -1 : fd, &ret, sizeof(ret)); - close(fd); + if (fd >= 0) { + close(fd); + } break; From efef54ccf56b298e935b8707c6808e7f4eebd030 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 10:41:10 +0900 Subject: [PATCH 06/13] wlr_keyboard: fix mmap leak + logic on close for keymap_fd mmap leak found through static analysis --- types/wlr_keyboard.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/types/wlr_keyboard.c b/types/wlr_keyboard.c index 0f0cb1cfe..3afbe517f 100644 --- a/types/wlr_keyboard.c +++ b/types/wlr_keyboard.c @@ -144,6 +144,7 @@ void wlr_keyboard_init(struct wlr_keyboard *kb, wl_signal_init(&kb->events.modifiers); wl_signal_init(&kb->events.keymap); wl_signal_init(&kb->events.repeat_info); + kb->keymap_fd = -1; // Sane defaults kb->repeat_info.rate = 25; @@ -156,7 +157,9 @@ void wlr_keyboard_destroy(struct wlr_keyboard *kb) { } xkb_state_unref(kb->xkb_state); xkb_keymap_unref(kb->keymap); - close(kb->keymap_fd); + if (kb->keymap_fd >= 0) { + close(kb->keymap_fd); + } if (kb->impl && kb->impl->destroy) { kb->impl->destroy(kb); } else { @@ -212,7 +215,7 @@ void wlr_keyboard_set_keymap(struct wlr_keyboard *kb, keymap_str = xkb_keymap_get_as_string(kb->keymap, XKB_KEYMAP_FORMAT_TEXT_V1); kb->keymap_size = strlen(keymap_str) + 1; - if (kb->keymap_fd) { + if (kb->keymap_fd >= 0) { close(kb->keymap_fd); } kb->keymap_fd = os_create_anonymous_file(kb->keymap_size); @@ -228,6 +231,7 @@ void wlr_keyboard_set_keymap(struct wlr_keyboard *kb, } strcpy(ptr, keymap_str); free(keymap_str); + munmap(ptr, kb->keymap_size); for (size_t i = 0; i < kb->num_keycodes; ++i) { xkb_keycode_t keycode = kb->keycodes[i] + 8; @@ -244,7 +248,10 @@ err: kb->xkb_state = NULL; xkb_keymap_unref(keymap); kb->keymap = NULL; - close(kb->keymap_fd); + if (kb->keymap_fd >= 0) { + close(kb->keymap_fd); + kb->keymap_fd = -1; + } free(keymap_str); } From 399de4d11bb36b71fdfb5f1a06e74cf7c4e6831c Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 10:55:33 +0900 Subject: [PATCH 07/13] util/create_tmpfile: set restrictive umask for these files Even if the file is removed right away, a race with someone using inotify is definitely possible, so play safe and restrict umask for our tmpfiles Found through static analysis. --- util/os-compatibility.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/util/os-compatibility.c b/util/os-compatibility.c index bd3067d21..38333605d 100644 --- a/util/os-compatibility.c +++ b/util/os-compatibility.c @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include "util/os-compatibility.h" @@ -61,6 +62,7 @@ int create_tmpfile_cloexec(char *tmpname) { int fd; + mode_t prev_umask = umask(0066); #ifdef HAVE_MKOSTEMP fd = mkostemp(tmpname, O_CLOEXEC); if (fd >= 0) @@ -72,6 +74,7 @@ int create_tmpfile_cloexec(char *tmpname) unlink(tmpname); } #endif + umask(prev_umask); return fd; } From b3313b7f395983482d6efc60db039ef7a97cd6e0 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 10:58:22 +0900 Subject: [PATCH 08/13] wlr_output: fix scope for 'now' 'when' points to now that was defined in the if, so compiler could reuse that memory area by the time 'when' is called Found through static analysis. --- types/wlr_output.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types/wlr_output.c b/types/wlr_output.c index 40332efdb..9f13bef4b 100644 --- a/types/wlr_output.c +++ b/types/wlr_output.c @@ -486,8 +486,8 @@ bool wlr_output_swap_buffers(struct wlr_output *output, struct timespec *when, pixman_region32_intersect(&render_damage, &render_damage, damage); } + struct timespec now; if (when == NULL) { - struct timespec now; clock_gettime(CLOCK_MONOTONIC, &now); when = &now; } From 4cc441248121493350eb50277d2815ec31e9ea59 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 11:04:17 +0900 Subject: [PATCH 09/13] wlr_renderer_destroy: fix renderer NULL check renderer is checked for NULL, but was dereferenced before that. Found through static analysis --- render/wlr_renderer.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/render/wlr_renderer.c b/render/wlr_renderer.c index 98c911327..00f1e4111 100644 --- a/render/wlr_renderer.c +++ b/render/wlr_renderer.c @@ -25,9 +25,12 @@ void wlr_renderer_init(struct wlr_renderer *renderer, } void wlr_renderer_destroy(struct wlr_renderer *r) { + if (!r) { + return; + } wlr_signal_emit_safe(&r->events.destroy, r); - if (r && r->impl && r->impl->destroy) { + if (r->impl && r->impl->destroy) { r->impl->destroy(r); } else { free(r); From 1940c6bbd9c0a8867e40a36f27b69c7069213cf0 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 11:08:49 +0900 Subject: [PATCH 10/13] wayland backend: fix width/height == 0 check We cannot handle just one of the two being NULL later down the road (e.g. divide by zero in matrix projection code), just ignore any such configure request. Found through static analysis --- backend/wayland/output.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/wayland/output.c b/backend/wayland/output.c index 42b415086..6aa59537c 100644 --- a/backend/wayland/output.c +++ b/backend/wayland/output.c @@ -220,7 +220,7 @@ static void xdg_toplevel_handle_configure(void *data, struct zxdg_toplevel_v6 *x struct wlr_wl_output *output = data; assert(output && output->xdg_toplevel == xdg_toplevel); - if (width == 0 && height == 0) { + if (width == 0 || height == 0) { return; } // loop over states for maximized etc? From e5348ad7d374713d4e1a386849b15fb0d68de31c Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 11:11:06 +0900 Subject: [PATCH 11/13] backend autocreate: fix leak when WLR_BACKENDS is set Found through static analysis --- backend/backend.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/backend/backend.c b/backend/backend.c index 07c171bca..07e05fcad 100644 --- a/backend/backend.c +++ b/backend/backend.c @@ -203,6 +203,7 @@ struct wlr_backend *wlr_backend_autocreate(struct wl_display *display, wlr_log(L_ERROR, "failed to start backend '%s'", name); wlr_backend_destroy(backend); wlr_session_destroy(session); + free(names); return NULL; } @@ -210,12 +211,14 @@ struct wlr_backend *wlr_backend_autocreate(struct wl_display *display, wlr_log(L_ERROR, "failed to add backend '%s'", name); wlr_backend_destroy(backend); wlr_session_destroy(session); + free(names); return NULL; } name = strtok_r(NULL, ",", &saveptr); } + free(names); return backend; } From 1fef1f88b2c28777d559f7240aafe0dc078cf9e5 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 11:17:30 +0900 Subject: [PATCH 12/13] export dmabuf manager_handle_capture_output: fix leak on error Found through static analysis --- types/wlr_export_dmabuf_v1.c | 1 + 1 file changed, 1 insertion(+) diff --git a/types/wlr_export_dmabuf_v1.c b/types/wlr_export_dmabuf_v1.c index 68adda02f..e68b0fefa 100644 --- a/types/wlr_export_dmabuf_v1.c +++ b/types/wlr_export_dmabuf_v1.c @@ -84,6 +84,7 @@ static void manager_handle_capture_output(struct wl_client *client, &zwlr_export_dmabuf_frame_v1_interface, version, id); if (frame->resource == NULL) { wl_client_post_no_memory(client); + free(frame); return; } wl_resource_set_implementation(frame->resource, &frame_impl, frame, From 0c2a64df18f8740ab795fb2970d1954a8aac34b1 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sat, 30 Jun 2018 11:18:42 +0900 Subject: [PATCH 13/13] headless add_input_device: fix leak on error Found through static analysis --- backend/headless/input_device.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/backend/headless/input_device.c b/backend/headless/input_device.c index a1e184286..63d28e8ec 100644 --- a/backend/headless/input_device.c +++ b/backend/headless/input_device.c @@ -39,7 +39,7 @@ struct wlr_input_device *wlr_headless_add_input_device( wlr_device->keyboard = calloc(1, sizeof(struct wlr_keyboard)); if (wlr_device->keyboard == NULL) { wlr_log(L_ERROR, "Unable to allocate wlr_keyboard"); - return NULL; + goto error; } wlr_keyboard_init(wlr_device->keyboard, NULL); break; @@ -47,7 +47,7 @@ struct wlr_input_device *wlr_headless_add_input_device( wlr_device->pointer = calloc(1, sizeof(struct wlr_pointer)); if (wlr_device->pointer == NULL) { wlr_log(L_ERROR, "Unable to allocate wlr_pointer"); - return NULL; + goto error; } wlr_pointer_init(wlr_device->pointer, NULL); break; @@ -55,7 +55,7 @@ struct wlr_input_device *wlr_headless_add_input_device( wlr_device->touch = calloc(1, sizeof(struct wlr_touch)); if (wlr_device->touch == NULL) { wlr_log(L_ERROR, "Unable to allocate wlr_touch"); - return NULL; + goto error; } wlr_touch_init(wlr_device->touch, NULL); break; @@ -63,7 +63,7 @@ struct wlr_input_device *wlr_headless_add_input_device( wlr_device->tablet_tool = calloc(1, sizeof(struct wlr_tablet_tool)); if (wlr_device->tablet_tool == NULL) { wlr_log(L_ERROR, "Unable to allocate wlr_tablet_tool"); - return NULL; + goto error; } wlr_tablet_tool_init(wlr_device->tablet_tool, NULL); break; @@ -71,7 +71,7 @@ struct wlr_input_device *wlr_headless_add_input_device( wlr_device->tablet_pad = calloc(1, sizeof(struct wlr_tablet_pad)); if (wlr_device->tablet_pad == NULL) { wlr_log(L_ERROR, "Unable to allocate wlr_tablet_pad"); - return NULL; + goto error; } wlr_tablet_pad_init(wlr_device->tablet_pad, NULL); break; @@ -84,4 +84,7 @@ struct wlr_input_device *wlr_headless_add_input_device( } return wlr_device; +error: + free(device); + return NULL; }