[Home]

Summary:ASTERISK-14765: SIP register via tls causes lock on sip reload
Reporter:David Vossel (dvossel)Labels:
Date Opened:2009-09-14 17:47:53Date Closed:2009-10-22 17:11:37
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/TCP-TLS
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) bt_full
( 1) threads
Description:I ran into this problem because of another issue Asterisk has with TLS.

register => tls://user:1234@mydomain

Given the register line above in sip.conf, I'd expect the register request to be send via port 5061.  Instead it defaults to port 5060 which is incorrect for TLS.

----------------------------------------------------------------
*CLI> sip show registry
Host                           dnsmgr Username       Refresh State                Reg.Time                
mydomain:5060               N      user             120 Request Sent
----------------------------------------------------------------

Now here's the odd part.  I decided to go into my sip.conf and explicitly set the register line to have a port, and when I issued a sip reload everything locked up, even the CLI.  I was able to reproduce the steps and got the same results everytime.

It seems that this is somehow caused by the receiving end binding to port 5061 and me sending the registration using port 5060.  If I bind the receiving end to 5060 this works even though that is not the correct port for tls.  I tried another test where I pointed the register at a fake domain, but nothing locked up.





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

*CLI> sip show peers
Name/username              Host            Dyn Forcerport ACL Port     Status    
5004                       (Unspecified)    D          5060     Unmonitored
5005                       (Unspecified)    D          5060     Unmonitored
5006                       (Unspecified)    D          5060     Unmonitored
5007                       (Unspecified)    D          5060     Unmonitored
5008                       (Unspecified)    D          5060     Unmonitored
6001/6001                  xx.xx.xx.xxx     D          5060     Unmonitored
6002/6002                  xx.xx.xx.xxx     D          5060     Unmonitored
SIP_cupbear                xx.xx.xx.xxx                5061     Unmonitored
SIP_cupbear_reg            (Unspecified)    D          5061     Unmonitored
9 sip peers [Monitored: 0 online, 0 offline Unmonitored: 9 online, 0 offline]
*CLI> sip show registry
Host                           dnsmgr Username       Refresh State                Reg.Time                
cupbear:5060                   N      SIP_manbearp       120 Request Sent                                  
1 SIP registrations.
*CLI> sip reload


Killed
Comments:By: David Vossel (dvossel) 2009-09-21 17:14:37

I know what is causing this now... Here's the situation.  Box1 has tls disabled and tcp enabled. Box2 is trying to register via tls to Box1.  Box1 one accepts the connection as TCP not TLS, and Box2 blocks forever at SSL_connect waiting for the TLS handshake to complete.
By: David Vossel (dvossel) 2009-10-07 14:00:48

there is a patch for this on reviewboard. https://reviewboard.asterisk.org/r/380/
By: Digium Subversion (svnbot) 2009-10-22 15:00:24

Repository: asterisk
Revision: 225445

U   trunk/apps/app_externalivr.c
U   trunk/channels/chan_sip.c
U   trunk/include/asterisk/tcptls.h
U   trunk/main/tcptls.c

------------------------------------------------------------------------
r225445 | dvossel | 2009-10-22 15:00:19 -0500 (Thu, 22 Oct 2009) | 50 lines

SIP TCP/TLS: move client connection setup/write into tcp helper thread, various related locking/memory fixes.

       What this patch fixes
1.Moves sip TCP/TLS connection setup into the TCP helper thread:
 Connection setup takes awhile and before this it was being
 done while holding the monitor lock.
2.Moves TCP/TLS writing to the TCP helper thread:  Through the
 use of a packet queue and an alert pipe, the TCP helper thread
 can now be woken up to write data as well as read data.
3.Locking error: sip_xmit returned an XMIT_ERROR without giving
 up the tcptls_session lock.  This lock has been completely removed
 from sip_xmit and placed in the new sip_tcptls_write() function.
4.Memory leak:  When creating a tcptls_client the tls_cfg was alloced
 but never freed unless the tcptls_session failed to start.  Now the
 session_args for a sip client are an ao2 object which frees the
 tls_cfg on destruction.
5.Pointer to stack variable: During sip_prepare_socket the creation
 of a client's ast_tcptls_session_args was done on the stack and
 stored as a pointer in the newly created tcptls_session.  Depending
 on the events that followed, there was a slight possibility that
 pointer could have been accessed after the stack returned.  Given
 the new changes, it is always accessed after the stack returns
 which is why I found it.

Notable code changes
1.I broke tcptls.c's ast_tcptls_client_start() function into two
 functions.  One for creating and allocating the new tcptls_session,
 and a separate one for starting and handling the new connection.
 This allowed me to create the tcptls_session, launch the helper
 thread, and then establish the connection within the helper thread.
2.Writes to a tcptls_session are now done within the helper thread.
 This is done by using an alert pipe to wake up the thread if new
 data needs to be sent.  The thread's sip_threadinfo object contains
 the alert pipe as well as the packet queue.
3.Since the threadinfo object contains the alert pipe, it must now be
 accessed outside of the helper thread for every write (queuing of a
 packet).  For easy lookup, I moved the threadinfo objects from a
 linked list to an ao2_container.

(closes issue ASTERISK-12433)
Reported by: pabelanger
Tested by: dvossel, whys

(closes issue ASTERISK-14765)
Reported by: dvossel
Tested by: dvossel

Review: https://reviewboard.asterisk.org/r/380/


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

http://svn.digium.com/view/asterisk?view=rev&revision=225445
By: Digium Subversion (svnbot) 2009-10-22 17:00:19

Repository: asterisk
Revision: 225489

_U  branches/1.6.2/
U   branches/1.6.2/apps/app_externalivr.c
U   branches/1.6.2/channels/chan_sip.c
U   branches/1.6.2/include/asterisk/tcptls.h
U   branches/1.6.2/main/tcptls.c

------------------------------------------------------------------------
r225489 | dvossel | 2009-10-22 17:00:16 -0500 (Thu, 22 Oct 2009) | 56 lines

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

........
 r225445 | dvossel | 2009-10-22 14:55:51 -0500 (Thu, 22 Oct 2009) | 50 lines
 
 SIP TCP/TLS: move client connection setup/write into tcp helper thread, various related locking/memory fixes.
 
         What this patch fixes
 1.Moves sip TCP/TLS connection setup into the TCP helper thread:
   Connection setup takes awhile and before this it was being
   done while holding the monitor lock.
 2.Moves TCP/TLS writing to the TCP helper thread:  Through the
   use of a packet queue and an alert pipe, the TCP helper thread
   can now be woken up to write data as well as read data.
 3.Locking error: sip_xmit returned an XMIT_ERROR without giving
   up the tcptls_session lock.  This lock has been completely removed
   from sip_xmit and placed in the new sip_tcptls_write() function.
 4.Memory leak:  When creating a tcptls_client the tls_cfg was alloced
   but never freed unless the tcptls_session failed to start.  Now the
   session_args for a sip client are an ao2 object which frees the
   tls_cfg on destruction.
 5.Pointer to stack variable: During sip_prepare_socket the creation
   of a client's ast_tcptls_session_args was done on the stack and
   stored as a pointer in the newly created tcptls_session.  Depending
   on the events that followed, there was a slight possibility that
   pointer could have been accessed after the stack returned.  Given
   the new changes, it is always accessed after the stack returns
   which is why I found it.
 
 Notable code changes
 1.I broke tcptls.c's ast_tcptls_client_start() function into two
   functions.  One for creating and allocating the new tcptls_session,
   and a separate one for starting and handling the new connection.
   This allowed me to create the tcptls_session, launch the helper
   thread, and then establish the connection within the helper thread.
 2.Writes to a tcptls_session are now done within the helper thread.
   This is done by using an alert pipe to wake up the thread if new
   data needs to be sent.  The thread's sip_threadinfo object contains
   the alert pipe as well as the packet queue.
 3.Since the threadinfo object contains the alert pipe, it must now be
   accessed outside of the helper thread for every write (queuing of a
   packet).  For easy lookup, I moved the threadinfo objects from a
   linked list to an ao2_container.
 
 (closes issue ASTERISK-12433)
 Reported by: pabelanger
 Tested by: dvossel, whys
 
 (closes issue ASTERISK-14765)
 Reported by: dvossel
 Tested by: dvossel
 
 Review: https://reviewboard.asterisk.org/r/380/
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=225489
By: Digium Subversion (svnbot) 2009-10-22 17:11:36

Repository: asterisk
Revision: 225490

_U  branches/1.6.1/
U   branches/1.6.1/apps/app_externalivr.c
U   branches/1.6.1/channels/chan_sip.c
U   branches/1.6.1/include/asterisk/tcptls.h
U   branches/1.6.1/main/tcptls.c

------------------------------------------------------------------------
r225490 | dvossel | 2009-10-22 17:11:33 -0500 (Thu, 22 Oct 2009) | 56 lines

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

........
 r225445 | dvossel | 2009-10-22 14:55:51 -0500 (Thu, 22 Oct 2009) | 50 lines
 
 SIP TCP/TLS: move client connection setup/write into tcp helper thread, various related locking/memory fixes.
 
         What this patch fixes
 1.Moves sip TCP/TLS connection setup into the TCP helper thread:
   Connection setup takes awhile and before this it was being
   done while holding the monitor lock.
 2.Moves TCP/TLS writing to the TCP helper thread:  Through the
   use of a packet queue and an alert pipe, the TCP helper thread
   can now be woken up to write data as well as read data.
 3.Locking error: sip_xmit returned an XMIT_ERROR without giving
   up the tcptls_session lock.  This lock has been completely removed
   from sip_xmit and placed in the new sip_tcptls_write() function.
 4.Memory leak:  When creating a tcptls_client the tls_cfg was alloced
   but never freed unless the tcptls_session failed to start.  Now the
   session_args for a sip client are an ao2 object which frees the
   tls_cfg on destruction.
 5.Pointer to stack variable: During sip_prepare_socket the creation
   of a client's ast_tcptls_session_args was done on the stack and
   stored as a pointer in the newly created tcptls_session.  Depending
   on the events that followed, there was a slight possibility that
   pointer could have been accessed after the stack returned.  Given
   the new changes, it is always accessed after the stack returns
   which is why I found it.
 
 Notable code changes
 1.I broke tcptls.c's ast_tcptls_client_start() function into two
   functions.  One for creating and allocating the new tcptls_session,
   and a separate one for starting and handling the new connection.
   This allowed me to create the tcptls_session, launch the helper
   thread, and then establish the connection within the helper thread.
 2.Writes to a tcptls_session are now done within the helper thread.
   This is done by using an alert pipe to wake up the thread if new
   data needs to be sent.  The thread's sip_threadinfo object contains
   the alert pipe as well as the packet queue.
 3.Since the threadinfo object contains the alert pipe, it must now be
   accessed outside of the helper thread for every write (queuing of a
   packet).  For easy lookup, I moved the threadinfo objects from a
   linked list to an ao2_container.
 
 (closes issue ASTERISK-12433)
 Reported by: pabelanger
 Tested by: dvossel, whys
 
 (closes issue ASTERISK-14765)
 Reported by: dvossel
 Tested by: dvossel
 
 Review: https://reviewboard.asterisk.org/r/380/
........

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

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