[asterisk-dev] [Code Review] SIP TCP/TLS: move client connection setup/write into tcp helper thread, various related locking/memory fixes.

[David Vossel]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/380/#review1113
-----------------------------------------------------------



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/380/#comment2619>

    Making reference to the tcptls_session object within the cleanup label is not safe since we are not guaranteed the tcptls_session still exists (if client connection setup fails, it will be destroyed).  This must be moved above the label.


- David


On 2009-09-28 11:40:51, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/380/
> -----------------------------------------------------------
> 
> (Updated 2009-09-28 11:40:51)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> ---- 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.
> 
> 
> This addresses bug 15894.
>     https://issues.asterisk.org/view.php?id=15894
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/tcptls.h 220675 
>   /trunk/main/tcptls.c 220675 
>   /trunk/apps/app_externalivr.c 220675 
>   /trunk/channels/chan_sip.c 220675 
> 
> Diff: https://reviewboard.asterisk.org/r/380/diff
> 
> 
> Testing
> -------
> 
> Established both TCP and TLS connections, verified registration and call setup continued to work.
> 
> 
> Thanks,
> 
> David
> 
>