Skip to contentSkip to conversation

obexd: fix crashed after cancel the on-going transfer

PATCH In review Youwan Wang 3 participants 5 messages Jul 1, 2022 Bluetooth drivers

lore
Share & tools
lore

Youwan Wang

author
#0 lore raw

PATCH 1/1 obexd: fix crashed after cancel the on-going transfer

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 | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
gobex/gobex-transfer.c +10 -13
index c94d018b2..a7d6c103a 100644
@@ -83,15 +83,20 @@ static struct transfer *find_transfer(guint id)
 
 static void transfer_complete(struct transfer *transfer, GError *err)
 {
-	guint id = transfer->id;
+	guint id;
 
-	g_obex_debug(G_OBEX_DEBUG_TRANSFER, "transfer %u", id);
+	if (!g_slist_find(transfers, transfer))
+		return;
+
+	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);
 	}
 
+	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 +111,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");

@@ -184,12 +186,6 @@ static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp,
 	struct transfer *transfer = user_data;
 	GObexPacket *req;
 	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);

@@ -203,6 +199,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 +221,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;
 	}
Signature · 1 line
2.20.1
#1 lore raw

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=655700

---Test result---

Test Summary:

CheckPatch                    PASS      0.95 seconds
GitLint                       PASS      0.48 seconds
Prep - Setup ELL              FAIL      29.22 seconds
Build - Prep                  PASS      0.78 seconds
Build - Configure             PASS      10.80 seconds
Build - Make                  PASS      1151.93 seconds
Make Check                    PASS      13.56 seconds
Make Check w/Valgrind         PASS      363.71 seconds
Make Distcheck                PASS      308.78 seconds
Build w/ext ELL - Configure   FAIL      7.36 seconds
Build w/ext ELL - Make        SKIPPED   0.15 seconds
Incremental Build w/ patches  PASS      0.00 seconds
Scan Build                    WARNING   664.12 seconds

Details
##############################
Test: Prep - Setup ELL - FAIL
Desc: Clone, build, and install ELL
Output:
writing RSA key
writing RSA key
writing RSA key
writing RSA key
writing RSA key
make[1]: *** [Makefile:3241: unit/cert-intca.pem] Error 1 make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1262: all] Error 2

##############################
Test: Build w/ext ELL - Configure - FAIL
Desc: Configure BlueZ source with '--enable-external-ell' configuration Output:
configure.ac:21: installing './compile'
configure.ac:36: installing './config.guess' configure.ac:36: installing './config.sub'
configure.ac:5: installing './install-sh'
configure.ac:5: installing './missing'
Makefile.am: installing './depcomp'
parallel-tests: installing './test-driver'
configure: error: Embedded Linux library >= 0.39 is required

##############################
Test: Build w/ext ELL - Make - SKIPPED
Desc: Build BlueZ source with '--enable-external-ell' configuration Output:
build_extell test did not pass

##############################
Test: Scan Build - WARNING
Desc: Run Scan Build with patches
Output:
*****************************************************************************
The bugs reported by the scan-build may or may not be caused by your patches. Please check the list and fix the bugs if they are caused by your patch.
*****************************************************************************
gobex/gobex-transfer.c:420:7: warning: Use of memory after it is freed
if (!g_slist_find(transfers, transfer))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

---
Regards,
Linux Bluetooth

maintainer
#2 lore raw
In reply to Youwan Wang

Hi Youwan,

Quoted 111 lines · Youwan Wang

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 | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
gobex/gobex-transfer.c +0 -0
index c94d018b2..a7d6c103a 100644

gobex/gobex-transfer.c static struct transfer *find_transfer(guint id) · line 89

Quoted 9 lines
gobex/gobex-transfer.c +3 -2
@@ -83,15 +83,20 @@ static struct transfer *find_transfer(guint id)
 
 static void transfer_complete(struct transfer *transfer, GError *err)
 {
-       guint id = transfer->id;
+       guint id;
 
-       g_obex_debug(G_OBEX_DEBUG_TRANSFER, "transfer %u", id);
+       if (!g_slist_find(transfers, transfer))
+               return;

If we have to do a lookup then something is wrong already and the transfer_complete shall not called.

Quoted 23 lines
gobex/gobex-transfer.c +3 -3
+       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);
        }
 
+       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 +111,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");

gobex/gobex-transfer.c static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp, · line 192

Quoted 9 lines
gobex/gobex-transfer.c +0 -6
@@ -184,12 +186,6 @@ static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp,
        struct transfer *transfer = user_data;
        GObexPacket *req;
        gboolean rspcode, final;
-       guint id;
-
-       g_obex_debug(G_OBEX_DEBUG_TRANSFER, "transfer %u", transfer->id);
-
-       id = transfer->req_id;
-       transfer->req_id = 0;

Don't think this is right, also I'm not sure why you are removing the transfer? If the transfer has been freed then there is something already quite wrong.

Quoted 13 lines
gobex/gobex-transfer.c +3 -0
        if (err != NULL) {
                transfer_complete(transfer, err);

@@ -203,6 +199,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)

gobex/gobex-transfer.c static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp, · line 226

Quoted 5 lines
gobex/gobex-transfer.c +0 -2
@@ -222,8 +221,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;

Not sure why you are removing this line?

Quoted 8 lines
gobex/gobex-transfer.c +0 -1
                return;
        }
 
--

2.20.1

Signature · 1 line
Luiz Augusto von Dentz
maintainer
#3 lore raw

Hi,

Quoted 189 lines · Luiz Augusto von Dentz

Hi Youwan,

Quoted 111 lines · Youwan Wang

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 | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
gobex/gobex-transfer.c +0 -0
index c94d018b2..a7d6c103a 100644

gobex/gobex-transfer.c static struct transfer *find_transfer(guint id) · line 89

Quoted 9 lines
gobex/gobex-transfer.c +3 -2
@@ -83,15 +83,20 @@ static struct transfer *find_transfer(guint id)
 
 static void transfer_complete(struct transfer *transfer, GError *err)
 {
-       guint id = transfer->id;
+       guint id;
 
-       g_obex_debug(G_OBEX_DEBUG_TRANSFER, "transfer %u", id);
+       if (!g_slist_find(transfers, transfer))
+               return;

If we have to do a lookup then something is wrong already and the transfer_complete shall not called.

Quoted 23 lines
gobex/gobex-transfer.c +3 -3
+       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);
        }
 
+       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 +111,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");

gobex/gobex-transfer.c static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp, · line 192

Quoted 9 lines
gobex/gobex-transfer.c +0 -6
@@ -184,12 +186,6 @@ static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp,
        struct transfer *transfer = user_data;
        GObexPacket *req;
        gboolean rspcode, final;
-       guint id;
-
-       g_obex_debug(G_OBEX_DEBUG_TRANSFER, "transfer %u", transfer->id);
-
-       id = transfer->req_id;
-       transfer->req_id = 0;

Don't think this is right, also I'm not sure why you are removing the transfer? If the transfer has been freed then there is something already quite wrong.

Quoted 13 lines
gobex/gobex-transfer.c +3 -0
        if (err != NULL) {
                transfer_complete(transfer, err);

@@ -203,6 +199,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)

gobex/gobex-transfer.c static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp, · line 226

Quoted 5 lines
gobex/gobex-transfer.c +0 -2
@@ -222,8 +221,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;

Not sure why you are removing this line?

Quoted 3 lines
gobex/gobex-transfer.c +0 -0
                return;
        }
Quoted 8 lines
Quoted 5 lines

--
2.20.1

--

Btw, I suspect there is something wrong with the session itself, as there is a test for the exact scenario of cancelling a transfer:

# GOBEX_DEBUG=transfer valgrind unit/test-gobex-transfer -d -p "/gobex/test_stream_put_req_abort"
==851604== Memcheck, a memory error detector
==851604== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al. ==851604== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info ==851604== Command: unit/test-gobex-transfer -d -p /gobex/test_stream_put_req_abort
==851604==

# random seed: R02S5f08b6298979742063c1891a8b1c92d6
# Start of gobex tests
# gobex-DEBUG: gobex/gobex-transfer.c:g_obex_put_req() obex 0x58e7ea0
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.488:
gobex/gobex-transfer.c:g_obex_put_req() obex 0x58e7ea0
# gobex-DEBUG: gobex/gobex-transfer.c:g_obex_put_req_pkt() obex 0x58e7ea0
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.517:
gobex/gobex-transfer.c:g_obex_put_req_pkt() obex 0x58e7ea0
# gobex-DEBUG: gobex/gobex-transfer.c:transfer_new() obex 0x58e7ea0 opcode 2
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.518:
gobex/gobex-transfer.c:transfer_new() obex 0x58e7ea0 opcode 2
# gobex-DEBUG: gobex/gobex-transfer.c:g_obex_put_req_pkt() transfer 1
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.522:
gobex/gobex-transfer.c:g_obex_put_req_pkt() transfer 1
# gobex-DEBUG: gobex/gobex-transfer.c:g_obex_cancel_transfer() transfer 1
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.559:
gobex/gobex-transfer.c:g_obex_cancel_transfer() transfer 1
# gobex-DEBUG: gobex/gobex-transfer.c:transfer_response() transfer 1
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.581:
gobex/gobex-transfer.c:transfer_response() transfer 1
# gobex-DEBUG: gobex/gobex-transfer.c:transfer_complete() transfer 1
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.582:
gobex/gobex-transfer.c:transfer_complete() transfer 1
# gobex-DEBUG: gobex/gobex-transfer.c:transfer_free() transfer 1
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.584:
gobex/gobex-transfer.c:transfer_free() transfer 1
ok 1 /gobex/test_stream_put_req_abort
# End of gobex tests
1..1
==851604==
==851604== HEAP SUMMARY:
==851604==     in use at exit: 22,307 bytes in 36 blocks
==851604==   total heap usage: 862 allocs, 826 frees, 904,691 bytes allocated
==851604==
==851604== LEAK SUMMARY:
==851604==    definitely lost: 0 bytes in 0 blocks
==851604==    indirectly lost: 0 bytes in 0 blocks
==851604==      possibly lost: 0 bytes in 0 blocks
==851604==    still reachable: 22,307 bytes in 36 blocks
==851604==         suppressed: 0 bytes in 0 blocks
==851604== Rerun with --leak-check=full to see details of leaked memory
==851604==
==851604== For lists of detected and suppressed errors, rerun with: -s
==851604== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Perhaps in your case it is not responding with:

static guint8 put_rsp_last[] = { G_OBEX_RSP_SUCCESS | FINAL_BIT, 0x00, 0x03 };

Anyway, please add a similar test to mimic the exact response you are getting.

Signature · 1 line
Luiz Augusto von Dentz
maintainer
#4 lore raw

Hi,

Quoted 259 lines · Luiz Augusto von Dentz

Hi,

Quoted 189 lines · Luiz Augusto von Dentz

Hi Youwan,

Quoted 111 lines · Youwan Wang

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 | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
gobex/gobex-transfer.c +0 -0
index c94d018b2..a7d6c103a 100644

gobex/gobex-transfer.c static struct transfer *find_transfer(guint id) · line 89

Quoted 9 lines
gobex/gobex-transfer.c +3 -2
@@ -83,15 +83,20 @@ static struct transfer *find_transfer(guint id)
 
 static void transfer_complete(struct transfer *transfer, GError *err)
 {
-       guint id = transfer->id;
+       guint id;
 
-       g_obex_debug(G_OBEX_DEBUG_TRANSFER, "transfer %u", id);
+       if (!g_slist_find(transfers, transfer))
+               return;

If we have to do a lookup then something is wrong already and the transfer_complete shall not called.

Quoted 23 lines
gobex/gobex-transfer.c +3 -3
+       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);
        }
 
+       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 +111,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");

gobex/gobex-transfer.c static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp, · line 192

Quoted 9 lines
gobex/gobex-transfer.c +0 -6
@@ -184,12 +186,6 @@ static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp,
        struct transfer *transfer = user_data;
        GObexPacket *req;
        gboolean rspcode, final;
-       guint id;
-
-       g_obex_debug(G_OBEX_DEBUG_TRANSFER, "transfer %u", transfer->id);
-
-       id = transfer->req_id;
-       transfer->req_id = 0;

Don't think this is right, also I'm not sure why you are removing the transfer? If the transfer has been freed then there is something already quite wrong.

Quoted 13 lines
gobex/gobex-transfer.c +3 -0
        if (err != NULL) {
                transfer_complete(transfer, err);

@@ -203,6 +199,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)

gobex/gobex-transfer.c static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp, · line 226

Quoted 5 lines
gobex/gobex-transfer.c +0 -2
@@ -222,8 +221,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;

Not sure why you are removing this line?

Quoted 3 lines
gobex/gobex-transfer.c +0 -0
                return;
        }
Quoted 8 lines
Quoted 5 lines

--
2.20.1

--

Btw, I suspect there is something wrong with the session itself, as there is a test for the exact scenario of cancelling a transfer:

# GOBEX_DEBUG=transfer valgrind unit/test-gobex-transfer -d -p "/gobex/test_stream_put_req_abort"
==851604== Memcheck, a memory error detector
==851604== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al. ==851604== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info ==851604== Command: unit/test-gobex-transfer -d -p /gobex/test_stream_put_req_abort
==851604==

# random seed: R02S5f08b6298979742063c1891a8b1c92d6
# Start of gobex tests
# gobex-DEBUG: gobex/gobex-transfer.c:g_obex_put_req() obex 0x58e7ea0
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.488:
gobex/gobex-transfer.c:g_obex_put_req() obex 0x58e7ea0
# gobex-DEBUG: gobex/gobex-transfer.c:g_obex_put_req_pkt() obex 0x58e7ea0
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.517:
gobex/gobex-transfer.c:g_obex_put_req_pkt() obex 0x58e7ea0
# gobex-DEBUG: gobex/gobex-transfer.c:transfer_new() obex 0x58e7ea0 opcode 2
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.518:
gobex/gobex-transfer.c:transfer_new() obex 0x58e7ea0 opcode 2
# gobex-DEBUG: gobex/gobex-transfer.c:g_obex_put_req_pkt() transfer 1
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.522:
gobex/gobex-transfer.c:g_obex_put_req_pkt() transfer 1
# gobex-DEBUG: gobex/gobex-transfer.c:g_obex_cancel_transfer() transfer 1
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.559:
gobex/gobex-transfer.c:g_obex_cancel_transfer() transfer 1
# gobex-DEBUG: gobex/gobex-transfer.c:transfer_response() transfer 1
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.581:
gobex/gobex-transfer.c:transfer_response() transfer 1
# gobex-DEBUG: gobex/gobex-transfer.c:transfer_complete() transfer 1
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.582:
gobex/gobex-transfer.c:transfer_complete() transfer 1
# gobex-DEBUG: gobex/gobex-transfer.c:transfer_free() transfer 1
(unit/test-gobex-transfer:851604): gobex-DEBUG: 15:37:37.584:
gobex/gobex-transfer.c:transfer_free() transfer 1
ok 1 /gobex/test_stream_put_req_abort
# End of gobex tests
1..1
==851604==
==851604== HEAP SUMMARY:
==851604==     in use at exit: 22,307 bytes in 36 blocks
==851604==   total heap usage: 862 allocs, 826 frees, 904,691 bytes allocated
==851604==
==851604== LEAK SUMMARY:
==851604==    definitely lost: 0 bytes in 0 blocks
==851604==    indirectly lost: 0 bytes in 0 blocks
==851604==      possibly lost: 0 bytes in 0 blocks
==851604==    still reachable: 22,307 bytes in 36 blocks
==851604==         suppressed: 0 bytes in 0 blocks
==851604== Rerun with --leak-check=full to see details of leaked memory
==851604==
==851604== For lists of detected and suppressed errors, rerun with: -s
==851604== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Quoted 5 lines

Perhaps in your case it is not responding with:

static guint8 put_rsp_last[] = { G_OBEX_RSP_SUCCESS | FINAL_BIT, 0x00, 0x03 };

Anyway, please add a similar test to mimic the exact response you are getting.

Ive tried removing the FINAL_BIT but that didn't resolve it, perhaps the device is rejecting the ABORT? Btw, what is that you are testing with?

Quoted 3 lines

--
Luiz Augusto von Dentz

Signature · 1 line
Luiz Augusto von Dentz