Fix crash when cancelling an XMPP file transfer

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

belgin
pidgin/pidgin
release-2.x.y
PIDGIN-17189
pidgin
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
Fix crash when cancelling an XMPP file transfer
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...