[Home]

Summary:ASTERISK-10808: [patch] find_context_locked() must return with conlock held
Reporter:Yuri (ys)Labels:
Date Opened:2007-11-19 07:57:07.000-0600Date Closed:2007-11-27 16:12:23.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) pbx.c.diff
( 1) trunk89444_pbx.c.diff
Description:main/pbx.c: function find_context_locked() not neet to do ast_unlock_contexts()



****** ADDITIONAL INFORMATION ******

This is also true for SVN trunk.
Comments:By: Yuri (ys) 2007-11-19 07:58:09.000-0600

Missed '[' in Summary
By: Joshua C. Colp (jcolp) 2007-11-19 08:20:56.000-0600

This is only relevant to trunk.
By: Eliel Sardanons (eliel) 2007-11-19 18:17:20.000-0600

I don't see why this should be changed, if the context isn't find you are leaving the lock held more time than expected, and if the context is found there is no logic improvement.
Your change makes the code more clean, but we are returning a NULL pointer with a lock held in a structure that we will not use it.
BUT
If you think is better to cleanup the code, until commiting this patch you should also update the function documentation.
By: Yuri (ys) 2007-11-20 05:49:39.000-0600

eliel, is file noted, this is only for trunk (my mistake).

Now i up to trunk rev 89444 and etst again.

Try to add extension in empty context:

*CLI> dialplan add extension 1,1,hangup(), into test
   -- Added extension '1' priority 1 to test
[Nov 20 14:45:49] ERROR[36280]: /usr/home/asterisk/src/asterisk/include/asterisk/lock.h:915 _ast_rwlock_unlock: pbx.c line 7208 (ast_unlock_contexts): rwlock '&conlock' freed more times than we've locked!
Extension '1,1,hangup,' added into 'test' context
*CLI> [Nov 20 14:45:49] ERROR[36280]: /usr/home/asterisk/src/asterisk/include/asterisk/lock.h:932 _ast_rwlock_unlock: pbx.c line 7208 (ast_unlock_contexts): Error releasing rwlock: Invalid argument
By: Yuri (ys) 2007-11-20 05:56:18.000-0600

I upload ugly patch for trunk. :)
By: Eliel Sardanons (eliel) 2007-11-20 07:50:40.000-0600

I think you could solved this just returning NULL if no context is found, or i am wrong?
By: Yuri (ys) 2007-11-20 12:40:48.000-0600

True, anticipate "#ifdef NOTNOW" exist.
By: Eliel Sardanons (eliel) 2007-11-21 21:46:17.000-0600

tested patch trunk89444_pbx.c.diff and fixed the double unlock that breaks conlock
By: Yuri (ys) 2007-11-22 03:58:27.000-0600

murf.

Also check this:
http://bugs.digium.com/file_download.php?file_id=16559&type=bug
I post here, but issue ASTERISK-1123290 are closed.

In ast_add_extension2() function, must be used ast_free_ptr , instead of ast_free. Otherwise I got: "asterisk in free(): error: modified (chunk-) pointer
Abort trap (core dumped)" (MALLOC_DEBUG enabled).
By: Eliel Sardanons (eliel) 2007-11-22 06:50:39.000-0600

This is an example of one of the things that fail because of the double unlock.

eliel*CLI> dialplan add extension 1,1,Hangup(), into test
Extension '1,1,Hangup,' added into 'test' context
eliel*CLI> dialplan save
Failed to lock contexts list
Command 'dialplan save ' failed.
By: Digium Subversion (svnbot) 2007-11-27 16:12:23.000-0600

Repository: asterisk
Revision: 89792

U   trunk/main/pbx.c
U   trunk/pbx/pbx_config.c

------------------------------------------------------------------------
r89792 | murf | 2007-11-27 16:12:22 -0600 (Tue, 27 Nov 2007) | 1 line

closes issue ASTERISK-10808; missed the conditional unlock of the contexts when the hash table is used instead; also, used the ast_free_ptr as advised.
------------------------------------------------------------------------