[Asterisk-code-review] res fax: Fix FAXOPT(faxdetect) timeout option. (asterisk[13])

[Richard Mudgett]

Richard Mudgett has uploaded a new change for review.

  https://gerrit.asterisk.org/3237

Change subject: res_fax: Fix FAXOPT(faxdetect) timeout option.
......................................................................

res_fax: Fix FAXOPT(faxdetect) timeout option.

The fax detection timeout option did not work because basically the wrong
variable was checked in fax_detect_framehook().  As a result, the timer
would timeout immediately and disable fax detection.

* Fixed ignoring negative timeout values.  We'd complain and then go right
on using the negative value.

* Fixed destroy_faxdetect() in the off-nominal case of an incomplete
object creation.

* Added more range checking to FAXOPT(gateway) timeout parameter.

ASTERISK-26214 #close
Reported by: Richard Mudgett

Change-Id: Idc5e698dfe33572de9840bc68cd9fc043cbad976
---
M include/asterisk/res_fax.h
M res/res_fax.c
2 files changed, 27 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/37/3237/1

diff --git a/include/asterisk/res_fax.h b/include/asterisk/res_fax.h
index 2304da7..5119bfa 100644
--- a/include/asterisk/res_fax.h
+++ b/include/asterisk/res_fax.h
@@ -179,11 +179,11 @@
 	unsigned int t38timeout;
 	/*! the id of the t.38 gateway framehook for this channel */
 	int gateway_id;
-	/*! the timeout for this gateway in seconds */
+	/*! The timeout for this gateway in ms */
 	int gateway_timeout;
 	/*! the id of the faxdetect framehook for this channel */
 	int faxdetect_id;
-	/*! The timeout for this fax detect in seconds */
+	/*! The timeout for this fax detect in ms */
 	int faxdetect_timeout;
 	/*! flags used for fax detection */
 	int faxdetect_flags;
diff --git a/res/res_fax.c b/res/res_fax.c
index ad6e238..9aec7f5 100644
--- a/res/res_fax.c
+++ b/res/res_fax.c
@@ -468,8 +468,6 @@
 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 */
@@ -3539,13 +3537,13 @@
 		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
  */
@@ -3652,8 +3650,9 @@
 		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;
@@ -3732,7 +3731,7 @@
 
 /*! \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
@@ -4480,8 +4479,14 @@
 				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);
 					}
@@ -4516,11 +4521,18 @@
 
 		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);
 					}
 				}
 

-- 
To view, visit https://gerrit.asterisk.org/3237
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idc5e698dfe33572de9840bc68cd9fc043cbad976
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>