* [PATCH] gatmux: don't free cmux data until watchers are destroyed
@ 2019-09-30 17:35 Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-09-30 17:38 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-09-30 20:59 ` Denis Kenzior
0 siblings, 2 replies; 9+ messages in thread
From: Martin =?unknown-8bit?q?Hundeb=C3=B8ll?= @ 2019-09-30 17:35 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 8335 bytes --]
When closing down a cmux object, the address sanitizer detects a
use-after-free in gatmux.c (see below).
Avoid this by taking a reference to the mux object during the processing
in received_data().
ofonod[3640549]: ../git/plugins/quectel.c:cfun_disable() 0x610000000b40
ofonod[3640549]: ../git/plugins/quectel.c:close_serial() 0x610000000b40
ofonod[3640549]: ../git/plugins/quectel.c:close_mux() 0x610000000b40
ofonod[3640549]: ../git/examples/emulator.c:powered_watch() Removing modem 0x610000000b40 from the list
ofonod[3640549]: ../git/examples/emulator.c:powered_watch() Removing server watch: 106
ofonod[3640549]: ../git/src/modem.c:modem_change_state() old state: 0, new state: 0
=================================================================
==3640549==ERROR: AddressSanitizer: heap-use-after-free on address 0x62100073dd28 at pc 0x5566b6402a21 bp 0x7ffe7a2db0e0 sp 0x7ffe7a2db0d0
READ of size 8 at 0x62100073dd28 thread T0
#0 0x5566b6402a20 in debug ../git/gatchat/gatmux.c:109
#1 0x5566b6404bd7 in channel_close ../git/gatchat/gatmux.c:525
#2 0x7fa0516e44a6 in g_io_channel_shutdown (/usr/lib/libglib-2.0.so.0+0x774a6)
#3 0x7fa0516e4644 in g_io_channel_unref (/usr/lib/libglib-2.0.so.0+0x77644)
#4 0x5566b64048a4 in watch_finalize ../git/gatchat/gatmux.c:474
#5 0x7fa0516d6f6f (/usr/lib/libglib-2.0.so.0+0x69f6f)
#6 0x7fa0516ac6a7 in g_slist_foreach (/usr/lib/libglib-2.0.so.0+0x3f6a7)
#7 0x7fa0516b277b in g_slist_free_full (/usr/lib/libglib-2.0.so.0+0x4577b)
#8 0x5566b6403413 in dispatch_sources ../git/gatchat/gatmux.c:224
#9 0x5566b64039ea in received_data ../git/gatchat/gatmux.c:268
#10 0x7fa0516d727e in g_main_context_dispatch (/usr/lib/libglib-2.0.so.0+0x6a27e)
#11 0x7fa0516d91c0 (/usr/lib/libglib-2.0.so.0+0x6c1c0)
#12 0x7fa0516da0d2 in g_main_loop_run (/usr/lib/libglib-2.0.so.0+0x6d0d2)
#13 0x5566b6429b1b in main ../git/src/main.c:286
#14 0x7fa05147fee2 in __libc_start_main (/usr/lib/libc.so.6+0x26ee2)
#15 0x5566b62531ad in _start (/home/martin/projects/ofono/x86/src/ofonod+0xfc1ad)
0x62100073dd28 is located 40 bytes inside of 4672-byte region [0x62100073dd00,0x62100073ef40)
freed by thread T0 here:
#0 0x7fa0519256c0 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:122
#1 0x5566b64052d7 in g_at_mux_unref ../git/gatchat/gatmux.c:645
#2 0x5566b63d6d19 in close_mux ../git/plugins/quectel.c:199
#3 0x5566b63d7047 in close_serial ../git/plugins/quectel.c:223
#4 0x5566b63db62a in cfun_disable ../git/plugins/quectel.c:1056
#5 0x5566b63f6ae1 in at_chat_finish_command ../git/gatchat/gatchat.c:459
#6 0x5566b63f701b in at_chat_handle_command_response ../git/gatchat/gatchat.c:521
#7 0x5566b63f785b in have_line ../git/gatchat/gatchat.c:600
#8 0x5566b63f87f1 in new_bytes ../git/gatchat/gatchat.c:759
#9 0x5566b640174c in received_data ../git/gatchat/gatio.c:122
#10 0x5566b64047b4 in watch_dispatch ../git/gatchat/gatmux.c:464
#11 0x5566b640313b in dispatch_sources ../git/gatchat/gatmux.c:183
#12 0x5566b64039ea in received_data ../git/gatchat/gatmux.c:268
#13 0x7fa0516d727e in g_main_context_dispatch (/usr/lib/libglib-2.0.so.0+0x6a27e)
previously allocated by thread T0 here:
#0 0x7fa051925ce8 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:153
#1 0x5566b6405009 in g_at_mux_new ../git/gatchat/gatmux.c:606
#2 0x5566b6407f6b in g_at_mux_new_gsm0710_basic ../git/gatchat/gatmux.c:1165
#3 0x5566b63da9ba in cmux_cb ../git/plugins/quectel.c:882
#4 0x5566b63f6ae1 in at_chat_finish_command ../git/gatchat/gatchat.c:459
#5 0x5566b63f701b in at_chat_handle_command_response ../git/gatchat/gatchat.c:521
#6 0x5566b63f785b in have_line ../git/gatchat/gatchat.c:600
#7 0x5566b63f87f1 in new_bytes ../git/gatchat/gatchat.c:759
#8 0x5566b640174c in received_data ../git/gatchat/gatio.c:122
#9 0x7fa0516d727e in g_main_context_dispatch (/usr/lib/libglib-2.0.so.0+0x6a27e)
SUMMARY: AddressSanitizer: heap-use-after-free ../git/gatchat/gatmux.c:109 in debug
Shadow bytes around the buggy address:
0x0c42800dfb50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c42800dfb60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c42800dfb70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c42800dfb80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c42800dfb90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c42800dfba0: fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd
0x0c42800dfbb0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c42800dfbc0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c42800dfbd0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c42800dfbe0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c42800dfbf0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==3640549==ABORTING
---
gatchat/gatmux.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/gatchat/gatmux.c b/gatchat/gatmux.c
index 757a5123..eabaa4db 100644
--- a/gatchat/gatmux.c
+++ b/gatchat/gatmux.c
@@ -227,12 +227,15 @@ static void dispatch_sources(GAtMuxChannel *channel, GIOCondition condition)
static gboolean received_data(GIOChannel *channel, GIOCondition cond,
gpointer data)
{
- GAtMux *mux = data;
+ GAtMux *mux = g_at_mux_ref(data);
int i;
GIOStatus status;
gsize bytes_read;
if (cond & G_IO_NVAL)
+ goto out_done;
+
+ if (mux->shutdown)
return FALSE;
debug(mux, "received data");
@@ -270,15 +273,22 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
}
if (cond & (G_IO_HUP | G_IO_ERR))
- return FALSE;
+ goto out_done;
if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN)
- return FALSE;
+ goto out_done;
if (mux->buf_used == sizeof(mux->buf))
- return FALSE;
+ goto out_done;
+
+ g_at_mux_unref(mux);
return TRUE;
+
+out_done:
+ g_at_mux_unref(mux);
+
+ return FALSE;
}
static void write_watcher_destroy_notify(gpointer user_data)
@@ -286,6 +296,9 @@ static void write_watcher_destroy_notify(gpointer user_data)
GAtMux *mux = user_data;
mux->write_watch = 0;
+
+ if (mux->read_watch <= 0 && mux->write_watch <= 0)
+ g_free(mux);
}
static gboolean can_write_data(GIOChannel *chan, GIOCondition cond,
@@ -297,6 +310,9 @@ static gboolean can_write_data(GIOChannel *chan, GIOCondition cond,
if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR))
return FALSE;
+ if (mux->shutdown)
+ return FALSE;
+
debug(mux, "can write data");
for (dlc = 0; dlc < MAX_CHANNELS; dlc += 1) {
@@ -607,6 +623,7 @@ GAtMux *g_at_mux_new(GIOChannel *channel, const GAtMuxDriver *driver)
if (mux == NULL)
return NULL;
+
mux->ref_count = 1;
mux->driver = driver;
mux->shutdown = TRUE;
@@ -642,7 +659,8 @@ void g_at_mux_unref(GAtMux *mux)
if (mux->driver->remove)
mux->driver->remove(mux);
- g_free(mux);
+ if (mux->read_watch <= 0 && mux->write_watch <= 0)
+ g_free(mux);
}
}
@@ -651,6 +669,9 @@ static void read_watcher_destroy_notify(gpointer user_data)
GAtMux *mux = user_data;
mux->read_watch = 0;
+
+ if (mux->read_watch <= 0 && mux->write_watch <= 0)
+ g_free(mux);
}
gboolean g_at_mux_start(GAtMux *mux)
@@ -695,6 +716,7 @@ gboolean g_at_mux_shutdown(GAtMux *mux)
continue;
channel_close((GIOChannel *) mux->dlcs[i], NULL);
+ mux->dlcs[i] = NULL;
}
if (mux->driver->shutdown)
--
2.23.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] gatmux: don't free cmux data until watchers are destroyed
2019-09-30 17:35 [PATCH] gatmux: don't free cmux data until watchers are destroyed Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
@ 2019-09-30 17:38 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-09-30 20:59 ` Denis Kenzior
1 sibling, 0 replies; 9+ messages in thread
From: Martin =?unknown-8bit?q?Hundeb=C3=B8ll?= @ 2019-09-30 17:38 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 9059 bytes --]
Uh,
Forgot to change the subject.
Feel free to use:
gatmux: take reference to mux object in received_data()
// Martin
On 30/09/2019 19.35, Martin Hundebøll wrote:
> When closing down a cmux object, the address sanitizer detects a
> use-after-free in gatmux.c (see below).
>
> Avoid this by taking a reference to the mux object during the processing
> in received_data().
>
> ofonod[3640549]: ../git/plugins/quectel.c:cfun_disable() 0x610000000b40
> ofonod[3640549]: ../git/plugins/quectel.c:close_serial() 0x610000000b40
> ofonod[3640549]: ../git/plugins/quectel.c:close_mux() 0x610000000b40
> ofonod[3640549]: ../git/examples/emulator.c:powered_watch() Removing modem 0x610000000b40 from the list
> ofonod[3640549]: ../git/examples/emulator.c:powered_watch() Removing server watch: 106
> ofonod[3640549]: ../git/src/modem.c:modem_change_state() old state: 0, new state: 0
>
> =================================================================
> ==3640549==ERROR: AddressSanitizer: heap-use-after-free on address 0x62100073dd28 at pc 0x5566b6402a21 bp 0x7ffe7a2db0e0 sp 0x7ffe7a2db0d0
> READ of size 8 at 0x62100073dd28 thread T0
> #0 0x5566b6402a20 in debug ../git/gatchat/gatmux.c:109
> #1 0x5566b6404bd7 in channel_close ../git/gatchat/gatmux.c:525
> #2 0x7fa0516e44a6 in g_io_channel_shutdown (/usr/lib/libglib-2.0.so.0+0x774a6)
> #3 0x7fa0516e4644 in g_io_channel_unref (/usr/lib/libglib-2.0.so.0+0x77644)
> #4 0x5566b64048a4 in watch_finalize ../git/gatchat/gatmux.c:474
> #5 0x7fa0516d6f6f (/usr/lib/libglib-2.0.so.0+0x69f6f)
> #6 0x7fa0516ac6a7 in g_slist_foreach (/usr/lib/libglib-2.0.so.0+0x3f6a7)
> #7 0x7fa0516b277b in g_slist_free_full (/usr/lib/libglib-2.0.so.0+0x4577b)
> #8 0x5566b6403413 in dispatch_sources ../git/gatchat/gatmux.c:224
> #9 0x5566b64039ea in received_data ../git/gatchat/gatmux.c:268
> #10 0x7fa0516d727e in g_main_context_dispatch (/usr/lib/libglib-2.0.so.0+0x6a27e)
> #11 0x7fa0516d91c0 (/usr/lib/libglib-2.0.so.0+0x6c1c0)
> #12 0x7fa0516da0d2 in g_main_loop_run (/usr/lib/libglib-2.0.so.0+0x6d0d2)
> #13 0x5566b6429b1b in main ../git/src/main.c:286
> #14 0x7fa05147fee2 in __libc_start_main (/usr/lib/libc.so.6+0x26ee2)
> #15 0x5566b62531ad in _start (/home/martin/projects/ofono/x86/src/ofonod+0xfc1ad)
>
> 0x62100073dd28 is located 40 bytes inside of 4672-byte region [0x62100073dd00,0x62100073ef40)
> freed by thread T0 here:
> #0 0x7fa0519256c0 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:122
> #1 0x5566b64052d7 in g_at_mux_unref ../git/gatchat/gatmux.c:645
> #2 0x5566b63d6d19 in close_mux ../git/plugins/quectel.c:199
> #3 0x5566b63d7047 in close_serial ../git/plugins/quectel.c:223
> #4 0x5566b63db62a in cfun_disable ../git/plugins/quectel.c:1056
> #5 0x5566b63f6ae1 in at_chat_finish_command ../git/gatchat/gatchat.c:459
> #6 0x5566b63f701b in at_chat_handle_command_response ../git/gatchat/gatchat.c:521
> #7 0x5566b63f785b in have_line ../git/gatchat/gatchat.c:600
> #8 0x5566b63f87f1 in new_bytes ../git/gatchat/gatchat.c:759
> #9 0x5566b640174c in received_data ../git/gatchat/gatio.c:122
> #10 0x5566b64047b4 in watch_dispatch ../git/gatchat/gatmux.c:464
> #11 0x5566b640313b in dispatch_sources ../git/gatchat/gatmux.c:183
> #12 0x5566b64039ea in received_data ../git/gatchat/gatmux.c:268
> #13 0x7fa0516d727e in g_main_context_dispatch (/usr/lib/libglib-2.0.so.0+0x6a27e)
>
> previously allocated by thread T0 here:
> #0 0x7fa051925ce8 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:153
> #1 0x5566b6405009 in g_at_mux_new ../git/gatchat/gatmux.c:606
> #2 0x5566b6407f6b in g_at_mux_new_gsm0710_basic ../git/gatchat/gatmux.c:1165
> #3 0x5566b63da9ba in cmux_cb ../git/plugins/quectel.c:882
> #4 0x5566b63f6ae1 in at_chat_finish_command ../git/gatchat/gatchat.c:459
> #5 0x5566b63f701b in at_chat_handle_command_response ../git/gatchat/gatchat.c:521
> #6 0x5566b63f785b in have_line ../git/gatchat/gatchat.c:600
> #7 0x5566b63f87f1 in new_bytes ../git/gatchat/gatchat.c:759
> #8 0x5566b640174c in received_data ../git/gatchat/gatio.c:122
> #9 0x7fa0516d727e in g_main_context_dispatch (/usr/lib/libglib-2.0.so.0+0x6a27e)
>
> SUMMARY: AddressSanitizer: heap-use-after-free ../git/gatchat/gatmux.c:109 in debug
> Shadow bytes around the buggy address:
> 0x0c42800dfb50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c42800dfb60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c42800dfb70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c42800dfb80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c42800dfb90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> =>0x0c42800dfba0: fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd
> 0x0c42800dfbb0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c42800dfbc0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c42800dfbd0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c42800dfbe0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c42800dfbf0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> Shadow byte legend (one shadow byte represents 8 application bytes):
> Addressable: 00
> Partially addressable: 01 02 03 04 05 06 07
> Heap left redzone: fa
> Freed heap region: fd
> Stack left redzone: f1
> Stack mid redzone: f2
> Stack right redzone: f3
> Stack after return: f5
> Stack use after scope: f8
> Global redzone: f9
> Global init order: f6
> Poisoned by user: f7
> Container overflow: fc
> Array cookie: ac
> Intra object redzone: bb
> ASan internal: fe
> Left alloca redzone: ca
> Right alloca redzone: cb
> Shadow gap: cc
> ==3640549==ABORTING
> ---
> gatchat/gatmux.c | 32 +++++++++++++++++++++++++++-----
> 1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/gatchat/gatmux.c b/gatchat/gatmux.c
> index 757a5123..eabaa4db 100644
> --- a/gatchat/gatmux.c
> +++ b/gatchat/gatmux.c
> @@ -227,12 +227,15 @@ static void dispatch_sources(GAtMuxChannel *channel, GIOCondition condition)
> static gboolean received_data(GIOChannel *channel, GIOCondition cond,
> gpointer data)
> {
> - GAtMux *mux = data;
> + GAtMux *mux = g_at_mux_ref(data);
> int i;
> GIOStatus status;
> gsize bytes_read;
>
> if (cond & G_IO_NVAL)
> + goto out_done;
> +
> + if (mux->shutdown)
> return FALSE;
>
> debug(mux, "received data");
> @@ -270,15 +273,22 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
> }
>
> if (cond & (G_IO_HUP | G_IO_ERR))
> - return FALSE;
> + goto out_done;
>
> if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN)
> - return FALSE;
> + goto out_done;
>
> if (mux->buf_used == sizeof(mux->buf))
> - return FALSE;
> + goto out_done;
> +
> + g_at_mux_unref(mux);
>
> return TRUE;
> +
> +out_done:
> + g_at_mux_unref(mux);
> +
> + return FALSE;
> }
>
> static void write_watcher_destroy_notify(gpointer user_data)
> @@ -286,6 +296,9 @@ static void write_watcher_destroy_notify(gpointer user_data)
> GAtMux *mux = user_data;
>
> mux->write_watch = 0;
> +
> + if (mux->read_watch <= 0 && mux->write_watch <= 0)
> + g_free(mux);
> }
>
> static gboolean can_write_data(GIOChannel *chan, GIOCondition cond,
> @@ -297,6 +310,9 @@ static gboolean can_write_data(GIOChannel *chan, GIOCondition cond,
> if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR))
> return FALSE;
>
> + if (mux->shutdown)
> + return FALSE;
> +
> debug(mux, "can write data");
>
> for (dlc = 0; dlc < MAX_CHANNELS; dlc += 1) {
> @@ -607,6 +623,7 @@ GAtMux *g_at_mux_new(GIOChannel *channel, const GAtMuxDriver *driver)
> if (mux == NULL)
> return NULL;
>
> +
> mux->ref_count = 1;
> mux->driver = driver;
> mux->shutdown = TRUE;
> @@ -642,7 +659,8 @@ void g_at_mux_unref(GAtMux *mux)
> if (mux->driver->remove)
> mux->driver->remove(mux);
>
> - g_free(mux);
> + if (mux->read_watch <= 0 && mux->write_watch <= 0)
> + g_free(mux);
> }
> }
>
> @@ -651,6 +669,9 @@ static void read_watcher_destroy_notify(gpointer user_data)
> GAtMux *mux = user_data;
>
> mux->read_watch = 0;
> +
> + if (mux->read_watch <= 0 && mux->write_watch <= 0)
> + g_free(mux);
> }
>
> gboolean g_at_mux_start(GAtMux *mux)
> @@ -695,6 +716,7 @@ gboolean g_at_mux_shutdown(GAtMux *mux)
> continue;
>
> channel_close((GIOChannel *) mux->dlcs[i], NULL);
> + mux->dlcs[i] = NULL;
> }
>
> if (mux->driver->shutdown)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gatmux: don't free cmux data until watchers are destroyed
2019-09-30 17:35 [PATCH] gatmux: don't free cmux data until watchers are destroyed Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-09-30 17:38 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
@ 2019-09-30 20:59 ` Denis Kenzior
2019-10-07 18:37 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-10-07 21:33 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
1 sibling, 2 replies; 9+ messages in thread
From: Denis Kenzior @ 2019-09-30 20:59 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 3520 bytes --]
Hi Martin,
> @@ -227,12 +227,15 @@ static void dispatch_sources(GAtMuxChannel *channel, GIOCondition condition)
> static gboolean received_data(GIOChannel *channel, GIOCondition cond,
> gpointer data)
> {
> - GAtMux *mux = data;
> + GAtMux *mux = g_at_mux_ref(data);
So you're taking a ref here unconditionally, yet...
> int i;
> GIOStatus status;
> gsize bytes_read;
>
> if (cond & G_IO_NVAL)
> + goto out_done;
> +
> + if (mux->shutdown)
> return FALSE;
You don't return it here?
>
> debug(mux, "received data");
> @@ -270,15 +273,22 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
> }
>
> if (cond & (G_IO_HUP | G_IO_ERR))
> - return FALSE;
> + goto out_done;
>
> if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN)
> - return FALSE;
> + goto out_done;
>
> if (mux->buf_used == sizeof(mux->buf))
> - return FALSE;
> + goto out_done;
> +
> + g_at_mux_unref(mux);
>
Why not do something like:
g_at_mux_ref(mux);
for (i = 1; i <= MAX_CHANNELS && !mux->shutdown; i++) {
int offset = i / 8;
int bit = i % 8;
if (!(mux->newdata[offset] & (1 << bit)))
continue;
dispatch_sources(mux->dlcs[i-1], G_IO_IN);
}
g_at_mux_unref(mux);
> return TRUE;
> +
> +out_done:
> + g_at_mux_unref(mux);
> +
> + return FALSE;
> }
>
> static void write_watcher_destroy_notify(gpointer user_data)
> @@ -286,6 +296,9 @@ static void write_watcher_destroy_notify(gpointer user_data)
> GAtMux *mux = user_data;
>
> mux->write_watch = 0;
> +
> + if (mux->read_watch <= 0 && mux->write_watch <= 0)
> + g_free(mux);
> }
>
> static gboolean can_write_data(GIOChannel *chan, GIOCondition cond,
> @@ -297,6 +310,9 @@ static gboolean can_write_data(GIOChannel *chan, GIOCondition cond,
> if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR))
> return FALSE;
>
> + if (mux->shutdown)
> + return FALSE;
> +
> debug(mux, "can write data");
>
> for (dlc = 0; dlc < MAX_CHANNELS; dlc += 1) {
> @@ -607,6 +623,7 @@ GAtMux *g_at_mux_new(GIOChannel *channel, const GAtMuxDriver *driver)
> if (mux == NULL)
> return NULL;
>
> +
> mux->ref_count = 1;
> mux->driver = driver;
> mux->shutdown = TRUE;
> @@ -642,7 +659,8 @@ void g_at_mux_unref(GAtMux *mux)
> if (mux->driver->remove)
> mux->driver->remove(mux);
>
> - g_free(mux);
> + if (mux->read_watch <= 0 && mux->write_watch <= 0)
> + g_free(mux);
This looks dangerous to me. You're going to cause a crash like this.
You really want to call the disconnect function instead. But I suspect
this is a different patch entirely.
> }
> }
>
> @@ -651,6 +669,9 @@ static void read_watcher_destroy_notify(gpointer user_data)
> GAtMux *mux = user_data;
>
> mux->read_watch = 0;
> +
> + if (mux->read_watch <= 0 && mux->write_watch <= 0)
> + g_free(mux);
Same here
> }
>
> gboolean g_at_mux_start(GAtMux *mux)
> @@ -695,6 +716,7 @@ gboolean g_at_mux_shutdown(GAtMux *mux)
> continue;
>
> channel_close((GIOChannel *) mux->dlcs[i], NULL);
> + mux->dlcs[i] = NULL;
This can probably be a separate cleanup patch.
> }
>
> if (mux->driver->shutdown)
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gatmux: don't free cmux data until watchers are destroyed
2019-09-30 20:59 ` Denis Kenzior
@ 2019-10-07 18:37 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-10-07 18:57 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-10-08 4:06 ` Denis Kenzior
2019-10-07 21:33 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
1 sibling, 2 replies; 9+ messages in thread
From: Martin =?unknown-8bit?q?Hundeb=C3=B8ll?= @ 2019-10-07 18:37 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 4047 bytes --]
Hi Denis,
On 30/09/2019 22.59, Denis Kenzior wrote:
>> @@ -227,12 +227,15 @@ static void dispatch_sources(GAtMuxChannel
>> *channel, GIOCondition condition)
>> static gboolean received_data(GIOChannel *channel, GIOCondition cond,
>> gpointer data)
>> {
>> - GAtMux *mux = data;
>> + GAtMux *mux = g_at_mux_ref(data);
>
> So you're taking a ref here unconditionally, yet...
>
>> int i;
>> GIOStatus status;
>> gsize bytes_read;
>> if (cond & G_IO_NVAL)
>> + goto out_done;
>> +
>> + if (mux->shutdown)
>> return FALSE;
>
> You don't return it here?
>
>> debug(mux, "received data");
>> @@ -270,15 +273,22 @@ static gboolean received_data(GIOChannel
>> *channel, GIOCondition cond,
>> }
>> if (cond & (G_IO_HUP | G_IO_ERR))
>> - return FALSE;
>> + goto out_done;
>> if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN)
>> - return FALSE;
>> + goto out_done;
>> if (mux->buf_used == sizeof(mux->buf))
>> - return FALSE;
>> + goto out_done;
>> +
>> + g_at_mux_unref(mux);
>
> Why not do something like:
>
> g_at_mux_ref(mux);
>
> for (i = 1; i <= MAX_CHANNELS && !mux->shutdown; i++) {
> int offset = i / 8;
> int bit = i % 8;
>
> if (!(mux->newdata[offset] & (1 << bit)))
> continue;
>
> dispatch_sources(mux->dlcs[i-1], G_IO_IN);
> }
>
> g_at_mux_unref(mux);
>
I'm afraid this isn't enough, as I still get use-after-free when using
gatmux in the quectel plugin (see attached log).
I've added g_at_mux_{ref,unref}() calls to the read and write watchers
too. This fixes it. Patch coming up.
>> return TRUE;
>> +
>> +out_done:
>> + g_at_mux_unref(mux);
>> +
>> + return FALSE;
>> }
>> static void write_watcher_destroy_notify(gpointer user_data)
>> @@ -286,6 +296,9 @@ static void write_watcher_destroy_notify(gpointer
>> user_data)
>> GAtMux *mux = user_data;
>> mux->write_watch = 0;
>> +
>> + if (mux->read_watch <= 0 && mux->write_watch <= 0)
>> + g_free(mux);
>> }
>> static gboolean can_write_data(GIOChannel *chan, GIOCondition cond,
>> @@ -297,6 +310,9 @@ static gboolean can_write_data(GIOChannel *chan,
>> GIOCondition cond,
>> if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR))
>> return FALSE;
>> + if (mux->shutdown)
>> + return FALSE;
>> +
>> debug(mux, "can write data");
>> for (dlc = 0; dlc < MAX_CHANNELS; dlc += 1) {
>> @@ -607,6 +623,7 @@ GAtMux *g_at_mux_new(GIOChannel *channel, const
>> GAtMuxDriver *driver)
>> if (mux == NULL)
>> return NULL;
>> +
>> mux->ref_count = 1;
>> mux->driver = driver;
>> mux->shutdown = TRUE;
>> @@ -642,7 +659,8 @@ void g_at_mux_unref(GAtMux *mux)
>> if (mux->driver->remove)
>> mux->driver->remove(mux);
>> - g_free(mux);
>> + if (mux->read_watch <= 0 && mux->write_watch <= 0)
>> + g_free(mux);
>
> This looks dangerous to me. You're going to cause a crash like this.
> You really want to call the disconnect function instead. But I suspect
> this is a different patch entirely.
Yeah, I was obviously too tired when doing these patches last week. More
coffee next time.
// Martin
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gatmux.log --]
[-- Type: text/x-log, Size: 3838 bytes --]
ofonod[5960]: ../git/plugins/quectel.c:cfun_disable() 0x610000000d40
ofonod[5960]: ../git/plugins/quectel.c:close_serial() 0x610000000d40
ofonod[5960]: ../git/examples/emulator.c:powered_watch() Removing modem 0x610000000d40 from the list
ofonod[5960]: ../git/src/modem.c:modem_change_state() old state: 0, new state: 0
=================================================================
==5960==ERROR: AddressSanitizer: heap-use-after-free on address 0x621000aca738 at pc 0x55b9a3ad6b8f bp 0x7ffeac9a2090 sp 0x7ffeac9a2080
READ of size 4 at 0x621000aca738 thread T0
#0 0x55b9a3ad6b8e in received_data ../git/gatchat/gatmux.c:282
#1 0x7fcf4c5082ce in g_main_context_dispatch (/usr/lib/libglib-2.0.so.0+0x6a2ce)
#2 0x7fcf4c50a210 (/usr/lib/libglib-2.0.so.0+0x6c210)
#3 0x7fcf4c50b122 in g_main_loop_run (/usr/lib/libglib-2.0.so.0+0x6d122)
#4 0x55b9a3afcc43 in main ../git/src/main.c:286
#5 0x7fcf4c2ad152 in __libc_start_main (/usr/lib/libc.so.6+0x27152)
#6 0x55b9a39251ad in _start (/home/martin/projects/ofono/x86/src/ofonod+0xfd1ad)
0x621000aca738 is located 4664 bytes inside of 4672-byte region [0x621000ac9500,0x621000aca740)
freed by thread T0 here:
#0 0x7fcf4c7566b0 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:122
#1 0x55b9a3ad83ff in g_at_mux_unref ../git/gatchat/gatmux.c:649
#2 0x55b9a3ad6b29 in received_data ../git/gatchat/gatmux.c:273
#3 0x7fcf4c5082ce in g_main_context_dispatch (/usr/lib/libglib-2.0.so.0+0x6a2ce)
previously allocated by thread T0 here:
#0 0x7fcf4c756cd8 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:153
#1 0x55b9a3ad8131 in g_at_mux_new ../git/gatchat/gatmux.c:610
#2 0x55b9a3adb093 in g_at_mux_new_gsm0710_basic ../git/gatchat/gatmux.c:1169
#3 0x55b9a3aad97d in cmux_cb ../git/plugins/quectel.c:848
#4 0x55b9a3ac9beb in at_chat_finish_command ../git/gatchat/gatchat.c:459
#5 0x55b9a3aca125 in at_chat_handle_command_response ../git/gatchat/gatchat.c:521
#6 0x55b9a3aca965 in have_line ../git/gatchat/gatchat.c:600
#7 0x55b9a3acb8fb in new_bytes ../git/gatchat/gatchat.c:759
#8 0x55b9a3ad4856 in received_data ../git/gatchat/gatio.c:122
#9 0x7fcf4c5082ce in g_main_context_dispatch (/usr/lib/libglib-2.0.so.0+0x6a2ce)
SUMMARY: AddressSanitizer: heap-use-after-free ../git/gatchat/gatmux.c:282 in received_data
Shadow bytes around the buggy address:
0x0c4280151490: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c42801514a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c42801514b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c42801514c0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c42801514d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c42801514e0: fd fd fd fd fd fd fd[fd]fa fa fa fa fa fa fa fa
0x0c42801514f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c4280151500: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c4280151510: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c4280151520: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c4280151530: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==5960==ABORTING
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gatmux: don't free cmux data until watchers are destroyed
2019-10-07 18:37 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
@ 2019-10-07 18:57 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-10-08 4:06 ` Denis Kenzior
1 sibling, 0 replies; 9+ messages in thread
From: Martin =?unknown-8bit?q?Hundeb=C3=B8ll?= @ 2019-10-07 18:57 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 1144 bytes --]
On 07/10/2019 20.37, Martin Hundebøll wrote:
>> Why not do something like:
>>
>> g_at_mux_ref(mux);
>>
>> for (i = 1; i <= MAX_CHANNELS && !mux->shutdown; i++) {
>> int offset = i / 8;
>> int bit = i % 8;
>>
>> if (!(mux->newdata[offset] & (1 << bit)))
>> continue;
>>
>> dispatch_sources(mux->dlcs[i-1], G_IO_IN);
>> }
>>
>> g_at_mux_unref(mux);
>>
>
> I'm afraid this isn't enough, as I still get use-after-free when using
> gatmux in the quectel plugin (see attached log).
>
> I've added g_at_mux_{ref,unref}() calls to the read and write watchers
> too. This fixes it. Patch coming up.
But, it breaks calling g_at_mux_unref() without calling
g_at_mux_shutdown() first, since the unref now doesn't decrement the
refcount to zero.
// Martin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gatmux: don't free cmux data until watchers are destroyed
2019-09-30 20:59 ` Denis Kenzior
2019-10-07 18:37 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
@ 2019-10-07 21:33 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
1 sibling, 0 replies; 9+ messages in thread
From: Martin =?unknown-8bit?q?Hundeb=C3=B8ll?= @ 2019-10-07 21:33 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 2298 bytes --]
On 30/09/2019 22.59, Denis Kenzior wrote:
> Hi Martin,
>
>> @@ -227,12 +227,15 @@ static void dispatch_sources(GAtMuxChannel
>> *channel, GIOCondition condition)
>> static gboolean received_data(GIOChannel *channel, GIOCondition cond,
>> gpointer data)
>> {
>> - GAtMux *mux = data;
>> + GAtMux *mux = g_at_mux_ref(data);
>
> So you're taking a ref here unconditionally, yet...
>
>> int i;
>> GIOStatus status;
>> gsize bytes_read;
>> if (cond & G_IO_NVAL)
>> + goto out_done;
>> +
>> + if (mux->shutdown)
>> return FALSE;
>
> You don't return it here?
>> debug(mux, "received data");
>> @@ -270,15 +273,22 @@ static gboolean received_data(GIOChannel
>> *channel, GIOCondition cond,
>> }
>> if (cond & (G_IO_HUP | G_IO_ERR))
>> - return FALSE;
>> + goto out_done;
>> if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN)
>> - return FALSE;
>> + goto out_done;
>> if (mux->buf_used == sizeof(mux->buf))
>> - return FALSE;
>> + goto out_done;
>> +
>> + g_at_mux_unref(mux);
>
> Why not do something like:
>
> g_at_mux_ref(mux);
>
> for (i = 1; i <= MAX_CHANNELS && !mux->shutdown; i++) {
> int offset = i / 8;
> int bit = i % 8;
>
> if (!(mux->newdata[offset] & (1 << bit)))
> continue;
>
> dispatch_sources(mux->dlcs[i-1], G_IO_IN);
> }
>
> g_at_mux_unref(mux);
I chose to stick with the unconditional reference. Your suggestion can
cause the mux object to be freed when unref'ed after the loop, but the
function accesses it again later in
if (mux->buf_used == sizeof(mux->buf))
The unconditional reference makes the flow nicer, IMO.
// Martin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gatmux: don't free cmux data until watchers are destroyed
2019-10-07 18:37 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-10-07 18:57 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
@ 2019-10-08 4:06 ` Denis Kenzior
2019-10-08 18:47 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
1 sibling, 1 reply; 9+ messages in thread
From: Denis Kenzior @ 2019-10-08 4:06 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]
Hi Martin,
On 10/7/19 1:37 PM, Martin Hundebøll wrote:
>> Why not do something like:
>>
>> g_at_mux_ref(mux);
>>
>> for (i = 1; i <= MAX_CHANNELS && !mux->shutdown; i++) {
>> int offset = i / 8;
>> int bit = i % 8;
>>
>> if (!(mux->newdata[offset] & (1 << bit)))
>> continue;
>>
>> dispatch_sources(mux->dlcs[i-1], G_IO_IN);
>> }
>>
>> g_at_mux_unref(mux);
>>
>
> I'm afraid this isn't enough, as I still get use-after-free when using
> gatmux in the quectel plugin (see attached log).
That buffer size check is a bit dubious and should never happen in
practice, it is also something that one cannot recover from if the
condition does get hit. I actually would just take it out, but you can
also just check the condition right before the mux_unref and scribble it
to a temporary. i.e.
bool buffer_full = false;
g_at_mux_ref();
for (...) {
...
dispatch_sources(...);
}
buffer_full = !mux->shutdown && mux->buf_used == sizeof(mux->buf);
g_at_mux_unref();
...
if (buffer_full)
return FALSE;
Regards,
-Denis
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gatmux: don't free cmux data until watchers are destroyed
2019-10-08 4:06 ` Denis Kenzior
@ 2019-10-08 18:47 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-10-08 19:44 ` Denis Kenzior
0 siblings, 1 reply; 9+ messages in thread
From: Martin =?unknown-8bit?q?Hundeb=C3=B8ll?= @ 2019-10-08 18:47 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]
On 08/10/2019 06.06, Denis Kenzior wrote:
>
> On 10/7/19 1:37 PM, Martin Hundebøll wrote:
>>> Why not do something like:
>>>
>>> g_at_mux_ref(mux);
>>>
>>> for (i = 1; i <= MAX_CHANNELS && !mux->shutdown; i++) {
>>> int offset = i / 8;
>>> int bit = i % 8;
>>>
>>> if (!(mux->newdata[offset] & (1 << bit)))
>>> continue;
>>>
>>> dispatch_sources(mux->dlcs[i-1], G_IO_IN);
>>> }
>>>
>>> g_at_mux_unref(mux);
>>>
>>
>> I'm afraid this isn't enough, as I still get use-after-free when using
>> gatmux in the quectel plugin (see attached log).
>
> That buffer size check is a bit dubious and should never happen in
> practice, it is also something that one cannot recover from if the
> condition does get hit. I actually would just take it out, but you can
> also just check the condition right before the mux_unref and scribble it
> to a temporary. i.e.
>
> bool buffer_full = false;
>
> g_at_mux_ref();
>
> for (...) {
> ...
> dispatch_sources(...);
> }
>
> buffer_full = !mux->shutdown && mux->buf_used == sizeof(mux->buf);
Why the "!mux->shutdown" here? I would expect that to be checked before
processing the data...
// Martin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gatmux: don't free cmux data until watchers are destroyed
2019-10-08 18:47 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
@ 2019-10-08 19:44 ` Denis Kenzior
0 siblings, 0 replies; 9+ messages in thread
From: Denis Kenzior @ 2019-10-08 19:44 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 991 bytes --]
Hi Martin,
>>> I'm afraid this isn't enough, as I still get use-after-free when
>>> using gatmux in the quectel plugin (see attached log).
>>
>> That buffer size check is a bit dubious and should never happen in
>> practice, it is also something that one cannot recover from if the
>> condition does get hit. I actually would just take it out, but you
>> can also just check the condition right before the mux_unref and
>> scribble it to a temporary. i.e.
>>
>> bool buffer_full = false;
>>
>> g_at_mux_ref();
>>
>> for (...) {
>> ...
>> dispatch_sources(...);
>> }
>>
>> buffer_full = !mux->shutdown && mux->buf_used == sizeof(mux->buf);
>
> Why the "!mux->shutdown" here? I would expect that to be checked before
> processing the data...
Ah you're right, strictly speaking checking mux->shutdown would be
redundant. The idea was to avoid returning FALSE if we're shut down,
but I guess it shouldn't matter.
Regards,
-Denis
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-10-08 19:44 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-30 17:35 [PATCH] gatmux: don't free cmux data until watchers are destroyed Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-09-30 17:38 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-09-30 20:59 ` Denis Kenzior
2019-10-07 18:37 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-10-07 18:57 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-10-08 4:06 ` Denis Kenzior
2019-10-08 18:47 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
2019-10-08 19:44 ` Denis Kenzior
2019-10-07 21:33 ` Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.