From 50ea9c5b1c08bac30be365dca05716a97ea65a92 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 7 Nov 2023 22:38:52 -0500 Subject: [PATCH] connection: avoid calling memcpy on NULL, 0 Due to what is arguably a mistake in the C language specification, passing NULL to memcpy and friends is undefined behavior (UB) even when the count is 0. C additionally mistakenly leaves NULL + 0 and NULL - NULL undefined. (C++ fixes this mistake.) These are very problematic because (NULL, 0) is a natural representation of the empty slice. Some details: https://github.com/llvm/llvm-project/issues/49459 https://www.imperialviolet.org/2016/06/26/nonnull.html Unfortunately, despite how clearly this is a mistake, glibc headers and GCC now try to exploit this specification mistake and will miscompile code, so C projects need to workaround this. In particular, UBSan from Clang will flag this as a bug (although Clang itself has the good sense to never lean on this bug). We've run into a few UBSan errors in Chromium stemming from Wayland's memcpy calls. Add runtime guards as needed to avoid these cases. Note: Chromium's copy of wayland has https://gitlab.freedesktop.org/wayland/wayland/-/merge_requests/188 applied. It is possible the ring_buffer_copy UB cases are only reachable with that MR applied, I'm not sure. But it seemed simplest to just add the fix to wayland as-is. Then when/if that MR lands, it will pick this up. Signed-off-by: David Benjamin --- src/connection.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/connection.c b/src/connection.c index 8a2d9e95..f2e5837f 100644 --- a/src/connection.c +++ b/src/connection.c @@ -83,6 +83,9 @@ ring_buffer_put(struct wl_ring_buffer *b, const void *data, size_t count) return -1; } + if (count == 0) + return 0; + head = MASK(b->head); if (head + count <= sizeof b->data) { memcpy(b->data + head, data, count); @@ -150,6 +153,9 @@ ring_buffer_copy(struct wl_ring_buffer *b, void *data, size_t count) { uint32_t tail, size; + if (count == 0) + return; + tail = MASK(b->tail); if (tail + count <= sizeof b->data) { memcpy(data, b->data + tail, count); @@ -1186,7 +1192,8 @@ serialize_closure(struct wl_closure *closure, uint32_t *buffer, if (p + div_roundup(size, sizeof *p) > end) goto overflow; - memcpy(p, closure->args[i].a->data, size); + if (size != 0) + memcpy(p, closure->args[i].a->data, size); p += div_roundup(size, sizeof *p); break; default: