diff mbox series

obexd: fix crashed after cancel the on-going transfer

Message ID 20220701055237.2946-1-wangyouwan@uniontech.com (mailing list archive)
State Superseded
Headers show
Series obexd: fix crashed after cancel the on-going transfer | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS
tedd_an/setupell success Setup ELL PASS
tedd_an/buildprep success Build Prep PASS
tedd_an/build success Build Configuration PASS
tedd_an/makecheck fail Make Check FAIL: gobex/gobex-transfer.c: In function ‘transfer_complete’: gobex/gobex-transfer.c:97:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 97 | guint id = transfer->id; | ^~~~~ gobex/gobex-transfer.c: In function ‘transfer_response’: gobex/gobex-transfer.c:187:8: error: unused variable ‘id’ [-Werror=unused-variable] 187 | guint id; | ^~ cc1: all warnings being treated as errors make[1]: *** [Makefile:7300: gobex/gobex-transfer.o] Error 1 make: *** [Makefile:11320: check] Error 2
tedd_an/makecheckvalgrind fail Make FAIL: tools/mgmt-tester.c: In function ‘main’: tools/mgmt-tester.c:12426:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without 12426 | int main(int argc, char *argv[]) | ^~~~ gobex/gobex-transfer.c: In function ‘transfer_complete’: gobex/gobex-transfer.c:97:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 97 | guint id = transfer->id; | ^~~~~ gobex/gobex-transfer.c: In function ‘transfer_response’: gobex/gobex-transfer.c:187:8: error: unused variable ‘id’ [-Werror=unused-variable] 187 | guint id; | ^~ cc1: all warnings being treated as errors make[1]: *** [Makefile:8801: gobex/obexd-gobex-transfer.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:4324: all] Error 2
tedd_an/makedistcheck success Make Distcheck PASS
tedd_an/build_extell success Build External ELL PASS
tedd_an/build_extell_make fail Build Make with External ELL FAIL: gobex/gobex-transfer.c: In function ‘transfer_complete’: gobex/gobex-transfer.c:97:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 97 | guint id = transfer->id; | ^~~~~ gobex/gobex-transfer.c: In function ‘transfer_response’: gobex/gobex-transfer.c:187:8: error: unused variable ‘id’ [-Werror=unused-variable] 187 | guint id; | ^~ cc1: all warnings being treated as errors make[1]: *** [Makefile:8801: gobex/obexd-gobex-transfer.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:4324: all] Error 2
tedd_an/scan_build fail Scan Build w/patched FAIL: gobex/gobex-transfer.c: In function ‘transfer_complete’: gobex/gobex-transfer.c:97:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] 97 | guint id = transfer->id; | ^~~~~ gobex/gobex-transfer.c: In function ‘transfer_response’: gobex/gobex-transfer.c:187:8: error: unused variable ‘id’ [-Werror=unused-variable] 187 | guint id; | ^~ cc1: all warnings being treated as errors make[1]: *** [Makefile:7300: gobex/gobex-transfer.o] Error 1 make: *** [Makefile:4324: all] Error 2

Commit Message

Youwan Wang July 1, 2022, 5:52 a.m. UTC
There is a use after released.transfer->req_id different
obex->pending_req->id,See the following log,
The packages is removd in cancel_complete func
are not the same package in req_timeout func,
but transfer pointer is released.

log:
g_obex_cancel_req()
transfer->req_id 23 id 22 obex->pending_req(0x55b642c3e100)

g_obex_cancel_req()
match->data (0x55b642c344a0)

g_obex_ref() ref 4

cancel_complete()
pending req timeout 176 id 22 obex(0x55b642c3e100)

transfer_response()
obex 0x55b642c36480 transfer(0x55b642c3d000)

g_obex_drop_tx_queue()

g_obex_unref() obex 0x55b642c36480
g_obex_unref() ref 3

transfer_free()
obex 0x55b642c36480 transfer 0x55b642c3d000

g_obex_unref() obex 0x55b642c36480
g_obex_unref() ref 2

pending_pkt_free()
timeout_id 0 pending_pkt (0x55b642c344a0)

step:
[obex]# connect 28:33:34:1E:96:98
Attempting to connect to 28:33:34:1E:96:98
[NEW] Session /org/bluez/obex/client/session2 [default]
[NEW] ObjectPush /org/bluez/obex/client/session2
Connection successful
[28:33:34:1E:96:98]# send /home/uos/Desktop/systemd.zip
Attempting to send /home/uos/Desktop/systemd.zip
[NEW] Transfer /org/bluez/obex/client/session2/transfer2
Transfer /org/bluez/obex/client/session2/transfer2
        Status: queued
        Name: systemd.zip
        Size: 33466053
        Filename: /home/uos/Desktop/systemd.zip
        Session: /org/bluez/obex/client/session2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
[CHG] Transfer /org/bluez/obex/client/session2/transfer2
er2 33:34:1E:96:98]# cancel /org/bluez/obex/client/sessi
Attempting to cancel transfer /org/bluez/obex/client/s
Cancel successful

valgrind trace:
==11431== Invalid read of size 4
==11431==    at 0x12B442: transfer_response ()
==11431==    by 0x127764: req_timeout ()
==11431==    by 0x49B8922: ??? ( )
==11431==    by 0x49B7E97: g_main_context_dispatch ()
==11431==    by 0x49B8287: ??? (in )
==11431==    by 0x49B8581: g_main_loop_run ()
==11431==    by 0x121834: main (main.c:322)
==11431==  Address 0x7344fa0 is 16 bytes inside a block of size
==11431==    at 0x48369AB: free ()
==11431==    by 0x12B459: transfer_response ()
==11431==    by 0x127B3D: cancel_complete ()
==11431==    by 0x49B7E97: g_main_context_dispatch ()
==11431==    by 0x49B8287: ??? ()
==11431==    by 0x49B8581: g_main_loop_run ()
==11431==    by 0x121834: main (main.c:322)
==11431==  Block was alloc'd at
==11431==    at 0x4837B65: calloc ()
==11431==    by 0x49BD9D8: g_malloc0 ()
==11431==    by 0x12AB89: transfer_new ()
==11431==    by 0x12B732: g_obex_put_req_pkt ()
==11431==    by 0x12B732: g_obex_put_req_pkt ()
==11431==    by 0x146982: transfer_start_put ()
==11431==    by 0x146982: obc_transfer_start ()
==11431==    by 0x13C5A7: session_process_transfer ()
==11431==    by 0x13D248: session_process_queue ()
==11431==    by 0x13D248: session_process_queue ()
==11431==    by 0x13D2AF: session_process ()
==11431==    by 0x49B7E97: g_main_context_dispatch ()
==11431==    by 0x49B8287: ??? (i)
==11431==    by 0x49B8581: g_main_loop_run ()
==11431==    by 0x121834: main ()
==11431==
==11431== (action on error) vgdb me ...
---
 gobex/gobex-transfer.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

Comments

bluez.test.bot@gmail.com July 1, 2022, 6:53 a.m. UTC | #1
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=655654

---Test result---

Test Summary:
CheckPatch                    PASS      1.71 seconds
GitLint                       PASS      1.07 seconds
Prep - Setup ELL              PASS      27.14 seconds
Build - Prep                  PASS      0.80 seconds
Build - Configure             PASS      8.53 seconds
Build - Make                  FAIL      96.42 seconds
Make Check                    FAIL      173.46 seconds
Make Check w/Valgrind         FAIL      77.25 seconds
Make Distcheck                PASS      229.35 seconds
Build w/ext ELL - Configure   PASS      8.55 seconds
Build w/ext ELL - Make        FAIL      34.65 seconds
Incremental Build w/ patches  PASS      0.00 seconds
Scan Build                    FAIL      466.61 seconds

Details
##############################
Test: Build - Make - FAIL
Desc: Build the BlueZ source tree
Output:
tools/mgmt-tester.c: In function ‘main’:
tools/mgmt-tester.c:12426:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
12426 | int main(int argc, char *argv[])
      |     ^~~~
gobex/gobex-transfer.c: In function ‘transfer_complete’:
gobex/gobex-transfer.c:97:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
   97 |  guint id = transfer->id;
      |  ^~~~~
gobex/gobex-transfer.c: In function ‘transfer_response’:
gobex/gobex-transfer.c:187:8: error: unused variable ‘id’ [-Werror=unused-variable]
  187 |  guint id;
      |        ^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8801: gobex/obexd-gobex-transfer.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4324: all] Error 2


##############################
Test: Make Check - FAIL
Desc: Run 'make check'
Output:
gobex/gobex-transfer.c: In function ‘transfer_complete’:
gobex/gobex-transfer.c:97:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
   97 |  guint id = transfer->id;
      |  ^~~~~
gobex/gobex-transfer.c: In function ‘transfer_response’:
gobex/gobex-transfer.c:187:8: error: unused variable ‘id’ [-Werror=unused-variable]
  187 |  guint id;
      |        ^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7300: gobex/gobex-transfer.o] Error 1
make: *** [Makefile:11320: check] Error 2


##############################
Test: Make Check w/Valgrind - FAIL
Desc: Run 'make check' with Valgrind
Output:
tools/mgmt-tester.c: In function ‘main’:
tools/mgmt-tester.c:12426:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
12426 | int main(int argc, char *argv[])
      |     ^~~~
gobex/gobex-transfer.c: In function ‘transfer_complete’:
gobex/gobex-transfer.c:97:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
   97 |  guint id = transfer->id;
      |  ^~~~~
gobex/gobex-transfer.c: In function ‘transfer_response’:
gobex/gobex-transfer.c:187:8: error: unused variable ‘id’ [-Werror=unused-variable]
  187 |  guint id;
      |        ^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8801: gobex/obexd-gobex-transfer.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4324: all] Error 2


##############################
Test: Build w/ext ELL - Make - FAIL
Desc: Build BlueZ source with '--enable-external-ell' configuration
Output:
gobex/gobex-transfer.c: In function ‘transfer_complete’:
gobex/gobex-transfer.c:97:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
   97 |  guint id = transfer->id;
      |  ^~~~~
gobex/gobex-transfer.c: In function ‘transfer_response’:
gobex/gobex-transfer.c:187:8: error: unused variable ‘id’ [-Werror=unused-variable]
  187 |  guint id;
      |        ^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8801: gobex/obexd-gobex-transfer.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4324: all] Error 2


##############################
Test: Scan Build - FAIL
Desc: Run Scan Build with patches
Output:
gobex/gobex-transfer.c: In function ‘transfer_complete’:
gobex/gobex-transfer.c:97:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
   97 |  guint id = transfer->id;
      |  ^~~~~
gobex/gobex-transfer.c: In function ‘transfer_response’:
gobex/gobex-transfer.c:187:8: error: unused variable ‘id’ [-Werror=unused-variable]
  187 |  guint id;
      |        ^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7300: gobex/gobex-transfer.o] Error 1
make: *** [Makefile:4324: all] Error 2




---
Regards,
Linux Bluetooth
diff mbox series

Patch

diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
index c94d018b2..bd820757d 100644
--- a/gobex/gobex-transfer.c
+++ b/gobex/gobex-transfer.c
@@ -83,15 +83,18 @@  static struct transfer *find_transfer(guint id)
 
 static void transfer_complete(struct transfer *transfer, GError *err)
 {
-	guint id = transfer->id;
+	if (!g_slist_find(transfers, transfer))
+		return;
 
-	g_obex_debug(G_OBEX_DEBUG_TRANSFER, "transfer %u", id);
+	transfer->req_id = 0;
+	g_obex_debug(G_OBEX_DEBUG_TRANSFER, "transfer %u", transfer->id);
 
 	if (err) {
 		/* No further tx must be performed */
 		g_obex_drop_tx_queue(transfer->obex);
 	}
 
+	guint id = transfer->id;
 	transfer->complete_func(transfer->obex, err, transfer->user_data);
 	/* Check if the complete_func removed the transfer */
 	if (find_transfer(id) == NULL)
@@ -106,9 +109,6 @@  static void transfer_abort_response(GObex *obex, GError *err, GObexPacket *rsp,
 	struct transfer *transfer = user_data;
 
 	g_obex_debug(G_OBEX_DEBUG_TRANSFER, "transfer %u", transfer->id);
-
-	transfer->req_id = 0;
-
 	/* Intentionally override error */
 	err = g_error_new(G_OBEX_ERROR, G_OBEX_ERROR_CANCELLED,
 						"Operation was aborted");
@@ -186,11 +186,6 @@  static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp,
 	gboolean rspcode, final;
 	guint id;
 
-	g_obex_debug(G_OBEX_DEBUG_TRANSFER, "transfer %u", transfer->id);
-
-	id = transfer->req_id;
-	transfer->req_id = 0;
-
 	if (err != NULL) {
 		transfer_complete(transfer, err);
 		return;
@@ -203,6 +198,9 @@  static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp,
 		goto failed;
 	}
 
+	if (!g_slist_find(transfers, transfer))
+		return;
+
 	if (transfer->opcode == G_OBEX_OP_GET) {
 		handle_get_body(transfer, rsp, &err);
 		if (err != NULL)
@@ -222,8 +220,6 @@  static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp,
 		req = g_obex_packet_new(transfer->opcode, TRUE,
 							G_OBEX_HDR_INVALID);
 	} else {
-		/* Keep id since request still outstanting */
-		transfer->req_id = id;
 		return;
 	}