don't mangle request->session_state_ctx manually

now that it's an actual list, and in the pair_root, we need to
remove it from the pair_root if it's freed, and add it to the
pair_root when it's allocated or restored.

and a check of

	git grep -- 'request->session_state_ctx = '

should return only one assignment in request_state_replace().
Nothing else should muck with the state

diff --git a/src/lib/server/request.c b/src/lib/server/request.c
index 635093fc1046331daa667d9b00f7dcf8616813c5..aaf0eb14df032e83a3b2264ee3d01864d40a93d2 100644 (file)
--- a/src/lib/server/request.c
+++ b/src/lib/server/request.c
@@ -538,6 +538,35 @@ request_t *_request_local_alloc(char const *file, int line, TALLOC_CTX *ctx,
        return request;
 }
 
+/** Replace the session_state_ctx with a new one.
+ *
+ *  NOTHING should rewrite request->session_state_ctx.
+ *
+ *  It's now a pair, and is stored in request->pair_root.
+ *  So it's wrong for anyone other than this function to play games with it.
+ */
+fr_pair_t *request_state_replace(request_t *request, fr_pair_t *state)
+{
+       fr_pair_t *old = request->session_state_ctx;
+
+       fr_assert(request->session_state_ctx != NULL);
+       fr_assert(request->session_state_ctx != state);
+
+       fr_pair_remove(&request->pair_root->children, old);
+
+       /*
+        *      Save (or delete) the existing state, and re-initialize
+        *      it with a brand new one.
+        */
+       if (!state) MEM(state = fr_pair_afrom_da(NULL, request_attr_state));
+
+       request->session_state_ctx = state;
+
+       fr_pair_append(&request->pair_root->children, state);
+
+       return old;
+}
+
 /** Unlink a subrequest from its parent
  *
  * @note This should be used for requests in preparation for freeing them.

diff --git a/src/lib/server/request.h b/src/lib/server/request.h
index 9db498072a62fa1864aebb48343130a6d15b955e..d66e09b93659af61a68c947466126bec4b38c1b3 100644 (file)
--- a/src/lib/server/request.h
+++ b/src/lib/server/request.h
@@ -315,6 +315,8 @@ request_t   *_request_alloc(char const *file, int line, TALLOC_CTX *ctx,
 request_t      *_request_local_alloc(char const *file, int line, TALLOC_CTX *ctx,
                                      request_type_t type, request_init_args_t const *args);
 
+fr_pair_t      *request_state_replace(request_t *request, fr_pair_t *state) CC_HINT(nonnull(1));
+
 int            request_detach(request_t *child);
 
 int            request_global_init(void);

diff --git a/src/lib/server/state.c b/src/lib/server/state.c
index b50057f5d33b69a716313310b0332a5d4ed2f260..59aa7a5ba65512b7deeb58b7efee92d4efbd41b1 100644 (file)
--- a/src/lib/server/state.c
+++ b/src/lib/server/state.c
@@ -629,15 +629,13 @@ void fr_state_discard(fr_state_tree_t *state, request_t *request)
 
        /*
         *      If fr_state_to_request was called, then the request
-        *      holds the state data, and we need to destroy it
+        *      holds the existing state data.  We need to destroy it,
         *      and return the request to the state it was in when
-        *      it was first alloced, just in case a user does something
-        *      stupid like add additional session-state attributes
+        *      it was first allocated, just in case a user does something
+        *      stupid like add more session-state attributes
         *      in  one of the later sections.
         */
-       TALLOC_FREE(request->session_state_ctx);
-
-       MEM(request->session_state_ctx = fr_pair_afrom_da(NULL, request_attr_state));
+       talloc_free(request_state_replace(request, NULL));
 
        RDEBUG3("%s - discarded", state->da->name);
 
@@ -662,7 +660,6 @@ void fr_state_discard(fr_state_tree_t *state, request_t *request)
 int fr_state_to_request(fr_state_tree_t *state, request_t *request)
 {
        fr_state_entry_t        *entry;
-       TALLOC_CTX              *old_ctx;
        fr_pair_t               *vp;
 
        /*
@@ -689,12 +686,16 @@ int fr_state_to_request(fr_state_tree_t *state, request_t *request)
                RERROR("State entry has already been thawed by a request %"PRIu64, entry->thawed->number);
                return -2;
        }
-       old_ctx = request->session_state_ctx;   /* Store for later freeing */
 
+       /*
+        *      Discard any existing session state, and replace it
+        *      with the cached one.
+        */
        fr_assert(entry->ctx);
+       talloc_free(request_state_replace(request, entry->ctx));
+       entry->ctx = NULL;
 
        request->seq_start = entry->seq_start;
-       request->session_state_ctx = entry->ctx;
 
        /*
         *      Associate old state with the request
@@ -708,7 +709,6 @@ int fr_state_to_request(fr_state_tree_t *state, request_t *request)
        request_data_add(request, state, 0, entry, true, true, false);
        request_data_restore(request, &entry->data);
 
-       entry->ctx = NULL;
        entry->thawed = request;
 
        if (!fr_pair_list_empty(&request->session_state_pairs)) {
@@ -716,11 +716,6 @@ int fr_state_to_request(fr_state_tree_t *state, request_t *request)
                log_request_pair_list(L_DBG_LVL_2, request, NULL, &request->session_state_pairs, "&session-state.");
        }
 
-       /*
-        *      Free this outside of the mutex for less contention.
-        */
-       talloc_free(old_ctx);
-
        RDEBUG3("%s - restored", state->da->name);
 
        /*
@@ -772,12 +767,10 @@ int fr_request_to_state(fr_state_tree_t *state, request_t *request)
        fr_assert(request->session_state_ctx);
 
        entry->seq_start = request->seq_start;
-       entry->ctx = request->session_state_ctx;
+       entry->ctx = request_state_replace(request, NULL);
        fr_dlist_move(&entry->data, &data);
        PTHREAD_MUTEX_UNLOCK(&state->mutex);
 
-       MEM(request->session_state_ctx = fr_pair_afrom_da(NULL, request_attr_state));   /* fixme - should use a pool */
-
        RDEBUG3("%s - saved", state->da->name);
        REQUEST_VERIFY(request);
 
@@ -819,7 +812,7 @@ void fr_state_store_in_parent(request_t *child, void const *unique_ptr, int uniq
                request_data_list_init(&child_entry->data);
                talloc_set_destructor(child_entry, _free_child_data);
 
-               child_entry->ctx = child->session_state_ctx;
+               child_entry->ctx = request_state_replace(child, NULL);
 
                /*
                 *      Pull everything out of the child,
@@ -838,14 +831,6 @@ void fr_state_store_in_parent(request_t *child, void const *unique_ptr, int uniq
                 */
                request_data_talloc_add(request->parent, unique_ptr, unique_int,
                                        state_child_entry_t, child_entry, true, false, true);
-
-               /*
-                *      Ensure fr_state_restore_to_child
-                *      can be called again if it's actually
-                *      needed, by giving the child it's own
-                *      unique state_ctx again.
-                */
-               MEM(request->session_state_ctx = fr_pair_afrom_da(NULL, request_attr_state));
        }
 }