I haven't merged it into our system yet but I did download and review the change. Looks good. I'll let you know if I have issues. Thanks again. Kevin On 2/2/11 1:57 PM, "Anthony Minessale" <anthony.minessale@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@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@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@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@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@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@lists.freeswitch.org http://lists.freeswitch.org/mailman/listinfo/freeswitch-dev UNSUBSCRIBE:http://lists.freeswitch.org/mailman/options/freeswitch-dev http://www.freeswitch.org