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

[Ed Hynan]

> On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
> > tags/11.7.0/main/stdtime/localtime.c, lines 482-484
> > <https://reviewboard.asterisk.org/r/4450/diff/1/?file=71624#file71624line482>
> >
> >     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.

I agree, no need for a message there. Re. stderr, it's used several other places in original code, so it seemed preferred in this context.


> On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
> > tags/11.7.0/main/stdtime/localtime.c, lines 566-587
> > <https://reviewboard.asterisk.org/r/4450/diff/1/?file=71624#file71624line566>
> >
> >     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.

Good eyes. I proceeded thinking calls to tzload() (which calls aadd_notify()) were guarded by AST_LIST_LOCK(&zonelist) but another look now shows a place where tzload()(and tzparse() and gmtload()) are called between an unlock and another lock (ast_tzset() line 1451 unpatched).  Yet other places make the calls while the lock is held.

This is an extra complication.


> On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
> > tags/11.7.0/main/stdtime/localtime.c, lines 293-297
> > <https://reviewboard.asterisk.org/r/4450/diff/1/?file=71624#file71624line293>
> >
> >     We typically don't leave unused code lying around. I'd go ahead and remove this section if it isn't useful.

OK. That's only there because a look at the manpage made me wonder about it; hope was that persons more familiar with Linux inotify would clear it up.

I'll remove it for next patch.


- Ed


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


On Feb. 25, 2015, 4:37 p.m., Ed Hynan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4450/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2015, 4:37 p.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/bbf65f25/attachment-0001.html>