Fix crash when cancelling an XMPP file transfer

Review Request #1485 — Created May 28, 2022 and submitted

Information

pidgin/pidgin
release-2.x.y

Reviewers

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.

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.

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
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 …

QuLogicQuLogic

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); …

grimgrim

I'm torn on whether or not we should go with this or g_assert_reached...

grimgrim
QuLogic
  1. 
      
  2. libpurple/protocols/jabber/jabber.c (Diff revision 1)
     
     

    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?

    1. Great catch! This was indeed happening.

  3. 
      
grim
  1. 
      
  2. libpurple/protocols/jabber/jabber.c (Diff revision 1)
     
     

    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.

  3. 
      
belgin
grim
  1. 
      
  2. libpurple/protocols/jabber/jabber.c (Diff revision 2)
     
     

    I'm torn on whether or not we should go with this or g_assert_reached...

    1. lets just go with whats here...

  3. 
      
grim
  1. Ship It!
  2. 
      
grim
Review request changed

Status: Closed (submitted)

Loading...