From 258705cfcd9608964da464af9c9bc495ba6e06dc Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Fri, 17 Apr 2015 18:05:37 -0500 Subject: [PATCH] res_fax: Fix latent bug exposed by ASTERISK-24841 changes. Three fax related tests started failing as a result of changes made for ASTERISK-24841: tests/fax/pjsip/gateway_t38_g711 tests/fax/sip/gateway_mix1 tests/fax/sip/gateway_mix3 Historically, ast_channel_make_compatible() did nothing if the channels were already "compatible" even if they had a sub-optimal translation path already setup. With the changes from ASTERISK-24841 this is no longer true in order to allow the best translation paths to always be picked. In res_fax.c:fax_gateway_framehook() code manually setup the channels to go through slin and then called ast_channel_make_compatible(). With the previous version of ast_channel_make_compatible() this was always a no-operation. * Remove call to ast_channel_make_compatible() in fax_gateway_framehook() that now undoes what was just setup when the framehook is attached. * Fixed locking around saving the channel formats in fax_gateway_framehook() to ensure that the formats that are saved are consistent. * Fix copy pasta errors in fax_gateway_framehook() that confuses read and write when dealing with saved channel formats. ASTERISK-24841 Reported by: Matt Jordan Backported from v13 revision 82bc0fd3ade77394e13062b6097732c224e77eaa mainly to get the diff between v11 and v13 smaller but this also fixes a potential deadlock when dealing with the chan and the peer together. Change-Id: I6fda0877104a370af586a5e8cf9e161a484da78d --- res/res_fax.c | 57 +++++++++++++++++++++++---------------------------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/res/res_fax.c b/res/res_fax.c index 5f660fe78d..ad62f026de 100644 --- a/res/res_fax.c +++ b/res/res_fax.c @@ -2959,8 +2959,13 @@ static void fax_gateway_framehook_destroy(void *data) static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct ast_frame *f, enum ast_framehook_event event, void *data) { struct fax_gateway *gateway = data; - struct ast_channel *peer, *active; - struct ast_fax_session_details *details; + struct ast_channel *active; + RAII_VAR(struct ast_fax_session_details *, details, NULL, ao2_cleanup); + RAII_VAR(struct ast_channel *, peer, NULL, ao2_cleanup); + RAII_VAR(struct ast_channel *, chan_ref, chan, ao2_cleanup); + + /* Ref bump channel for when we have to unlock it */ + ao2_ref(chan_ref, 1); if (gateway->s) { details = gateway->s->details; @@ -2979,37 +2984,38 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct if (gateway->bridged) { ast_set_read_format(chan, &gateway->chan_read_format); - ast_set_read_format(chan, &gateway->chan_write_format); + ast_set_write_format(chan, &gateway->chan_write_format); - if ((peer = ast_bridged_channel(chan))) { + peer = ast_bridged_channel(chan); + if (peer) { + ao2_ref(peer, +1); + ast_channel_unlock(chan); ast_set_read_format(peer, &gateway->peer_read_format); - ast_set_read_format(peer, &gateway->peer_write_format); + ast_set_write_format(peer, &gateway->peer_write_format); ast_channel_make_compatible(chan, peer); + ast_channel_lock(chan); } } - - ao2_ref(details, -1); return NULL; } if (!f || (event == AST_FRAMEHOOK_EVENT_ATTACHED)) { - ao2_ref(details, -1); return NULL; }; /* this frame was generated by the fax gateway, pass it on */ if (ast_test_flag(f, AST_FAX_FRFLAG_GATEWAY)) { - ao2_ref(details, -1); return f; } - if (!(peer = ast_bridged_channel(chan))) { - /* not bridged, don't do anything */ - ao2_ref(details, -1); + /* If we aren't bridged or we don't have a peer, don't do anything */ + peer = ast_bridged_channel(chan); + if (!peer) { return f; } + ao2_ref(peer, +1); - if (!gateway->bridged && peer) { + if (!gateway->bridged) { /* don't start a gateway if neither channel can handle T.38 */ if (ast_channel_get_t38_state(chan) == T38_STATE_UNAVAILABLE && ast_channel_get_t38_state(peer) == T38_STATE_UNAVAILABLE) { ast_debug(1, "not starting gateway for %s and %s; neither channel supports T.38\n", ast_channel_name(chan), ast_channel_name(peer)); @@ -3020,7 +3026,6 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct ast_string_field_set(details, resultstr, "neither channel supports T.38"); ast_string_field_set(details, error, "T38_NEG_ERROR"); set_channel_variables(chan, details); - ao2_ref(details, -1); return f; } @@ -3028,13 +3033,16 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct gateway->timeout_start = ast_tvnow(); } + ast_channel_unlock(chan); + ast_channel_lock_both(chan, peer); + /* we are bridged, change r/w formats to SLIN for v21 preamble * detection and T.30 */ ast_format_copy(&gateway->chan_read_format, ast_channel_readformat(chan)); - ast_format_copy(&gateway->chan_write_format, ast_channel_readformat(chan)); + ast_format_copy(&gateway->chan_write_format, ast_channel_writeformat(chan)); ast_format_copy(&gateway->peer_read_format, ast_channel_readformat(peer)); - ast_format_copy(&gateway->peer_write_format, ast_channel_readformat(peer)); + ast_format_copy(&gateway->peer_write_format, ast_channel_writeformat(peer)); ast_set_read_format_by_id(chan, AST_FORMAT_SLINEAR); ast_set_write_format_by_id(chan, AST_FORMAT_SLINEAR); @@ -3042,7 +3050,8 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct ast_set_read_format_by_id(peer, AST_FORMAT_SLINEAR); ast_set_write_format_by_id(peer, AST_FORMAT_SLINEAR); - ast_channel_make_compatible(chan, peer); + ast_channel_unlock(peer); + gateway->bridged = 1; if (!(gateway->peer_v21_session = fax_v21_session_new(peer))) { ast_log(LOG_ERROR, "Can't create V21 session on chan %s for T.38 gateway session\n", ast_channel_name(peer)); @@ -3061,7 +3070,6 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct ast_string_field_build(details, resultstr, "no fax activity after %d ms", details->gateway_timeout); ast_string_field_set(details, error, "TIMEOUT"); set_channel_variables(chan, details); - ao2_ref(details, -1); return f; } } @@ -3075,7 +3083,6 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct case AST_FORMAT_ULAW: break; default: - ao2_ref(details, -1); return f; } break; @@ -3083,16 +3090,13 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct if (f->subclass.integer == AST_MODEM_T38) { break; } - ao2_ref(details, -1); return f; case AST_FRAME_CONTROL: if (f->subclass.integer == AST_CONTROL_T38_PARAMETERS) { break; } - ao2_ref(details, -1); return f; default: - ao2_ref(details, -1); return f; } @@ -3106,20 +3110,17 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct break; default: ast_log(LOG_WARNING, "unhandled framehook event %u\n", event); - ao2_ref(details, -1); return f; } /* handle control frames */ if (f->frametype == AST_FRAME_CONTROL && f->subclass.integer == AST_CONTROL_T38_PARAMETERS) { - ao2_ref(details, -1); return fax_gateway_detect_t38(gateway, chan, peer, active, f); } if (!gateway->detected_v21 && gateway->t38_state == T38_STATE_UNAVAILABLE && f->frametype == AST_FRAME_VOICE) { /* not in gateway mode and have not detected v21 yet, listen * for v21 */ - ao2_ref(details, -1); return fax_gateway_detect_v21(gateway, chan, peer, active, f); } @@ -3132,7 +3133,6 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct && (readtrans = ast_channel_readtrans(active))) { if ((f = ast_translate(readtrans, f, 1)) == NULL) { f = &ast_null_frame; - ao2_ref(details, -1); return f; } /* XXX we ignore the return value here, perhaps we should @@ -3146,7 +3146,6 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct } f = &ast_null_frame; - ao2_ref(details, -1); return f; } @@ -3162,16 +3161,12 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct }; ast_format_set(&silence_frame.subclass.format, AST_FORMAT_SLINEAR, 0); memset(silence_buf, 0, sizeof(silence_buf)); - - ao2_ref(details, -1); return ast_frisolate(&silence_frame); } else { - ao2_ref(details, -1); return &ast_null_frame; } } - ao2_ref(details, -1); return f; } -- 2.47.2