| Summary: | ASTERISK-11106: [patch?] SIP t38state is not properly set | ||
| Reporter: | Dmitry Andrianov (dimas) | Labels: | |
| Date Opened: | 2007-12-23 18:52:51.000-0600 | Date Closed: | 2008-01-29 10:57:19.000-0600 |
| Priority: | Minor | Regression? | No |
| Status: | Closed/Complete | Components: | Channels/chan_sip/T.38 |
| Versions: | Frequency of Occurrence | ||
| Related Issues: | |||
| Environment: | Attachments: | ( 0) sip-t38state.patch ( 1) v2-sip-t38state.patch | |
| Description: | There is a bug in the current implementation of process_sdp when it finds T38 UDPTL information. The code sets state to either T38_PEER_REINVITE or T38_PEER_DIRECT depending on if it is a reinvite or not. However the process_sdp is also called to process 200 OK message we receive in reply to our T38 INVITE. As result, when we reinvite for T38, t38state just changed to T38_LOCAL_REINVITE will be reset to T38_PEER_something upon receipt of OK and never reaches T38_ENABLED. ****** ADDITIONAL INFORMATION ****** The patch fixes the problem for me but it MUST be reviewed by some with good SIP knowledge (oej, file?) because my understanding of which messages may contain SDP with T38 may be wrong. About the patch: I added third parameter t38action to process_sdp which may contain thsse flags: /* Presense of T38 options means that remote side initiates T38 to us */ #define SDP_T38_INITIATE 1 /* Presense of T38 options means that remote side accepts our T38 proposal */ #define SDP_T38_ACCEPT 2 So the current t38state + presence of T38 data + value of t38action flags allows calculation of new value for t38state. I changed all invocations of process_sdp to pass the third parameter. When we received an INVITE, we know that presence of T38 in SDP indicates remote side initiates T38 session while presense of T38 in 200 OK SDP means that remote accepts our proposal. To my understanding, for other invocations of process_sdp (like for 180 Ringing, 182 Queued, or ACKs) we should not expect T38 data at all so zero action is passed. This is the place where review is needed mostly. | ||
| Comments: | By: Olle Johansson (oej) 2008-01-22 09:25:53.000-0600 Sorry that no one has answered you. I'll assign this to file as our T.38 expert :-) By: Dmitry Andrianov (dimas) 2008-01-27 03:34:40.000-0600 Patch updated for the latest trunk. By: Digium Subversion (svnbot) 2008-01-28 14:38:49.000-0600 Repository: asterisk Revision: 100671 U trunk/channels/chan_sip.c ------------------------------------------------------------------------ r100671 | file | 2008-01-28 14:38:48 -0600 (Mon, 28 Jan 2008) | 6 lines Fix up some T38 state change issues. (closes issue ASTERISK-11106) Reported by: dimas Patches: v2-sip-t38state.patch uploaded by dimas (license 88) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=100671 By: Digium Subversion (svnbot) 2008-01-29 10:57:19.000-0600 Repository: asterisk Revision: 100881 _U team/murf/bug11210/ U team/murf/bug11210/apps/app_voicemail.c U team/murf/bug11210/build_tools/menuselect-deps.in U team/murf/bug11210/channels/Makefile U team/murf/bug11210/channels/chan_h323.c U team/murf/bug11210/channels/chan_iax2.c U team/murf/bug11210/channels/chan_local.c U team/murf/bug11210/channels/chan_mgcp.c U team/murf/bug11210/channels/chan_misdn.c U team/murf/bug11210/channels/chan_sip.c A team/murf/bug11210/channels/chan_vpb.cc U team/murf/bug11210/channels/chan_zap.c A team/murf/bug11210/configs/vpb.conf.sample U team/murf/bug11210/configure U team/murf/bug11210/configure.ac U team/murf/bug11210/doc/tex/channelvariables.tex U team/murf/bug11210/include/asterisk/autoconfig.h.in U team/murf/bug11210/include/asterisk/channel.h U team/murf/bug11210/include/asterisk/sched.h U team/murf/bug11210/main/cdr.c U team/murf/bug11210/main/channel.c U team/murf/bug11210/main/dnsmgr.c U team/murf/bug11210/main/features.c U team/murf/bug11210/main/file.c U team/murf/bug11210/main/logger.c U team/murf/bug11210/main/pbx.c U team/murf/bug11210/main/rtp.c U team/murf/bug11210/makeopts.in U team/murf/bug11210/pbx/pbx_dundi.c ------------------------------------------------------------------------ r100881 | murf | 2008-01-29 10:57:16 -0600 (Tue, 29 Jan 2008) | 213 lines Merged revisions 100488,100497,100514,100532-100533,100549,100565,100582,100625,100627-100628,100630-100632,100671,100674,100676-100679 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ................ r100488 | tilghman | 2008-01-27 15:35:29 -0700 (Sun, 27 Jan 2008) | 19 lines Merged revisions 100465 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r100465 | tilghman | 2008-01-27 15:59:53 -0600 (Sun, 27 Jan 2008) | 11 lines When deleting a task from the scheduler, ignoring the return value could possibly cause memory to be accessed after it is freed, which causes all sorts of random memory corruption. Instead, if a deletion fails, wait a bit and try again (noting that another thread could change our taskid value). (closes issue ASTERISK-10897) Reported by: flujan Patches: 20080124__bug11386.diff.txt uploaded by Corydon76 (license 14) Tested by: Corydon76, flujan, stuarth` ........ ................ r100497 | tilghman | 2008-01-27 16:14:48 -0700 (Sun, 27 Jan 2008) | 5 lines With the switch to the ast_sched_replace* API in trunk, we lose the correction that was just merged from 1.4, so this is a changeover to those APIs to use the macro versions, so that we properly detect errors from ast_sched_del, instead of simply ignoring the return values. ................ r100514 | russell | 2008-01-27 17:56:14 -0700 (Sun, 27 Jan 2008) | 5 lines These readlocks always fail for me on my mac, and I saw it happen again today on another mac. We ignore the return value of locking operations almost everywhere in Asterisk. So, ignore these, as well, so Asterisk will actually work on systems where this is occurring while I look into what the issue is. ................ r100532 | russell | 2008-01-27 21:30:44 -0700 (Sun, 27 Jan 2008) | 3 lines - Simplify a line with ARRAY_LEN() - Make a few little formatting changes ................ r100533 | russell | 2008-01-27 21:43:14 -0700 (Sun, 27 Jan 2008) | 2 lines Make a couple more uses of ARRAY_LEN, and convert some spaces to tabs ................ r100549 | file | 2008-01-28 06:57:38 -0700 (Mon, 28 Jan 2008) | 4 lines Don't do a network byte order conversion when setting the socket's port variable to that of bindaddr's. It is already in the correct network byte order. (closes issue ASTERISK-11268) Reported by: hmodes ................ r100565 | russell | 2008-01-28 07:27:28 -0700 (Mon, 28 Jan 2008) | 2 lines Clean up some formatting, and simplify a bit of code using ast_str ................ r100582 | russell | 2008-01-28 10:21:24 -0700 (Mon, 28 Jan 2008) | 17 lines Merged revisions 100581 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r100581 | russell | 2008-01-28 11:15:41 -0600 (Mon, 28 Jan 2008) | 9 lines Make some deadlock related fixes. These bugs were discovered and reported internally at Digium by Steve Pitts. - Fix up chan_local to ensure that the channel lock is held before the local pvt lock. - Don't hold the channel lock when executing the timing function, as it can cause a deadlock when using chan_local. This actually changes the code back to be how it was before the change for issue ASTERISK-10339. But, I added some other locking that I think will prevent the problem reported there, as well. ........ ................ r100625 | qwell | 2008-01-28 11:24:40 -0700 (Mon, 28 Jan 2008) | 9 lines Merged revisions 100624 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r100624 | qwell | 2008-01-28 12:23:09 -0600 (Mon, 28 Jan 2008) | 1 line Correct a comment which made little/no sense. ........ ................ r100627 | russell | 2008-01-28 11:27:08 -0700 (Mon, 28 Jan 2008) | 15 lines Merged revisions 100626 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r100626 | russell | 2008-01-28 12:26:31 -0600 (Mon, 28 Jan 2008) | 7 lines Fix a crash in ast_masq_park_call() (issue ASTERISK-10856) Reported by: DEA Patches: res_features-park.txt uploaded by DEA (license 3) ........ ................ r100628 | tilghman | 2008-01-28 11:27:29 -0700 (Mon, 28 Jan 2008) | 3 lines Normalize the detection for execinfo, so that Linux (glibc) and other platforms with libexecinfo will generate inline stack backtraces correctly. ................ r100630 | russell | 2008-01-28 11:38:56 -0700 (Mon, 28 Jan 2008) | 13 lines Merged revisions 100629 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r100629 | russell | 2008-01-28 12:34:20 -0600 (Mon, 28 Jan 2008) | 5 lines For some reason, the use of this strdupa() is leading to memory corruption on freebsd sparc64. This trivial workaround fixes it. (closes issue ASTERISK-9956, closes issue ASTERISK-11316, reported by mattias04 and Home-of-the-Brave) ........ ................ r100631 | russell | 2008-01-28 11:41:23 -0700 (Mon, 28 Jan 2008) | 3 lines Merge rev 100626 from Asterisk 1.4. The svnmerge of this commit was a NoOp, since res_features doesn't exist in trunk. Thanks to qwell for pointing it out! ................ r100632 | file | 2008-01-28 12:04:53 -0700 (Mon, 28 Jan 2008) | 2 lines Fix up two scheduling issues. In one instance a scheduled item was not deleted when it should have been and in the other it was scheduled again when it shouldn't have been. ................ r100671 | file | 2008-01-28 13:40:08 -0700 (Mon, 28 Jan 2008) | 6 lines Fix up some T38 state change issues. (closes issue ASTERISK-11106) Reported by: dimas Patches: v2-sip-t38state.patch uploaded by dimas (license 88) ................ r100674 | mmichelson | 2008-01-28 13:58:12 -0700 (Mon, 28 Jan 2008) | 10 lines Blocked revisions 100673 via svnmerge ........ r100673 | mmichelson | 2008-01-28 14:55:56 -0600 (Mon, 28 Jan 2008) | 3 lines Undoing the deprecation of chan_vpb. It is alive and well. ........ ................ r100676 | qwell | 2008-01-28 14:02:11 -0700 (Mon, 28 Jan 2008) | 16 lines Merged revisions 100672 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 (closes issue ASTERISK-11263) ........ r100672 | qwell | 2008-01-28 14:42:43 -0600 (Mon, 28 Jan 2008) | 7 lines When using ODBC_STORAGE, make sure we put greeting files into the database like we do with the others. Issue ASTERISK-11263 Reported by: dimas Patches: vmgreet.patch uploaded by dimas (license 88) ........ ................ r100677 | tilghman | 2008-01-28 14:05:29 -0700 (Mon, 28 Jan 2008) | 10 lines Merged revisions 100675 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r100675 | tilghman | 2008-01-28 15:02:02 -0600 (Mon, 28 Jan 2008) | 2 lines WaitExten didn't handle AbsoluteTimeout properly (went to 't' instead of 'T') ........ ................ r100678 | mmichelson | 2008-01-28 14:07:18 -0700 (Mon, 28 Jan 2008) | 3 lines Re-inserting chan_vpb into trunk. ................ r100679 | qwell | 2008-01-28 14:11:24 -0700 (Mon, 28 Jan 2008) | 1 line Reintroduce more chan_vpb stuff that was removed in r100421 and r100422 ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=100881 | ||