[Asterisk-code-review] res_fax: fix segfault on inactive "reserved" fax session (...asterisk[16])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/c/asterisk/+/11417 )

Change subject: res_fax: fix segfault on inactive "reserved" fax session
......................................................................

res_fax: fix segfault on inactive "reserved" fax session

The change #10017 "Handle fax gateway being started more than once"
introdiced a bug which leads to segfault in res_fax_spandsp.

The res_fax_spandsp module does not support reserving sessions, so
fax_session_reserve returns a fax session with state AST_FAX_STATE_INACTIVE.

The fax_gateway_start does not create a real fax session if the fax session
is already present and the state is not AST_FAX_STATE_RESERVED.
But the "reserved" session created for res_fax_spandsp has state
AST_FAX_STATE_INACTIVE, so fax_gateway_start not starting.

Then when fax_gateway_framehook is called and gateway T.38 state is
NEGOTIATED the call of gateway->s->tech->write(gateway->s, f) leads to
segfault, because session tech_pvt is not set, i.e. the tech session
was not initialized/started.

This patch adds check also on AST_FAX_STATE_INACTIVE to the "reserved"
session created for res_fax_spandsp will start.

This patch also adds extra check and log ERROR if tech_pvt is not set
before call tech->write.

ASTERISK-27981 #close

Change-Id: Ife3e65e5f18c902db2ff0538fccf7d28f88fa803
---
M res/res_fax.c
1 file changed, 9 insertions(+), 1 deletion(-)

Approvals:
  George Joseph: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Approved for Submit



diff --git a/res/res_fax.c b/res/res_fax.c
index 7338507..647ec2a 100644
--- a/res/res_fax.c
+++ b/res/res_fax.c
@@ -1106,6 +1106,7 @@
 			s->details->caps &= ~AST_FAX_TECH_GATEWAY;
 		}
 		ao2_ref(s->details, -1);
+		s->details = NULL;
 	}
 
 	if (s->debug_info) {
@@ -2915,7 +2916,8 @@
 	int start_res;
 
 	/* if the fax gateway is already started then do nothing */
-	if (gateway->s && gateway->s->state != AST_FAX_STATE_RESERVED) {
+	if (gateway->s &&
+		gateway->s->state != AST_FAX_STATE_RESERVED && gateway->s->state != AST_FAX_STATE_INACTIVE) {
 		return 0;
 	}
 
@@ -3510,6 +3512,12 @@
 	/* in gateway mode, gateway some packets */
 	if (gateway->t38_state == T38_STATE_NEGOTIATED) {
 		struct ast_trans_pvt *readtrans;
+
+		if (!gateway->s || !gateway->s->tech_pvt) {
+			ast_log(LOG_ERROR, "no FAX session on chan %s for T.38 gateway session, odd", ast_channel_name(chan));
+			return f;
+		}
+
 		/* framehooks are called in __ast_read() before frame format
 		 * translation is done, so we need to translate here */
 		if ((f->frametype == AST_FRAME_VOICE) && (ast_format_cmp(f->subclass.format, ast_format_slin) != AST_FORMAT_CMP_EQUAL)

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11417
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ife3e65e5f18c902db2ff0538fccf7d28f88fa803
Gerrit-Change-Number: 11417
Gerrit-PatchSet: 1
Gerrit-Owner: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190604/58325fdc/attachment.html>