From ea4116f5bd111a0a86af2084e50e7d1211fa8e5c Mon Sep 17 00:00:00 2001 From: Matthew Jones Date: Wed, 2 Jun 2021 15:06:59 -0600 Subject: [PATCH] LibGUI+LibGfx+WindowServer: Sanity check window size dimensions Previous to this commit, if a `Window` wanted to set its width or height greater than `INT16_MAX` (32768), both the application owning the Window and the WindowServer would crash. The root of this issue is that `size_would_overflow` check in `Bitmap` has checks for `INT16_MAX`, and `Window.cpp:786` that is called by `Gfx::Bitmap::create_with_anonymous_buffer` would get null back, then causing a chain of events resulting in crashes. Crashes can still occur but with `VERIFY` and `did_misbehave` the causes of the crash can be more readily identified. --- Userland/Libraries/LibGUI/Window.cpp | 5 ++++- Userland/Libraries/LibGfx/Bitmap.cpp | 2 +- Userland/Services/WindowServer/ClientConnection.cpp | 4 ++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/Userland/Libraries/LibGUI/Window.cpp b/Userland/Libraries/LibGUI/Window.cpp index 648371985dd..07a3de700e6 100644 --- a/Userland/Libraries/LibGUI/Window.cpp +++ b/Userland/Libraries/LibGUI/Window.cpp @@ -788,8 +788,11 @@ OwnPtr Window::create_backing_store(const Gfx::IntSize& size // FIXME: Plumb scale factor here eventually. auto bitmap = Gfx::Bitmap::create_with_anonymous_buffer(format, buffer, size, 1, {}); - if (!bitmap) + if (!bitmap) { + VERIFY(size.width() <= INT16_MAX); + VERIFY(size.height() <= INT16_MAX); return {}; + } return make(bitmap.release_nonnull()); } diff --git a/Userland/Libraries/LibGfx/Bitmap.cpp b/Userland/Libraries/LibGfx/Bitmap.cpp index 35011614a29..67d1ed8704c 100644 --- a/Userland/Libraries/LibGfx/Bitmap.cpp +++ b/Userland/Libraries/LibGfx/Bitmap.cpp @@ -58,7 +58,7 @@ static bool size_would_overflow(BitmapFormat format, const IntSize& size, int sc if (size.width() < 0 || size.height() < 0) return true; // This check is a bit arbitrary, but should protect us from most shenanigans: - if (size.width() >= 32768 || size.height() >= 32768 || scale_factor < 1 || scale_factor > 4) + if (size.width() >= INT16_MAX || size.height() >= INT16_MAX || scale_factor < 1 || scale_factor > 4) return true; // In contrast, this check is absolutely necessary: size_t pitch = Bitmap::minimum_pitch(size.width() * scale_factor, format); diff --git a/Userland/Services/WindowServer/ClientConnection.cpp b/Userland/Services/WindowServer/ClientConnection.cpp index f9bb1484632..a6302309064 100644 --- a/Userland/Services/WindowServer/ClientConnection.cpp +++ b/Userland/Services/WindowServer/ClientConnection.cpp @@ -363,6 +363,10 @@ Messages::WindowServer::SetWindowRectResponse ClientConnection::set_window_rect( dbgln("ClientConnection: Ignoring SetWindowRect request for fullscreen window"); return nullptr; } + if (rect.width() > INT16_MAX || rect.height() > INT16_MAX) { + did_misbehave(String::formatted("SetWindowRect: Bad window sizing(width={}, height={}), dimension exceeds INT16_MAX", rect.width(), rect.height()).characters()); + return nullptr; + } if (rect.location() != window.rect().location()) { window.set_default_positioned(false);