From cff0a0bda20b1f0aabaf5466095ee9fe2dbd7808 Mon Sep 17 00:00:00 2001
From: Victor Julien <victor@inliniac.net>
Date: Sat, 26 Jun 2010 12:20:36 +0200
Subject: [PATCH] Fix segv conditions caused by broken flow cleanup code.

---
 src/detect-engine-state.c |  8 ++------
 src/flow-hash.c           |  2 +-
 src/flow-util.h           | 29 +++++++++++++++--------------
 src/flow.c                |  2 +-
 4 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/src/detect-engine-state.c b/src/detect-engine-state.c
index dcc2abbfb3..77533799d3 100644
--- a/src/detect-engine-state.c
+++ b/src/detect-engine-state.c
@@ -261,12 +261,12 @@ static void DeStateSignatureAppend(DetectEngineState *state, Signature *s, SigMa
 int DeStateFlowHasState(Flow *f) {
     SCEnter();
     int r = 0;
-    SCMutexLock(&f->m);
+    SCMutexLock(&f->de_state_m);
     if (f->de_state == NULL || f->de_state->cnt == 0)
         r = 0;
     else
         r = 1;
-    SCMutexUnlock(&f->m);
+    SCMutexUnlock(&f->de_state_m);
     SCReturnInt(r);
 }
 
@@ -364,7 +364,6 @@ int DeStateDetectStartDetection(ThreadVars *tv, DetectEngineCtx *de_ctx,
             sm, umatch, dmatch);
 
     SCMutexLock(&f->de_state_m);
-    SCMutexLock(&f->m);
     /* match or no match, we store the state anyway
      * "sm" here is either NULL (complete match) or
      * the last SigMatch that didn't match */
@@ -375,7 +374,6 @@ int DeStateDetectStartDetection(ThreadVars *tv, DetectEngineCtx *de_ctx,
         DeStateSignatureAppend(f->de_state, s, sm, umatch, dmatch);
     }
 
-    SCMutexUnlock(&f->m);
     SCMutexUnlock(&f->de_state_m);
 
     SCReturnInt(r);
@@ -543,13 +541,11 @@ int DeStateRestartDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, DetectEngin
 
     /* first clear the existing state as it belongs
      * to the previous transaction */
-    SCMutexLock(&f->m);
     SCMutexLock(&f->de_state_m);
     if (f->de_state != NULL) {
         DetectEngineStateReset(f->de_state);
     }
     SCMutexUnlock(&f->de_state_m);
-    SCMutexUnlock(&f->m);
 
     SCReturnInt(0);
 }
diff --git a/src/flow-hash.c b/src/flow-hash.c
index f032dc8051..bf2f022184 100644
--- a/src/flow-hash.c
+++ b/src/flow-hash.c
@@ -348,7 +348,7 @@ static Flow *FlowGetNew(Packet *p) {
 
         /* flow is initialized but *unlocked* */
     } else {
-        FLOW_RECYCLE(f);
+        /* flow has been recycled before it went into the spare queue */
 
         /* flow is initialized (recylced) but *unlocked* */
     }
diff --git a/src/flow-util.h b/src/flow-util.h
index 358e51e806..98386e486a 100644
--- a/src/flow-util.h
+++ b/src/flow-util.h
@@ -53,11 +53,12 @@
         (f)->tag_list = NULL; \
     } while (0)
 
+/** \brief macro to recycle a flow before it goes into the spare queue for reuse.
+ *
+ *  Note that the lnext, lprev, hnext, hprev fields are untouched, those are
+ *  managed by the queueing code. Same goes for fb (FlowBucket ptr) field.
+ */
 #define FLOW_RECYCLE(f) do { \
-        (f)->lnext = NULL; \
-        (f)->lprev = NULL; \
-        (f)->hnext = NULL; \
-        (f)->hprev = NULL; \
         (f)->sp = 0; \
         (f)->dp = 0; \
         (f)->flags = 0; \
@@ -70,18 +71,18 @@
         (f)->flowvar = NULL; \
         (f)->protoctx = NULL; \
         SC_ATOMIC_RESET((f)->use_cnt); \
-        SCMutexLock(&(f)->de_state_m); \
         if ((f)->de_state != NULL) { \
             DetectEngineStateReset((f)->de_state); \
+            (f)->de_state = NULL; \
         } \
-        (f)->de_state = NULL; \
-        SCMutexUnlock(&(f)->de_state_m); \
         (f)->sgh_toserver = NULL; \
         (f)->sgh_toclient = NULL; \
         AppLayerParserCleanupState(f); \
         FlowL7DataPtrFree(f); \
-        SCFree((f)->aldata); \
-        (f)->aldata = NULL; \
+        if ((f)->aldata != NULL) { \
+            SCFree((f)->aldata); \
+            (f)->aldata = NULL; \
+        } \
         (f)->alflags = 0; \
         (f)->alproto = 0; \
         DetectTagDataListFree((f)->tag_list); \
@@ -90,21 +91,21 @@
 
 #define FLOW_DESTROY(f) do { \
         SCMutexDestroy(&(f)->m); \
+        SCMutexDestroy(&(f)->de_state_m); \
         GenericVarFree((f)->flowvar); \
         (f)->flowvar = NULL; \
         (f)->protoctx = NULL; \
         SC_ATOMIC_DESTROY((f)->use_cnt); \
-        SCMutexLock(&(f)->de_state_m); \
         if ((f)->de_state != NULL) { \
             DetectEngineStateFree((f)->de_state); \
         } \
         (f)->de_state = NULL; \
-        SCMutexUnlock(&(f)->de_state_m); \
-        SCMutexDestroy(&(f)->de_state_m); \
         AppLayerParserCleanupState(f); \
         FlowL7DataPtrFree(f); \
-        SCFree((f)->aldata); \
-        (f)->aldata = NULL; \
+        if ((f)->aldata != NULL) { \
+            SCFree((f)->aldata); \
+            (f)->aldata = NULL; \
+        } \
         (f)->alflags = 0; \
         (f)->alproto = 0; \
         DetectTagDataListFree((f)->tag_list); \
diff --git a/src/flow.c b/src/flow.c
index edf4fe8ce6..81a43fa1dd 100644
--- a/src/flow.c
+++ b/src/flow.c
@@ -1351,7 +1351,7 @@ static int FlowClearMemory(Flow* f, uint8_t proto_map) {
         flow_proto[proto_map].Freefunc(f->protoctx);
     }
 
-    FLOW_DESTROY(f);
+    FLOW_RECYCLE(f);
 
     SCReturnInt(1);
 }