From: Victor Julien Date: Tue, 15 Apr 2014 08:31:48 +0000 (+0200) Subject: detect: modify AMATCH locking X-Git-Tag: suricata-2.0.1rc1~51 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=6e0112d7378f4879e41448068e2366e05a3cefb3;p=thirdparty%2Fsuricata.git 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 bc8fa81bf4..2cabeef1f4 100644 --- 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 4bc1a3e452..92285d5d3a 100644 --- 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 7f3b2e8d91..aba6c2869c 100644 --- 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 499e6c0dd4..a86c6b5319 100644 --- 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 44d81ae9bf..c9942816b6 100644 --- 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 8c781aa54f..9073b8bcfb 100644 --- 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 690b0e17d6..277f035476 100644 --- 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 ec4e917c49..d8556d4f90 100644 --- 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 30abfdd1f2..e95fbc20a4 100644 --- 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 155e5fcc54..06fd67384d 100644 --- 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 4110f0c02a..05169e6036 100644 --- 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); }