GNOME Bugzilla – Bug 741685
Invalid memory read on resizing window
Last modified: 2015-07-26 04:46:38 UTC
Created attachment 292942 [details] ASAN dump and gdb backtrace An attempt to resize a window results in an invalid memory read when using the broadwell backend. See the attached backtrace. Steps to reproduce: 0. broadwayd :5 -a 0.0.0.0 1. Open a GTK3 application using broadway (in my case, wireshark-gtk, built with -fsanitize=address): ASAN_OPTIONS=abort_on_error=1 BROADWAY_DISPLAY=:5 \ gdb -q -ex r --args run/wireshark-gtk 2. Click on the border to start the resize. 3. Observe that the application crashes due to a memory access error. Package version: gtk3 3.14.6-1 (Arch Linux)
Created attachment 292945 [details] [review] broadway: prevent out-of-bounds reading Tested with a laptop (pointer) and a tablet (touch), the crash does not occur anymore and resizing works.
Reliable reproducers: npm install ws wscat --connect ws://$HOST:8085/socket or printf 'GET /socket HTTP/1.1\r\nUpgrade:websocket\r\nHost: x\r\nSec-WebSocket-Key: y\r\n\r\n' | nc $HOST 8085
Created attachment 292947 [details] [review] [PATCH] broadway: fix use-after-free on read errors Not sure whether this is the best solution, but it avoids the crash at least.
Review of attachment 292947 [details] [review]: I'm not familiar enough with the broadway code to judge quickly if the added boolean return does the right thing, but the extraneous whitespace changes in the patch obscure whats going on.
This file is inconsistent with whitespace, but since I am editing it anyway I decided to fix the context. If you apply it on a git tree, then you can use `git diff -w` to skip whitespace changes, or `git diff --color-words`
(In reply to comment #5) > If you apply it on a git tree, then you can use `git diff -w` to skip > whitespace changes, or `git diff --color-words` sure, but the review is happening here...
Basically, the function returns a boolean now and one caller checks it to determine whether 'input' is a valid pointer or not. Here is the output of `git show -w -U1` (without commit message): --- a/gdk/broadway/broadway-server.c +++ b/gdk/broadway/broadway-server.c @@ -657,3 +657,3 @@ queue_process_input_at_idle (BroadwayServer *server) -static void +static gboolean broadway_server_read_all_input_nonblocking (BroadwayInput *input) @@ -666,3 +666,3 @@ broadway_server_read_all_input_nonblocking (BroadwayInput *input) if (input == NULL) - return; + return FALSE; @@ -680,3 +680,3 @@ broadway_server_read_all_input_nonblocking (BroadwayInput *input) g_error_free (error); - return; + return TRUE; } @@ -691,3 +691,3 @@ broadway_server_read_all_input_nonblocking (BroadwayInput *input) } - return; + return FALSE; } @@ -697,2 +697,3 @@ broadway_server_read_all_input_nonblocking (BroadwayInput *input) parse_input (input); + return TRUE; } @@ -717,3 +718,4 @@ input_data_cb (GObject *stream, - broadway_server_read_all_input_nonblocking (input); + if (!broadway_server_read_all_input_nonblocking (input)) + return FALSE;
3.14.7 has just been released without the two proposed patches. Any comments?
It would be really nice to have this fixed.
I asked for a git-formatted patch without the whitespace changes to be attached here, so I can review it...
Created attachment 307999 [details] [review] [PATCH] broadway: fix use-after-free on read errors (v2 without whitespace) Same patch, without whitespace changes. Based on 3.17.5-30-g7ed5816, but it applies cleanly on 3.14.15-1-g34e6e1a too.
Review of attachment 307999 [details] [review]: Thanks, looks good to me.