[Asterisk-code-review] res fax: Fix latent bug exposed by ASTERISK-24841 changes. (asterisk[11])
Richard Mudgett
asteriskteam at digium.com
Thu Aug 25 18:23:01 CDT 2016
Richard Mudgett has uploaded a new change for review.
https://gerrit.asterisk.org/3700
Change subject: res_fax: Fix latent bug exposed by ASTERISK-24841 changes.
......................................................................
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
---
M res/res_fax.c
1 file changed, 26 insertions(+), 31 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/00/3700/1
diff --git a/res/res_fax.c b/res/res_fax.c
index 5f660fe..ad62f02 100644
--- a/res/res_fax.c
+++ b/res/res_fax.c
@@ -2959,8 +2959,13 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
case AST_FORMAT_ULAW:
break;
default:
- ao2_ref(details, -1);
return f;
}
break;
@@ -3083,16 +3090,13 @@
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 @@
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 @@
&& (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 @@
}
f = &ast_null_frame;
- ao2_ref(details, -1);
return f;
}
@@ -3162,16 +3161,12 @@
};
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;
}
--
To view, visit https://gerrit.asterisk.org/3700
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6fda0877104a370af586a5e8cf9e161a484da78d
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
More information about the asterisk-code-review
mailing list