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

Tue Oct 20 11:40:42 CDT 2009

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


/trunk/apps/app_externalivr.c
<https://reviewboard.asterisk.org/r/380/#comment2627>

    You could collapse this a bit ...
    
    if (!(ser = create())) {
        goto exit;
    }
    
    start();
    

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

    Make this a size_t


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

    minor wording change ... "is ready to be written"


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

    I guess "threadl" isn't a good name anymore, since it's not a list.


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

    main/astobj2.c does the abs() internally now, so you can remove it here.


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

    You can remove the NULL check here.  ast_free(NULL) is a no-op.


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

    You can remove these NULL checks, too.


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

    "brief\"


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

    If pipe() fails, log an ERROR and include strerror(errno)


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

    "brief\"


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

    This initialization is unnecessary.


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

    This initialization is unnecessary.


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

    Is packet->len not set properly by ast_str_set()?  If not, why not?


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

    FWIW, the lock isn't actually necessary for the write to the alert pipe.  However, it is certainly needed for managing the frame queue.


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

    This is a bit odd - you add the frame to the tail of the queue, but if the alert pipe write fails, you remove a frame off of the head.  I would remove the frame off of the tail.  Alternatively, you could wait to put it on the queue until after the successful write, but before releasing the lock.


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

    should we log a warning here or something?


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

    Here is a suggestion for improvement.
    
    Instead of setting up the pollfd array here, do it inside the for loop.  Also, alternate which fd goes in the 0 and 1 position each time so that we alternate the priority between them.  This is to avoid starvation of one side under load.
    
    Besides, you really should be reinitializing the pollfd array each time anyway, because otherwise, you're leaving fields (such as events) in an unknown state.


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

    Add a space around the '|'


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

    It looks like all of this code is duplicated instead of moved.


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

    1) You don't need to lock the thread container when using an ao2 iterator.
    
    2) You need to add a call to ao2_iterator_destroy().  Otherwise, you leak a container reference.


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

    This can be simplified using inaddrcmp().  inaddrcmp() doesn't check sin_family, but it should never be anything other than AF_INET in the code today.


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

    Check for failure here.


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

    Is a shallow copy safe here?  Doesn't this config have some mallocd strings in it that will get replaced on a reload?


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

    Add ao2_iterator_destroy()


/trunk/include/asterisk/tcptls.h
<https://reviewboard.asterisk.org/r/380/#comment2796>

    Please add doxygen docs for the new API call.


/trunk/main/tcptls.c
<https://reviewboard.asterisk.org/r/380/#comment2797>

    Is this for an unrelated bug you noticed?  If so, we should probably merge it as its own patch.


/trunk/main/tcptls.c
<https://reviewboard.asterisk.org/r/380/#comment2798>

    "brief\"


- Russell


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
> 
>