From 5ba41c78903ab34adfbb6289d5263d7b290f92c8 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Thu, 23 Feb 2012 18:40:47 +0100 Subject: [PATCH] Fix locking error in filestore handling. Add debug validate check for asserting a flow is locked. --- src/app-layer-parser.c | 22 ++++++++++++++++++++++ src/app-layer.c | 5 +++++ src/detect-engine-state.c | 8 ++++++++ src/util-file.c | 5 ++++- src/util-validate.h | 15 +++++++++++++++ 5 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/app-layer-parser.c b/src/app-layer-parser.c index c6dcfee57a..dc52cedf10 100644 --- a/src/app-layer-parser.c +++ b/src/app-layer-parser.c @@ -58,6 +58,7 @@ #include "util-debug.h" #include "decode-events.h" #include "util-unittest-helper.h" +#include "util-validate.h" static AppLayerProto al_proto_table[ALPROTO_MAX]; /**< Application layer protocol table mapped to their @@ -842,6 +843,8 @@ int AppLayerParse(void *local_data, Flow *f, uint8_t proto, { SCEnter(); + DEBUG_ASSERT_FLOW_LOCKED(f); + uint16_t parser_idx = 0; AppLayerProto *p = &al_proto_table[proto]; TcpSession *ssn = NULL; @@ -1034,6 +1037,8 @@ error: int AppLayerTransactionGetInspectId(Flow *f) { SCEnter(); + DEBUG_ASSERT_FLOW_LOCKED(f); + AppLayerParserStateStore *parser_state_store = (AppLayerParserStateStore *)f->alparser; @@ -1051,6 +1056,8 @@ error: uint16_t AppLayerTransactionGetAvailId(Flow *f) { SCEnter(); + DEBUG_ASSERT_FLOW_LOCKED(f); + AppLayerParserStateStore *parser_state_store = (AppLayerParserStateStore *)f->alparser; @@ -1066,6 +1073,8 @@ uint16_t AppLayerTransactionGetAvailId(Flow *f) { int AppLayerTransactionGetLoggableId(Flow *f) { SCEnter(); + DEBUG_ASSERT_FLOW_LOCKED(f); + AppLayerParserStateStore *parser_state_store = (AppLayerParserStateStore *)f->alparser; @@ -1093,6 +1102,8 @@ error: void AppLayerTransactionUpdateLoggedId(Flow *f) { SCEnter(); + DEBUG_ASSERT_FLOW_LOCKED(f); + AppLayerParserStateStore *parser_state_store = (AppLayerParserStateStore *)f->alparser; @@ -1111,6 +1122,8 @@ error: int AppLayerTransactionGetLoggedId(Flow *f) { SCEnter(); + DEBUG_ASSERT_FLOW_LOCKED(f); + AppLayerParserStateStore *parser_state_store = (AppLayerParserStateStore *)f->alparser; @@ -1133,6 +1146,9 @@ error: */ uint16_t AppLayerGetStateVersion(Flow *f) { SCEnter(); + + DEBUG_ASSERT_FLOW_LOCKED(f); + uint16_t version = 0; AppLayerParserStateStore *parser_state_store = NULL; @@ -1156,6 +1172,8 @@ int AppLayerTransactionUpdateInspectId(Flow *f, char direction) { SCEnter(); + DEBUG_ASSERT_FLOW_LOCKED(f); + int r = 0; AppLayerParserStateStore *parser_state_store = NULL; @@ -1206,6 +1224,8 @@ int AppLayerTransactionUpdateInspectId(Flow *f, char direction) AppLayerDecoderEvents *AppLayerGetDecoderEventsForFlow(Flow *f) { + DEBUG_ASSERT_FLOW_LOCKED(f); + /* Get the parser state (if any) */ AppLayerParserStateStore *parser_state_store = NULL; @@ -1233,6 +1253,8 @@ AppLayerDecoderEvents *AppLayerGetDecoderEventsForFlow(Flow *f) void AppLayerTriggerRawStreamReassembly(Flow *f) { SCEnter(); + DEBUG_ASSERT_FLOW_LOCKED(f); + #ifdef DEBUG BUG_ON(f == NULL); #endif diff --git a/src/app-layer.c b/src/app-layer.c index 562df7f6ee..609889b31e 100644 --- a/src/app-layer.c +++ b/src/app-layer.c @@ -34,6 +34,7 @@ #include "util-debug.h" #include "util-print.h" #include "util-profiling.h" +#include "util-validate.h" //#define PRINT extern uint8_t engine_mode; @@ -78,6 +79,8 @@ void *AppLayerGetProtoStateFromPacket(Packet *p) { void *AppLayerGetProtoStateFromFlow(Flow *f) { SCEnter(); + DEBUG_ASSERT_FLOW_LOCKED(f); + if (f == NULL) { SCReturnPtr(NULL, "void"); } @@ -110,6 +113,8 @@ int AppLayerHandleTCPData(AlpProtoDetectThreadCtx *dp_ctx, Flow *f, { SCEnter(); + DEBUG_ASSERT_FLOW_LOCKED(f); + int r = 0; #if DEBUG diff --git a/src/detect-engine-state.c b/src/detect-engine-state.c index bc0b94d382..275258b925 100644 --- a/src/detect-engine-state.c +++ b/src/detect-engine-state.c @@ -676,8 +676,12 @@ int DeStateDetectStartDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, if (DeStateStoreFilestoreSigsCantMatch(det_ctx->sgh, f->de_state, flags) == 1) { SCLogDebug("disabling file storage for transaction %u", det_ctx->tx_id); + + SCMutexLock(&f->m); FileDisableStoringForTransaction(f, flags & (STREAM_TOCLIENT|STREAM_TOSERVER), det_ctx->tx_id); + SCMutexUnlock(&f->m); + f->de_state->flags |= DE_STATE_FILE_STORE_DISABLED; } } @@ -1140,8 +1144,12 @@ int DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, Dete if (!(f->de_state->flags & DE_STATE_FILE_STORE_DISABLED)) { if (DeStateStoreFilestoreSigsCantMatch(det_ctx->sgh, f->de_state, flags) == 1) { SCLogDebug("disabling file storage for transaction"); + + SCMutexLock(&f->m); FileDisableStoringForTransaction(f, flags & (STREAM_TOCLIENT|STREAM_TOSERVER), det_ctx->tx_id); + SCMutexUnlock(&f->m); + f->de_state->flags |= DE_STATE_FILE_STORE_DISABLED; } } diff --git a/src/util-file.c b/src/util-file.c index 6cf52e78ef..faad596f8b 100644 --- a/src/util-file.c +++ b/src/util-file.c @@ -32,6 +32,7 @@ #include "util-memcmp.h" #include "util-print.h" #include "app-layer-parser.h" +#include "util-validate.h" /** \brief switch to force magic checks on all files * regardless of the rules. @@ -647,13 +648,15 @@ void FileDisableStoringForFile(File *ff) { /** * \brief disable file storing for files in a transaction * - * \param f flow + * \param f *LOCKED* flow * \param direction flow direction * \param tx_id transaction id */ void FileDisableStoringForTransaction(Flow *f, uint8_t direction, uint16_t tx_id) { File *ptr = NULL; + DEBUG_ASSERT_FLOW_LOCKED(f); + SCEnter(); FileContainer *ffc = AppLayerGetFilesFromFlow(f, direction); diff --git a/src/util-validate.h b/src/util-validate.h index 3dd6c6eb3b..b26bab6175 100644 --- a/src/util-validate.h +++ b/src/util-validate.h @@ -33,6 +33,20 @@ #ifdef DEBUG_VALIDATION +/** \brief test if a flow is locked. + * + * If trylock returns 0 it got a lock. Which means + * the flow was previously unlocked. + */ +#define DEBUG_ASSERT_FLOW_LOCKED(f) do { \ + if ((f) != NULL) { \ + int r = SCMutexTrylock(&(f)->m); \ + if (r == 0) { \ + BUG_ON(1); \ + } \ + } \ +} while(0) + /** \brief validate the integrity of the flow * * BUG_ON's on problems @@ -82,6 +96,7 @@ #else /* DEBUG_VALIDATE */ +#define DEBUG_ASSERT_FLOW_LOCKED(f) #define DEBUG_VALIDATE_FLOW(f) #define DEBUG_VALIDATE_PACKET(p)