View Issue Details

IDProjectCategoryView StatusLast Update
0005090unrealircdpublic2018-09-09 17:21
ReportersyzopAssigned Tosyzop 
PrioritynormalSeverityminorReproducibilitysometimes
Status resolvedResolutionfixed 
Product Version 
Target Version4.0.18Fixed in Version4.0.18 
Summary0005090: memory leak via dead_link()
Description==00:06:03:28.652 26593== 25 bytes in 1 blocks are definitely lost in loss record 559 of 1,646
==00:06:03:28.652 26593== at 0x4C27BB5: malloc (vg_replace_malloc.c:299)
==00:06:03:28.652 26593== by 0x6476989: strdup (strdup.c:42)
==00:06:03:28.652 26593== by 0x16B9B6: dead_link (send.c:87)
==00:06:03:28.652 26593== by 0x16BB26: send_queued (send.c:142)
==00:06:03:28.652 26593== by 0x16C1BE: sendbufto_one (send.c:282)
==00:06:03:28.652 26593== by 0x16F88F: vsendto_prefix_one (send.c:1207)
==00:06:03:28.652 26593== by 0x16F961: sendto_prefix_one (send.c:1224)
==00:06:03:28.652 26593== by 0x1705FF: sendto_message_one (send.c:1411)
==00:06:03:28.652 26593== by 0x81C3AF6: m_message (m_message.c:419)
==00:06:03:28.652 26593== by 0x81C3CBC: m_private (m_message.c:453)
==00:06:03:28.652 26593== by 0x13A007: parse (parse.c:454)
==00:06:03:28.652 26593== by 0x138A73: dopacket (packet.c:65)
==00:06:03:28.652 26593==
==00:06:03:28.652 26593== 25 bytes in 1 blocks are definitely lost in loss record 560 of 1,646
==00:06:03:28.652 26593== at 0x4C27BB5: malloc (vg_replace_malloc.c:299)
==00:06:03:28.652 26593== by 0x6476989: strdup (strdup.c:42)
==00:06:03:28.652 26593== by 0x16B9B6: dead_link (send.c:87)
==00:06:03:28.652 26593== by 0x16BB26: send_queued (send.c:142)
==00:06:03:28.652 26593== by 0x16C1BE: sendbufto_one (send.c:282)
==00:06:03:28.652 26593== by 0x16BD7A: vsendto_one (send.c:175)
==00:06:03:28.652 26593== by 0x16BD07: sendto_one (send.c:168)
==00:06:03:28.652 26593== by 0x169BB3: hunt_server (s_user.c:253)
==00:06:03:28.652 26593== by 0x85D2CC8: m_motd (m_motd.c:70)
==00:06:03:28.652 26593== by 0x13A007: parse (parse.c:454)
==00:06:03:28.652 26593== by 0x138A73: dopacket (packet.c:65)
==00:06:03:28.652 26593== by 0x1232F6: parse_client_queued (s_bsd.c:1235)
==00:06:03:28.652 26593==
==00:06:03:28.652 26593== 25 bytes in 1 blocks are definitely lost in loss record 561 of 1,646
==00:06:03:28.652 26593== at 0x4C27BB5: malloc (vg_replace_malloc.c:299)
==00:06:03:28.652 26593== by 0x6476989: strdup (strdup.c:42)
==00:06:03:28.652 26593== by 0x16B9B6: dead_link (send.c:87)
==00:06:03:28.652 26593== by 0x16BB26: send_queued (send.c:142)
==00:06:03:28.652 26593== by 0x16C1BE: sendbufto_one (send.c:282)
==00:06:03:28.652 26593== by 0x16BD7A: vsendto_one (send.c:175)
==00:06:03:28.652 26593== by 0x16BD07: sendto_one (send.c:168)
==00:06:03:28.652 26593== by 0x16237A: m_dalinfo (s_serv.c:352)
==00:06:03:28.652 26593== by 0x13A007: parse (parse.c:454)
==00:06:03:28.652 26593== by 0x138A73: dopacket (packet.c:65)
==00:06:03:28.652 26593== by 0x1232F6: parse_client_queued (s_bsd.c:1235)
==00:06:03:28.652 26593== by 0x1233BE: process_packet (s_bsd.c:1247)
==00:06:03:28.652 26593==
==00:06:03:28.652 26593== 25 bytes in 1 blocks are definitely lost in loss record 562 of 1,646
==00:06:03:28.652 26593== at 0x4C27BB5: malloc (vg_replace_malloc.c:299)
==00:06:03:28.652 26593== by 0x6476989: strdup (strdup.c:42)
==00:06:03:28.652 26593== by 0x16B9B6: dead_link (send.c:87)
==00:06:03:28.652 26593== by 0x16BB26: send_queued (send.c:142)
==00:06:03:28.652 26593== by 0x16C1BE: sendbufto_one (send.c:282)
==00:06:03:28.652 26593== by 0x16BD7A: vsendto_one (send.c:175)
==00:06:03:28.652 26593== by 0x16BD07: sendto_one (send.c:168)
==00:06:03:28.652 26593== by 0x6DA5D69: m_admin (m_admin.c:83)
==00:06:03:28.652 26593== by 0x13A007: parse (parse.c:454)
==00:06:03:28.652 26593== by 0x138A73: dopacket (packet.c:65)
==00:06:03:28.652 26593== by 0x1232F6: parse_client_queued (s_bsd.c:1235)
==00:06:03:28.652 26593== by 0x1233BE: process_packet (s_bsd.c:1247)
==00:06:03:28.652 26593==
Additional InformationNOTE: this bug was already present 2 days ago, before my mass cleanup ;)
TagsNo tags attached.
3rd party modules

Activities

syzop

2018-04-23 08:47

administrator   ~0020105

So: either it isn't freed via some exit_client function or the pointer is overwritten. Will check later...

syzop

2018-04-28 19:55

administrator   ~0020106

Seems to me that the latter is not true (not overwritten). There are two places where error_str is set.. in dead_link() and in fatal_ssl_error() and both check for a dead socket and return -1 if the socket is already marked as such (in which case no new strdup() will happen and there will be no overwrite)

As for not being freed:
free_client frees sptr->local->error_str with the only condition that sptr->local is not NULL (duh)
The free_client() function is called from remove_client_from_list() which is called from exit_one_client() unconditionally for everyone.

Anyway, sounds like it should be reproducible, so better try that...

syzop

2018-05-01 11:19

administrator   ~0020107

In check_deadsockets there is this line:
    cptr->flags &= ~FLAGS_DEADSOCKET; /* CPR. So we send the error. */

which makes sense, but if sending the error results in another error (not too uncommon) then this check in dead_link is skipped:

    if ((to->flags & FLAGS_DEADSOCKET) && to->local->error_str)
        return -1; /* already pending to be closed */

and we overwrite error_str:

    to->local->error_str = strdup(notice);

syzop

2018-05-01 11:26

administrator   ~0020108

I think the appropriate (and most conservative) fix would be:

if (to->flags & FLAGS_DEADSOCKET)
    return -1; /* already pending to be closed */

to->flags |= FLAGS_DEADSOCKET;

if (to->local->error_str)
    return -1; /* don't overwrite & don't send multiple times */

syzop

2018-09-09 17:19

administrator   ~0020305

declassify

syzop

2018-09-09 17:21

administrator   ~0020306

Fixed on June 11: https://github.com/unrealircd/unrealircd/commit/21af7689c043e25705059a3c0f972f0a9438623d
Which went into 4.0.18.

Issue History

Date Modified Username Field Change
2018-04-23 08:47 syzop New Issue
2018-04-23 08:47 syzop Note Added: 0020105
2018-04-23 08:47 syzop Status new => confirmed
2018-04-28 19:55 syzop Note Added: 0020106
2018-05-01 11:19 syzop Note Added: 0020107
2018-05-01 11:26 syzop Note Added: 0020108
2018-05-01 11:26 syzop View Status public => private
2018-09-09 17:19 syzop View Status private => public
2018-09-09 17:19 syzop Note Added: 0020305
2018-09-09 17:21 syzop Assigned To => syzop
2018-09-09 17:21 syzop Status confirmed => resolved
2018-09-09 17:21 syzop Resolution open => fixed
2018-09-09 17:21 syzop Fixed in Version => 4.0.18
2018-09-09 17:21 syzop Note Added: 0020306