122 lines
4.4 KiB
Diff
122 lines
4.4 KiB
Diff
From 3806c948b5e3af1e13383ce3b24b0cebfde0a721 Mon Sep 17 00:00:00 2001
|
|
From: Thomas Lukaszewicz <tluk@chromium.org>
|
|
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
|
|
|