From f05d4a68666076476ae1f38d48f6f33e0a407b30 Mon Sep 17 00:00:00 2001 From: Ken Raeburn Date: Wed, 23 Feb 2005 22:47:14 +0000 Subject: [PATCH] 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 --- src/util/profile/ChangeLog | 19 +++++++++++++++++++ src/util/profile/prof_file.c | 12 +++++++----- src/util/profile/prof_int.h | 21 ++++++++++++++++++--- src/util/profile/prof_tree.c | 1 + 4 files changed, 45 insertions(+), 8 deletions(-) diff --git a/src/util/profile/ChangeLog b/src/util/profile/ChangeLog index f5f860271..62da77179 100644 --- a/src/util/profile/ChangeLog +++ b/src/util/profile/ChangeLog @@ -1,3 +1,22 @@ +2005-02-23 Ken Raeburn + + * 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 * prof_file.c (profile_library_initializer, diff --git a/src/util/profile/prof_file.c b/src/util/profile/prof_file.c index 6201dd351..cdeaf50a7 100644 --- 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 b7c90961e..43d15b0e4 100644 --- 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 eea34f60b..4dec26f38 100644 --- 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) -- 2.26.2