From ce5c57ca770057bbe04a00be8e471858117e4933 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 10 Aug 2024 18:03:45 -0400 Subject: [PATCH 1/4] connection: Use a #define for maximum buffer size power of 2 This avoids open-coding it. No functional change intended. Signed-off-by: Demi Marie Obenour --- src/connection.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/connection.c b/src/connection.c index 34495211..354d8b3c 100644 --- a/src/connection.c +++ b/src/connection.c @@ -70,12 +70,13 @@ struct wl_connection { int fd; int want_flush; }; +#define WL_BUFFER_MAX_SIZE_POT ((size_t)(8 * sizeof(size_t) - 1)) static inline size_t size_pot(uint32_t size_bits) { - if (!(size_bits < 8 * sizeof(size_t))) - wl_abort("Too many bits for size_t\n"); + if (size_bits > WL_BUFFER_MAX_SIZE_POT) + wl_abort("Too many bits for ring buffer\n"); return ((size_t)1) << size_bits; } From 507884a123cb3ab4a0005c3ce6419531266c30d8 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 5 Aug 2024 13:29:40 -0400 Subject: [PATCH 2/4] connection: Convert 0 to an actual buffer size limit sooner This avoids special cases in get_max_size_bits_for_size() and ring_buffer_get_bits_for_size(). Signed-off-by: Demi Marie Obenour --- src/connection.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/connection.c b/src/connection.c index 354d8b3c..a62da785 100644 --- a/src/connection.c +++ b/src/connection.c @@ -58,7 +58,7 @@ struct wl_ring_buffer { char *data; size_t head, tail; uint32_t size_bits; - uint32_t max_size_bits; /* 0 for unlimited */ + uint32_t max_size_bits; }; #define MAX_FDS_OUT 28 @@ -197,9 +197,6 @@ get_max_size_bits_for_size(size_t buffer_size) { uint32_t max_size_bits = WL_BUFFER_DEFAULT_SIZE_POT; - /* buffer_size == 0 means unbound buffer size */ - if (buffer_size == 0) - return 0; while (max_size_bits < 8 * sizeof(size_t) && size_pot(max_size_bits) < buffer_size) max_size_bits++; @@ -231,10 +228,7 @@ ring_buffer_get_bits_for_size(struct wl_ring_buffer *b, size_t net_size) { size_t max_size_bits = get_max_size_bits_for_size(net_size); - if (max_size_bits < WL_BUFFER_DEFAULT_SIZE_POT) - max_size_bits = WL_BUFFER_DEFAULT_SIZE_POT; - - if (b->max_size_bits > 0 && max_size_bits > b->max_size_bits) + if (max_size_bits > b->max_size_bits) max_size_bits = b->max_size_bits; return max_size_bits; @@ -300,7 +294,12 @@ wl_connection_set_max_buffer_size(struct wl_connection *connection, { uint32_t max_size_bits; - max_size_bits = get_max_size_bits_for_size(max_buffer_size); + /* Max buffer size 0 means allow buffers limited only by integer + * sizes or available memory, whichever runs out first. + */ + max_size_bits = max_buffer_size != 0 ? + get_max_size_bits_for_size(max_buffer_size) : + WL_BUFFER_MAX_SIZE_POT; connection->fds_in.max_size_bits = max_size_bits; ring_buffer_ensure_space(&connection->fds_in, 0); From f8ecb6d5319a4b909f5f06b38dd3c525bbde9ba0 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 10 Aug 2024 18:05:59 -0400 Subject: [PATCH 3/4] connection: Limit buffer size bits to WL_BUFFER_DEFAULT_SIZE_POT The previous code tried to do this but had an off-by-1 error. Signed-off-by: Demi Marie Obenour --- src/connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/connection.c b/src/connection.c index a62da785..a2232799 100644 --- a/src/connection.c +++ b/src/connection.c @@ -198,7 +198,7 @@ get_max_size_bits_for_size(size_t buffer_size) uint32_t max_size_bits = WL_BUFFER_DEFAULT_SIZE_POT; - while (max_size_bits < 8 * sizeof(size_t) && size_pot(max_size_bits) < buffer_size) + while (max_size_bits < WL_BUFFER_MAX_SIZE_POT && size_pot(max_size_bits) < buffer_size) max_size_bits++; return max_size_bits; From 2ee9ef098b9b84f914ae503ded2bc85e97746bc9 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 5 Aug 2024 12:17:02 -0400 Subject: [PATCH 4/4] connection: Ensure buffer sizes do not exceed INT_MAX or PTRDIFF_MAX Pointer arithmetic beyond PTRDIFF_MAX is broken, so buffer sizes exceeding PTRDIFF_MAX (which is half of the address space!) are a bad idea. Furthermore, the code uses int for sizes in various places, so buffer sizes exceeding INT_MAX are also a bad idea. Therefore, limit buffer sizes to (PTRDIFF_MAX / 2) + 1 or (INT_MAX / 2) + 1, whichever is smaller. Tests would require 2GiB of RAM and so have been omitted. The test would check for wl_connection_flush() flushing more than INT_MAX bytes in one call, causing it to return a negative number and causing its caller to wrongly believe an error occurred. Signed-off-by: Demi Marie Obenour --- src/connection.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/connection.c b/src/connection.c index a2232799..41d821eb 100644 --- a/src/connection.c +++ b/src/connection.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -70,7 +71,19 @@ struct wl_connection { int fd; int want_flush; }; -#define WL_BUFFER_MAX_SIZE_POT ((size_t)(8 * sizeof(size_t) - 1)) + +/* Pointer arithmetic beyond PTRDIFF_MAX is broken, so don't rely on it. + * ((ptrdiff_t)1 << (CHAR_BIT * sizeof(ptrdiff_t) - 1)) is undefined behavior + * (wrapping to PTRDIFF_MIN with -fwrapv), not PTRDIFF_MAX, so limit buffer size + * to ((size_t)1 << (CHAR_BIT * sizeof(ptrdiff_t) - 2)). int is used for + * sizes in various places, so for safety also use + * ((size_t)1 << (CHAR_BIT * sizeof(int) - 2)) as a limit. + */ +#if PTRDIFF_MAX < INT_MAX +# define WL_BUFFER_MAX_SIZE_POT ((size_t)(CHAR_BIT * sizeof(ptrdiff_t) - 2)) +#else +# define WL_BUFFER_MAX_SIZE_POT ((size_t)(CHAR_BIT * sizeof(int) - 2)) +#endif static inline size_t size_pot(uint32_t size_bits)