Re: [asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4450/ --- (Updated March 10, 2015, 12:43 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 432691 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 432443 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 Test r3 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/06/a29ea5c5-3253-429b-aa0f-873bbd652a6f__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
Re: [asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4450/ --- (Updated March 6, 2015, 10:59 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 432443 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 (updated) Test r3 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/06/a29ea5c5-3253-429b-aa0f-873bbd652a6f__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
Re: [asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4450/#review14608 --- Ship it! I've given this several looks, and I think it's good to be shipped. Thanks! - Mark Michelson On March 3, 2015, 3:25 p.m., Ed Hynan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4450/ --- (Updated March 3, 2015, 3:25 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 432443 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 Test r2: patchtest.tgz https://reviewboard.asterisk.org/media/uploaded/files/2015/03/03/e0cb7fb2-68b0-425a-b131-68406094e66e__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
Re: [asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4450/ --- (Updated March 3, 2015, 3:25 p.m.) Review request for Asterisk Developers. Changes --- Review issues addressed, revised patch uploaded which also fixes three new-found bugs (see patch description). Revised test program uploaded, being very mean to localtime.c. 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 432443 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 (updated) localtime.c (11.7.0) test program https://reviewboard.asterisk.org/media/uploaded/files/2015/02/25/f7a19f2a-4a8a-4aff-b7d6-44a4146c358c__patchtest.tgz Test r2: patchtest.tgz https://reviewboard.asterisk.org/media/uploaded/files/2015/03/03/e0cb7fb2-68b0-425a-b131-68406094e66e__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
Re: [asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems
On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
tags/11.7.0/main/stdtime/localtime.c, lines 395-403
https://reviewboard.asterisk.org/r/4450/diff/1/?file=71624#file71624line395
Using a global static struct without locking is not thread safe. More
on this below.
This should be guarded by the zonelist lock, like the list itsel and all struct
state. It is only touched in kqueue_daemon(), where in the next patch all
handling of sp and psx_sp is with the lock, and in add_notify() to which all
code paths should be entered only with the lock held. More below.
On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
tags/11.7.0/main/stdtime/localtime.c, lines 287-302
https://reviewboard.asterisk.org/r/4450/diff/1/?file=71624#file71624line287
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.
Done. Also, names changed to SP_HEAP_{INIT,FREE}, and an additional macro
SP_STACK_INIT for one instance of a temporary struct state as an automatic -- a
bug I hadn't spotted before.
On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
tags/11.7.0/main/stdtime/localtime.c, line 408
https://reviewboard.asterisk.org/r/4450/diff/1/?file=71624#file71624line408
If you move kqueue_daemon_freestate(struct state *sp) above
kqueue_daemon, then you won't need this forward declaration.
I've removed the prototype; also, references to the proc are now only in the
SP_HEAP_FREE macro which is only expanded below the definition.
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.
Ed Hynan wrote:
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.
BTW, don't the ast log functions use time formatting? Are they suitable within
localtime.c?
On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
tags/11.7.0/main/stdtime/localtime.c, lines 765-768
https://reviewboard.asterisk.org/r/4450/diff/1/?file=71624#file71624line765
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 :-)
Done.
On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
tags/11.7.0/main/stdtime/localtime.c, lines 775-778
https://reviewboard.asterisk.org/r/4450/diff/1/?file=71624#file71624line775
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);
}
Done.
On Feb. 25, 2015, 6:15 p.m., Matt Jordan wrote:
tags/11.7.0/main/stdtime/localtime.c, lines 1609-1610
https://reviewboard.asterisk.org/r/4450/diff/1/?file=71624#file71624line1609
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.
Removed. Had been added with a poor picture of the cause of corruption, and
seemed to do the trick, but only due to coincidence with other changes. Works
without it.
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
Re: [asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4450/#review14550 --- - Ed Hynan On March 3, 2015, 3:25 p.m., Ed Hynan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4450/ --- (Updated March 3, 2015, 3:25 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 432443 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 Test r2: patchtest.tgz https://reviewboard.asterisk.org/media/uploaded/files/2015/03/03/e0cb7fb2-68b0-425a-b131-68406094e66e__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
Re: [asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4450/ --- (Updated March 3, 2015, 2:47 p.m.) Review request for Asterisk Developers. Changes --- 1st patch revision: addresses issues raised on review, also addresses more bugs found found during that process: common_startup() was ineffective due to relative paths stored in struct state, ast_tzset() allowed code paths to add_notify() without zonelist lock held, and a temporary struct state on stack rather than heap that should not have been added in add_notify(). (Also, test program revised to more completely exercise the code.) 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 (updated) - tags/11.7.0/main/stdtime/localtime.c 432443 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
Re: [asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems
---
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
Re: [asterisk-dev] [Code Review] 4450: locatime.c file descriptor leak on kqueue(2) systems
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
