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/trunk@201344 65c4cc65-6c06-0410-ace0-fbb531ad65f3

1 files changed, 18 insertions, 29 deletions

diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 5cbf028ad..9e3e238b0 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -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));