> On Dec. 7, 2013, 9:48 a.m., Matt Jordan wrote: > > /branches/1.8/main/pbx.c, lines 4477-4491 > > <https://reviewboard.asterisk.org/r/3055/diff/4/?file=49180#file49180line4477> > > > > I don't think we need the extra lock here. > > > > The purpose of the contexts lock is to prevent changes to the dialplan > > while some portion of it is being used. In this case, > > pbx_substitute_variables will extract data from the ast_exten object e when > > it is called. > > > > Interestingly enough, variable substitution does not actually need the > > ast_exten object. pbx_substitute_variables acts as a wrapper for > > pbx_substitute_variables_helper, which is what actually does the work. > > pbx_substitute_variables_helper takes in char *. > > > > The bug here seems to be that we unlock the context lock before we > > should. Either pbx_substitute_variables should be removed and the values > > extracted in this function, or pbx_substitute_variables should be > > responsible for unlocking the contexts lock before calling the variable > > substiution function. > > Scott Griepentrog wrote: > The pbx_substitute_* functions *can* refer to the ast_exten object's data > member, as that's where the expression to evaluate is stored. This is the > very nature of the fault that valgrind is exposing - e->data has been free'd > because of a reload operation on another thread before a reference to it > occurring during pbx_substitute_variables and functions it calls. > > This lock accurately and reliably (per testing) prevents this failure. > > An option to eliminate the need for a lock would be to rewrite > pbx_substitute_variables() so that it is passed e->data instead of e, which > is the only element it references (and then passes into > pbx_substitute_variables_helper()). Then a copy of e->data could be put > locally on stack or other allocation prior to releaseing the global contexts > mutex, and then pass it to pbx_substitute_variables(). That could be more > performance impacting than the lock, depending on the speed of the lock. I > could run a test to determine which is the higher speed approach. > > > Matt Jordan wrote: > Some of the pbx_substitute_* functions pass pointers from an ast_exten > object to pbx_substitute_variables_helper_full. However, if you look at > pbx_substitute_variables_helper_full - which is what actually does the work > of substituting variables - it doesn't use ast_exten objects. It uses char * > to set the variable on the channel (or global variable). All other functions > are just wrappers on getting to that point. > > My point is that wrappers around pbx_substitute_variables_helper_full > should not be passing pointers to data on an ast_exten object to this > function. You're right that if you do that, you have to have something that > protects usage of the ast_exten objects while elements stored on them are > being manipulated by pbx_substitute_variables_helper_full - but if you first > copy the data onto some other storage location - such as a variable on the > stack - then you no longer need to hold the contexts lock during this > function. > > My point still stands: you don't need this mutex. It is duplicating an > existing lock, and by copying elements off of ast_exten and then calling a > variant of pbx_substitute_variables_helper_full that takes in char *, you > solve the problem without introducing more complicated logic.
We're agreed except where I was presuming that a read lock would be less overhead than allocating storage and copying the string. But I believe you're right, the lock isn't a valid option here. Reworking to use a stack allocation instead. - Scott ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3055/#review10328 ----------------------------------------------------------- On Dec. 10, 2013, 1:39 p.m., Scott Griepentrog wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3055/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2013, 1:39 p.m.) > > > Review request for Asterisk Developers and Matt Jordan. > > > Bugs: AST-1179 and AST-1246 > https://issues.asterisk.org/jira/browse/AST-1179 > https://issues.asterisk.org/jira/browse/AST-1246 > > > Repository: Asterisk > > > Description > ------- > > During dialplan execution in pbx_extension_helper(), the contexts global read > lock is used prevent changes to the dialplan. This patch puts a copy of > exten->data on the stack so that can be referenced safely during variable > substitution (expression evaluation) even if another thread is reloading > dialplan and has deleted ast_exten. > > > Diffs > ----- > > /branches/1.8/main/pbx.c 403615 > > Diff: https://reviewboard.asterisk.org/r/3055/diff/ > > > Testing > ------- > > Testsuite test 'dialplan_stress' (https://reviewboard.asterisk.org/r/3056/) > created to isolate this problem and test this fix. > > > Thanks, > > Scott Griepentrog > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
