[Freeswitch-dev] Switch_core_get_variable

[Kevin Snow]

Anthony - You're awesome!

I'll sync up and take a look.

Thanks!

Kevin



On 2/2/11 1:57 PM, "Anthony Minessale" <anthony.minessale at gmail.com> wrote:

> I read this email then I went and solved it.
> I see some replies came in.
> 
> Leaning on global vars is a bit of abuse but we can't afford to be
> susceptible to a crash.
> I have modified 26 files and changed all places to use one of the
> following new functions:
> 
> char *switch_core_get_variable_dup(const char *var)
> char *switch_core_get_variable_pdup(const char *var, switch_memory_pool_t
> *pool)
> 
> one strdups the value, the other pool dups it.
> 
> All that I ask is that you now review it to make sure I did not mess
> up anything.
> 
> 
> 
> On Wed, Feb 2, 2011 at 3:48 PM, Kevin Snow <kevin.snow at ooma.com> wrote:
>> 
>> I can enter it into jira.
>> 
>> I¹m not sure if I agree with your assessment. There is already a mutex
>> lock/unlock around switch_core_get_variable¹s internals today, no additional
>> risk of deadlock. As for switch_channel_get_variable, which also has a mutex
>> of it¹s own, is already using switch_core_session_strdup before returning.
>> I¹m proposing the equivalent to switch_channel_get_variable_dup be added for
>> switch_core_get_variable. To me it seems we must have this if you want to
>> reliably read a variable that might change at runtime.
>> 
>> Kevin
>> 
>> 
>> 
>> 
>> On 2/2/11 12:01 PM, "Steven Ayre" <steveayre at gmail.com> wrote:
>> 
>> I found a hole in the way we handle core variables in FS.
>> Switch_core_get_variable does the lookup and returns the found pointer. In
>> Switch_core_set_variable the first step it does is look up the variable and
>> free it (if it exists). This would free it out from under another that has
>> just done a get on it. This is how I stumbled on this.
>> 
>> The correct place to report bugs is the bug
>> tracker: http://jira.freeswitch.org/
>> 
>> It's a known problem. Basically the answer is you shouldn't really be
>> changing core variables after FS has loaded, at least not while handling
>> traffic. If you're doing so as part of your application, you're doing it
>> wrong. mod_db is probably better suited to what you want to do.
>> 
>> Is the right fix is to add a switch_core_get_variable_dup that dups the
>> string while in the mutex protection? I realize in the core case this will
>> require the caller to then free the returned memory, but this is better than
>> getting a bad pointer. This is analogous to the switch_channel_get_variable
>> and it¹s _dup implementation, although it dups it to session memory.
>> 
>> Just my 2 cents (I'm not a core developer)... That could be done. But it's
>> also slower because it means dynamically assigning memory and then copying
>> the string. And it can't be cached. And the mutex will make all calls block,
>> not just the single call as with the channel variable - which might possibly
>> introduce a risk of deadlock.
>> 
>> -Steve
>> 
>> 
>> 
>> On 2 February 2011 19:39, Kevin Snow <kevin.snow at ooma.com> wrote:
>> 
>> I found a hole in the way we handle core variables in FS.
>> Switch_core_get_variable does the lookup and returns the found pointer. In
>> Switch_core_set_variable the first step it does is look up the variable and
>> free it (if it exists). This would free it out from under another that has
>> just done a get on it. This is how I stumbled on this.
>> 
>> Is the right fix is to add a switch_core_get_variable_dup that dups the
>> string while in the mutex protection? I realize in the core case this will
>> require the caller to then free the returned memory, but this is better than
>> getting a bad pointer. This is analogous to the switch_channel_get_variable
>> and it¹s _dup implementation, although it dups it to session memory.
>> 
>> Switch_channel_get_variable¹s ability to peak through to the core variables
>> is susceptible to this. If a core variable is changed after
>> switch_channel_get_variable looks it up but before it dups to the session
>> pool, it¹ll have a bad pointer.
>> 
>> Kevin Snow
>> Ooma, Inc
>> 
>> _______________________________________________
>> FreeSWITCH-dev mailing list
>> FreeSWITCH-dev at lists.freeswitch.org
>> http://lists.freeswitch.org/mailman/listinfo/freeswitch-dev
>> UNSUBSCRIBE:http://lists.freeswitch.org/mailman/options/freeswitch-dev
>> http://www.freeswitch.org
>> 
>> 
>> 
>> ________________________________
>> _______________________________________________
>> FreeSWITCH-dev mailing list
>> FreeSWITCH-dev at lists.freeswitch.org
>> http://lists.freeswitch.org/mailman/listinfo/freeswitch-dev
>> UNSUBSCRIBE:http://lists.freeswitch.org/mailman/options/freeswitch-dev
>> http://www.freeswitch.org
>> 
>> 
>> _______________________________________________
>> FreeSWITCH-dev mailing list
>> FreeSWITCH-dev at lists.freeswitch.org
>> http://lists.freeswitch.org/mailman/listinfo/freeswitch-dev
>> UNSUBSCRIBE:http://lists.freeswitch.org/mailman/options/freeswitch-dev
>> http://www.freeswitch.org
>> 
>> 
> 
>