diff options
author | Russell Bryant <russell@russellbryant.com> | 2009-04-08 12:35:57 +0000 |
---|---|---|
committer | Russell Bryant <russell@russellbryant.com> | 2009-04-08 12:35:57 +0000 |
commit | 0fab071d13a0abe98981403fc8da078a2595b6bb (patch) | |
tree | 196c9daa0f5f9c1a1d1fd20f6f249d2f276610f8 /channels | |
parent | b289374dfe7a7d9a2fdf655f3c070c548e7b1e94 (diff) |
Update some comments and resolve potential memory corruption in chan_sip.
While browsing chan_sip the other day, I noticed this dangerous code in
dialog_needdestroy(). This function is an ao2_callback. It is absolutely
_not_ okay to unlock the container from within this function. It's also not
clear why it was useful. Given that it could cause memory corruption, I have
removed it.
There was also a TODO comment left describing a potential implementation of
an improvement to the needdestroy handling. I'm not convinced that what was
described is the best choice here, so I have briefly described the way that
this function is used today that could be improved.
git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@186928 65c4cc65-6c06-0410-ace0-fbb531ad65f3
Diffstat (limited to 'channels')
-rw-r--r-- | channels/chan_sip.c | 41 |
1 files changed, 13 insertions, 28 deletions
diff --git a/channels/chan_sip.c b/channels/chan_sip.c index b73d9138d..d75f9507c 100644 --- a/channels/chan_sip.c +++ b/channels/chan_sip.c @@ -14341,41 +14341,26 @@ static void cleanup_stale_contexts(char *new, char *old) } } -/*! \brief this func is used with ao2_callback to unlink/delete all dialogs that - are marked needdestroy. It will return CMP_MATCH for candidates, and they - will be unlinked - - TODO: Implement a queue and instead of marking a dialog - to be destroyed, toss it into the queue. Have a separate - thread do the locking and destruction */ +/*! + * \brief Match dialogs that need to be destroyed + * + * \details This is used with ao2_callback to unlink/delete all dialogs that + * are marked needdestroy. It will return CMP_MATCH for candidates, and they + * will be unlinked. + * + * \todo Re-work this to improve efficiency. Currently, this function is called + * on _every_ dialog after processing _every_ incoming SIP/UDP packet, or + * potentially even more often when the scheduler has entries to run. + */ static int dialog_needdestroy(void *dialogobj, void *arg, int flags) { struct sip_pvt *dialog = dialogobj; time_t *t = arg; - - /* log_show_lock(ao2_object_get_lockaddr(dialog)); this is an example of how to use log_show_lock() */ if (sip_pvt_trylock(dialog)) { - /* In very short-duration calls via sipp, - this path gets executed fairly frequently (3% or so) even in low load - situations; the routines we most commonly fight for a lock with: - sip_answer (7 out of 9) - sip_hangup (2 out of 9) - */ - ao2_unlock(dialogs); - usleep(1); - ao2_lock(dialogs); - - /* I had previously returned CMP_STOP here; but changed it to return - a zero instead, because there is no need to stop at the first sign - of trouble. The old algorithm would restart in such circumstances, - but there is no need for this now in astobj2-land. We'll - just skip over this element this time, and catch it on the - next traversal, which is about once a second or so. See the - ao2_callback call in do_monitor. We don't want the number of - dialogs to back up too much between runs. - */ + /* Don't block the monitor thread. This function is called often enough + * that we can wait for the next time around. */ return 0; } |