Convert the bonjour protocol plugin to the new status api and contacts

Review Request #2427 — Created April 7, 2023 and submitted

Information

pidgin/pidgin
default

Reviewers

This has been tested on linux and windows. Apparently we don't build for mac right now. I didn't test file transfers at all though.

Created and account and sent some messages to my Pidgin2 account.

Summary ID
Convert the bonjour protocol plugin to the new status api and contacts
e212b1476be720af6c20d367916c7422568790e5
Description From Last Updated

When testing file transfer I got a SIGSEGV which I do not get without these changes. Here's the backtrace: #0 …

ivanhoeivanhoe

I'm getting a seemingly infinite loop when closing Pidgin3 with lots of g_object_get_data: assertion 'G_IS_OBJECT (object)' failed Steps to reproduce: …

ivanhoeivanhoe

My understanding is that g_object_get_data(G_OBJECT(contact), "bonjour-buddy"); will return a BonjourBuddy not a BonjourData so bd seems to be the wrong …

ivanhoeivanhoe

Leaks

QuLogicQuLogic

Leaks

QuLogicQuLogic

Leaks

QuLogicQuLogic

Leaks

QuLogicQuLogic

I think this person leaks since purple_contact_info_set_person is transfer none.

QuLogicQuLogic

'pointer is valid'

QuLogicQuLogic

Leaks

QuLogicQuLogic

Leaks

QuLogicQuLogic

Leaks

QuLogicQuLogic

Leaks

QuLogicQuLogic

Leaks

QuLogicQuLogic

Leaks

QuLogicQuLogic

Probably leaks, but I did not confirm all callers of _find_or_start_conversation.

QuLogicQuLogic

contact is passed to an async call here; should we ref it and unref in the callback?

QuLogicQuLogic

This doesn't change in the loop, I think, so can probably be lifted out?

QuLogicQuLogic

Sorry, missed that the last arg is a GDestroyNotify, so that should be passed instead of an unref in the …

QuLogicQuLogic

Should we add an ref here as well?

QuLogicQuLogic

Ditto.

QuLogicQuLogic

Missing contact unref here.

QuLogicQuLogic
grim
grim
grim
grim
ivanhoe
  1. 
      
  2. Show all issues

    My understanding is that g_object_get_data(G_OBJECT(contact), "bonjour-buddy"); will return a BonjourBuddy not a BonjourData so bd seems to be the wrong pointer in which to store that information.

    1. This looks like an errorenous change anyways so great eye there! The BonjourBuddy was the protocol data of a PurpleBuddy not PurpleConnection.

  3. 
      
grim
ivanhoe
  1. 
      
  2. Show all issues

    When testing file transfer I got a SIGSEGV which I do not get without these changes. Here's the backtrace:

    #0  0x00007ffff7a4c783 in g_type_check_instance_cast () at /lib/x86_64-linux-gnu/libgobject-2.0.so.0
    #1  0x00007ffff0448aea in XEP_XFER (ptr=0x5555555b51d0) at ../libpurple/protocols/bonjour/bonjour_ft.h:30
    #2  0x00007ffff0449078 in bonjour_si_xfer_find (bd=0x555556d04970, sid=0x555557279b20 "2", from=0x555556c5aea0 "driftwood@driftwood")
        at ../libpurple/protocols/bonjour/bonjour_ft.c:167
    #3  0x00007ffff044a7f2 in xep_bytestreams_parse (pc=0x555555649d20, packet=0x5555573e4ba0, contact=0x555556c08a60)
        at ../libpurple/protocols/bonjour/bonjour_ft.c:706
    #4  0x00007ffff044774a in xep_iq_parse (packet=0x5555573e4ba0, contact=0x555556c08a60) at ../libpurple/protocols/bonjour/xmpp.c:1359
    #5  0x00007ffff0444b04 in bonjour_xmpp_process_packet (contact=0x555556c08a60, packet=0x5555573e4ba0) at ../libpurple/protocols/bonjour/xmpp.c:382
    #6  0x00007ffff04487cf in bonjour_parser_element_end_libxml
        (user_data=0x555556bfe760, element_name=0x55555711c8b9 "iq", prefix=0x0, namespace=0x55555711c82e "jabber:client")
        at ../libpurple/protocols/bonjour/parser.c:142
    #7  0x00007ffff67ccf25 in  () at /lib/x86_64-linux-gnu/libxml2.so.2
    #8  0x00007ffff67da459 in  () at /lib/x86_64-linux-gnu/libxml2.so.2
    #9  0x00007ffff67db70b in xmlParseChunk () at /lib/x86_64-linux-gnu/libxml2.so.2
    #10 0x00007ffff0448999 in bonjour_parser_process
        (bconv=0x555556bfe760, buf=0x7ffff045dc00 <message> "<iq to='maat@maat' from='driftwood@driftwood' id='2' type='set'><query xmlns='http://jabber.org/protocol/bytestreams' sid='2' mode='tcp'><streamhost jid='2' host='fe80::8b35:34c6:6140:6e4b' port='3322"..., len=390) at ../libpurple/protocols/bonjour/parser.c:232
    #11 0x00007ffff0444e3c in _client_socket_handler (stream=0x55555705b670, data=0x555556bfe760) at ../libpurple/protocols/bonjour/xmpp.c:459
    #12 0x00007ffff7e1267f in g_main_context_dispatch () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
    
  3. 
      
grim
ivanhoe
  1. 
      
  2. Show all issues

    I'm getting a seemingly infinite loop when closing Pidgin3 with lots of g_object_get_data: assertion 'G_IS_OBJECT (object)' failed

    Steps to reproduce: * in Pidgin3 open a conversation with a Bonjour contact * Conversation -> Send File... * Choose a file and send it * Accept the file on the other end (in my case with Pidgin2) * Pidgin3 reports File transfer failed (which it does also without the changes of this RR) * Close all dialogs and the Pidgin3

    1. Just to be clear, this is still happening with the latest patch? I went through and I think I got all of the BonjourData's I accidentally changed.

    2. Yes it is. I also checked and I did not find any other wrongly assigned BonjourData. Can you not reproduce it?

    3. I just managed to reproduce it twice outside of gdb, but I can't seem to reproduce it in gdb...

  3. 
      
QuLogic
  1. 
      
  2. libpurple/protocols/bonjour/bonjour.c (Diff revision 5)
     
     
    Show all issues

    Leaks

  3. libpurple/protocols/bonjour/bonjour.c (Diff revision 5)
     
     
    Show all issues

    Leaks

  4. Show all issues

    Leaks

  5. libpurple/protocols/bonjour/buddy.c (Diff revision 5)
     
     
    Show all issues

    Leaks

  6. libpurple/protocols/bonjour/buddy.c (Diff revision 5)
     
     
     
     
    Show all issues

    I think this person leaks since purple_contact_info_set_person is transfer none.

  7. Show all issues

    Leaks

  8. Show all issues

    Leaks

  9. Show all issues

    Leaks

  10. Show all issues

    Leaks

  11. Show all issues

    Leaks

  12. libpurple/protocols/bonjour/xmpp.c (Diff revision 5)
     
     
    Show all issues

    Leaks

  13. libpurple/protocols/bonjour/xmpp.c (Diff revision 5)
     
     
    Show all issues

    Probably leaks, but I did not confirm all callers of _find_or_start_conversation.

  14. libpurple/protocols/bonjour/xmpp.c (Diff revision 5)
     
     
    Show all issues

    This doesn't change in the loop, I think, so can probably be lifted out?

  15. 
      
grim
grim
QuLogic
  1. 
      
  2. libpurple/protocols/bonjour/buddy.c (Diff revisions 5 - 7)
     
     
    Show all issues

    'pointer is valid'

  3. libpurple/protocols/bonjour/xmpp.c (Diff revisions 5 - 7)
     
     
    Show all issues

    contact is passed to an async call here; should we ref it and unref in the callback?

  4. libpurple/protocols/bonjour/xmpp.c (Diff revision 7)
     
     
    Show all issues

    Should we add an ref here as well?

  5. 
      
grim
QuLogic
  1. 
      
  2. libpurple/protocols/bonjour/xmpp.c (Diff revisions 7 - 8)
     
     
    Show all issues

    Sorry, missed that the last arg is a GDestroyNotify, so that should be passed instead of an unref in the callback.

  3. libpurple/protocols/bonjour/xmpp.c (Diff revisions 7 - 8)
     
     
    Show all issues

    Ditto.

  4. libpurple/protocols/bonjour/xmpp.c (Diff revisions 7 - 8)
     
     
    Show all issues

    Missing contact unref here.

  5. 
      
grim
QuLogic
  1. Ship It!
  2. 
      
grim
Review request changed
Status:
Completed