clean up ast_verbose(), don't hold the lock any longer than needed
authorKevin P. Fleming <kpfleming@digium.com>
Wed, 14 Sep 2005 23:56:50 +0000 (23:56 +0000)
committerKevin P. Fleming <kpfleming@digium.com>
Wed, 14 Sep 2005 23:56:50 +0000 (23:56 +0000)
git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@6606 65c4cc65-6c06-0410-ace0-fbb531ad65f3

logger.c

index 1727a0f..1d5bace 100755 (executable)
--- a/logger.c
+++ b/logger.c
@@ -812,46 +812,60 @@ void ast_log(int level, const char *file, int line, const char *function, const
        }
 }
 
-extern void ast_verbose(const char *fmt, ...)
+void ast_verbose(const char *fmt, ...)
 {
        static char stuff[4096];
-       static int pos = 0, opos;
-       static int replacelast = 0, complete;
+       static int len = 0;
+       static int replacelast = 0;
+
+       int complete;
+       int olen;
        struct msglist *m;
        struct verb *v;
-       time_t t;
-       struct tm tm;
-       char date[40];
-       char *datefmt;
        
        va_list ap;
        va_start(ap, fmt);
-       ast_mutex_lock(&msglist_lock);
-       time(&t);
-       localtime_r(&t, &tm);
-       strftime(date, sizeof(date), dateformat, &tm);
 
        if (option_timestamp) {
+               time_t t;
+               struct tm tm;
+               char date[40];
+               char *datefmt;
+
+               time(&t);
+               localtime_r(&t, &tm);
+               strftime(date, sizeof(date), dateformat, &tm);
                datefmt = alloca(strlen(date) + 3 + strlen(fmt) + 1);
                if (datefmt) {
                        sprintf(datefmt, "[%s] %s", date, fmt);
                        fmt = datefmt;
                }
        }
-       vsnprintf(stuff + pos, sizeof(stuff) - pos, fmt, ap);
-       opos = pos;
-       pos = strlen(stuff);
 
+       /* this lock is also protecting against multiple threads
+          being in this function at the same time, so it must be
+          held before any of the static variables are accessed
+       */
+       ast_mutex_lock(&msglist_lock);
+
+       /* there is a potential security problem here: if formatting
+          the current date using 'dateformat' results in a string
+          containing '%', then the vsnprintf() call below will
+          probably try to access random memory
+       */
+       vsnprintf(stuff + len, sizeof(stuff) - len, fmt, ap);
+       va_end(ap);
+
+       olen = len;
+       len = strlen(stuff);
+
+       complete = (stuff[len - 1] == '\n') ? 1 : 0;
 
-       if (stuff[strlen(stuff)-1] == '\n') 
-               complete = 1;
-       else
-               complete=0;
        if (complete) {
                if (msgcnt < MAX_MSG_QUEUE) {
                        /* Allocate new structure */
-                       m = malloc(sizeof(struct msglist));
-                       msgcnt++;
+                       if ((m = malloc(sizeof(*m))))
+                               msgcnt++;
                } else {
                        /* Recycle the oldest entry */
                        m = list;
@@ -874,22 +888,18 @@ extern void ast_verbose(const char *fmt, ...)
                        }
                }
        }
-       if (verboser) {
-               v = verboser;
-               while(v) {
-                       v->verboser(stuff, opos, replacelast, complete);
-                       v = v->next;
-               }
-       } /* else
-               fprintf(stdout, stuff + opos); */
+
+       for (v = verboser; v; v = v->next)
+               v->verboser(stuff, olen, replacelast, complete);
+
        ast_log(LOG_VERBOSE, "%s", stuff);
-       if (strlen(stuff)) {
-               if (stuff[strlen(stuff)-1] != '\n') 
+
+       if (len) {
+               if (complete)
                        replacelast = 1;
                else 
-                       replacelast = pos = 0;
+                       replacelast = len = 0;
        }
-       va_end(ap);
 
        ast_mutex_unlock(&msglist_lock);
 }