[asterisk-dev] [Code Review] 2930: pbx.c: eliminate confusion when matching caller id and prevent removal of extension entry still in hashtable

Guenther Kelleter reviewboard at asterisk.org
Wed Oct 23 04:39:55 CDT 2013


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2930/#review9980
-----------------------------------------------------------


I don't know whats wrong here, but I can crash asterisk with this patch on top of asterisk 11.2-cert2/Rev:397839 after a while in strcmp() with the dialplan from AST-1235 and executing "core restart gracefully" every 5 seconds:

(gdb) bt
#0  0x40088c10 in strcmp () from /lib/libc.so.6
#1  0x08144206 in ast_context_remove_extension_callerid2 (con=0x41418ed0, extension=0xba9ff014 "60.", priority=1,
    callerid=0xba9fefc4 "", matchcallerid=0, registrar=0x0, already_locked=1) at pbx.c:6976
#2  0x08152ab1 in __ast_context_destroy (list=0x88a3f78, contexttab=0x41414d00, con=0x0, registrar=0x413115a0 "pbx_config")
    at pbx.c:10607
#3  0x08152d21 in ast_context_destroy (con=0x0, registrar=0x413115a0 "pbx_config") at pbx.c:10673
#4  0x4130d88f in unload_module () at pbx_config.c:1407
#5  0x08119186 in ast_module_shutdown () at loader.c:519
#6  0x0807f726 in really_quit (num=0, niceness=-1163923436, restart=1) at asterisk.c:1774
#7  0x0807fc05 in handle_restart_gracefully (e=0xba9ff014, cmd=0, a=0x0) at asterisk.c:1692
#8  0x080cb91c in ast_cli_command_full (uid=0, gid=0, fd=50, s=0xba9ff3c4 "core restart gracefully") at cli.c:2560
#9  0x080cbac9 in ast_cli_command_multiple_full (uid=0, gid=0, fd=50, size=24, s=0xba9ff863 "core restart gracefully")
    at cli.c:2583
#10 0x0807d6aa in netconsole (vconsole=0x82276a0) at asterisk.c:1336
#11 0x0819580b in dummy_start (data=0xba9ff014) at utils.c:1028
#12 0x40515d92 in pthread_detach () from /lib/libpthread.so.0
#13 0x400e6e4a in clone () from /lib/libc.so.6
(gdb) info registers
eax            0x0      0
ecx            0x0      0                           <------
edx            0xba9ff014       -1163923436
ebx            0x414197a8       1094817704
esp            0xba9feae4       0xba9feae4
ebp            0xba9feaec       0xba9feaec
esi            0x0      0
edi            0x41419030       1094815792
eip            0x40088c10       0x40088c10
eflags         0x10202  66050
cs             0x73     115
ss             0x7b     123
ds             0x7b     123
es             0x7b     123
fs             0x0      0
gs             0x0      0
(gdb) disas
Dump of assembler code for function strcmp:
0x40088c00 <strcmp+0>:  push   %ebp
0x40088c01 <strcmp+1>:  mov    %esp,%ebp
0x40088c03 <strcmp+3>:  push   %eax
0x40088c04 <strcmp+4>:  push   %eax
0x40088c05 <strcmp+5>:  mov    0x8(%ebp),%ecx
0x40088c08 <strcmp+8>:  mov    0xc(%ebp),%edx
0x40088c0b <strcmp+11>: nop
0x40088c0c <strcmp+12>: lea    0x0(%esi),%esi
0x40088c10 <strcmp+16>: mov    (%ecx),%al            <------
0x40088c12 <strcmp+18>: inc    %ecx

(gdb) bt full 4
#0  0x40088c10 in strcmp () from /lib/libc.so.6
No symbol table info available.
#1  0x08144206 in ast_context_remove_extension_callerid2 (con=0x41418ed0, extension=0xba9ff014 "60.", priority=1,
    callerid=0xba9fefc4 "", matchcallerid=0, registrar=0x0, already_locked=1) at pbx.c:6976
        x = (struct match_char *) 0x0
        exten = (struct ast_exten *) 0x0
        prev_exten = (struct ast_exten *) 0x0
        peer = (struct ast_exten *) 0x414197a8
        ex = {exten = 0xba9feb14 "60.", matchcid = 0, cidmatch = 0xba9fefc4 "", priority = 1, label = 0x413115a0 "pbx_config",
  parent = 0xba9ff014, app = 0xba9fef5c "|?\237?\024?\237??\216AA", cached_app = 0x811c32b, data = 0x81db87a, datad = 0x296a,
  peer = 0x81de187, peer_table = 0x3, peer_label_table = 0x0,
  registrar = 0x81e0e48 "Remove %s/%s/%d, registrar=%s; con=%s(%p); con->root=%p\n", next = 0xba9fef78,
  stuff = 0xba9fef50 "\001"}
        exten2 = (struct ast_exten *) 0x0
        exten3 = (struct ast_exten *) 0x0
        dummy_name = "60.\000$\016R@@?Q at X\016R@@\016R@`?Q at H\016R@\000\000\000\000\002\000\000\000\000\000\000\000nD\b@\025\000\000\000.?Q@??Q@?\002\216@\020\000p@\204?\237???Q@\020\000p@\020\000p@\204?\237??sQ@ \000p@\020\000p at d\202\023@?rQ at d\202\023@?\002\216@\234?\237?\227g\b@\020\000p@\017\000\000\000\204?\237?\000\001\000\000??\237?\006P\030\b?\002\216@\236?Q@?\002\216@\204?\237?\236?Q@\016?Q@??\237?\016?Q at H\230\037\bX\213Q at p\030\000\000??Q@\210w\037\b*\227AA"...
        previous_peer = (struct ast_exten *) 0x41418f50
        next_peer = (struct ast_exten *) 0x0
        found = 1
        __PRETTY_FUNCTION__ = "ast_context_remove_extension_callerid2"
#2  0x08152ab1 in __ast_context_destroy (list=0x88a3f78, contexttab=0x41414d00, con=0x0, registrar=0x413115a0 "pbx_config")
    at pbx.c:10607
        extension = "60.\000??Q@\020\000p@\020\000p at D?\237??sQ@??\237???Q@??\237?\000\212\037\b\\?\237??oQ@\\?\237?\227g\b@\020\000p at d\202\023@\b?E\b??D\bl?\237?g \017@"
        cidmatch = "\000\016R@`?Q at H\016R@??\023@\236?Q@$\016R@@?Q at X\016R@@\016R@`?Q at H\016R@\020\000p@$?\237?\000\000\000\000nD\b@\035\000\000\000.?Q@\236?Q@?LAA\020\000p@"
        end_traversal = 1
        tmp = (struct ast_context *) 0x41418ed0
---Type <return> to continue, or q <return> to quit---
        tmpl = (struct ast_context *) 0x88a3f78
        exten_item = (struct ast_exten *) 0x0
        prio_item = (struct ast_exten *) 0x41419a38
        __PRETTY_FUNCTION__ = "__ast_context_destroy"
#3  0x08152d21 in ast_context_destroy (con=0x0, registrar=0x413115a0 "pbx_config") at pbx.c:10673
No locals.
(More stack frames follow...)

-----------------------------------------------
And this crash also happens very often in this test:

(gdb) bt
#0  0x0814879d in ast_unregister_application (app=0x4067fc7f "Monitor") at pbx.c:8403
#1  0x4067f8d0 in unload_module () at res_monitor.c:939
#2  0x08119186 in ast_module_shutdown () at loader.c:519
#3  0x0807f726 in really_quit (num=0, niceness=1099017792, restart=1) at asterisk.c:1774
#4  0x0807fc05 in handle_restart_gracefully (e=0x4181ae40, cmd=1099007104, a=0x41818480) at asterisk.c:1692
#5  0x080cb91c in ast_cli_command_full (uid=0, gid=0, fd=50, s=0xba3ff3c4 "core restart gracefully") at cli.c:2560
#6  0x080cbac9 in ast_cli_command_multiple_full (uid=0, gid=0, fd=50, size=24, s=0xba3ff863 "core restart gracefully")
    at cli.c:2583
#7  0x0807d6aa in netconsole (vconsole=0x82276a0) at asterisk.c:1336
#8  0x0819580b in dummy_start (data=0x4181ae40) at utils.c:1028
#9  0x40515d92 in pthread_detach () from /lib/libpthread.so.0
#10 0x400e6e4a in clone () from /lib/libc.so.6

this is here (comments by me):
static void unreference_cached_app(struct ast_app *app)
{
        struct ast_context *context = NULL;
        struct ast_exten *eroot = NULL, *e = NULL;

        ast_rdlock_contexts();        // take a read-lock...
        while ((context = ast_walk_contexts(context))) {
                while ((eroot = ast_walk_context_extensions(context, eroot))) {
                        while ((e = ast_walk_extension_priorities(eroot, e))) {
//line 8304---->                if (e->cached_app == app)
                                        e->cached_app = NULL;    // ...and then write???
                        }
                }
        }
        ast_unlock_contexts();

        return;
}

There is write operations under a read-lock? 


- Guenther Kelleter


On Oct. 21, 2013, 11:18 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2930/
> -----------------------------------------------------------
> 
> (Updated Oct. 21, 2013, 11:18 p.m.)
> 
> 
> Review request for Asterisk Developers and Guenther Kelleter.
> 
> 
> Bugs: AST-1235
>     https://issues.asterisk.org/jira/browse/AST-1235
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch fixes a bug report where restarting Asterisk repeatedly would cause a crash due to accidentally deleting extensions that were still referenced in the context's root_table.  This occurred due to confusion over matching or not matching extensions based on callerid, which is also being cleared up via an enum'd third state.
> 
> There are two values in an extension entry:  matchcid which says whether cid should be matched, and cidmatch which is the string value to match.  The matchcid value was previously non-zero when cidmatch was signficant.  This is important since a null length callerid match must be kept distinct from a match-anything condition.
> 
> This patch adds an enum to define the AST_EXT_MATCHCID_OFF (0) or AST_EXT_MATCHCID_ON (1) behaviors, and adds AST_EXT_MATCHCID_ANY (2) which allows matching to both exten->matchcid=0 and exten->matchcid=1 cases.  For example, all exten's with the same exten (number) can be deleted with ANY, but when looking for a dialplan match it must match to matchcid, and if set to ON, further match cidmatch.
> 
> The logic to match extensions has been reworked to implement these three types, and then matchcid values updated to use the enum for clarity.
> 
> The most significant change is in ast_context_remove_extension_callerid2() where after removing extensions from the hashtable, the deletion would destroy the extensions.  This was deleting exten's that didn't have matchcid set but matched a null cidmatch, where they hadn't been removed from the hashtable.
> 
> - (!callerid || !matchcallerid || (matchcallerid && !strcmp(peer->cidmatch, callerid))) &&
> + (!callerid || (!matchcallerid && !peer->matchcid) || (matchcallerid && peer->matchcid && !strcmp(peer->cidmatch, callerid))) &&
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/include/asterisk/pbx.h 400531 
>   /branches/1.8/main/pbx.c 400531 
> 
> Diff: https://reviewboard.asterisk.org/r/2930/diff/
> 
> 
> Testing
> -------
> 
> This patch has been tested with the testsuite additions dialplan_reload and pbx/callerid_match (posted separately) to insure that the crash case has been resolved without any other unwanted changes in behavior.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131023/0bcb8a35/attachment-0001.html>


More information about the asterisk-dev mailing list