Do you have a ticket open on bugs.digium.com for this? If so could you
let me have the number so I can monitor it, if not would you mind opening
one? This could really do with review from people with good understanding
of the Asterisk code.
On 2015/01/29 18:04, Ed Hynan wrote:
> Asterisk does its own TZ time formatting rather than using the
> C library functions. This seems to be so that it can watch the
> zoneinfo files for changes (TZ data updates are not very uncommon
> on GNU/Linux systems). To implement file watching there is
> cpp conditional code for Linux inotify, else for kqueue(2), else
> sleep-and-stat code. The file watch code runs in a thread.
>
> There are a few bugs in the time formatting code. One in particular
> results from the fact that kqueue is used with open file descriptors
> and some non-fatal errors can cause a proc tzparse() to be called
> repeatedly, and tzparse() unconditionally causes a zoneinfo file
> (/usr/share/zoneinfo/posixrules) to be opened and added to the kqueue
> each time, and these descriptors are not associated the state
> structs that the code maintains. Needless to say, I got into this
> code when a call failed and I found Asterisk was out of files.
>
> Next problem is: why the non-fatal errors mentioned above that
> trigger the leak? Seems to be that a macro that removes list
> items from the head of a state list fails to NULL pointers when
> the last item is removed.
>
> There are a couple more misc. bugs addressed in the patch I am
> appending.
>
> The patch is against 11.15.0, and if requested I can post a patch
> against 11.7.0 (5.5 ports), or Asterisk svn trunk of a couple days ago.
>
> The patch might appear to be larger than it would seem if it were
> smaller than it is. It guards against reopening posixrules, adds
> {de,}allocators for the structs that hold descriptors for the queue,
> initializes the (list) empty head, and misc.
>
> -Ed
>
>
> --- main/stdtime/localtime.c.orig Thu Jan 29 15:05:25 2015
> +++ main/stdtime/localtime.c Thu Jan 29 15:25:35 2015
> @@ -253,6 +253,9 @@
> int doextend));
> static int tzparse P((const char * name, struct state * sp,
> int lastditch));
> +/* struct state allocator with additional setup as needed */
> +static struct state * sstate_alloc(void);
> +static void sstate_free(struct state *p);
>
> static AST_LIST_HEAD_STATIC(zonelist, state);
> #ifdef HAVE_NEWLOCALE
> @@ -282,6 +285,20 @@
> #ifdef HAVE_INOTIFY
> static int inotify_fd = -1;
>
> +/* extra initialisation for sstate_alloc() */
> +#define SP_INIT_EX(sp) do { \
> + (sp)->wd[0] = -1; \
> + (sp)->wd[1] = -1; \
> +} while (0)
> +#if 0 /* untested: inotify users can try if they wish */
> +#define SP_FREE_EX(sp) do { \
> + if ((sp)->wd[0] > -1) { inotify_rm_watch(inotify_fd, (sp)->wd[0]);
> (sp)->wd[0] = -1; } \
> + if ((sp)->wd[1] > -1) { inotify_rm_watch(inotify_fd, (sp)->wd[1]);
> (sp)->wd[1] = -1; } \
> +} while (0)
> +#else /* this results in no change */
> +#define SP_FREE_EX(sp) do {;} while (0)
> +#endif
> +
> static void *inotify_daemon(void *data)
> {
> struct {
> @@ -327,7 +344,7 @@
> AST_LIST_TRAVERSE_SAFE_BEGIN(&zonelist, cur, list) {
> if (cur->wd[0] == buf.iev.wd || cur->wd[1] ==
> buf.iev.wd) {
> AST_LIST_REMOVE_CURRENT(list);
> - ast_free(cur);
> + sstate_free(cur);
> break;
> }
> }
> @@ -376,14 +393,60 @@
> #elif defined(HAVE_KQUEUE)
> static int queue_fd = -1;
>
> +/*
> + * static struct state *psx_sp and associated code will guard againt
> + * add_notify() called repeatedly for /usr/share/zoneinfo/posixrules
> + * and leaking a desciptor each time as a result of some errors
> + * (any code where tzparse() is called if tzload() fails --
> + * tzparse() re-calls tzload() for /usr/share/zoneinfo/posixrules)
> + */
> +static struct state *psx_sp = NULL;
> +/*
> + * kqueue_daemon_freestate() called from kqueue_daemon() --
> + * replaces resource release code that was in line there
> + */
> +static void kqueue_daemon_freestate(struct state *);
> +
> +/* collect EVFILT_VNODE fflags in macro;
> + * RENAME and LINK and TRUNCATE added */
> +#ifdef NOTE_TRUNCATE
> +# define EVVN_NOTES_BITS \
> + (NOTE_DELETE|NOTE_WRITE|NOTE_EXTEND|NOTE_REVOKE|NOTE_ATTRIB \
> + |NOTE_RENAME|NOTE_LINK|NOTE_TRUNCATE)
> +#else
> +# define EVVN_NOTES_BITS \
> + (NOTE_DELETE|NOTE_WRITE|NOTE_EXTEND|NOTE_REVOKE|NOTE_ATTRIB \
> + |NOTE_RENAME|NOTE_LINK)
> +#endif
> +
> +/* extra initialisation for sstate_alloc() */
> +#ifdef HAVE_O_SYMLINK
> +#define SP_INIT_EX(sp) do { \
> + (sp)->fd = -1; \
> + (sp)->fds = -1; \
> +} while (0)
> +#define SP_FREE_EX(sp) do { \
> + if ((sp)->fd > -1) { close((sp)->fd); (sp)->fd = -1; } \
> + if ((sp)->fds > -1) { close((sp)->fds); (sp)->fds = -1; } \
> +} while (0)
> +#else
> +#define SP_INIT_EX(sp) do { \
> + (sp)->fd = -1; \
> + (sp)->dir = NULL; \
> +} while (0)
> +#define SP_FREE_EX(sp) do { \
> + if ((sp)->fd > -1) { close((sp)->fd); (sp)->fd = -1; } \
> + if ((sp)->dir != NULL) { closedir((sp)->dir); (sp)->dir = NULL; } \
> +} while (0)
> +#endif
> +
> static void *kqueue_daemon(void *data)
> {
> struct kevent kev;
> struct state *sp;
> - struct timespec no_wait = { 0, 1 };
>
> ast_mutex_lock(&initialization_lock);
> - if ((queue_fd = kqueue()) < 0) {
> + if (queue_fd < 0 && (queue_fd = kqueue()) < 0) {
> /* ast_log uses us to format messages, so if we called ast_log,
> we'd be
> * in for a nasty loop (seen already in testing) */
> fprintf(stderr, "Unable to initialize kqueue(): %s\n",
> strerror(errno));
> @@ -410,56 +473,106 @@
>
> sp = kev.udata;
>
> - /*!\note
> - * If the file event fired, then the file was removed, so we'll
> need
> - * to reparse the entry. The directory event is a bit more
> - * interesting. Unfortunately, the queue doesn't contain
> information
> - * about the file that changed (only the directory itself), so
> unless
> - * we kept a record of the directory state before, it's not
> really
> - * possible to know what change occurred. But if we act
> paranoid and
> - * just purge the associated file, then it will get reparsed,
> and
> - * everything works fine. It may be more work, but it's a vast
> - * improvement over the alternative implementation, which is to
> stat
> - * the file repeatedly in what is essentially a busy loop. */
> + /* see comment near psx_sp in add_notify() */
> + if (sp == psx_sp) {
> + AST_LIST_LOCK(&zonelist);
> + psx_sp = NULL;
> + fprintf(stderr,
> + "kevent %#x on default TZ rules %s\n",
> + (unsigned int)kev.fflags, sp->name);
> +
> + kqueue_daemon_freestate(sp);
> +
> + while ((sp = AST_LIST_REMOVE_HEAD(&zonelist, list))) {
> + kqueue_daemon_freestate(sp);
> + }
> +
> + AST_LIST_HEAD_INIT_NOLOCK(&zonelist);
> + AST_LIST_UNLOCK(&zonelist);
> + } else {
> + AST_LIST_LOCK(&zonelist);
> + AST_LIST_REMOVE(&zonelist, sp, list);
> + kqueue_daemon_freestate(sp);
> + AST_LIST_UNLOCK(&zonelist);
> + }
> +
> + /* Just in case the signal was sent late */
> AST_LIST_LOCK(&zonelist);
> - AST_LIST_REMOVE(&zonelist, sp, list);
> + ast_cond_broadcast(&initialization);
> AST_LIST_UNLOCK(&zonelist);
> + }
>
> + inotify_thread = AST_PTHREADT_NULL;
> + return NULL;
> +}
> +
> +static void kqueue_daemon_freestate(struct state *sp)
> +{
> + struct kevent kev;
> + struct timespec no_wait = { 0, 1 };
> +
> + /*!\note
> + * If the file event fired, then the file was removed, so we'll need
> + * to reparse the entry. The directory event is a bit more
> + * interesting. Unfortunately, the queue doesn't contain information
> + * about the file that changed (only the directory itself), so unless
> + * we kept a record of the directory state before, it's not really
> + * possible to know what change occurred. But if we act paranoid and
> + * just purge the associated file, then it will get reparsed, and
> + * everything works fine. It may be more work, but it's a vast
> + * improvement over the alternative implementation, which is to stat
> + * the file repeatedly in what is essentially a busy loop. */
> +
> + if (sp->fd > -1) {
> /* If the directory event fired, remove the file event */
> EV_SET(&kev, sp->fd, EVFILT_VNODE, EV_DELETE, 0, 0, NULL);
> kevent(queue_fd, &kev, 1, NULL, 0, &no_wait);
> - close(sp->fd);
> + }
>
> #ifdef HAVE_O_SYMLINK
> - if (sp->fds > -1) {
> - /* If the file event fired, remove the symlink event */
> - EV_SET(&kev, sp->fds, EVFILT_VNODE, EV_DELETE, 0, 0,
> NULL);
> - kevent(queue_fd, &kev, 1, NULL, 0, &no_wait);
> - close(sp->fds);
> - }
> + if (sp->fds > -1) {
> + /* If the file event fired, remove the symlink event */
> + EV_SET(&kev, sp->fds, EVFILT_VNODE, EV_DELETE, 0, 0, NULL);
> + kevent(queue_fd, &kev, 1, NULL, 0, &no_wait);
> + }
> #else
> - if (sp->dir) {
> - /* If the file event fired, remove the directory event
> */
> - EV_SET(&kev, dirfd(sp->dir), EVFILT_VNODE, EV_DELETE,
> 0, 0, NULL);
> - kevent(queue_fd, &kev, 1, NULL, 0, &no_wait);
> - closedir(sp->dir);
> - }
> + if (sp->dir) {
> + /* If the file event fired, remove the directory event */
> + EV_SET(&kev, dirfd(sp->dir), EVFILT_VNODE, EV_DELETE, 0, 0,
> NULL);
> + kevent(queue_fd, &kev, 1, NULL, 0, &no_wait);
> + }
> #endif
> - ast_free(sp);
>
> - /* Just in case the signal was sent late */
> - AST_LIST_LOCK(&zonelist);
> - ast_cond_broadcast(&initialization);
> - AST_LIST_UNLOCK(&zonelist);
> - }
> + sstate_free(sp);
> }
>
> static void add_notify(struct state *sp, const char *path)
> {
> struct kevent kev;
> struct timespec no_wait = { 0, 1 };
> - char watchdir[PATH_MAX + 1] = "";
> + char watchdir[PATH_MAX + 1] = "";
>
> + /* this block guards against a result of a failure return
> + * from tzload() in several procs, in which tzparse() is then
> + * called, and that calls tzload() again, unconditionally,
> + * for TZDEFRULES (e.g., /usr/share/zoneinfo/posixrules);
> + * if errors repeat, each call open(2)s and watches and a
> + * descriptor leak is the result. So, make sure only one
> + * 'watch' is used (and if it gets an event, all state are
> + * freed).
> + */
> + if (! strcmp(path, TZDEFRULES)) {
> + if (psx_sp != NULL ||
> + (psx_sp = sstate_alloc()) == NULL) {
> + return;
> + }
> + ast_copy_string(psx_sp->name, path,
> + sizeof(psx_sp->name));
> + sp = psx_sp;
> + } else if (sp->fd != -1) {
> + return;
> + }
> +
> if (inotify_thread == AST_PTHREADT_NULL) {
> ast_cond_init(&initialization, NULL);
> ast_mutex_init(&initialization_lock);
> @@ -482,7 +595,8 @@
> | O_EVTONLY
> # endif
> )) >= 0) {
> - EV_SET(&kev, sp->fds, EVFILT_VNODE, EV_ADD | EV_ENABLE |
> EV_ONESHOT, NOTE_WRITE | NOTE_EXTEND | NOTE_DELETE | NOTE_REVOKE |
> NOTE_ATTRIB, 0, sp);
> + EV_SET(&kev, sp->fds, EVFILT_VNODE, EV_ADD | EV_ENABLE |
> EV_ONESHOT, EVVN_NOTES_BITS, 0, sp);
> + errno = 0;
> if (kevent(queue_fd, &kev, 1, NULL, 0, &no_wait) < 0 && errno
> != 0) {
> /* According to the API docs, we may get -1 return
> value, due to the
> * NULL space for a returned event, but errno should be
> 0 unless
> @@ -518,7 +632,8 @@
> * Likewise, there's no potential leak of a descriptor.
> */
> EV_SET(&kev, dirfd(sp->dir), EVFILT_VNODE, EV_ADD | EV_ENABLE |
> EV_ONESHOT,
> - NOTE_DELETE | NOTE_WRITE | NOTE_EXTEND |
> NOTE_REVOKE | NOTE_ATTRIB, 0, sp);
> + EVVN_NOTES_BITS, 0, sp);
> + errno = 0;
> if (kevent(queue_fd, &kev, 1, NULL, 0, &no_wait) < 0 && errno
> != 0) {
> fprintf(stderr, "Unable to watch '%s': %s\n", watchdir,
> strerror(errno));
> closedir(sp->dir);
> @@ -538,7 +653,8 @@
> return;
> }
>
> - EV_SET(&kev, sp->fd, EVFILT_VNODE, EV_ADD | EV_ENABLE | EV_ONESHOT,
> NOTE_WRITE | NOTE_EXTEND | NOTE_DELETE | NOTE_REVOKE | NOTE_ATTRIB, 0, sp);
> + EV_SET(&kev, sp->fd, EVFILT_VNODE, EV_ADD | EV_ENABLE | EV_ONESHOT,
> EVVN_NOTES_BITS, 0, sp);
> + errno = 0;
> if (kevent(queue_fd, &kev, 1, NULL, 0, &no_wait) < 0 && errno != 0) {
> /* According to the API docs, we may get -1 return value, due
> to the
> * NULL space for a returned event, but errno should be 0 unless
> @@ -550,6 +666,11 @@
> }
> }
> #else
> +
> +/* extra initialisation for sstate_alloc() */
> +#define SP_INIT_EX(sp) do {;} while (0)
> +#define SP_FREE_EX(sp) do {;} while (0)
> +
> static void *notify_daemon(void *data)
> {
> struct stat st, lst;
> @@ -589,7 +710,7 @@
> ast_log(LOG_NOTICE, "Removing cached TZ
> entry '%s' because underlying file changed.\n", name);
> }
> AST_LIST_REMOVE_CURRENT(list);
> - ast_free(cur);
> + sstate_free(cur);
> continue;
> }
> }
> @@ -623,6 +744,28 @@
> }
> #endif
>
> +/*
> + * struct state allocator with additional setup as needed
> + */
> +static struct state *sstate_alloc(void)
> +{
> + struct state *p;
> +
> + if ((p = ast_calloc(1, sizeof(*p))) != NULL) {
> + SP_INIT_EX(p);
> + }
> +
> + return p;
> +}
> +
> +static void sstate_free(struct state *p)
> +{
> + if (p != NULL) {
> + SP_FREE_EX(p);
> + ast_free(p);
> + }
> +}
> +
> void ast_localtime_wakeup_monitor(struct ast_test *info)
> {
> if (inotify_thread != AST_PTHREADT_NULL) {
> @@ -737,7 +880,9 @@
> }
> }
> nread = read(fid, u.buf, sizeof u.buf);
> - if (close(fid) < 0 || nread <= 0)
> + /* comp was nread <= 0;
> + use nread < sizeof u.tzhead against unexpected short files */
> + if (close(fid) < 0 || nread < sizeof u.tzhead)
> return -1;
> for (stored = 4; stored <= 8; stored *= 2) {
> int ttisstdcnt;
> @@ -864,6 +1009,11 @@
> nread -= p - u.buf;
> for (i = 0; i < nread; ++i)
> u.buf[i] = p[i];
> + /* next loop iter. will assume at least
> + sizeof(struct tzhead) bytes */
> + if (nread < sizeof(u.tzhead)) {
> + break;
> + }
> /*
> ** If this is a narrow integer time_t system, we're done.
> */
> @@ -1443,8 +1593,9 @@
>
> AST_LIST_LOCK(&zonelist);
> while ((sp = AST_LIST_REMOVE_HEAD(&zonelist, list))) {
> - ast_free(sp);
> + sstate_free(sp);
> }
> + AST_LIST_HEAD_INIT_NOLOCK(&zonelist);
> AST_LIST_UNLOCK(&zonelist);
> }
>
> @@ -1472,7 +1623,7 @@
> }
> AST_LIST_UNLOCK(&zonelist);
>
> - if (!(sp = ast_calloc(1, sizeof *sp)))
> + if (!(sp = sstate_alloc()))
> return NULL;
>
> if (tzload(zone, sp, TRUE) != 0) {
> @@ -1724,8 +1875,10 @@
> }
>
> if (!sp) {
> - if (!(sp = (struct state *) ast_calloc(1, sizeof *sp)))
> + if (!(sp = sstate_alloc())) {
> + AST_LIST_UNLOCK(&zonelist);
> return NULL;
> + }
> gmtload(sp);
> AST_LIST_INSERT_TAIL(&zonelist, sp, list);
> }
> @@ -2304,6 +2457,7 @@
> long fraction;
> const char *prevlocale;
>
> + buf[0] = '\0';/* Ensure the buffer is initialized. */
> if (!format) {
> return -1;
> }
>
