Summary:	ASTERISK-06099: assorted pbx.c issues (some bugs, some don't know)
Reporter:	Luigi Rizzo (rizzo)	Labels:
Date Opened:	2006-01-16 14:01:21.000-0600	Date Closed:	2006-05-11 03:26:20
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:
Description:	I was carefully reviewing pbx.c today and found a number of issues, some of which are clearly bugs (more or less serious), others are possible bugs but i am not knowledgeable enough with the code to tell for sure. A word from some of the authors may shed some light on how to deal with them [BUG] --  pbx_extension_helper() has two ' switch(action) '      statements. In both, the 'default:' case returns without  releasing the lock; [BUG] -- __ast_pbx_run() near the beginning checks for c->pbx being  unset, but in case of error just prints a warning and then overwrites it.  Unless the check is unnecessary, the function should probably return with  an error here. [BUG] -- ast_context_remove_extension2()  the code here is very convoluted, but when it comes the case to remove the  first priority in the first extension in the context, and there are  other entries in such extension, we should set        peer->peer->next = exten->next;  as we do in the other case, otherwise the rest of the extension  chain is unreachable. [LIKELY BUG] -- ast_context_remove_extension2() line 2685 has this:                if (!peer->priority==PRIORITY_HINT)   which almost surely is not what you want; in particular, it is   NOT the same as (peer->priority != PRIORITY_HINT) because   ! has higher priority [LIKELY BUG] -- ast_add_extension2() when replacing an entry, runs   ast_change_hint() twice. Probably one is unnecessary; [UNCLEAR] __ast_pbx_run() in the block "/* Start by trying whatever..."   near the beginning, sets c->priority = 1 in line 2196;   shouldn't it go right after  the      ast_copy_string(c->exten, "s", sizeof(c->exten)) ?
Comments:	By: Tilghman Lesher (tilghman) 2006-01-16 14:48:26.000-0600 1)  Code is never reached. 2)  Should never happen.  A more appropriate action might be to PANIC. 3)  It's not clear which section you mean.  Revision and line number? 4)  Actually, the ! should probably be completely removed, as it otherwise wouldn't make sense in this context. 5)  I agree. 6)  I agree. By: Luigi Rizzo (rizzo) 2006-01-16 15:24:43.000-0600 0) thanks; this reply was quick! 1) if it is unreachable code we could as well remove the default   case (and probably use an enum as argument so the compiler won't   complain ? 2) ok 3) revision 8108 the block at line 2721 should do the same as the block   at line 2712, just using con->root instead of prev_exten->next; 4) i agree. On a related (cleanup of pbx.c) topic, what is the intended behaviour of ast_extension_close() ? Unlike ast_extension_match() i don't find it documented anywhere and the various cases overlap too much to let me derive a specification from the code. By: Tilghman Lesher (tilghman) 2006-01-16 15:59:20.000-0600 1)  Good plan. 2)  I suspect the reason why not PANIC was that it is better to continue on and process the call, leaking a little memory, rather than dropping the call and dumping core.  It would definitely be a programming error, so it's logged for later debugging. 3)  Don't think so.  For this, it is pointing the list pointer to the second item in the list, so the first item can be freed.  There's no need to set a second pointer. ast_extension_close() is a "This pattern may not be exactly right or exactly right yet, but it's close enough for our purposes" routine. By: Luigi Rizzo (rizzo) 2006-01-16 16:15:40.000-0600 3) the list should look like the picture below, with horizontal pointers being the "peer" field, and vertical pointers being the "next" field. The "next" pointers are always NULL (enforced in the insert routine) except in the first element of each horizontal chain. In the block at line 2721 we are removing [ext1-pri1] ("peer"), and we know that the horizontal chain is not empty (peer->peer is not NULL). So cur->root must point to [ext1-pri2] ("peer->peer") and [ext1-pri2]->next ("peer->peer->next") must point to [ext2-pri1] ("cur->root->next" which is the same as "peer->next") to preserve the vertical chain. Same as it is done in line 2712, except that there we remove [ext2-pri1] and link [ext2-pri2] to [ext3-pri1]. cur->root --> [ext1-pri1]->[ext1-pri2]->[ext1-pri3] ...                 |                 v               [ext2-pri1]->[ext2-pri2]-> ...                 |                 v               [ext3-pri1]-> ... By: Luigi Rizzo (rizzo) 2006-01-16 16:21:41.000-0600 re. ast_extension_close() - what is the criterium for "close enough" ? 1) match until the end of "data", 2) match until the end of "pattern", 3) match until the end of either "data" or "pattern" ? or none of the above ? By: Tilghman Lesher (tilghman) 2006-02-23 16:48:25.000-0600 1) Match until the end of "data". By: Andrey S Pankov (casper) 2006-03-01 22:07:46.000-0600 I found one more bug there... I hope to post details later this week. By: Andrey S Pankov (casper) 2006-03-02 14:44:49.000-0600 7) Channel mutex may be locked before ast_hangup() call resulting in "Device/resource busy" error on ast_mutex_destroy() call. That's trivial to find and fix that. You may look at ast_pbx_outgoing_exten for example... By: Tilghman Lesher (tilghman) 2006-03-02 14:51:11.000-0600 casper:  please file that as a _separate_ bug, unless rizzo cares to find and fix that within this patch.  This bug report is meant to fix several issues, not to be a universal category for filing all troubles with pbx.c. By: Serge Vecher (serge-v) 2006-05-01 15:12:39 does this still need to be open? -Housekeeping- By: Luigi Rizzo (rizzo) 2006-05-08 13:51:34 yes please keep it open so i can address individual items after i am done with the other extension match work. By: Luigi Rizzo (rizzo) 2006-05-11 03:26:19 all issues reported here are fixed in the current version.