> 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
