Summary

Fix crash when cancelling an XMPP file transfer

Review Request #1485 — Created May 28, 2022 and submitted June 2, 2022, 9:18 p.m.

Information

Owner

belgin

Repository

pidgin/pidgin

Branch

release-2.x.y

Bugs

PIDGIN-17189

Depends On

Reviewers

Groups

pidgin

People

Description

When an XMPP account has file transfers and the
connection to the XMPP server drops, the JabberStream object
gets g_free()'d, but the PurpleXfers still have pointers to it,
so when the file transfer finishes or is canceled, it tries
to remove itself from the JabberStream object and it ends up
accessing invalid memory, causing a crash.

This patch closes all file transfers associated to
a JabberStream object, right before the object gets g_free()'d.

Testing Done

Reliably reproduced the crash by closing internet connection while a transfer was in progress. After the patch, the crash no longer occurs because the transfers all end when the internet connection drops.

Commits

Summary	ID
Fix crash when cancelling an XMPP file transfer When an XMPP account has file transfers and the connection to the XMPP server drops, the JabberStream object gets g_free()'d, but the PurpleXfers still have pointers to it, so when the file transfer finishes or is canceled, it tries to remove itself from the JabberStream object and it ends up accessing invalid memory, causing a crash. This patch closes all file transfers associated to a JabberStream object, right before the object gets g_free()'d. Testing Done: Reliably reproduced the crash by closing internet connection while a transfer was in progress. After the patch, the crash no longer occurs because the transfers all end when the internet connection drops. Bugs closed: PIDGIN-17189 Reviewed at https://reviews.imfreedom.org/r/1485/	6a43b9d5ac797ea77c83c3e6a4230ad67ce72716

Summary

Fix crash when cancelling an XMPP file transfer

When an XMPP account has file transfers and the connection to the XMPP server drops, the JabberStream object gets g_free()'d, but the PurpleXfers still have pointers to it, so when the file transfer finishes or is canceled, it tries to remove itself from the JabberStream object and it ends up accessing invalid memory, causing a crash. This patch closes all file transfers associated to a JabberStream object, right before the object gets g_free()'d. Testing Done: Reliably reproduced the crash by closing internet connection while a transfer was in progress. After the patch, the crash no longer occurs because the transfers all end when the internet connection drops. Bugs closed: PIDGIN-17189 Reviewed at https://reviews.imfreedom.org/r/1485/

6a43b9d5ac797ea77c83c3e6a4230ad67ce72716

Issues

Description	From	Last Updated
My only question is, will ending a file transfer cause the protocol side to remove it from js->file_transfers, thereby causing …	QuLogic	May 31, 2022, 4:46 p.m.
if we're destroying the transfers and freeing them, it's probably best to use something like while(js->file_transfers != NULL) { purple_xfer_end(js->file_transfers->data); …	grim	May 31, 2022, 4:47 p.m.
I'm torn on whether or not we should go with this or g_assert_reached...	grim	June 2, 2022, 9:11 p.m.

libpurple/protocols/jabber/jabber.c (Diff revision 1)

The issue has been resolved. Show all issues

My only question is, will ending a file transfer cause the protocol side to remove it from js->file_transfers, thereby causing p to be invalid?

belgin May 31, 2022, 4:45 p.m.
```
Great catch! This was indeed happening.
```

libpurple/protocols/jabber/jabber.c (Diff revision 1)

The issue has been resolved. Show all issues

if we're destroying the transfers and freeing them, it's probably best to use something like

while(js->file_transfers != NULL) {
    purple_xfer_end(js->file_transfers->data);
        js->file_transfers = g_list_delete_link(js->file_transfers, js->file_transfers);
}


To clean up the list as we're iterating it.

Change Summary:

Ending a file transfer removes it from the file_transfers list, so
there's no need to iterate through the list, just check if a link really
has been removed (beacuse jabber_si_xfer_free calls g_list_remove
conditionally) and remove it manually if not.

Commits:

	Summary	ID
	Fix crash when cancelling an XMPP file transfer When an XMPP account has file transfers and the connection to the XMPP server drops, the JabberStream object gets g_free()'d, but the PurpleXfers still have pointers to it, so when the file transfer finishes or is canceled, it tries to remove itself from the JabberStream object and it ends up accessing invalid memory, causing a crash. This patch closes all file transfers associated to a JabberStream object, right before the object gets g_free()'d.	4b0d6160d170a1bb80755402320b37993dcba7d5
	Fix crash when cancelling an XMPP file transfer When an XMPP account has file transfers and the connection to the XMPP server drops, the JabberStream object gets g_free()'d, but the PurpleXfers still have pointers to it, so when the file transfer finishes or is canceled, it tries to remove itself from the JabberStream object and it ends up accessing invalid memory, causing a crash. This patch closes all file transfers associated to a JabberStream object, right before the object gets g_free()'d. Testing Done: Reliably reproduced the crash by closing internet connection while a transfer was in progress. After the patch, the crash no longer occurs because the transfers all end when the internet connection drops. Bugs closed: PIDGIN-17189 Reviewed at https://reviews.imfreedom.org/r/1485/	6a43b9d5ac797ea77c83c3e6a4230ad67ce72716

Diff:

Revision 2 (+46)

Show changes

libpurple/protocols/jabber/jabber.c

libpurple/protocols/jabber/jabber.c (Diff revision 2)
The issue has been dropped. Show all issues
```
I'm torn on whether or not we should go with this or g_assert_reached...
```
1. grim June 2, 2022, 9:11 p.m.
  lets just go with whats here...

Ship it!

```
Ship It!
```

Status:: Completed