[Asterisk-code-review] Bridge system: Fix memory leaks and double frees on impart f... (asterisk[13])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: Bridge system: Fix memory leaks and double frees on impart failure.
......................................................................


Bridge system: Fix memory leaks and double frees on impart failure.

You cannot reference the passed in features struct after calling
ast_bridge_impart().  Even if the call fails.

Change-Id: I902b88ba0d5d39520e670fb635078a367268ea21
---
M apps/confbridge/conf_chan_announce.c
M include/asterisk/features.h
M main/bridge.c
M main/core_unreal.c
M main/features.c
5 files changed, 12 insertions(+), 7 deletions(-)

Approvals:
  Mark Michelson: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Verified



diff --git a/apps/confbridge/conf_chan_announce.c b/apps/confbridge/conf_chan_announce.c
index 6596a85..ee46606 100644
--- a/apps/confbridge/conf_chan_announce.c
+++ b/apps/confbridge/conf_chan_announce.c
@@ -199,7 +199,6 @@
 	/* Impart the output channel into the bridge */
 	if (ast_bridge_impart(p->bridge, chan, NULL, features,
 		AST_BRIDGE_IMPART_CHAN_DEPARTABLE)) {
-		ast_bridge_features_destroy(features);
 		ast_channel_unref(chan);
 		return -1;
 	}
diff --git a/include/asterisk/features.h b/include/asterisk/features.h
index b63124c..a4aed5d 100644
--- a/include/asterisk/features.h
+++ b/include/asterisk/features.h
@@ -51,6 +51,7 @@
 /*!
  * \brief Bridge a call, and add additional flags to the bridge
  *
+ * \details
  * This does the same thing as \ref ast_bridge_call, except that once the bridge
  * is created, the provided flags are set on the bridge. The provided flags are
  * added to the bridge's flags; they will not clear any flags already set.
@@ -70,6 +71,7 @@
  * \brief Add an arbitrary channel to a bridge
  * \since 12.0.0
  *
+ * \details
  * The channel that is being added to the bridge can be in any state: unbridged,
  * bridged, answered, unanswered, etc. The channel will be added asynchronously,
  * meaning that when this function returns once the channel has been added to
@@ -87,11 +89,16 @@
  * \param features Features for this channel in the bridge
  * \param play_tone Indicates if a tone should be played to the channel
  * \param xfersound Sound that should be used to indicate transfer with play_tone
+ *
+ * \note The features parameter must be NULL or obtained by
+ * ast_bridge_features_new().  You must not dereference features
+ * after calling even if the call fails.
+ *
  * \retval 0 Success
  * \retval -1 Failure
  */
 int ast_bridge_add_channel(struct ast_bridge *bridge, struct ast_channel *chan,
-		struct ast_bridge_features *features, int play_tone, const char *xfersound);
+	struct ast_bridge_features *features, int play_tone, const char *xfersound);
 
 
 
diff --git a/main/bridge.c b/main/bridge.c
index dad7cfa..c6f5e49 100644
--- a/main/bridge.c
+++ b/main/bridge.c
@@ -2320,6 +2320,9 @@
 	if (chan_bridge) {
 		struct ast_bridge_channel *bridge_channel;
 
+		/* The channel is in a bridge so it is not getting any new features. */
+		ast_bridge_features_destroy(features);
+
 		ast_bridge_lock_both(bridge, chan_bridge);
 		bridge_channel = bridge_find_channel(chan_bridge, chan);
 
@@ -2342,9 +2345,6 @@
 		bridge_dissolve_check_stolen(chan_bridge, bridge_channel);
 		ast_bridge_unlock(chan_bridge);
 		ast_bridge_unlock(bridge);
-
-		/* The channel was in a bridge so it is not getting any new features. */
-		ast_bridge_features_destroy(features);
 	} else {
 		/* Slightly less easy case. We need to yank channel A from
 		 * where he currently is and impart him into our bridge.
@@ -2352,6 +2352,7 @@
 		yanked_chan = ast_channel_yank(chan);
 		if (!yanked_chan) {
 			ast_log(LOG_WARNING, "Could not gain control of channel %s\n", ast_channel_name(chan));
+			ast_bridge_features_destroy(features);
 			return -1;
 		}
 		if (ast_channel_state(yanked_chan) != AST_STATE_UP) {
diff --git a/main/core_unreal.c b/main/core_unreal.c
index e9b7a8d..f2404df 100644
--- a/main/core_unreal.c
+++ b/main/core_unreal.c
@@ -808,7 +808,6 @@
 	/* Impart the semi2 channel into the bridge */
 	if (ast_bridge_impart(bridge, chan, NULL, features,
 		AST_BRIDGE_IMPART_CHAN_INDEPENDENT)) {
-		ast_bridge_features_destroy(features);
 		ast_channel_unref(chan);
 		return -1;
 	}
diff --git a/main/features.c b/main/features.c
index 1810b15..b96cbd6 100644
--- a/main/features.c
+++ b/main/features.c
@@ -1104,7 +1104,6 @@
 		xfer_cfg ? xfer_cfg->xfersound : NULL);
 	ao2_cleanup(xfer_cfg);
 	if (bridge_add_failed) {
-		ast_bridge_features_destroy(peer_features);
 		ast_bridge_features_cleanup(&chan_features);
 		ast_bridge_destroy(bridge, 0);
 		goto done;

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I902b88ba0d5d39520e670fb635078a367268ea21
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>