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

diff --git a/res/res_fax.c b/res/res_fax.c
index 5f660fe78d9a4a2b71d65b3431459172dc160af2..ad62f026de1a507bf8eb8f52b24a1fa79ad7c1cc 100644 (file)
--- 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;
 }