Merged revisions 201344 via svnmerge from

https://origsvn.digium.com/svn/asterisk/trunk

........
  r201344 | dvossel | 2009-06-17 10:20:26 -0500 (Wed, 17 Jun 2009) | 16 lines

  SIP registry ref count error

  During a sip reload, the list of sip_registry objects are
  supposed to be traversed, unlinked, and destroyed, but
  destruction never takes place due to a ref counting error.
  This causes a memory leak when registry items are removed
  from sip.conf and reloaded.  While the registries are removed
  from the global list, they are not removed from the scheduler.
  Because of this, SIP register attempts continue to be sent
  out for the item even though it may no longer be in the .conf.

  (closes issue #15295)
  Reported by: amorsen

  Review: https://reviewboard.asterisk.org/r/282/
........

git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.6.1@201365 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 9d4884c3a69474fe9c324947c439138681d5163b..e340a0cc90a6dc4ee968655e0785da86c52bcb75 100644 (file)
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -1601,12 +1601,6 @@ struct sip_peer {
  * or once the previously completed registration one expires).
  * The registration can be in one of many states, though at the moment
  * the handling is a bit mixed.
- *
- * XXX \todo Reference count handling for this object has some problems with
- * respect to scheduler entries.  The ref count is handled in some places,
- * but not all of them.  There are some places where references get leaked
- * when this scheduler entry gets cancelled.  At worst, this would cause
- * memory leaks on reloads if registrations get removed from configuration.
  */
 struct sip_registry {
        ASTOBJ_COMPONENTS_FULL(struct sip_registry,1,1);
@@ -10071,7 +10065,7 @@ static int sip_reregister(const void *data)
 
        r->expire = -1;
        __sip_do_register(r);
-       registry_unref(r, "unreg the re-registered");
+       registry_unref(r, "unref the re-register scheduled event");
        return 0;
 }
 
@@ -16531,7 +16525,7 @@ static int handle_response_register(struct sip_pvt *p, int resp, char *rest, str
                break;
        case 403:       /* Forbidden */
                ast_log(LOG_WARNING, "Forbidden - wrong password on authentication for REGISTER for '%s' to '%s'\n", p->registry->username, p->registry->hostname);
-               AST_SCHED_DEL(sched, r->timeout);
+               AST_SCHED_DEL_UNREF(sched, r->timeout, registry_unref(r, "reg ptr unref from handle_response_register 403"));
                r->regstate = REG_STATE_NOAUTH;
                p->needdestroy = 1;
                break;
@@ -16541,7 +16535,7 @@ static int handle_response_register(struct sip_pvt *p, int resp, char *rest, str
                if (r->call)
                        r->call = dialog_unref(r->call, "unsetting registry->call pointer-- case 404");
                r->regstate = REG_STATE_REJECTED;
-               AST_SCHED_DEL(sched, r->timeout);
+               AST_SCHED_DEL_UNREF(sched, r->timeout, registry_unref(r, "reg ptr unref from handle_response_register 404"));
                break;
        case 407:       /* Proxy auth */
                if (p->authtries == MAX_AUTHTRIES || do_register_auth(p, req, resp)) {
@@ -16560,8 +16554,7 @@ static int handle_response_register(struct sip_pvt *p, int resp, char *rest, str
        case 423:       /* Interval too brief */
                r->expiry = atoi(get_header(req, "Min-Expires"));
                ast_log(LOG_WARNING, "Got 423 Interval too brief for service %s@%s, minimum is %d seconds\n", p->registry->username, p->registry->hostname, r->expiry);
-               AST_SCHED_DEL(sched, r->timeout);
-               r->timeout = -1;
+               AST_SCHED_DEL_UNREF(sched, r->timeout, registry_unref(r, "reg ptr unref from handle_response_register 423"));
                if (r->call) {
                        r->call = dialog_unref(r->call, "unsetting registry->call pointer-- case 423");
                        p->needdestroy = 1;
@@ -16582,7 +16575,7 @@ static int handle_response_register(struct sip_pvt *p, int resp, char *rest, str
                if (r->call)
                        r->call = dialog_unref(r->call, "unsetting registry->call pointer-- case 479");
                r->regstate = REG_STATE_REJECTED;
-               AST_SCHED_DEL(sched, r->timeout);
+               AST_SCHED_DEL_UNREF(sched, r->timeout, registry_unref(r, "reg ptr unref from handle_response_register 479"));
                break;
        case 200:       /* 200 OK */
                if (!r) {
@@ -16599,7 +16592,7 @@ static int handle_response_register(struct sip_pvt *p, int resp, char *rest, str
                if (r->timeout > -1) {
                        ast_debug(1, "Cancelling timeout %d\n", r->timeout);
                }
-               AST_SCHED_DEL(sched, r->timeout);
+               AST_SCHED_DEL_UNREF(sched, r->timeout, registry_unref(r, "reg ptr unref from handle_response_register 200"));
                if (r->call)
                        r->call = dialog_unref(r->call, "unsetting registry->call pointer-- case 200");
                p->registry = registry_unref(p->registry, "unref registry entry p->registry");
@@ -16607,14 +16600,12 @@ static int handle_response_register(struct sip_pvt *p, int resp, char *rest, str
                sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT);
                /* p->needdestroy = 1; */
                
-               /* set us up for re-registering */
-               /* figure out how long we got registered for */
-               AST_SCHED_DEL(sched, r->expire);
-               
-               /* according to section 6.13 of RFC, contact headers override
-                  expires headers, so check those first */
+               /* set us up for re-registering
+                * figure out how long we got registered for
+                * according to section 6.13 of RFC, contact headers override
+                * expires headers, so check those first */
                expires = 0;
-               
+
                /* XXX todo: try to save the extra call */
                if (!ast_strlen_zero(get_header(req, "Contact"))) {
                        const char *contact = NULL;
@@ -16658,9 +16649,6 @@ static int handle_response_register(struct sip_pvt *p, int resp, char *rest, str
                                                                registry_unref(_data,"unref in REPLACE del fail"), 
                                                                registry_unref(r,"unref in REPLACE add fail"), 
                                                                registry_addref(r,"The Addition side of REPLACE")); 
-               /* it is clear that we would not want to destroy the registry entry if we just
-                  scheduled a callback and recorded it in there! */
-               /* since we never bumped the count, we shouldn't decrement it! registry_unref(r, "unref registry ptr r"); if this gets deleted, p->registry will be a bad pointer! */ 
        }
        return 1;
 }
@@ -22426,18 +22414,19 @@ static int reload_config(enum channelreloadreason reason)
                /* First, destroy all outstanding registry calls */
                /* This is needed, since otherwise active registry entries will not be destroyed */
                ASTOBJ_CONTAINER_TRAVERSE(&regl, 1, do {  /* regl is locked */
-                               
-                               /* avoid a deadlock in the unlink_all call, if iterator->call's (a dialog) registry entry
-                                  is this registry entry. In other words, if the dialog we are pointing to points back to 
-                                  us, then if we get a lock on this object, and try to UNREF it, we will deadlock, because
-                                  we already ... NO. This is not the problem. */
+
                                ASTOBJ_RDLOCK(iterator); /* now regl is locked, and the object is also locked */
                                if (iterator->call) {
                                        ast_debug(3, "Destroying active SIP dialog for registry %s@%s\n", iterator->username, iterator->hostname);
                                        /* This will also remove references to the registry */
                                        dialog_unlink_all(iterator->call, TRUE, TRUE);
                                        iterator->call = dialog_unref(iterator->call, "remove iterator->call from registry traversal");
-                                       /* iterator->call = sip_destroy(iterator->call); */
+                               }
+                               if (iterator->expire > -1) {
+                                       AST_SCHED_DEL_UNREF(sched, iterator->expire, registry_unref(iterator, "reg ptr unref from reload config"));
+                               }
+                               if (iterator->timeout > -1) {
+                                       AST_SCHED_DEL_UNREF(sched, iterator->timeout, registry_unref(iterator, "reg ptr unref from reload config"));
                                }
                                ASTOBJ_UNLOCK(iterator);