[asterisk-commits] res fax.c, res fax spandsp.c: Misc deadlock fixes. (asterisk[11])

[SVN commits to the Asterisk project]

Joshua Colp has submitted this change and it was merged.

Change subject: res_fax.c, res_fax_spandsp.c: Misc deadlock fixes.
......................................................................


res_fax.c, res_fax_spandsp.c: Misc deadlock fixes.

* Fixed locking inconsistency in fax_gateway_start() when calling the
gateway technology start_session callback.  The callback was called with
and without the channel lock held.  Calling the callback with the channel
lock held could deadlock in spandsp_fax_gateway_start() which calls
ast_channel_get_t38_state().

* Fixed deadlock potential in fax_detect_framehook() when calling
ast_channel_make_compatible().

ASTERISK-26203
Reported by: Etienne Lessard

ASTERISK-24822
Reported by: David Brillert

ASTERISK-22732
Reported by: Richard Mudgett

Change-Id: I5a7b8e9e7b198802df06dff0ea48d96bd6d7b912
---
M res/res_fax.c
M res/res_fax_spandsp.c
2 files changed, 22 insertions(+), 4 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Verified



diff --git a/res/res_fax.c b/res/res_fax.c
index 979b683..83ca282 100644
--- a/res/res_fax.c
+++ b/res/res_fax.c
@@ -1365,7 +1365,7 @@
 	report_fax_status(chan, details, "Allocating Resources");
 
 	if (details->caps & AST_FAX_TECH_AUDIO) {
-		expected_frametype = AST_FRAME_VOICE;;
+		expected_frametype = AST_FRAME_VOICE;
 		ast_format_set(&expected_framesubclass.format, AST_FORMAT_SLINEAR, 0);
 		ast_format_copy(&orig_write_format, ast_channel_writeformat(chan));
 		if (ast_set_write_format_by_id(chan, AST_FORMAT_SLINEAR) < 0) {
@@ -2591,6 +2591,7 @@
 static int fax_gateway_start(struct fax_gateway *gateway, struct ast_fax_session_details *details, struct ast_channel *chan)
 {
 	struct ast_fax_session *s;
+	int start_res;
 
 	/* create the FAX session */
 	if (!(s = fax_session_new(details, chan, gateway->s, gateway->token))) {
@@ -2611,7 +2612,10 @@
 	gateway->s = s;
 	gateway->token = NULL;
 
-	if (gateway->s->tech->start_session(gateway->s) < 0) {
+	ast_channel_unlock(chan);
+	start_res = gateway->s->tech->start_session(gateway->s);
+	ast_channel_lock(chan);
+	if (start_res < 0) {
 		ast_string_field_set(details, result, "FAILED");
 		ast_string_field_set(details, resultstr, "error starting gateway session");
 		ast_string_field_set(details, error, "INIT_ERROR");
@@ -3316,7 +3320,11 @@
 	struct ast_fax_session_details *details;
 	struct ast_control_t38_parameters *control_params;
 	struct ast_channel *peer;
+	RAII_VAR(struct ast_channel *, chan_ref, chan, ao2_cleanup);
 	int result = 0;
+
+	/* Ref bump the channel for when we have to unlock it */
+	ao2_ref(chan, 1);
 
 	details = faxdetect->details;
 
@@ -3340,8 +3348,13 @@
 	case AST_FRAMEHOOK_EVENT_DETACHED:
 		/* restore audio formats when we are detached */
 		ast_set_read_format(chan, &faxdetect->orig_format);
-		if ((peer = ast_bridged_channel(chan))) {
+		peer = ast_bridged_channel(chan);
+		if (peer) {
+			ao2_ref(peer, +1);
+			ast_channel_unlock(chan);
 			ast_channel_make_compatible(chan, peer);
+			ast_channel_lock(chan);
+			ao2_ref(peer, -1);
 		}
 		return NULL;
 	case AST_FRAMEHOOK_EVENT_READ:
diff --git a/res/res_fax_spandsp.c b/res/res_fax_spandsp.c
index d8b8af1..5aa354c 100644
--- a/res/res_fax_spandsp.c
+++ b/res/res_fax_spandsp.c
@@ -831,10 +831,14 @@
 
 	p->ist38 = 1;
 	p->ast_t38_state = ast_channel_get_t38_state(s->chan);
-	if (!(peer = ast_bridged_channel(s->chan))) {
+	ast_channel_lock(s->chan);
+	peer = ast_bridged_channel(s->chan);
+	if (!peer) {
 		ast_channel_unlock(s->chan);
 		return -1;
 	}
+	ao2_ref(peer, +1);
+	ast_channel_unlock(s->chan);
 
 	/* we can be in T38_STATE_NEGOTIATING or T38_STATE_NEGOTIATED when the
 	 * gateway is started. We treat both states the same. */
@@ -843,6 +847,7 @@
 	}
 
 	ast_activate_generator(p->ast_t38_state == T38_STATE_NEGOTIATED ? peer : s->chan, &t30_gen , s);
+	ao2_ref(peer, -1);
 
 	set_logging(&p->t38_gw_state.logging, s->details);
 	set_logging(&p->t38_core_state->logging, s->details);

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5a7b8e9e7b198802df06dff0ea48d96bd6d7b912
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>