possible profile null pointer deref in threaded app

There seems to be a problem with a null pointer popping up when
profile_node_iterator reads ...->data->root to start walking through the
contents.  Don't have a lot of details, but I've got some patches that might
tighten things up a little.

* prof_tree.c (profile_node_iterator): Check that the root node pointer is not
null; raise assertion failure if it is.

* prof_int.h: Include k5-platform.h.
(struct _prf_data_t): Reorder fields, and insert some padding.

* prof_file.c (scan_shared_trees_locked): Check that the "root" field isn't
null.
(profile_open_file): Update the in-memory file contents after updating the
refcount instead of before.
(profile_update_file_data): If the root node in the file data is null, always
do the update.  Check that it's not null before returning a success
indication.
(profile_dereference_data_locked): Scan linked list of file data objects for
sanity check, before and after.
(profile_dereference_data_locked): Don't do it here.

ticket: new
status: open

git-svn-id: svn://anonsvn.mit.edu/krb5/trunk@17106 dc483132-0cff-0310-8789-dd5450dbe970

diff --git a/src/util/profile/ChangeLog b/src/util/profile/ChangeLog
index f5f860271e54c580b56d15477afcbb5bccc53b59..62da771798141ecf4933e269a42e2f5165a7d299 100644 (file)
--- a/src/util/profile/ChangeLog
+++ b/src/util/profile/ChangeLog
@@ -1,3 +1,22 @@
+2005-02-23  Ken Raeburn  <raeburn@mit.edu>
+
+       * prof_tree.c (profile_node_iterator): Check that the root node
+       pointer is not null; raise assertion failure if it is.
+
+       * prof_int.h: Include k5-platform.h.
+       (struct _prf_data_t): Reorder fields, and insert some padding.
+
+       * prof_file.c (scan_shared_trees_locked): Check that the "root"
+       field isn't null.
+       (profile_open_file): Update the in-memory file contents after
+       updating the refcount instead of before.
+       (profile_update_file_data): If the root node in the file data is
+       null, always do the update.  Check that it's not null before
+       returning a success indication.
+       (profile_dereference_data_locked): Scan linked list of file data
+       objects for sanity check, before and after.
+       (profile_dereference_data_locked): Don't do it here.
+
 2005-02-08  Ken Raeburn  <raeburn@mit.edu>
 
        * prof_file.c (profile_library_initializer,

diff --git a/src/util/profile/prof_file.c b/src/util/profile/prof_file.c
index 6201dd3516f668c9c7bdbfc989aa8d5e70d096d9..cdeaf50a7704c5c69e3361a2ec5ccfd85bb2b95e 100644 (file)
--- a/src/util/profile/prof_file.c
+++ b/src/util/profile/prof_file.c
@@ -89,6 +89,7 @@ static void profile_free_file_data(prf_data_t);
                assert(d->fslen <= 1000); /* XXX */             \
                assert(d->filespec[d->fslen] == 0);             \
                assert(d->fslen = strlen(d->filespec));         \
+               assert(d->root != NULL);                        \
            }                                                   \
        }
 
@@ -261,9 +262,9 @@ errcode_t profile_open_file(const_profile_filespec_t filespec,
                break;
        }
        if (data) {
-           retval = profile_update_file_data(data);
            data->refcount++;
            (void) k5_mutex_unlock(&g_shared_trees_mutex);
+           retval = profile_update_file_data(data);
            free(expanded_filename);
            prf->data = data;
            *ret_prof = prf;
@@ -328,7 +329,7 @@ errcode_t profile_update_file_data(prf_data_t data)
 #ifdef HAVE_STAT
 #ifdef STAT_ONCE_PER_SECOND
        now = time(0);
-       if (now == data->last_stat) {
+       if (now == data->last_stat && data->root != NULL) {
            k5_mutex_unlock(&data->lock);
            return 0;
        }
@@ -341,7 +342,7 @@ errcode_t profile_update_file_data(prf_data_t data)
 #ifdef STAT_ONCE_PER_SECOND
        data->last_stat = now;
 #endif
-       if (st.st_mtime == data->timestamp) {
+       if (st.st_mtime == data->timestamp && data->root != NULL) {
            k5_mutex_unlock(&data->lock);
            return 0;
        }
@@ -383,6 +384,7 @@ errcode_t profile_update_file_data(prf_data_t data)
            k5_mutex_unlock(&data->lock);
            return retval;
        }
+       assert(data->root != NULL);
 #ifdef HAVE_STAT
        data->timestamp = st.st_mtime;
 #endif
@@ -537,19 +539,19 @@ errcode_t profile_flush_file_data_to_file(prf_data_t data, const char *outfile)
 void profile_dereference_data(prf_data_t data)
 {
     int err;
-    scan_shared_trees_unlocked();
     err = k5_mutex_lock(&g_shared_trees_mutex);
     if (err)
        return;
     profile_dereference_data_locked(data);
     (void) k5_mutex_unlock(&g_shared_trees_mutex);
-    scan_shared_trees_unlocked();
 }
 void profile_dereference_data_locked(prf_data_t data)
 {
+    scan_shared_trees_locked();
     data->refcount--;
     if (data->refcount == 0)
        profile_free_file_data(data);
+    scan_shared_trees_locked();
 }
 
 int profile_lock_global()

diff --git a/src/util/profile/prof_int.h b/src/util/profile/prof_int.h
index b7c90961e9f4997110da3094f6c40bbdd9c2d9bb..43d15b0e44ba5741826b6dd15677073b09c7c4f3 100644 (file)
--- a/src/util/profile/prof_int.h
+++ b/src/util/profile/prof_int.h
@@ -11,6 +11,7 @@
 #endif
 
 #include "k5-thread.h"
+#include "k5-platform.h"
 #include "com_err.h"
 #include "profile.h"
 
@@ -29,14 +30,13 @@ typedef long prf_magic_t;
  * particular configuration file.
  *
  * Locking strategy:
- * - filespec is fixed after creation
+ * - filespec, fslen are fixed after creation
  * - refcount and next should only be tweaked with the global lock held
  * - other fields can be tweaked after grabbing the in-struct lock
  */
 struct _prf_data_t {
        prf_magic_t     magic;
        k5_mutex_t      lock;
-       char            *comment;
        struct profile_node *root;
 #ifdef STAT_ONCE_PER_SECOND
        time_t          last_stat;
@@ -44,12 +44,27 @@ struct _prf_data_t {
        time_t          timestamp; /* time tree was last updated from file */
        int             flags;  /* r/w, dirty */
        int             upd_serial; /* incremented when data changes */
+       char            *comment;
+
+       size_t          fslen;
+
+       /* Some separation between fields controlled by different
+          mutexes.  Theoretically, both could be accessed at the same
+          time from different threads on different CPUs with separate
+          caches.  Don't let the threads clobber each other's
+          changes.  One mutex controlling the whole thing would be
+          better, but sufficient separation might suffice.
+
+          This is icky.  I just hope it's adequate.
+
+          For next major release, fix this.  */
+       union { double d; void *p; UINT64_TYPE ll; k5_mutex_t m; } pad;
+
        int             refcount; /* prf_file_t references */
        struct _prf_data_t *next;
        /* Was: "profile_filespec_t filespec".  Now: flexible char
           array ... except, we need to work in C89, so an array
           length must be specified.  */
-       size_t          fslen;
        const char      filespec[sizeof("/etc/krb5.conf")];
 };
 

diff --git a/src/util/profile/prof_tree.c b/src/util/profile/prof_tree.c
index eea34f60b622ca8f464ff89ace9123fe69f22216..4dec26f3899566701771045b791e84d3b3c60149 100644 (file)
--- a/src/util/profile/prof_tree.c
+++ b/src/util/profile/prof_tree.c
@@ -527,6 +527,7 @@ get_new_file:
                 * or find the containing section if not.
                 */
                section = iter->file->data->root;
+               assert(section != NULL);
                for (cpp = iter->names; cpp[iter->done_idx]; cpp++) {
                        for (p=section->first_child; p; p = p->next) {
                                if (!strcmp(p->name, *cpp) && !p->value)