From 3806c948b5e3af1e13383ce3b24b0cebfde0a721 Mon Sep 17 00:00:00 2001 From: Thomas Lukaszewicz Date: Fri, 21 Jul 2023 23:28:12 +0000 Subject: [PATCH 4/5] Fix buffer resize crash Currently chromium wayland clients can crash if a connection buffer (which is initialized at a capacity of 2^12) is asked to resize exactly at its resize threshold. This threshold is in steps of powers of 2. Take net_size to be the size the connection buffer is required to support for the current outbound request queue. This is crash due to certain parts of the code assuming that net_size can exactly match the capacity of the connection buffer, while other parts of the code assume the capacity of the buffer must exceed the net_size. The latter assumption is the root cause of the crash. Specifically: 1. ring_buffer_ensure_space() attempts to find the new buffer `size_bits` required to contain the new `net_size` of the request buffer [1] 2. This eventually calls into get_max_size_bits_for_size() which finds the `size_bits` required for `net_size`[2] - This code assumes that `net_size <= 2^size_bits` in order to fit the data 3. Back in ring_buffer_ensure_space() the `size_bits` returned is checked to ensure it can contain `net_size` and if a resize is needed. - This code assumes that `net_size < 2^size_bits` in order to fit the data [3] - If a resize is not needed, the buffer is checked whether it can hold `net_size` without resizing and a crash occurs [4] The assumptions about buffer capacity differ in 2 and 3, and the assertion in 3 causes the crash in the associated bug. This only occurs when `net_size == 2^size_bits` which satisfies the condition in 2 but fails the condition in 3. In Lacros code there were a set of users encountering this crash when buffers were being asked to resize when clients made the zwp_text_input_v1_set_surrounding_text() with large blocks of text (typically from a ssh session). This CL enforces the requirement that the connection buffer must be greater than the net_size. For more context see below. https://gitlab.freedesktop.org/wayland/wayland/-/merge_requests/188#note_2010874 [1] https://chromium.googlesource.com/external/anongit.freedesktop.org/git/wayland/wayland/+/refs/heads/chromium_1_21_0/src/connection.c#242 [2] https://chromium.googlesource.com/external/anongit.freedesktop.org/git/wayland/wayland/+/refs/heads/chromium_1_21_0/src/connection.c#183 [3] https://chromium.googlesource.com/external/anongit.freedesktop.org/git/wayland/wayland/+/refs/heads/chromium_1_21_0/src/connection.c#249 [4] https://chromium.googlesource.com/external/anongit.freedesktop.org/git/wayland/wayland/+/refs/heads/chromium_1_21_0/src/connection.c#229 Bug: 1451333 Change-Id: I87853e3ff26c1d693f199fd0045fef796c3730c9 Upstream-Status: Pending --- src/connection.c | 2 +- tests/connection-test.c | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/connection.c b/src/connection.c index 2b3c7e4..bea4714 100644 --- a/src/connection.c +++ b/src/connection.c @@ -180,7 +180,7 @@ get_max_size_bits_for_size(size_t buffer_size) return 0; do { - if (buffer_size <= (((size_t)1) << max_size_bits)) + if (buffer_size < (((size_t)1) << max_size_bits)) break; } while (max_size_bits++ < (8 * sizeof(size_t) - 1)); diff --git a/tests/connection-test.c b/tests/connection-test.c index 0c76045..7f64443 100644 --- a/tests/connection-test.c +++ b/tests/connection-test.c @@ -684,6 +684,32 @@ TEST(connection_marshal_big_enough) free(big_string); } +/* Ensures that buffers are resiable to accomodate requests for sizes at the + * resize threshold (power of 2). Regression test for crbug.com/1451333. */ +TEST(connection_marshal_resize_at_buffer_growth_threshold) +{ + /* A string of lenth 8178 requires a buffer size of exactly 2^13. + * TODO(tluk): Determine a way to require a resize at 2^13 in a more robust + * way */ + struct marshal_data data; + const int kStrSize = 8178; + char *big_string = malloc(kStrSize); + assert(big_string); + + memset(big_string, ' ', kStrSize-1); + big_string[kStrSize-1] = '\0'; + + setup_marshal_data(&data); + + /* Set the max size to 0 (unbounded). */ + wl_connection_set_max_buffer_size(data.write_connection, 0); + + marshal_send(&data, "s", big_string); + + release_marshal_data(&data); + free(big_string); +} + static void marshal_helper(const char *format, void *handler, ...) { -- 2.17.1