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.

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);
 }
 

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;
 }

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);
 }
 

src/detect-engine-state.c

@@ -376,8 +376,13 @@ 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);
+    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) {
@@ -404,7 +409,8 @@ int DeStateDetectStartDetection(ThreadVars *tv, DetectEngineCtx *de_ctx,
                 }
             }
         }
-    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,6 +631,11 @@ void DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx,
             total_matches = 0;
 
             KEYWORD_PROFILING_SET_LIST(det_ctx, DETECT_SM_LIST_AMATCH);
+            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)
                     {
@@ -651,6 +662,8 @@ void DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx,
                             total_matches++;
                     }
                 }
+                FLOWLOCK_UNLOCK(f);
+            }
             RULE_PROFILING_END(det_ctx, s, match, p);
 
             if (s->sm_lists[DETECT_SM_LIST_AMATCH] != NULL) {

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);
 }

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);
 }
 

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);
 }
 

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;
 }
 

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)

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);
 }

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);
 }