From a16e6f12ea8191addbd261bb0d56684a58bdf46d Mon Sep 17 00:00:00 2001 From: Ken Raeburn Date: Wed, 15 Dec 2004 03:02:43 +0000 Subject: [PATCH] insufficient locking in profile re-reading case If profiles are open and iterators in use while the on-disk file is being modified (see tests/threads/prof1.c), the re-reading of the file can cause data to be freed up. The iterator code does no locking and assumes that the profile node tree won't be touched. During our Monday meeting we discussed changing the iterator code to "snapshot" the current state of the file if it were modified, so that a more consistent picture could be returned, essentially by bumping a reference count for the life of the iterator object. The reference count I was thinking of turns out to be used for a different purpose; we'd have to add another layer of indirection, another ref count, and another mutex to accomplish this. There might be a more reasonable way to go about it, but I don't want to tackle it for 1.4 when we're already shipping beta releases. This patch just adds locking to the current iterator code so that the file data can't be replaced while the iterator is being processed. The inconsistent-view issue remains. * prof_tree.c (profile_node_iterator): When the iterator has a current file, lock it, and unlock it before changing it or returning. ticket: new status: resolved target_version: 1.4 tags: pullup git-svn-id: svn://anonsvn.mit.edu/krb5/trunk@16932 dc483132-0cff-0310-8789-dd5450dbe970 --- src/util/profile/ChangeLog | 6 ++++++ src/util/profile/prof_tree.c | 42 +++++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/util/profile/ChangeLog b/src/util/profile/ChangeLog index 1499d25a0..bfe663636 100644 --- a/src/util/profile/ChangeLog +++ b/src/util/profile/ChangeLog @@ -1,3 +1,9 @@ +2004-12-14 Ken Raeburn + + * prof_tree.c (profile_node_iterator): When the iterator has a + current file, lock it, and unlock it before changing it or + returning. + 2004-11-04 Alexandra Ellwood * prof_init.c, profile.hin: added profile_is_modified diff --git a/src/util/profile/prof_tree.c b/src/util/profile/prof_tree.c index 1d96ffbb8..eea34f60b 100644 --- a/src/util/profile/prof_tree.c +++ b/src/util/profile/prof_tree.c @@ -466,17 +466,27 @@ errcode_t profile_node_iterator(void **iter_p, struct profile_node **ret_node, * If the file has changed, then the node pointer is invalid, * so we'll have search the file again looking for it. */ + if (iter->file) { + retval = k5_mutex_lock(&iter->file->data->lock); + if (retval) + return retval; + } if (iter->node && (iter->file->data->upd_serial != iter->file_serial)) { iter->flags &= ~PROFILE_ITER_FINAL_SEEN; skip_num = iter->num; iter->node = 0; } - if (iter->node && iter->node->magic != PROF_MAGIC_NODE) + if (iter->node && iter->node->magic != PROF_MAGIC_NODE) { + if (iter->file) + k5_mutex_unlock(&iter->file->data->lock); return PROF_MAGIC_NODE; + } get_new_file: if (iter->node == 0) { if (iter->file == 0 || (iter->flags & PROFILE_ITER_FINAL_SEEN)) { + if (iter->file) + k5_mutex_unlock(&iter->file->data->lock); profile_node_iterator_free(iter_p); if (ret_node) *ret_node = 0; @@ -486,10 +496,18 @@ get_new_file: *ret_value =0; return 0; } + k5_mutex_unlock(&iter->file->data->lock); if ((retval = profile_update_file(iter->file))) { if (retval == ENOENT || retval == EACCES) { /* XXX memory leak? */ iter->file = iter->file->next; + if (iter->file) { + retval = k5_mutex_lock(&iter->file->data->lock); + if (retval) { + profile_node_iterator_free(iter_p); + return retval; + } + } skip_num = 0; retval = 0; goto get_new_file; @@ -498,6 +516,11 @@ get_new_file: return retval; } } + retval = k5_mutex_lock(&iter->file->data->lock); + if (retval) { + profile_node_iterator_free(iter_p); + return retval; + } iter->file_serial = iter->file->data->upd_serial; /* * Find the section to list if we are a LIST_SECTION, @@ -518,7 +541,15 @@ get_new_file: iter->flags |= PROFILE_ITER_FINAL_SEEN; } if (!section) { + k5_mutex_unlock(&iter->file->data->lock); iter->file = iter->file->next; + if (iter->file) { + retval = k5_mutex_lock(&iter->file->data->lock); + if (retval) { + profile_node_iterator_free(iter_p); + return retval; + } + } skip_num = 0; goto get_new_file; } @@ -546,11 +577,20 @@ get_new_file: } iter->num++; if (!p) { + k5_mutex_unlock(&iter->file->data->lock); iter->file = iter->file->next; + if (iter->file) { + retval = k5_mutex_lock(&iter->file->data->lock); + if (retval) { + profile_node_iterator_free(iter_p); + return retval; + } + } iter->node = 0; skip_num = 0; goto get_new_file; } + k5_mutex_unlock(&iter->file->data->lock); if ((iter->node = p->next) == NULL) iter->file = iter->file->next; if (ret_node) -- 2.26.2