Use-after-free when leaving invited (temporary) channels

[dx]

Originally reported by belak, with hipchat, but probably happens to any jabber invited channel.

Valgrind:

==3237== Invalid read of size 8
==3237==    at 0x125BB4: irc_channel_has_user (irc_channel.c:297)
==3237==    by 0x125A49: irc_channel_del_user (irc_channel.c:256)
==3237==    by 0x124249: bee_irc_chat_remove_user (irc_im.c:668)
==3237==    by 0x143F7F: imcb_chat_remove_buddy (bee_chat.c:232)
==3237==    by 0x14EA6F: jabber_chat_pkt_presence (conference.c:378)
==3237==    by 0x157841: jabber_pkt_presence (presence.c:110)
==3237==    by 0x140F76: xt_handle (xmltree.c:195)
==3237==    by 0x140DCF: xt_handle (xmltree.c:174)
==3237==    by 0x140D93: xt_handle (xmltree.c:169)
==3237==    by 0x14F376: jabber_feed_input (io.c:167)
==3237==    by 0x14F5C6: jabber_read_callback (io.c:237)
==3237==    by 0x136753: gaim_io_invoke (events_glib.c:86)
==3237==    by 0x537C90C: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.4400.1)
==3237==    by 0x537CCDF: ??? (in /usr/lib/libglib-2.0.so.0.4400.1)
==3237==    by 0x537D001: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.4400.1)
==3237==    by 0x1366C3: b_main_run (events_glib.c:59)
==3237==    by 0x134381: main (unix.c:170)
==3237==  Address 0x9fc2598 is 56 bytes inside a block of size 112 free'd
==3237==    at 0x4C2A65B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3237==    by 0x125719: irc_channel_free (irc_channel.c:163)
==3237==    by 0x1257A4: irc_channel_free_callback (irc_channel.c:181)
==3237==    by 0x537D3A2: ??? (in /usr/lib/libglib-2.0.so.0.4400.1)
==3237==    by 0x537C90C: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.4400.1)
==3237==    by 0x537CCDF: ??? (in /usr/lib/libglib-2.0.so.0.4400.1)
==3237==    by 0x537D001: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.4400.1)
==3237==    by 0x1366C3: b_main_run (events_glib.c:59)
==3237==    by 0x134381: main (unix.c:170)

What happens here, step by step:

1. User gets invite
2. User joins to temporary channel
3. User leaves channel
4. irc_cmd_part
4.1. irc_channel_del_user
4.1.1. irc_channel_free_soon (adds a timeout until the main loop is free)
4.2. When irc_channel_del_user returns 1, bee_irc_channel_chat_part
4.2.1. jabber_chat_leave, only sends a presence unavailable message to the server
4.2.2. "ic->data = NULL", comment says
       "/* Remove the reference. We don't need it anymore. */"
5. Main loop is free now, irc_channel_free_callback
5.1. irc_channel_free
5.1.1. bee_irc_channel_free, which finds that ic->data is null
       and can't set ic->data->ui_data to null.
5.2. the irc channel is freed, ic->data (struct groupchat) isn't
6. jabber_chat_pkt_presence (Reply from the jabber server arrives)
6.1. imcb_chat_remove_buddy
6.1.1. bee_irc_chat_remove_user
6.1.1.1. "irc_channel_t *ic = c->ui_data;", that's a non-null already freed value
6.1.1.2. irc_channel_del_user gets called, and at this point stuff breaks

4.2.2 seems to be the delicate part. That's irc_im.c:866, in bee_irc_channel_chat_part.

Relevant commits here, just click the three things and admire the diffs:

cc20520bd29c88d424f44ac5669c3026e9fd99fb

6963230895c26d054cc3543b03787118e2334c32

664bac38fcdf6889d3ceb29b73a0c3a4e27820ce

Second commit there references #780, which is suspiciously similar to this issue

FWIW, my fix idea was this:

-       /* Remove the reference. We don't need it anymore. */
-       ic->data = NULL;
+
+       if (!(ic->flags & IRC_CHANNEL_TEMP)) {
+               /* Remove the reference.
+                * We only need it for temp channels that are being freed */
+               ic->data = NULL;
+       }

After looking at those commits i linked above, i'm not so sure

[dx]

I just noticed, third commit (jgeboski's) is essentially just a revert of the second one (which fixes exactly the same issue i'm reporting here, which is the same as #780)

I'll have to dig irc logs for more context of the issue jgeboski was trying to fix there.

Having a serious case of deja vu right now.

[dx]

From my irc logs back when jgeboski was implementing the twitter thing and made that commit that introduced this regression:

<jgeboski> https://github.com/dequis/bitlbee-old/blob/68a11dd/irc_im.c#L906
<jgeboski> so, prpl->chat_leave() is support to free the groupchat
<jgeboski> since c, still has the free'd address below the call to
           chat_leave(), c->ui_data is still modified

==6977== Invalid write of size 8
==6977==    at 0x124C3A: bee_irc_channel_chat_part (irc_im.c:916)
==6977==    by 0x127D2D: irc_cmd_part (irc_commands.c:238)
==6977==    by 0x1297EC: irc_exec (irc_commands.c:798)
==6977==    by 0x120FDB: irc_process (irc.c:401)
==6977==    by 0x11B33C: bitlbee_io_current_client_read (bitlbee.c:222)
==6977==    by 0x136AA6: gaim_io_invoke (events_glib.c:82)
==6977==    by 0x5382924: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.4002.0)
==6977==    by 0x5382C87: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.4002.0)
==6977==    by 0x5382F49: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.4002.0)
==6977==    by 0x136A18: b_main_run (events_glib.c:58)
==6977==    by 0x134683: main (unix.c:178)
==6977==  Address 0x88a19d0 is 48 bytes inside a block of size 56 free'd
==6977==    at 0x4C2A20C: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==6977==    by 0x14669E: imcb_chat_free (bee_chat.c:79)
==6977==    by 0x18DE0D: twitter_chat_leave (twitter.c:737)
==6977==    by 0x124C22: bee_irc_channel_chat_part (irc_im.c:911)
==6977==    by 0x127D2D: irc_cmd_part (irc_commands.c:238)
==6977==    by 0x1297EC: irc_exec (irc_commands.c:798)
==6977==    by 0x120FDB: irc_process (irc.c:401)
==6977==    by 0x11B33C: bitlbee_io_current_client_read (bitlbee.c:222)
==6977==    by 0x136AA6: gaim_io_invoke (events_glib.c:82)
==6977==    by 0x5382924: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.4002.0)
==6977==    by 0x5382C87: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.4002.0)
==6977==    by 0x5382F49: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.4002.0)
==6977== 

(line numbers above apply to 68a11dd in bitlbee-old, or fed4f76 in the current repo)

[dx]

Applied my original patch idea in e1bea35419d0851323fb9d90b6284ded0bae802c

Let's see how this goes.