SIP registry ref count error
authorDavid Vossel <dvossel@digium.com>
Wed, 17 Jun 2009 15:20:26 +0000 (15:20 +0000)
committerDavid Vossel <dvossel@digium.com>
Wed, 17 Jun 2009 15:20:26 +0000 (15:20 +0000)
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/trunk@201344 65c4cc65-6c06-0410-ace0-fbb531ad65f3

channels/chan_sip.c

index 5cbf028..9e3e238 100644 (file)
@@ -2062,12 +2062,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);
@@ -11223,7 +11217,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;
 }
 
@@ -18172,7 +18166,7 @@ static int handle_response_register(struct sip_pvt *p, int resp, const char *res
                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;
                pvt_set_needdestroy(p, "received 403 response");
                break;
@@ -18182,7 +18176,7 @@ static int handle_response_register(struct sip_pvt *p, int resp, const char *res
                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)) {
@@ -18201,8 +18195,7 @@ static int handle_response_register(struct sip_pvt *p, int resp, const char *res
        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");
                        pvt_set_needdestroy(p, "received 423 response");
@@ -18223,7 +18216,7 @@ static int handle_response_register(struct sip_pvt *p, int resp, const char *res
                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) {
@@ -18240,7 +18233,7 @@ static int handle_response_register(struct sip_pvt *p, int resp, const char *res
                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");
@@ -18248,14 +18241,12 @@ static int handle_response_register(struct sip_pvt *p, int resp, const char *res
                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;
@@ -18299,9 +18290,6 @@ static int handle_response_register(struct sip_pvt *p, int resp, const char *res
                                                                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;
 }
@@ -24319,18 +24307,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);
                } while(0));