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

diff --git a/res/res_fax.c b/res/res_fax.c
index 979b683505e2804017a2e69de45db0f0aeb366aa..83ca28280af5ed1846c7c75b4a3cd9989c10125e 100644 (file)
--- a/res/res_fax.c
+++ b/res/res_fax.c
@@ -1365,7 +1365,7 @@ static int generic_fax_exec(struct ast_channel *chan, struct ast_fax_session_det
        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 struct fax_gateway *fax_gateway_new(struct ast_channel *chan, struct ast_
 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 @@ static int fax_gateway_start(struct fax_gateway *gateway, struct ast_fax_session
        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,8 +3320,12 @@ static struct ast_frame *fax_detect_framehook(struct ast_channel *chan, struct a
        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;
 
        switch (event) {
@@ -3340,8 +3348,13 @@ static struct ast_frame *fax_detect_framehook(struct ast_channel *chan, struct a
        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 d8b8af1d0f740be331af23e4e1f0918d57089039..5aa354cf933eabe5cb33efc376aad78744ac0a91 100644 (file)
--- a/res/res_fax_spandsp.c
+++ b/res/res_fax_spandsp.c
@@ -831,10 +831,14 @@ static int spandsp_fax_gateway_start(struct ast_fax_session *s)
 
        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 @@ static int spandsp_fax_gateway_start(struct ast_fax_session *s)
        }
 
        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);