[Asterisk-code-review] bridge.c: Fixed race condition during attended transfer (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: bridge.c: Fixed race condition during attended transfer
......................................................................


Patch Set 2: Code-Review-1

(7 comments)

https://gerrit.asterisk.org/#/c/840/2/include/asterisk/bridge.h
File include/asterisk/bridge.h:

Line 512:  * \note This given bridge must be unlocked when calling this function.
The given bridge...


https://gerrit.asterisk.org/#/c/840/2/main/bridge.c
File main/bridge.c:

Line 1718: 		if (!res) {
         : 			res = bridge_channel_internal_wait(&cond);
         : 		}
This is changing the meaning of the return value!
Before if -1 is returned the caller still has full control over the returned channel.  Now the caller doesn't know if he still has control of the channel and doesn't know if he still has ownership of the channel ref.  Also this function's own cleanup code is confused about what the return value means and could cleanup things that is should not do.

You need to distinguish between
1) A successful impart
2) A failed impart that the caller still has control over the channel
3) A failed impart because the channel failed to get into the bridge but the caller no longer has control over the channel.


Line 2359: 
Superfluous blank line deletion that groups nonrelated code lines together.


Line 4004: 	/*
         : 	 * Since bridges need to be unlocked before entering ast_bridge_impart and
         : 	 * core_local may call into it then bridge2 needs to be unlocked here
         : 	 */
         : 	if (bridge2) {
         : 		ast_bridge_unlock(bridge2);
         : 	}
         : 	if (ast_call(local_chan, dest, 0)) {
         : 		ast_hangup(local_chan);
         : 		if (bridge2) {
         : 			ast_bridge_lock(bridge2);
         : 		}
         : 		return AST_BRIDGE_TRANSFER_FAIL;
         : 	}
         : 	if (bridge2) {
         : 		ast_bridge_lock(bridge2);
         : 	}
         : 
         : 	/* Get a ref for use later since this one is being stolen */
         : 	ao2_ref(local_chan, +1);
         : 	ast_bridge_unlock(bridge1);
         : 	if (ast_bridge_impart(bridge1, local_chan, chan1, NULL,
         : 		AST_BRIDGE_IMPART_CHAN_INDEPENDENT)) {
         : 		ast_hangup(local_chan);
         : 		ao2_cleanup(local_chan);
         : 		ast_bridge_lock(bridge1);
         : 		return AST_BRIDGE_TRANSFER_FAIL;
         : 	}
         : 	ast_bridge_lock(bridge1);
You will need to be a little smarter when relocking the bridge because you have to do deadlock avoidance if there are two bridges involved.

ast_bridge_lock_both(bridge_1, bridge_2);


https://gerrit.asterisk.org/#/c/840/2/main/bridge_channel.c
File main/bridge_channel.c:

Line 2594: 		bridge_channel, ast_channel_name(bridge_channel->chan));
         : 	/*
Supurfluous line deletion that now groups unrelated code blocks together.


Line 2599: 	ast_bridge_lock(bridge_channel->bridge);
         : 	ast_channel_lock(bridge_channel->chan);
Supurfluous line deletion that now groups unrelated code blocks together.  In this case the block comment does not apply to the channel lock line.


Line 2685: 	bridge_channel_internal_signal(cond, 0);
Should be
bridge_channel_internal_signale(cond, res)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I08fe33a2560da924e676df55b181e46fca604577
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Kevin Harwell <kharwell 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>
Gerrit-HasComments: Yes