Author: mjordan Date: Tue Mar 10 12:42:57 2015 New Revision: 432691 URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=432691 Log: localtime: Fix file descriptor leak on kqueue(2) systems
The localtime management in the Asterisk core contains a thread that watches for changes in the local timezone. On systems where the directory containing /etc/localtime is modified frequently, the thread monitoring the changes will be woken up to determine if any changes in timezone have occurred. When using kqueue(2), this can cause a leak of file descriptors due to some improper management of resources. This patch updates the kqueue(2) handling in localtime, such that is no longer leaks resources. Review: https://reviewboard.asterisk.org/r/4450/ ASTERISK-24739 #close Reported by: Ed Hynan patches: 11.15.0-u.diff uploaded by Ed Hynan (Licnese 6680) 11.7.0-u.diff uploaded by Ed Hynan (License 6680) svn-trunk-Jan-26-2015-u.diff uploaded by Ed Hynan (License 6680) Modified: branches/11/main/stdtime/localtime.c Modified: branches/11/main/stdtime/localtime.c URL: http://svnview.digium.com/svn/asterisk/branches/11/main/stdtime/localtime.c?view=diff&rev=432691&r1=432690&r2=432691 ============================================================================== --- branches/11/main/stdtime/localtime.c (original) +++ branches/11/main/stdtime/localtime.c Tue Mar 10 12:42:57 2015 @@ -186,6 +186,60 @@ #endif AST_LIST_ENTRY(state) list; }; + +/* extra initialisation for sstate_alloc() */ +#define SP_STACK_FLAG INT_MIN +#ifdef HAVE_INOTIFY +# define SP_STACK_INIT(sp) do { \ + (sp).wd[0] = SP_STACK_FLAG; \ + } while (0) +# define SP_STACK_CHECK(sp) ((sp)->wd[0] == SP_STACK_FLAG) +# define SP_HEAP_INIT(sp) do { \ + (sp)->wd[0] = -1; \ + (sp)->wd[1] = -1; \ + } while (0) +# define SP_HEAP_FREE(sp) do {} while (0) + +#elif defined(HAVE_KQUEUE) +# define SP_STACK_INIT(sp) do { \ + (sp).fd = SP_STACK_FLAG; \ + } while (0) +# define SP_STACK_CHECK(sp) ((sp)->fd == SP_STACK_FLAG) +#ifdef HAVE_O_SYMLINK +# define SP_HEAP_INIT(sp) do { \ + (sp)->fd = -1; \ + (sp)->fds = -1; \ + } while (0) +# define SP_HEAP_FREE(sp) do { \ + if ( (sp) ) { \ + kqueue_daemon_freestate(sp); \ + if ((sp)->fd > -1) { close((sp)->fd); (sp)->fd = -1; } \ + if ((sp)->fds > -1) { close((sp)->fds); (sp)->fds = -1; } \ + } \ + } while (0) + +#else /* HAVE_O_SYMLINK */ +# define SP_HEAP_INIT(sp) do { \ + (sp)->fd = -1; \ + (sp)->dir = NULL; \ + } while (0) +# define SP_HEAP_FREE(sp) do { \ + if ( (sp) ) { \ + kqueue_daemon_freestate(sp); \ + if ((sp)->fd > -1) { close((sp)->fd); (sp)->fd = -1; } \ + if ((sp)->dir != NULL) { closedir((sp)->dir); (sp)->dir = NULL; } \ + } \ + } while (0) + +#endif /* HAVE_O_SYMLINK */ + +#else /* defined(HAVE_KQUEUE) */ +# define SP_STACK_INIT(sp) do {} while (0) +# define SP_STACK_CHECK(sp) (0) +# define SP_HEAP_INIT(sp) do {} while (0) +# define SP_HEAP_FREE(sp) do {} while (0) + +#endif struct locale_entry { AST_LIST_ENTRY(locale_entry) list; @@ -253,6 +307,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 @@ -274,7 +331,20 @@ struct state *sp; AST_LIST_LOCK(&zonelist); AST_LIST_TRAVERSE(&zonelist, sp, list) { - add_notify(sp, sp->name); + /* ensure sp->name is not relative -- it + * often is -- otherwise add_notify() fails + */ + char name[FILENAME_MAX + 1]; + + if (sp->name[0] == '/') { + snprintf(name, sizeof(name), "%s", sp->name); + } else if (!strcmp(sp->name, TZDEFAULT)) { + snprintf(name, sizeof(name), "/etc/%s", sp->name); + } else { + snprintf(name, sizeof(name), "%s/%s", TZDIR, sp->name); + } + + add_notify(sp, name); } AST_LIST_UNLOCK(&zonelist); } @@ -327,7 +397,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; } } @@ -342,6 +412,13 @@ static void add_notify(struct state *sp, const char *path) { + /* watch for flag indicating stack automatic sp, + * should not be added to watch + */ + if (SP_STACK_CHECK(sp)) { + return; + } + if (inotify_thread == AST_PTHREADT_NULL) { ast_cond_init(&initialization, NULL); ast_mutex_init(&initialization_lock); @@ -376,14 +453,35 @@ #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 + * without zonelist check as a result of some errors + * (any code where tzparse() is called if tzload() fails -- + * tzparse() re-calls tzload() for /usr/share/zoneinfo/posixrules) + * the pointer itself is guarded by the zonelist lock + */ +static struct state *psx_sp = NULL; + +/* collect EVFILT_VNODE fflags in macro; + */ +#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 + 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,55 +508,104 @@ 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. */ AST_LIST_LOCK(&zonelist); - AST_LIST_REMOVE(&zonelist, sp, list); + /* see comment near psx_sp in add_notify() */ + if (sp == psx_sp) { + psx_sp = NULL; + + sstate_free(sp); + + while ((sp = AST_LIST_REMOVE_HEAD(&zonelist, list))) { + sstate_free(sp); + } + } else { + AST_LIST_REMOVE(&zonelist, sp, list); + sstate_free(sp); + } + + /* Just in case the signal was sent late */ + 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); - } } 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] = ""; + + /* watch for flag indicating stack automatic sp, + * should not be added to watch + */ + if (SP_STACK_CHECK(sp) || sp->fd != -1) { + return; + } + + /* some errors might cause repeated calls to tzload() + * for TZDEFRULES more than once if errors repeat, + * so psx_sp is used to keep just one + */ + if (!strcmp(path, TZDEFRULES) || + !strcmp(path, TZDIR "/" TZDEFRULES)) { + int lckgot = AST_LIST_TRYLOCK(&zonelist); + + if (lckgot) { + return; + } + + if (psx_sp != NULL || + (psx_sp = sstate_alloc()) == NULL) { + AST_LIST_UNLOCK(&zonelist); + return; + } + + ast_copy_string(psx_sp->name, TZDIR "/" TZDEFRULES, + sizeof(psx_sp->name)); + sp = psx_sp; + AST_LIST_UNLOCK(&zonelist); + } if (inotify_thread == AST_PTHREADT_NULL) { ast_cond_init(&initialization, NULL); @@ -482,7 +629,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 +666,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 +687,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 +700,7 @@ } } #else + static void *notify_daemon(void *data) { struct stat st, lst; @@ -589,7 +740,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; } } @@ -622,6 +773,26 @@ sp->mtime[1] = st.st_mtime; } #endif + +/* + * struct state allocator with additional setup as needed + */ +static struct state *sstate_alloc(void) +{ + struct state *p = ast_calloc(1, sizeof(*p)); + + if (p != NULL) { + SP_HEAP_INIT(p); + } + + return p; +} + +static void sstate_free(struct state *p) +{ + SP_HEAP_FREE(p); + ast_free(p); +} void ast_localtime_wakeup_monitor(struct ast_test *info) { @@ -737,7 +908,8 @@ } } nread = read(fid, u.buf, sizeof u.buf); - if (close(fid) < 0 || nread <= 0) + /* comp 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 +1036,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. */ @@ -875,6 +1052,12 @@ sp->typecnt + 2 <= TZ_MAX_TYPES) { struct state ts; int result; + + /* for temporary struct state -- + * macro flags the the struct as a stack temp. + * to prevent use within add_notify() + */ + SP_STACK_INIT(ts); u.buf[nread - 1] = '\0'; result = tzparse(&u.buf[1], &ts, FALSE); @@ -1443,7 +1626,7 @@ AST_LIST_LOCK(&zonelist); while ((sp = AST_LIST_REMOVE_HEAD(&zonelist, list))) { - ast_free(sp); + sstate_free(sp); } AST_LIST_UNLOCK(&zonelist); } @@ -1470,17 +1653,17 @@ return sp; } } - AST_LIST_UNLOCK(&zonelist); - - if (!(sp = ast_calloc(1, sizeof *sp))) + + if (!(sp = sstate_alloc())) { + AST_LIST_UNLOCK(&zonelist); return NULL; + } if (tzload(zone, sp, TRUE) != 0) { if (zone[0] == ':' || tzparse(zone, sp, FALSE) != 0) (void) gmtload(sp); } ast_copy_string(sp->name, zone, sizeof(sp->name)); - AST_LIST_LOCK(&zonelist); AST_LIST_INSERT_TAIL(&zonelist, sp, list); AST_LIST_UNLOCK(&zonelist); return sp; @@ -1724,8 +1907,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 +2489,7 @@ long fraction; const char *prevlocale; + buf[0] = '\0';/* Ensure the buffer is initialized. */ if (!format) { return -1; } -- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- svn-commits mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/svn-commits
