detect: modify AMATCH locking

This is an intrusive change. This patch modifies the way AMATCH
inspection uses locking.

So far, each keyword did it's own locking. This lead to a situation
where a 'alstate' pointer was passed around that was not always
protected by a lock.

This patch moves the locking to the Stateful detection functions.

diff --git a/src/detect-app-layer-event.c b/src/detect-app-layer-event.c
index bc8fa81bf4de6ba95ddc472cbffc921d2566f966..2cabeef1f402553dfcb74bc87abb83fa1bd4c2a1 100644 (file)
--- a/src/detect-app-layer-event.c
+++ b/src/detect-app-layer-event.c
@@ -91,8 +91,6 @@ static int DetectAppLayerEventAppMatch(ThreadVars *t, DetectEngineThreadCtx *det
     int r = 0;
     DetectAppLayerEventData *aled = (DetectAppLayerEventData *)m->ctx;
 
-    FLOWLOCK_RDLOCK(f);
-
     if (r == 0) {
         decoder_events = AppLayerParserGetDecoderEvents(f->alparser);
         if (decoder_events != NULL &&
@@ -101,7 +99,6 @@ static int DetectAppLayerEventAppMatch(ThreadVars *t, DetectEngineThreadCtx *det
         }
     }
 
-    FLOWLOCK_UNLOCK(f);
     SCReturnInt(r);
 }
 

diff --git a/src/detect-app-layer-protocol.c b/src/detect-app-layer-protocol.c
index 4bc1a3e45292d4bcd0e97dcfc1bfce5b583f48ac..92285d5d3abf66342b07d2ab5629e1357ec3986c 100644 (file)
--- a/src/detect-app-layer-protocol.c
+++ b/src/detect-app-layer-protocol.c
@@ -40,10 +40,8 @@ int DetectAppLayerProtocolMatch(ThreadVars *t, DetectEngineThreadCtx *det_ctx,
     int r = 0;
     DetectAppLayerProtocolData *data = (DetectAppLayerProtocolData *)m->ctx;
 
-    FLOWLOCK_RDLOCK(f);
     r = (data->negated) ? (f->alproto != data->alproto) :
         (f->alproto == data->alproto);
-    FLOWLOCK_UNLOCK(f);
 
     return r;
 }

diff --git a/src/detect-dce-iface.c b/src/detect-dce-iface.c
index 7f3b2e8d910efc7b97792c9243ba08939da7b714..aba6c2869c4b59486cb81012d551e00dde324e45 100644 (file)
--- a/src/detect-dce-iface.c
+++ b/src/detect-dce-iface.c
@@ -286,8 +286,6 @@ int DetectDceIfaceMatch(ThreadVars *t, DetectEngineThreadCtx *det_ctx, Flow *f,
         SCReturnInt(0);
     }
 
-    FLOWLOCK_RDLOCK(f);
-
     /* we still haven't seen a request */
     if (!dcerpc_state->dcerpc.dcerpcrequest.first_request_seen)
         goto end;
@@ -332,7 +330,6 @@ int DetectDceIfaceMatch(ThreadVars *t, DetectEngineThreadCtx *det_ctx, Flow *f,
     }
 
 end:
-    FLOWLOCK_UNLOCK(f);
     SCReturnInt(ret);
 }
 

diff --git a/src/detect-engine-state.c b/src/detect-engine-state.c
index 499e6c0dd427a6ed5444eabc52dfd54aa3a8e0fc..a86c6b5319e1047d9e417801bf7d260f906f4b37 100644 (file)
--- a/src/detect-engine-state.c
+++ b/src/detect-engine-state.c
@@ -376,35 +376,41 @@ int DeStateDetectStartDetection(ThreadVars *tv, DetectEngineCtx *de_ctx,
         }
     }
 
-    sm = s->sm_lists[DETECT_SM_LIST_AMATCH];
     KEYWORD_PROFILING_SET_LIST(det_ctx, DETECT_SM_LIST_AMATCH);
-    for (match = 0; sm != NULL; sm = sm->next) {
-        match = 0;
-        if (sigmatch_table[sm->type].AppLayerMatch != NULL) {
-            if (alproto == ALPROTO_SMB || alproto == ALPROTO_SMB2) {
-                smb_state = (SMBState *)alstate;
-                if (smb_state->dcerpc_present) {
+    sm = s->sm_lists[DETECT_SM_LIST_AMATCH];
+    if (sm != NULL) {
+        /* RDLOCK would be nicer, but at least tlsstore needs
+         * write lock currently. */
+        FLOWLOCK_WRLOCK(f);
+
+        for (match = 0; sm != NULL; sm = sm->next) {
+            match = 0;
+            if (sigmatch_table[sm->type].AppLayerMatch != NULL) {
+                if (alproto == ALPROTO_SMB || alproto == ALPROTO_SMB2) {
+                    smb_state = (SMBState *)alstate;
+                    if (smb_state->dcerpc_present) {
+                        KEYWORD_PROFILING_START;
+                        match = sigmatch_table[sm->type].
+                            AppLayerMatch(tv, det_ctx, f, flags, &smb_state->dcerpc, s, sm);
+                        KEYWORD_PROFILING_END(det_ctx, sm->type, (match > 0));
+                    }
+                } else {
                     KEYWORD_PROFILING_START;
                     match = sigmatch_table[sm->type].
-                        AppLayerMatch(tv, det_ctx, f, flags, &smb_state->dcerpc, s, sm);
+                        AppLayerMatch(tv, det_ctx, f, flags, alstate, s, sm);
                     KEYWORD_PROFILING_END(det_ctx, sm->type, (match > 0));
                 }
-            } else {
-                KEYWORD_PROFILING_START;
-                match = sigmatch_table[sm->type].
-                    AppLayerMatch(tv, det_ctx, f, flags, alstate, s, sm);
-                KEYWORD_PROFILING_END(det_ctx, sm->type, (match > 0));
-            }
 
-            if (match == 0)
-                break;
-            if (match == 2) {
-                inspect_flags |= DE_STATE_FLAG_SIG_CANT_MATCH;
-                break;
+                if (match == 0)
+                    break;
+                if (match == 2) {
+                    inspect_flags |= DE_STATE_FLAG_SIG_CANT_MATCH;
+                    break;
+                }
             }
         }
-    }
-    if (s->sm_lists[DETECT_SM_LIST_AMATCH] != NULL) {
+        FLOWLOCK_UNLOCK(f);
+
         store_de_state = 1;
         if (sm == NULL || inspect_flags & DE_STATE_FLAG_SIG_CANT_MATCH) {
             if (match == 1) {
@@ -625,31 +631,38 @@ void DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx,
             total_matches = 0;
 
             KEYWORD_PROFILING_SET_LIST(det_ctx, DETECT_SM_LIST_AMATCH);
-            for (sm = item->nm; sm != NULL; sm = sm->next) {
-                if (sigmatch_table[sm->type].AppLayerMatch != NULL)
-                {
-                    if (alproto == ALPROTO_SMB || alproto == ALPROTO_SMB2) {
-                        smb_state = (SMBState *)alstate;
-                        if (smb_state->dcerpc_present) {
+            if (item->nm != NULL) {
+                /* RDLOCK would be nicer, but at least tlsstore needs
+                 * write lock currently. */
+                FLOWLOCK_WRLOCK(f);
+
+                for (sm = item->nm; sm != NULL; sm = sm->next) {
+                    if (sigmatch_table[sm->type].AppLayerMatch != NULL)
+                    {
+                        if (alproto == ALPROTO_SMB || alproto == ALPROTO_SMB2) {
+                            smb_state = (SMBState *)alstate;
+                            if (smb_state->dcerpc_present) {
+                                KEYWORD_PROFILING_START;
+                                match = sigmatch_table[sm->type].
+                                    AppLayerMatch(tv, det_ctx, f, flags, &smb_state->dcerpc, s, sm);
+                                KEYWORD_PROFILING_END(det_ctx, sm->type, (match > 0));
+                            }
+                        } else {
                             KEYWORD_PROFILING_START;
                             match = sigmatch_table[sm->type].
-                                AppLayerMatch(tv, det_ctx, f, flags, &smb_state->dcerpc, s, sm);
+                                AppLayerMatch(tv, det_ctx, f, flags, alstate, s, sm);
                             KEYWORD_PROFILING_END(det_ctx, sm->type, (match > 0));
                         }
-                    } else {
-                        KEYWORD_PROFILING_START;
-                        match = sigmatch_table[sm->type].
-                            AppLayerMatch(tv, det_ctx, f, flags, alstate, s, sm);
-                        KEYWORD_PROFILING_END(det_ctx, sm->type, (match > 0));
-                    }
 
-                    if (match == 0)
-                        break;
-                    else if (match == 2)
-                        inspect_flags |= DE_STATE_FLAG_SIG_CANT_MATCH;
-                    else if (match == 1)
-                        total_matches++;
+                        if (match == 0)
+                            break;
+                        else if (match == 2)
+                            inspect_flags |= DE_STATE_FLAG_SIG_CANT_MATCH;
+                        else if (match == 1)
+                            total_matches++;
+                    }
                 }
+                FLOWLOCK_UNLOCK(f);
             }
             RULE_PROFILING_END(det_ctx, s, match, p);
 

diff --git a/src/detect-ftpbounce.c b/src/detect-ftpbounce.c
index 44d81ae9bf330bda97baea6c3e785821bc710d8d..c9942816b6ad3a0e9de7f9a33911d70181f2f709 100644 (file)
--- a/src/detect-ftpbounce.c
+++ b/src/detect-ftpbounce.c
@@ -180,14 +180,12 @@ int DetectFtpbounceALMatch(ThreadVars *t, DetectEngineThreadCtx *det_ctx,
     }
 
     int ret = 0;
-    FLOWLOCK_RDLOCK(f);
 
     if (ftp_state->command == FTP_COMMAND_PORT) {
         ret = DetectFtpbounceMatchArgs(ftp_state->port_line,
                   ftp_state->port_line_len, f->src.address.address_un_data32[0],
                   ftp_state->arg_offset);
     }
-    FLOWLOCK_UNLOCK(f);
 
     SCReturnInt(ret);
 }

diff --git a/src/detect-ssh-proto-version.c b/src/detect-ssh-proto-version.c
index 8c781aa54fb77b91b7f46a7ed01f65ba0f022ebf..9073b8bcfbff628b614b402786d408091bc0fdd6 100644 (file)
--- a/src/detect-ssh-proto-version.c
+++ b/src/detect-ssh-proto-version.c
@@ -126,7 +126,6 @@ int DetectSshVersionMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx, Flow *
     }
 
     int ret = 0;
-    FLOWLOCK_RDLOCK(f);
     if ((flags & STREAM_TOCLIENT) && (ssh_state->srv_hdr.flags & SSH_FLAG_VERSION_PARSED)) {
         if (ssh->flags & SSH_FLAG_PROTOVERSION_2_COMPAT) {
             SCLogDebug("looking for ssh server protoversion 2 compat");
@@ -150,7 +149,6 @@ int DetectSshVersionMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx, Flow *
             ret = (strncmp((char *) ssh_state->cli_hdr.proto_version, (char *) ssh->ver, ssh->len) == 0)? 1 : 0;
         }
     }
-    FLOWLOCK_UNLOCK(f);
     SCReturnInt(ret);
 }
 

diff --git a/src/detect-ssh-software-version.c b/src/detect-ssh-software-version.c
index 690b0e17d6019beadfacd2978202603a083dd7b3..277f03547622482dc88ef9c3fff98f5a6917a5ea 100644 (file)
--- a/src/detect-ssh-software-version.c
+++ b/src/detect-ssh-software-version.c
@@ -131,7 +131,6 @@ int DetectSshSoftwareVersionMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx
     }
 
     int ret = 0;
-    FLOWLOCK_RDLOCK(f);
     if ((flags & STREAM_TOCLIENT) && (ssh_state->srv_hdr.flags & SSH_FLAG_VERSION_PARSED)) {
         SCLogDebug("looking for ssh server softwareversion %s length %"PRIu16" on %s", ssh->software_ver, ssh->len, ssh_state->srv_hdr.software_version);
         ret = (strncmp((char *) ssh_state->srv_hdr.software_version, (char *) ssh->software_ver, ssh->len) == 0)? 1 : 0;
@@ -139,7 +138,6 @@ int DetectSshSoftwareVersionMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx
         SCLogDebug("looking for ssh client softwareversion %s length %"PRIu16" on %s", ssh->software_ver, ssh->len, ssh_state->cli_hdr.software_version);
         ret = (strncmp((char *) ssh_state->cli_hdr.software_version, (char *) ssh->software_ver, ssh->len) == 0)? 1 : 0;
     }
-    FLOWLOCK_UNLOCK(f);
     SCReturnInt(ret);
 }
 

diff --git a/src/detect-ssl-state.c b/src/detect-ssl-state.c
index ec4e917c4968eda26fe727d6de57523c982b2906..d8556d4f9079c6be11aa27cfbe047a219ef7c69b 100644 (file)
--- a/src/detect-ssl-state.c
+++ b/src/detect-ssl-state.c
@@ -142,8 +142,6 @@ int DetectSslStateMatch(ThreadVars *t, DetectEngineThreadCtx *det_ctx,
         return 0;
     }
 
-    FLOWLOCK_RDLOCK(f);
-
     if ((ssd->flags & SSL_AL_FLAG_STATE_CLIENT_HELLO) &&
         !(ssl_state->flags & SSL_AL_FLAG_STATE_CLIENT_HELLO)) {
         result = 0;
@@ -166,7 +164,6 @@ int DetectSslStateMatch(ThreadVars *t, DetectEngineThreadCtx *det_ctx,
     }
 
  end:
-    FLOWLOCK_UNLOCK(f);
     return result;
 }
 

diff --git a/src/detect-ssl-version.c b/src/detect-ssl-version.c
index 30abfdd1f27bceea947caa00918f4bea98ceed9f..e95fbc20a4a6dd8de0d196456a3be8725ce014e3 100644 (file)
--- a/src/detect-ssl-version.c
+++ b/src/detect-ssl-version.c
@@ -131,8 +131,6 @@ int DetectSslVersionMatch(ThreadVars *t, DetectEngineThreadCtx *det_ctx,
         SCReturnInt(0);
     }
 
-    FLOWLOCK_RDLOCK(f);
-
     if (flags & STREAM_TOCLIENT) {
         SCLogDebug("server (toclient) version is 0x%02X",
                    app_state->server_connp.version);
@@ -143,8 +141,6 @@ int DetectSslVersionMatch(ThreadVars *t, DetectEngineThreadCtx *det_ctx,
         ver = app_state->client_connp.version;
     }
 
-    FLOWLOCK_UNLOCK(f);
-
     switch (ver) {
         case SSL_VERSION_2:
             if (ver == ssl->data[SSLv2].ver)

diff --git a/src/detect-tls-version.c b/src/detect-tls-version.c
index 155e5fcc54d62afd27191b8146d110e254b77ab3..06fd67384d9ed518c1ee0c8e93bbc59fc0c80363 100644 (file)
--- a/src/detect-tls-version.c
+++ b/src/detect-tls-version.c
@@ -125,7 +125,6 @@ int DetectTlsVersionMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx, Flow *
     }
 
     int ret = 0;
-    FLOWLOCK_RDLOCK(f);
     SCLogDebug("looking for tls_data->ver 0x%02X (flags 0x%02X)", tls_data->ver, flags);
 
     if (flags & STREAM_TOCLIENT) {
@@ -137,7 +136,6 @@ int DetectTlsVersionMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx, Flow *
         if (tls_data->ver == ssl_state->client_connp.version)
             ret = 1;
     }
-    FLOWLOCK_UNLOCK(f);
 
     SCReturnInt(ret);
 }

diff --git a/src/detect-tls.c b/src/detect-tls.c
index 4110f0c02a09d8203ce14c0b001d26e72b7d5335..05169e603668dd0358173eed4193fc78b1b82b71 100644 (file)
--- a/src/detect-tls.c
+++ b/src/detect-tls.c
@@ -210,7 +210,6 @@ static int DetectTlsSubjectMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx,
     }
 
     int ret = 0;
-    FLOWLOCK_RDLOCK(f);
 
     SSLStateConnp *connp = NULL;
     if (flags & STREAM_TOSERVER) {
@@ -240,8 +239,6 @@ static int DetectTlsSubjectMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx,
         ret = 0;
     }
 
-    FLOWLOCK_UNLOCK(f);
-
     SCReturnInt(ret);
 }
 
@@ -418,7 +415,6 @@ static int DetectTlsIssuerDNMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx
     }
 
     int ret = 0;
-    FLOWLOCK_RDLOCK(f);
 
     SSLStateConnp *connp = NULL;
     if (flags & STREAM_TOSERVER) {
@@ -448,8 +444,6 @@ static int DetectTlsIssuerDNMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx
         ret = 0;
     }
 
-    FLOWLOCK_UNLOCK(f);
-
     SCReturnInt(ret);
 }
 
@@ -696,7 +690,6 @@ static int DetectTlsFingerprintMatch (ThreadVars *t, DetectEngineThreadCtx *det_
     }
 
     int ret = 0;
-    FLOWLOCK_RDLOCK(f);
 
     if (ssl_state->server_connp.cert0_fingerprint != NULL) {
         SCLogDebug("TLS: Fingerprint is [%s], looking for [%s]\n",
@@ -723,8 +716,6 @@ static int DetectTlsFingerprintMatch (ThreadVars *t, DetectEngineThreadCtx *det_
         ret = 0;
     }
 
-    FLOWLOCK_UNLOCK(f);
-
     SCReturnInt(ret);
 }
 
@@ -831,6 +822,7 @@ error:
 
 }
 
+/** \warning modifies state */
 static int DetectTlsStoreMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx, Flow *f, uint8_t flags, void *state, Signature *s, SigMatch *m)
 {
     SCEnter();
@@ -841,12 +833,10 @@ static int DetectTlsStoreMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx, F
         SCReturnInt(1);
     }
 
-    FLOWLOCK_WRLOCK(f);
     if (s->flags & SIG_FLAG_TLSSTORE) {
         ssl_state->server_connp.cert_log_flag |= SSL_TLS_LOG_PEM;
     }
 
-    FLOWLOCK_UNLOCK(f);
     SCReturnInt(1);
 }