res_fax.c: Add chan locked precondition comments.
[asterisk/asterisk.git] / res / res_fax.c
index b5e5411..94c512d 100644 (file)
@@ -65,7 +65,7 @@
 
 #include "asterisk.h"
 
-ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
+ASTERISK_REGISTER_FILE()
 
 #include "asterisk/io.h"
 #include "asterisk/file.h"
@@ -233,7 +233,7 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
                </syntax>
                <description>
                        <para>FAXOPT can be used to override the settings for a FAX session listed in <filename>res_fax.conf</filename>,
-                       it can also be used to retreive information about a FAX session that has finished eg. pages/status.</para>
+                       it can also be used to retrieve information about a FAX session that has finished eg. pages/status.</para>
                </description>
                <see-also>
                        <ref type="application">ReceiveFax</ref>
@@ -468,8 +468,6 @@ struct fax_gateway {
 struct fax_detect {
        /*! \brief the start of our timeout counter */
        struct timeval timeout_start;
-       /*! \brief faxdetect timeout */
-       int timeout;
        /*! \brief DSP Processor */
        struct ast_dsp *dsp;
        /*! \brief original audio formats */
@@ -523,7 +521,7 @@ static AST_RWLIST_HEAD_STATIC(faxmodules, fax_module);
 #define RES_FAX_MINRATE 4800
 #define RES_FAX_MAXRATE 14400
 #define RES_FAX_STATUSEVENTS 0
-#define RES_FAX_MODEM (AST_FAX_MODEM_V17 | AST_FAX_MODEM_V27 | AST_FAX_MODEM_V29)
+#define RES_FAX_MODEM (AST_FAX_MODEM_V17 | AST_FAX_MODEM_V27TER | AST_FAX_MODEM_V29)
 #define RES_FAX_T38TIMEOUT 5000
 
 struct fax_options {
@@ -626,6 +624,8 @@ static const struct ast_datastore_info fax_datastore = {
 static int fax_gateway_attach(struct ast_channel *chan, struct ast_fax_session_details *details);
 static int fax_detect_attach(struct ast_channel *chan, int timeout, int flags);
 static struct ast_fax_session_details *find_or_create_details(struct ast_channel *chan);
+static struct ast_fax_session *fax_v21_session_new (struct ast_channel *chan);
+
 
 /*! \brief Copies fax detection and gateway framehooks during masquerades
  *
@@ -828,7 +828,7 @@ static int update_modem_bits(enum ast_fax_modems *bits, const char *value)
                if (!strcasecmp(m[j], "v17")) {
                        *bits |= AST_FAX_MODEM_V17;
                } else if (!strcasecmp(m[j], "v27")) {
-                       *bits |= AST_FAX_MODEM_V27;
+                       *bits |= AST_FAX_MODEM_V27TER;
                } else if (!strcasecmp(m[j], "v29")) {
                        *bits |= AST_FAX_MODEM_V29;
                } else if (!strcasecmp(m[j], "v34")) {
@@ -839,6 +839,7 @@ static int update_modem_bits(enum ast_fax_modems *bits, const char *value)
        }
        return 0;
 }
+
 static char *ast_fax_caps_to_str(enum ast_fax_capabilities caps, char *buf, size_t bufsize)
 {
        char *out = buf;
@@ -906,7 +907,7 @@ static int ast_fax_modem_to_str(enum ast_fax_modems bits, char *tbuf, size_t buf
                strcat(tbuf, "V17");
                count++;
        }
-       if (bits & AST_FAX_MODEM_V27) {
+       if (bits & AST_FAX_MODEM_V27TER) {
                if (count) {
                        strcat(tbuf, ",");
                }
@@ -935,22 +936,14 @@ static int check_modem_rate(enum ast_fax_modems modems, unsigned int rate)
 {
        switch (rate) {
        case 2400:
-               if (!(modems & (AST_FAX_MODEM_V34))) {
-                       return 1;
-               }
-               break;
        case 4800:
-               if (!(modems & (AST_FAX_MODEM_V27 | AST_FAX_MODEM_V34))) {
+               if (!(modems & (AST_FAX_MODEM_V27TER | AST_FAX_MODEM_V34))) {
                        return 1;
                }
                break;
        case 7200:
-               if (!(modems & (AST_FAX_MODEM_V17 | AST_FAX_MODEM_V29 | AST_FAX_MODEM_V34))) {
-                       return 1;
-               }
-               break;
        case 9600:
-               if (!(modems & (AST_FAX_MODEM_V17 | AST_FAX_MODEM_V27 | AST_FAX_MODEM_V29 | AST_FAX_MODEM_V34))) {
+               if (!(modems & (AST_FAX_MODEM_V17 | AST_FAX_MODEM_V29 | AST_FAX_MODEM_V34))) {
                        return 1;
                }
                break;
@@ -1356,7 +1349,8 @@ static struct ast_json *generate_filenames_json(struct ast_fax_session_details *
        return json_array;
 }
 
-/* \brief Generate a string of filenames using the given prefix and separator.
+/*!
+ * \brief Generate a string of filenames using the given prefix and separator.
  * \param details the fax session details
  * \param prefix the prefix to each filename
  * \param separator the separator between filenames
@@ -1453,6 +1447,12 @@ static void set_channel_variables(struct ast_channel *chan, struct ast_fax_sessi
        pbx_builtin_setvar_helper(chan, "FAXBITRATE", S_OR(details->transfer_rate, NULL));
        pbx_builtin_setvar_helper(chan, "FAXRESOLUTION", S_OR(details->resolution, NULL));
 
+       if (ast_channel_get_t38_state(chan) == T38_STATE_NEGOTIATED) {
+               pbx_builtin_setvar_helper(chan, "FAXMODE", "T38");
+       } else {
+               pbx_builtin_setvar_helper(chan, "FAXMODE", "audio");
+       }
+
        snprintf(buf, sizeof(buf), "%u", details->pages_transferred);
        pbx_builtin_setvar_helper(chan, "FAXPAGES", buf);
 }
@@ -1624,21 +1624,15 @@ static int generic_fax_exec(struct ast_channel *chan, struct ast_fax_session_det
                orig_write_format = ao2_bump(ast_channel_writeformat(chan));
                if (ast_set_write_format(chan, ast_format_slin) < 0) {
                        ast_log(LOG_ERROR, "channel '%s' failed to set write format to signed linear'.\n", ast_channel_name(chan));
-                       ao2_lock(faxregistry.container);
-                       ao2_unlink(faxregistry.container, fax);
-                       ao2_unlock(faxregistry.container);
-                       ao2_ref(fax, -1);
-                       ast_channel_unlock(chan);
+                       ao2_unlink(faxregistry.container, fax);
+                       ao2_ref(fax, -1);
                        return -1;
                }
                orig_read_format = ao2_bump(ast_channel_readformat(chan));
                if (ast_set_read_format(chan, ast_format_slin) < 0) {
                        ast_log(LOG_ERROR, "channel '%s' failed to set read format to signed linear.\n", ast_channel_name(chan));
-                       ao2_lock(faxregistry.container);
-                       ao2_unlink(faxregistry.container, fax);
-                       ao2_unlock(faxregistry.container);
-                       ao2_ref(fax, -1);
-                       ast_channel_unlock(chan);
+                       ao2_unlink(faxregistry.container, fax);
+                       ao2_ref(fax, -1);
                        return -1;
                }
                if (fax->smoother) {
@@ -1816,9 +1810,7 @@ static int generic_fax_exec(struct ast_channel *chan, struct ast_fax_session_det
        }
 
        if (fax) {
-               ao2_lock(faxregistry.container);
                ao2_unlink(faxregistry.container, fax);
-               ao2_unlock(faxregistry.container);
                ao2_ref(fax, -1);
        }
 
@@ -2042,14 +2034,14 @@ static int report_receive_fax_status(struct ast_channel *chan, const char *filen
                        fax_bitrate = ast_strdupa(fax_bitrate);
                }
 
-               json_object = ast_json_pack("{s: s, s: s, s: s, s: s, s: s, s: s, s: O}",
+               json_object = ast_json_pack("{s: s, s: s, s: s, s: s, s: s, s: s, s: o}",
                                "type", "receive",
                                "remote_station_id", S_OR(remote_station_id, ""),
                                "local_station_id", S_OR(local_station_id, ""),
                                "fax_pages", S_OR(fax_pages, ""),
                                "fax_resolution", S_OR(fax_resolution, ""),
                                "fax_bitrate", S_OR(fax_bitrate, ""),
-                               "filenames", json_array);
+                               "filenames", ast_json_ref(json_array));
                if (!json_object) {
                        return -1;
                }
@@ -2085,6 +2077,7 @@ static int receivefax_exec(struct ast_channel *chan, const char *data)
        pbx_builtin_setvar_helper(chan, "FAXPAGES", "0");
        pbx_builtin_setvar_helper(chan, "FAXBITRATE", NULL);
        pbx_builtin_setvar_helper(chan, "FAXRESOLUTION", NULL);
+       pbx_builtin_setvar_helper(chan, "FAXMODE", NULL);
 
        /* Get a FAX session details structure from the channel's FAX datastore and create one if
         * it does not already exist. */
@@ -2592,6 +2585,7 @@ static int sendfax_exec(struct ast_channel *chan, const char *data)
        pbx_builtin_setvar_helper(chan, "FAXPAGES", "0");
        pbx_builtin_setvar_helper(chan, "FAXBITRATE", NULL);
        pbx_builtin_setvar_helper(chan, "FAXRESOLUTION", NULL);
+       pbx_builtin_setvar_helper(chan, "FAXMODE", NULL);
 
        /* Get a requirement structure and set it.  This structure is used
         * to tell the FAX technology module about the higher level FAX session */
@@ -2804,18 +2798,14 @@ static int sendfax_exec(struct ast_channel *chan, const char *data)
 static void destroy_v21_sessions(struct fax_gateway *gateway)
 {
        if (gateway->chan_v21_session) {
-               ao2_lock(faxregistry.container);
                ao2_unlink(faxregistry.container, gateway->chan_v21_session);
-               ao2_unlock(faxregistry.container);
 
                ao2_ref(gateway->chan_v21_session, -1);
                gateway->chan_v21_session = NULL;
        }
 
        if (gateway->peer_v21_session) {
-               ao2_lock(faxregistry.container);
                ao2_unlink(faxregistry.container, gateway->peer_v21_session);
-               ao2_unlock(faxregistry.container);
 
                ao2_ref(gateway->peer_v21_session, -1);
                gateway->peer_v21_session = NULL;
@@ -2833,9 +2823,7 @@ static void destroy_gateway(void *data)
                fax_session_release(gateway->s, gateway->token);
                gateway->token = NULL;
 
-               ao2_lock(faxregistry.container);
                ao2_unlink(faxregistry.container, gateway->s);
-               ao2_unlock(faxregistry.container);
 
                ao2_ref(gateway->s, -1);
                gateway->s = NULL;
@@ -2847,6 +2835,20 @@ static void destroy_gateway(void *data)
        ao2_cleanup(gateway->peer_write_format);
 }
 
+static struct ast_fax_session *fax_v21_session_new (struct ast_channel *chan) {
+       struct ast_fax_session_details *v21_details;
+       struct ast_fax_session *v21_session;
+
+       if (!chan || !(v21_details = session_details_new())) {
+               return NULL;
+       }
+
+       v21_details->caps = AST_FAX_TECH_V21_DETECT;
+       v21_session = fax_session_new(v21_details, chan, NULL, NULL);
+       ao2_ref(v21_details, -1);
+       return v21_session;
+}
+
 /*! \brief Create a new fax gateway object.
  * \param chan the channel the gateway object will be attached to
  * \param details the fax session details
@@ -2855,30 +2857,16 @@ static void destroy_gateway(void *data)
 static struct fax_gateway *fax_gateway_new(struct ast_channel *chan, struct ast_fax_session_details *details)
 {
        struct fax_gateway *gateway = ao2_alloc(sizeof(*gateway), destroy_gateway);
-       struct ast_fax_session_details *v21_details;
        if (!gateway) {
                return NULL;
        }
 
-       if (!(v21_details = session_details_new())) {
-               ao2_ref(gateway, -1);
-               return NULL;
-       }
-
-       v21_details->caps = AST_FAX_TECH_V21_DETECT;
-       if (!(gateway->chan_v21_session = fax_session_new(v21_details, chan, NULL, NULL))) {
-               ao2_ref(v21_details, -1);
+       if (!(gateway->chan_v21_session = fax_v21_session_new(chan))) {
+               ast_log(LOG_ERROR, "Can't create V21 session on chan %s for T.38 gateway session\n", ast_channel_name(chan));
                ao2_ref(gateway, -1);
                return NULL;
        }
 
-       if (!(gateway->peer_v21_session = fax_session_new(v21_details, chan, NULL, NULL))) {
-               ao2_ref(v21_details, -1);
-               ao2_ref(gateway, -1);
-               return NULL;
-       }
-       ao2_ref(v21_details, -1);
-
        gateway->framehook = -1;
 
        details->caps = AST_FAX_TECH_GATEWAY;
@@ -2892,14 +2880,21 @@ static struct fax_gateway *fax_gateway_new(struct ast_channel *chan, struct ast_
        return gateway;
 }
 
-/*! \brief Create a fax session and start T.30<->T.38 gateway mode
+/*!
+ * \brief Create a fax session and start T.30<->T.38 gateway mode
+ *
  * \param gateway a fax gateway object
  * \param details fax session details
  * \param chan active channel
- * \return 0 on error 1 on success*/
+ *
+ * \pre chan is locked on entry
+ *
+ * \return 0 on error 1 on success
+ */
 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))) {
@@ -2920,7 +2915,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");
@@ -2936,6 +2934,7 @@ static int fax_gateway_start(struct fax_gateway *gateway, struct ast_fax_session
        return 0;
 }
 
+/*! \pre chan is locked on entry */
 static struct ast_frame *fax_gateway_request_t38(struct fax_gateway *gateway, struct ast_channel *chan, struct ast_frame *f)
 {
        struct ast_frame *fp;
@@ -2974,6 +2973,7 @@ static struct ast_frame *fax_gateway_request_t38(struct fax_gateway *gateway, st
        return fp;
 }
 
+/*! \pre chan is locked on entry */
 static struct ast_frame *fax_gateway_detect_v21(struct fax_gateway *gateway, struct ast_channel *chan, struct ast_channel *peer, struct ast_channel *active, struct ast_frame *f)
 {
        struct ast_channel *other = (active == chan) ? peer : chan;
@@ -3010,12 +3010,17 @@ static int fax_gateway_indicate_t38(struct ast_channel *chan, struct ast_channel
        }
 }
 
-/*! \brief T38 Gateway Negotiate t38 parameters
+/*!
+ * \brief T38 Gateway Negotiate t38 parameters
+ *
  * \param gateway gateway object
  * \param chan channel running the gateway
  * \param peer channel im bridged too
  * \param active channel the frame originated on
  * \param f the control frame to process
+ *
+ * \pre chan is locked on entry
+ *
  * \return processed control frame or null frame
  */
 static struct ast_frame *fax_gateway_detect_t38(struct fax_gateway *gateway, struct ast_channel *chan, struct ast_channel *peer, struct ast_channel *active, struct ast_frame *f)
@@ -3236,7 +3241,8 @@ static struct ast_frame *fax_gateway_detect_t38(struct fax_gateway *gateway, str
 
 /*! \brief Destroy the gateway data structure when the framehook is detached
  * \param data framehook data (gateway data)*/
-static void fax_gateway_framehook_destroy(void *data) {
+static void fax_gateway_framehook_destroy(void *data)
+{
        struct fax_gateway *gateway = data;
 
        if (gateway->s) {
@@ -3257,7 +3263,8 @@ static void fax_gateway_framehook_destroy(void *data) {
        ao2_ref(gateway, -1);
 }
 
-/*! \brief T.30<->T.38 gateway framehook.
+/*!
+ * \brief T.30<->T.38 gateway framehook.
  *
  * Intercept packets on bridged channels and determine if a T.38 gateway is
  * required. If a gateway is required, start a gateway and handle T.38
@@ -3268,9 +3275,12 @@ static void fax_gateway_framehook_destroy(void *data) {
  * \param event framehook event
  * \param data framehook data (struct fax_gateway *)
  *
+ * \pre chan is locked on entry
+ *
  * \return processed frame or NULL when f is NULL or a null frame
  */
-static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct ast_frame *f, enum ast_framehook_event event, 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 *active;
        RAII_VAR(struct ast_fax_session_details *, details, NULL, ao2_cleanup);
@@ -3297,13 +3307,13 @@ 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);
 
                        ast_channel_unlock(chan);
                        peer = ast_channel_bridge_peer(chan);
                        if (peer) {
                                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);
@@ -3346,24 +3356,31 @@ 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 */
                ao2_replace(gateway->chan_read_format, ast_channel_readformat(chan));
-               ao2_replace(gateway->chan_write_format, ast_channel_readformat(chan));
+               ao2_replace(gateway->chan_write_format, ast_channel_writeformat(chan));
 
                ao2_replace(gateway->peer_read_format, ast_channel_readformat(peer));
-               ao2_replace(gateway->peer_write_format, ast_channel_readformat(peer));
+               ao2_replace(gateway->peer_write_format, ast_channel_writeformat(peer));
 
                ast_set_read_format(chan, ast_format_slin);
                ast_set_write_format(chan, ast_format_slin);
 
-               ast_channel_unlock(chan);
                ast_set_read_format(peer, ast_format_slin);
                ast_set_write_format(peer, ast_format_slin);
 
-               ast_channel_make_compatible(chan, peer);
-               ast_channel_lock(chan);
+               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));
+                       ast_framehook_detach(chan, gateway->framehook);
+                       return f;
+               }
        }
 
        if (gateway->bridged && !ast_tvzero(gateway->timeout_start)) {
@@ -3490,6 +3507,10 @@ static int fax_gateway_attach(struct ast_channel *chan, struct ast_fax_session_d
                .disable_inheritance = 1, /* Masquerade inheritance is handled through the datastore fixup */
        };
 
+       if (global_fax_debug) {
+               details->option.debug = AST_FAX_OPTFLAG_TRUE;
+       }
+
        ast_string_field_set(details, result, "SUCCESS");
        ast_string_field_set(details, resultstr, "gateway operation started successfully");
        ast_string_field_set(details, error, "NO_ERROR");
@@ -3532,13 +3553,13 @@ static void destroy_faxdetect(void *data)
                ast_dsp_free(faxdetect->dsp);
                faxdetect->dsp = NULL;
        }
-       ao2_ref(faxdetect->details, -1);
+       ao2_cleanup(faxdetect->details);
        ao2_cleanup(faxdetect->orig_format);
 }
 
 /*! \brief Create a new fax detect object.
  * \param chan the channel attaching to
- * \param timeout remove framehook in this time if set
+ * \param timeout in ms to remove framehook in this time if not zero
  * \param flags required options
  * \return NULL or a fax gateway object
  */
@@ -3575,7 +3596,8 @@ static struct fax_detect *fax_detect_new(struct ast_channel *chan, int timeout,
 
 /*! \brief Deref the faxdetect data structure when the faxdetect framehook is detached
  * \param data framehook data (faxdetect data)*/
-static void fax_detect_framehook_destroy(void *data) {
+static void fax_detect_framehook_destroy(void *data)
+{
        struct fax_detect *faxdetect = data;
 
        ao2_ref(faxdetect, -1);
@@ -3592,7 +3614,8 @@ static void fax_detect_framehook_destroy(void *data) {
  *
  * \return processed frame or NULL when f is NULL or a null frame
  */
-static struct ast_frame *fax_detect_framehook(struct ast_channel *chan, struct ast_frame *f, enum ast_framehook_event event, void *data) {
+static struct ast_frame *fax_detect_framehook(struct ast_channel *chan, struct ast_frame *f, enum ast_framehook_event event, void *data)
+{
        struct fax_detect *faxdetect = data;
        struct ast_fax_session_details *details;
        struct ast_control_t38_parameters *control_params;
@@ -3643,8 +3666,9 @@ static struct ast_frame *fax_detect_framehook(struct ast_channel *chan, struct a
                return f;
        }
 
-       if ((!ast_tvzero(faxdetect->timeout_start) &&
-           (ast_tvdiff_ms(ast_tvnow(), faxdetect->timeout_start) > faxdetect->timeout))) {
+       if (!ast_tvzero(faxdetect->timeout_start)
+               && ast_tvdiff_ms(ast_tvnow(), faxdetect->timeout_start) > details->faxdetect_timeout) {
+               ast_debug(1, "FAXOPT(faxdetect) timeout on %s\n", ast_channel_name(chan));
                ast_framehook_detach(chan, details->faxdetect_id);
                details->faxdetect_id = -1;
                return f;
@@ -3692,30 +3716,36 @@ static struct ast_frame *fax_detect_framehook(struct ast_channel *chan, struct a
        }
 
        if (result) {
-               const char *target_context = S_OR(ast_channel_macrocontext(chan), ast_channel_context(chan));
+               const char *target_context;
+
                switch (result) {
                case 'f':
                case 't':
+                       target_context = S_OR(ast_channel_macrocontext(chan), ast_channel_context(chan));
+
                        ast_channel_unlock(chan);
+                       ast_frfree(f);
+                       f = &ast_null_frame;
                        if (ast_exists_extension(chan, target_context, "fax", 1,
                            S_COR(ast_channel_caller(chan)->id.number.valid, ast_channel_caller(chan)->id.number.str, NULL))) {
-                               ast_channel_lock(chan);
                                ast_verb(2, "Redirecting '%s' to fax extension due to %s detection\n",
                                        ast_channel_name(chan), (result == 'f') ? "CNG" : "T38");
                                pbx_builtin_setvar_helper(chan, "FAXEXTEN", ast_channel_exten(chan));
                                if (ast_async_goto(chan, target_context, "fax", 1)) {
                                        ast_log(LOG_NOTICE, "Failed to async goto '%s' into fax of '%s'\n", ast_channel_name(chan), target_context);
                                }
-                               ast_frfree(f);
-                               f = &ast_null_frame;
                        } else {
-                               ast_channel_lock(chan);
                                ast_log(LOG_NOTICE, "FAX %s detected but no fax extension in context (%s)\n",
                                        (result == 'f') ? "CNG" : "T38", target_context);
                        }
+                       ast_channel_lock(chan);
+
+                       ast_framehook_detach(chan, details->faxdetect_id);
+                       details->faxdetect_id = -1;
+                       break;
+               default:
+                       break;
                }
-               ast_framehook_detach(chan, details->faxdetect_id);
-               details->faxdetect_id = -1;
        }
 
        return f;
@@ -3723,7 +3753,7 @@ static struct ast_frame *fax_detect_framehook(struct ast_channel *chan, struct a
 
 /*! \brief Attach a faxdetect framehook object to a channel.
  * \param chan the channel to attach to
- * \param timeout remove framehook in this time if set
+ * \param timeout in ms to remove framehook in this time if not zero
  * \return the faxdetect structure or NULL on error
  * \param flags required options
  * \retval -1 error
@@ -4190,17 +4220,18 @@ static int manager_fax_sessions_entry(struct mansession *s,
 static int manager_fax_sessions(struct mansession *s, const struct message *m)
 {
        const char *action_id = astman_get_header(m, "ActionID");
-       char id_text[256] = "";
+       char id_text[256];
        struct ast_fax_session *session;
        struct ao2_iterator iter;
        int session_count = 0;
 
-       astman_send_listack(s, m, "FAXSessionsEntry event list will follow", "Start");
-
+       id_text[0] = '\0';
        if (!ast_strlen_zero(action_id)) {
                snprintf(id_text, sizeof(id_text), "ActionID: %s\r\n", action_id);
        }
 
+       astman_send_listack(s, m, "FAXSessionsEntry event list will follow", "Start");
+
        iter = ao2_iterator_init(faxregistry.container, 0);
        while ((session = ao2_iterator_next(&iter))) {
                if (!manager_fax_sessions_entry(s, session, id_text)) {
@@ -4210,13 +4241,9 @@ static int manager_fax_sessions(struct mansession *s, const struct message *m)
        }
        ao2_iterator_destroy(&iter);
 
-       astman_append(s, "Event: FAXSessionsComplete\r\n"
-               "%s"
-               "EventList: Complete\r\n"
-               "Total: %d\r\n"
-               "\r\n",
-               id_text,
-               session_count);
+       astman_send_list_complete_start(s, m, "FAXSessionsComplete", session_count);
+       astman_append(s, "Total: %d\r\n", session_count);
+       astman_send_list_complete_end(s);
 
        return 0;
 }
@@ -4474,8 +4501,14 @@ static int acf_faxopt_write(struct ast_channel *chan, const char *cmd, char *dat
                                details->gateway_timeout = 0;
                                if (timeout) {
                                        unsigned int gwtimeout;
-                                       if (sscanf(timeout, "%u", &gwtimeout) == 1) {
-                                               details->gateway_timeout = gwtimeout * 1000;
+
+                                       if (sscanf(timeout, "%30u", &gwtimeout) == 1) {
+                                               if (gwtimeout >= 0) {
+                                                       details->gateway_timeout = gwtimeout * 1000;
+                                               } else {
+                                                       ast_log(LOG_WARNING, "%s(%s) timeout cannot be negative.  Ignoring timeout\n",
+                                                               cmd, data);
+                                               }
                                        } else {
                                                ast_log(LOG_WARNING, "Unsupported timeout '%s' passed to FAXOPT(%s).\n", timeout, data);
                                        }
@@ -4492,7 +4525,9 @@ static int acf_faxopt_write(struct ast_channel *chan, const char *cmd, char *dat
                                ast_log(LOG_WARNING, "Attempt to attach a T.38 gateway on channel (%s) with gateway already running.\n", ast_channel_name(chan));
                        }
                } else if (ast_false(val)) {
+                       ast_channel_lock(chan);
                        ast_framehook_detach(chan, details->gateway_id);
+                       ast_channel_unlock(chan);
                        details->gateway_id = -1;
                } else {
                        ast_log(LOG_WARNING, "Unsupported value '%s' passed to FAXOPT(%s).\n", value, data);
@@ -4510,11 +4545,18 @@ static int acf_faxopt_write(struct ast_channel *chan, const char *cmd, char *dat
 
                if (ast_true(val) || !strcasecmp(val, "t38") || !strcasecmp(val, "cng")) {
                        if (details->faxdetect_id < 0) {
-                               if (timeout && (sscanf(timeout, "%u", &fdtimeout) == 1)) {
-                                       if (fdtimeout > 0) {
-                                               fdtimeout = fdtimeout * 1000;
+                               if (timeout) {
+                                       if (sscanf(timeout, "%30u", &fdtimeout) == 1) {
+                                               if (fdtimeout >= 0) {
+                                                       fdtimeout *= 1000;
+                                               } else {
+                                                       ast_log(LOG_WARNING, "%s(%s) timeout cannot be negative.  Ignoring timeout\n",
+                                                               cmd, data);
+                                                       fdtimeout = 0;
+                                               }
                                        } else {
-                                               ast_log(LOG_WARNING, "Timeout cannot be negative ignoring timeout\n");
+                                               ast_log(LOG_WARNING, "Unsupported timeout '%s' passed to FAXOPT(%s).\n",
+                                                       timeout, data);
                                        }
                                }
 
@@ -4537,7 +4579,9 @@ static int acf_faxopt_write(struct ast_channel *chan, const char *cmd, char *dat
                                ast_log(LOG_WARNING, "Attempt to attach a FAX detect on channel (%s) with FAX detect already running.\n", ast_channel_name(chan));
                        }
                } else if (ast_false(val)) {
+                       ast_channel_lock(chan);
                        ast_framehook_detach(chan, details->faxdetect_id);
+                       ast_channel_unlock(chan);
                        details->faxdetect_id = -1;
                } else {
                        ast_log(LOG_WARNING, "Unsupported value '%s' passed to FAXOPT(%s).\n", value, data);
@@ -4691,9 +4735,9 @@ static int reload_module(void)
 
 
 AST_MODULE_INFO(ASTERISK_GPL_KEY, AST_MODFLAG_GLOBAL_SYMBOLS | AST_MODFLAG_LOAD_ORDER, "Generic FAX Applications",
-               .support_level = AST_MODULE_SUPPORT_CORE,
-               .load = load_module,
-               .unload = unload_module,
-               .reload = reload_module,
-               .load_pri = AST_MODPRI_APP_DEPEND,
-              );
+       .support_level = AST_MODULE_SUPPORT_CORE,
+       .load = load_module,
+       .unload = unload_module,
+       .reload = reload_module,
+       .load_pri = AST_MODPRI_APP_DEPEND,
+);