[Home]

Summary:ASTERISK-13127: Invalid SDP attributes for boolean T.38 parameters (T38FaxFillBitRemoval, etc.)
Reporter:Dmitry Semyonov (linulin)Labels:
Date Opened:2008-11-26 10:39:31.000-0600Date Closed:2009-01-06 09:09:25.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/T.38
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_sip.c.1.4.diff
( 1) chan_sip.c.1.4-relaxedT38_update1.diff
( 2) chan_sip.c.1.4-relaxedT38.diff
( 3) chan_sip.c.1.4-update1.diff
( 4) chan_sip.c.1.6.0.diff
( 5) chan_sip.c.1.6.0-relaxedT38_update1.diff
( 6) chan_sip.c.1.6.0-relaxedT38.diff
( 7) chan_sip.c.1.6.0-update1.diff
( 8) chan_sip.c.1.6.1.diff
( 9) chan_sip.c.1.6.1-relaxedT38_update1.diff
(10) chan_sip.c.1.6.1-relaxedT38.diff
(11) chan_sip.c.1.6.1-update1.diff
Description:SDP part with enabled T38FaxFillBitRemoval, T38FaxTranscodingMMR, and T38FaxTranscodingJBIG parameters must look like:

 a=T38FaxFillBitRemoval
 a=T38FaxTranscodingMMR
 a=T38FaxTranscodingJBIG

instead of:

 a=T38FaxFillBitRemoval:1
 a=T38FaxTranscodingMMR:1
 a=T38FaxTranscodingJBIG:1

SDP part with disabled T38FaxFillBitRemoval, T38FaxTranscodingMMR, and T38FaxTranscodingJBIG parameters must be empty instead of:

 a=T38FaxFillBitRemoval:0
 a=T38FaxTranscodingMMR:0
 a=T38FaxTranscodingJBIG:0

Otherwise interoperability issues with other T.38 implementations are likely to arise.

Quoting ITU-T Recommendation T.38:

Section “D.2.3.1 UDPTL and TCP negotiation”:

“Note that options without values are boolean – their presence indicates that they are valid for the session.”

Section “D.2.3.2 RTP negotiation”:

Those parameters are supplied in a semi-colon separated list of “parameter” or “parameter=value” pairs using the “a=fmtp” parameter defined in SDP; the “parameter” form is used for boolean values, where presence equals “true” and absence “false”.


****** ADDITIONAL INFORMATION ******

It seems channels/chan_sip.c requires a few trivial modifications to fix the issue
Comments:By: Roman Yeryomin (leroi05) 2008-12-01 10:18:36.000-0600

did you have it fixed?
can you post a patch?
By: Dmitry Semyonov (linulin) 2008-12-02 12:27:11.000-0600

http://www.callweaver.org/ticket/476
By: Arcadiy Ivanov (arcivanov) 2008-12-19 20:57:15.000-0600

The proposed patches are attached.
NOTE: I found that near
              if ((sscanf(a, "T38FaxTranscodingJBIG:%d", &x) == 1))
the if/else/if chain was broken for not apparent reason. I rejoined it to the code above.

The patches were made against 1.4, 1.6.0 and 1.6.1 branches as of 2008-12-19 around 9PM EST.


By: Arcadiy Ivanov (arcivanov) 2008-12-19 23:02:32.000-0600

I'm testing right now with Gafachi <=> NAT <=> Asterisk <=> Grandstream 286v3, and it appears that Grandstream violates the RFC in exactly the same way Asterisk does. Can someone weigh-in, whether we want to support both schemes?
By: Arcadiy Ivanov (arcivanov) 2008-12-20 19:28:07.000-0600

Please delete the first version of the files, since they contain a few problems.
"Update1" solves the bugs and allows * to understand non-compliant peers (i.e. Grandstream). This patch, however, will make * respond in RFC-compliant manner only.
By: Arcadiy Ivanov (arcivanov) 2008-12-20 21:14:03.000-0600

Since we're on the subject of standards compliance. Below please find the excerpt from T.38 negotiation between Gafachi <=> NAT <=> Asterisk <=> Grandstream 286v3:

=====================================================

INVITE sip:fax1 SIP/2.0
User-Agent: Grandstream HT287 1.1.0.30
v=0
o=fax1 8000 8001 IN IP4 iphidden
s=SIP Call
c=IN IP4 iphidden
t=0 0
m=image 60200 udptl t38
a=T38FaxVersion:0
a=T38MaxBitRate:9600
a=T38FaxFillBitRemoval:0
a=T38FaxTranscodingMMR:0
a=T38FaxTranscodingJBIG:0
a=T38FaxRateManagement:transferredTCF
a=T38FaxMaxBuffer:400
a=T38FaxMaxDatagram:280
a=T38FaxUdpEC:t38UDPRedundancy



INVITE sip:asterisk-proxy SIP/2.0
User-Agent: Asterisk PBX
v=0
o=root 16579 16580 IN IP4 iphidden
s=session
c=IN IP4 iphidden
t=0 0
m=image 60198 udptl t38
a=T38FaxVersion:0
a=T38MaxBitRate:9600
a=T38FaxRateManagement:transferredTCF
a=T38FaxMaxBuffer:280
a=T38FaxMaxDatagram:280
a=T38FaxUdpEC:t38UDPRedundancy



SIP/2.0 200 OK
Server: Gafachi UAS v110.08
v=0
o=root 278153574 278153575 IN IP4 iphidden
s=session
c=IN IP4 iphidden
t=0 0
m=image 46570 udptl t38
a=T38FaxVersion:0
a=T38FaxRateManagement:transferredTCFlocalTCF
a=T38FaxUdpEC:t38UDPFEC
a=T38FaxMaxBufferSize:2000
a=T38MaxDatagram:512
a=T38FaxMaxRate:14400

=====================================================


Notice that when Grandstream and Asterisk use T38FaxMaxDatagram and T38MaxBitRate, Gafachi UAS uses T38MaxDatagram and T38FaxMaxRate respectively.

Gafachi UAS is in violation of T.38 04/2004. In Gafachi's defense T.38 Rec. contains INCORRECT parameters, but only in examples (no where in actual BNF).

I'm going to add patches to make sure we understand and answer with both compliant and non-compliant parameters. Stay tuned.
By: Arcadiy Ivanov (arcivanov) 2008-12-21 00:01:38.000-0600

Uploaded relaxed T.38 patches mentioned in a previous note. They are separate and distinct from the "update1" patches and might require some massaging to be applied together.
By: Mark Michelson (mmichelson) 2008-12-23 13:13:36.000-0600

arcivanov: Thanks for all the patches.

I've taken a look at them, and all the _update patches look good by my eyes. As far as the _relaxed patches go, I'm all for being able to accept the non-standard T38 a parameters that the Gafachi uses, but I'm hesitant about also adding those lines to our outgoing SDPs. I'll have to give it some more thought.
By: Arcadiy Ivanov (arcivanov) 2008-12-23 21:34:43.000-0600

putnopvut: Thanks for your comments.

I tried using my relaxed patch with IPcomms.net provider and the non-standard attributes caused IPcomms to refuse re-invite with 488. Not including the Gafachi-specific non-standard attributes fixed the problem. While including the Gafachi-specific attributes could be controlled with something like "t38pt_gafachi" parameter in the config file, I think I agree with you that even doing that wouldn't be kosher. I will, therefore, update the _relaxed patches to understand the non-compliant peers, but only respond in a compliant manner.
By: Arcadiy Ivanov (arcivanov) 2008-12-23 21:50:49.000-0600

uploaded relaxedT38_update1 patches
By: Mark Michelson (mmichelson) 2009-01-05 10:23:18.000-0600

I'm finally back in the office and the bugtracker is actually up, so I'll take a closer look at the patches today. I suspect that if the _update1 patches are of the same quality as the rest then there will be no problems.

Thanks for your submissions :)
By: Digium Subversion (svnbot) 2009-01-05 10:51:56.000-0600

Repository: asterisk
Revision: 167179

U   branches/1.4/channels/chan_sip.c

------------------------------------------------------------------------
r167179 | mmichelson | 2009-01-05 10:51:55 -0600 (Mon, 05 Jan 2009) | 41 lines

A couple of changes to T.38 SDP attribute handling

There are some boolean attributes for T.38 such
as T38FaxFillBitRemoval, T38FaxTranscodingMMR, and
T38FaxTranscodingJBIG. By simply being present, we
should treat these as a "true" value. The current
code, however, was requiring a 1 or 0 as the value
of the attribute in order to parse it. This is due
to the fact that there are some T.38 endpoints and
gateways that also transmit this information
incorrectly. This patch follows the "be liberal in
what you accept and strict in what you send"
philosophy by accepting both the correctly- and
incorrectly-formatted attributes, but only sending
information as it is supposed to be sent.

It was also discovered that a particular type of
T.38 gateway sends some non-standard T.38 SDP
attributes. Instead of using T38FaxMaxDatagram
and T38MaxBitRate, it used T38MaxDatagram and
T38FaxMaxRate respectively. We now will properly
accept these attributes as well.

Note that there are a lot of patches cited in
the below commit message template. This is
because the person who submitted these patches is
an awesome person and wrote 1.4, 1.6.0, and 1.6.1
variants.

(closes issue ASTERISK-13127)
Reported by: linulin
Patches:
    chan_sip.c.1.4-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.0-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.1-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.4-relaxedT38_update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.0-relaxedT38_update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.1-relaxedT38_update1.diff uploaded by arcivanov (license 648)
Tested by: arcivanov


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=167179
By: Digium Subversion (svnbot) 2009-01-05 10:59:32.000-0600

Repository: asterisk
Revision: 167180

_U  trunk/
U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r167180 | mmichelson | 2009-01-05 10:59:32 -0600 (Mon, 05 Jan 2009) | 49 lines

Merged revisions 167179 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r167179 | mmichelson | 2009-01-05 10:51:59 -0600 (Mon, 05 Jan 2009) | 41 lines

A couple of changes to T.38 SDP attribute handling

There are some boolean attributes for T.38 such
as T38FaxFillBitRemoval, T38FaxTranscodingMMR, and
T38FaxTranscodingJBIG. By simply being present, we
should treat these as a "true" value. The current
code, however, was requiring a 1 or 0 as the value
of the attribute in order to parse it. This is due
to the fact that there are some T.38 endpoints and
gateways that also transmit this information
incorrectly. This patch follows the "be liberal in
what you accept and strict in what you send"
philosophy by accepting both the correctly- and
incorrectly-formatted attributes, but only sending
information as it is supposed to be sent.

It was also discovered that a particular type of
T.38 gateway sends some non-standard T.38 SDP
attributes. Instead of using T38FaxMaxDatagram
and T38MaxBitRate, it used T38MaxDatagram and
T38FaxMaxRate respectively. We now will properly
accept these attributes as well.

Note that there are a lot of patches cited in
the below commit message template. This is
because the person who submitted these patches is
an awesome person and wrote 1.4, 1.6.0, and 1.6.1
variants.

(closes issue ASTERISK-13127)
Reported by: linulin
Patches:
    chan_sip.c.1.4-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.0-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.1-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.4-relaxedT38_update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.0-relaxedT38_update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.1-relaxedT38_update1.diff uploaded by arcivanov (license 648)
Tested by: arcivanov


........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=167180
By: Digium Subversion (svnbot) 2009-01-05 11:04:19.000-0600

Repository: asterisk
Revision: 167181

U   branches/1.6.0/channels/chan_sip.c

------------------------------------------------------------------------
r167181 | mmichelson | 2009-01-05 11:04:19 -0600 (Mon, 05 Jan 2009) | 57 lines

Merged revisions 167180 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r167180 | mmichelson | 2009-01-05 10:59:36 -0600 (Mon, 05 Jan 2009) | 49 lines

Merged revisions 167179 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r167179 | mmichelson | 2009-01-05 10:51:59 -0600 (Mon, 05 Jan 2009) | 41 lines

A couple of changes to T.38 SDP attribute handling

There are some boolean attributes for T.38 such
as T38FaxFillBitRemoval, T38FaxTranscodingMMR, and
T38FaxTranscodingJBIG. By simply being present, we
should treat these as a "true" value. The current
code, however, was requiring a 1 or 0 as the value
of the attribute in order to parse it. This is due
to the fact that there are some T.38 endpoints and
gateways that also transmit this information
incorrectly. This patch follows the "be liberal in
what you accept and strict in what you send"
philosophy by accepting both the correctly- and
incorrectly-formatted attributes, but only sending
information as it is supposed to be sent.

It was also discovered that a particular type of
T.38 gateway sends some non-standard T.38 SDP
attributes. Instead of using T38FaxMaxDatagram
and T38MaxBitRate, it used T38MaxDatagram and
T38FaxMaxRate respectively. We now will properly
accept these attributes as well.

Note that there are a lot of patches cited in
the below commit message template. This is
because the person who submitted these patches is
an awesome person and wrote 1.4, 1.6.0, and 1.6.1
variants.

(closes issue ASTERISK-13127)
Reported by: linulin
Patches:
    chan_sip.c.1.4-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.0-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.1-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.4-relaxedT38_update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.0-relaxedT38_update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.1-relaxedT38_update1.diff uploaded by arcivanov (license 648)
Tested by: arcivanov


........

................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=167181
By: Digium Subversion (svnbot) 2009-01-05 11:10:04.000-0600

Repository: asterisk
Revision: 167182

_U  branches/1.6.1/
U   branches/1.6.1/channels/chan_sip.c

------------------------------------------------------------------------
r167182 | mmichelson | 2009-01-05 11:10:04 -0600 (Mon, 05 Jan 2009) | 57 lines

Merged revisions 167180 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r167180 | mmichelson | 2009-01-05 10:59:36 -0600 (Mon, 05 Jan 2009) | 49 lines

Merged revisions 167179 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r167179 | mmichelson | 2009-01-05 10:51:59 -0600 (Mon, 05 Jan 2009) | 41 lines

A couple of changes to T.38 SDP attribute handling

There are some boolean attributes for T.38 such
as T38FaxFillBitRemoval, T38FaxTranscodingMMR, and
T38FaxTranscodingJBIG. By simply being present, we
should treat these as a "true" value. The current
code, however, was requiring a 1 or 0 as the value
of the attribute in order to parse it. This is due
to the fact that there are some T.38 endpoints and
gateways that also transmit this information
incorrectly. This patch follows the "be liberal in
what you accept and strict in what you send"
philosophy by accepting both the correctly- and
incorrectly-formatted attributes, but only sending
information as it is supposed to be sent.

It was also discovered that a particular type of
T.38 gateway sends some non-standard T.38 SDP
attributes. Instead of using T38FaxMaxDatagram
and T38MaxBitRate, it used T38MaxDatagram and
T38FaxMaxRate respectively. We now will properly
accept these attributes as well.

Note that there are a lot of patches cited in
the below commit message template. This is
because the person who submitted these patches is
an awesome person and wrote 1.4, 1.6.0, and 1.6.1
variants.

(closes issue ASTERISK-13127)
Reported by: linulin
Patches:
    chan_sip.c.1.4-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.0-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.1-update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.4-relaxedT38_update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.0-relaxedT38_update1.diff uploaded by arcivanov (license 648)
chan_sip.c.1.6.1-relaxedT38_update1.diff uploaded by arcivanov (license 648)
Tested by: arcivanov


........

................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=167182