[Asterisk-code-review] chan sip.c: Fix INVITE with replaces channel ref leak. (asterisk[master])

[George Joseph]

George Joseph has submitted this change and it was merged. ( https://gerrit.asterisk.org/8732 )

Change subject: chan_sip.c: Fix INVITE with replaces channel ref leak.
......................................................................

chan_sip.c: Fix INVITE with replaces channel ref leak.

Given the below call scenario:
A -> Ast1 -> B
C <- Ast2 <- B

1) A calls B through Ast1
2) B calls C through Ast2
3) B transfers A to C

When party B transfers A to C, B sends a REFER to Ast1 causing Ast1 to
send an INVITE with replaces to Ast2.  Ast2 then leaks a channel ref of
the channel between Ast1 and Ast2.

Channel ref leaks are easily seen in the CLI "core show channels" output.
The leaked channels appear in the output but you can do nothing with them
and they never go away unless you restart Asterisk.

* Properly account for the channel refs when imparting a channel into a
bridge when handling an INVITE with replaces in handle_invite_replaces().
The ast_bridge_impart() function steals a channel ref but the code didn't
account for how many refs were held by the code at the time and which ref
was stolen.

* Eliminated RAII_VAR in handle_invite_replaces().

ASTERISK-27740

Change-Id: I7edbed774314b55acf0067b2762bfe984ecaa9a4
---
M channels/chan_sip.c
1 file changed, 10 insertions(+), 3 deletions(-)

Approvals:
  Corey Farrell: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved; Approved for Submit



diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 138021e..c357785 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -25856,7 +25856,7 @@
 		int *nounlock, struct sip_pvt *replaces_pvt, struct ast_channel *replaces_chan)
 {
 	struct ast_channel *c;
-	RAII_VAR(struct ast_bridge *, bridge, NULL, ao2_cleanup);
+	struct ast_bridge *bridge;
 
 	if (req->ignore) {
 		return 0;
@@ -25872,6 +25872,7 @@
 	}
 	append_history(p, "Xfer", "INVITE/Replace received");
 
+	/* Get a ref to ensure the channel cannot go away on us. */
 	c = ast_channel_ref(p->owner);
 
 	/* Fake call progress */
@@ -25891,16 +25892,22 @@
 	ast_channel_unlock(replaces_chan);
 
 	if (bridge) {
+		/*
+		 * We have two refs of the channel.  One is held in c and the other
+		 * is notionally represented by p->owner.  The impart is "stealing"
+		 * the p->owner ref on success so the bridging system can have
+		 * control of when the channel is hung up.
+		 */
 		if (ast_bridge_impart(bridge, c, replaces_chan, NULL,
 			AST_BRIDGE_IMPART_CHAN_INDEPENDENT)) {
 			ast_hangup(c);
-			ast_channel_unref(c);
 		}
+		ao2_ref(bridge, -1);
 	} else {
 		ast_channel_move(replaces_chan, c);
 		ast_hangup(c);
-		ast_channel_unref(c);
 	}
+	ast_channel_unref(c);
 	sip_pvt_lock(p);
 	return 0;
 }

-- 
To view, visit https://gerrit.asterisk.org/8732
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7edbed774314b55acf0067b2762bfe984ecaa9a4
Gerrit-Change-Number: 8732
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180410/4f17538c/attachment.html>