[asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems

[Matt Jordan]

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



tags/11.7.0/main/stdtime/localtime.c
<https://reviewboard.asterisk.org/r/4450/#comment25100>

    Having the various blocks of SP_INIT_EX/SP_FREE_EX defined around the code is actually a bit confusing, particularly since the structure they initialize/destroy already has various #defines for the various system options it has to support. How about having this defined immediately after struct state, and handling the "flavors" there:
    
    struct state {
        ...
    };
    
    #if defined(HAVE_INOTIFY)
    #define SP_INIT_EX ...
    #define SP_FREE_EX ...
    #elif defined(HAVE_KQUEUE)
    #define SP_INIT_EX ...
    #define SP_FREE_EX ...
    #else
    #define SP_INIT_EX ...
    #define SP_FREE_EX ...
    #endif
    
    That way it's clear why this is getting defined multiple times, and the declaration of the macro immediately following the definition of the structure makes it easy to see why the various variants exist.



tags/11.7.0/main/stdtime/localtime.c
<https://reviewboard.asterisk.org/r/4450/#comment25099>

    We typically don't leave unused code lying around. I'd go ahead and remove this section if it isn't useful.



tags/11.7.0/main/stdtime/localtime.c
<https://reviewboard.asterisk.org/r/4450/#comment25102>

    Using a global static struct without locking is not thread safe. More on this below.



tags/11.7.0/main/stdtime/localtime.c
<https://reviewboard.asterisk.org/r/4450/#comment25104>

    If you move kqueue_daemon_freestate(struct state *sp) above kqueue_daemon, then you won't need this forward declaration.



tags/11.7.0/main/stdtime/localtime.c
<https://reviewboard.asterisk.org/r/4450/#comment25103>

    I'm not sure why we are printing out to stderr here, but generally Asterisk does not do that unless (a) the logger couldn't be created and (b) things have gone horribly wrong. That doesn't seem like that would necessarily be the case here.



tags/11.7.0/main/stdtime/localtime.c
<https://reviewboard.asterisk.org/r/4450/#comment25101>

    This is not actually thread safe. You cannot guarantee that two threads, at the same time, won't attempt to test and allocate the structure and assign it to psx_sp.
    
    What's more, the initialization of localtime happens very early in the Asterisk startup, and has some very interesting race conditions with forking/threading. I would definitely protect initialization of the psx_sp pointer with a lock.



tags/11.7.0/main/stdtime/localtime.c
<https://reviewboard.asterisk.org/r/4450/#comment25105>

    Generally, we tend to prefer allocations to be written as:
    
    p = ast_calloc(1, sizeof(*p));
    if (!p) {
        /* Handle memory error */
    }
    
    It generally prevents paranthesis errors, and newlines are relatively inexpensive :-)



tags/11.7.0/main/stdtime/localtime.c
<https://reviewboard.asterisk.org/r/4450/#comment25106>

    Since ast_free is NULL tolerant, I would go ahead and make SP_FREE_EX NULL tolerant as well. That makes this:
    
    static void sstate_free(struct state *p)
    {
        SP_FREE_EX(p);
        ast_free(p);
    }



tags/11.7.0/main/stdtime/localtime.c
<https://reviewboard.asterisk.org/r/4450/#comment25107>

    This shouldn't be necessary. Called in a loop, AST_LIST_REMOVE_HEAD will:
    
    (1) Remove items until (cur = head->first) == NULL. That implies head->first is NULL when complete.
    (2) Will set head->last = NULL when the (cur == head->last), which should happen on the last loop iteration.
    
    Since AST_LIST_HEAD_INIT_NOLOCK simply sets both of those to NULL, it is redundant with the final loop state.


- Matt Jordan


On Feb. 25, 2015, 10:37 a.m., Ed Hynan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4450/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2015, 10:37 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24739
>     https://issues.asterisk.org/jira/browse/ASTERISK-24739
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> File descriptor leak on kqueue(2) systems (found on OpenBSD 5.5) on main/stdtime/localtime.c -- TZ data files change watch code within the time functions. (Issue at https://issues.asterisk.org/jira/browse/ASTERISK-24739.)
> 
> 
> Diffs
> -----
> 
>   tags/11.7.0/main/stdtime/localtime.c 432235 
> 
> Diff: https://reviewboard.asterisk.org/r/4450/diff/
> 
> 
> Testing
> -------
> 
> Patch initially developed against OpenBSD 5.5 ports package sources, and Asterisk in use. Subsequently developed test program that can run original or patched 11.7.0 code.
> 
> 
> File Attachments
> ----------------
> 
> localtime.c (11.7.0) test program
>   https://reviewboard.asterisk.org/media/uploaded/files/2015/02/25/f7a19f2a-4a8a-4aff-b7d6-44a4146c358c__patchtest.tgz
> 
> 
> Thanks,
> 
> Ed Hynan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150225/4aaa08cc/attachment-0001.html>