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
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
_______________________________________________
ofono mailing list -- [email protected]
To unsubscribe send an email to [email protected]
