From e784480e2bc986d6da11d6ad00f64331b68f3d5f Mon Sep 17 00:00:00 2001 From: Haleema Khan Date: Sat, 15 Oct 2022 09:17:42 +0500 Subject: [PATCH 1/7] dns/eve: add 'HTTPS' type logging Add a new DNS record type to represent HTTPS Ticket: #4751 (cherry picked from commit 8d5c5f24a184ed412d1d78f1c0346b205f80fe6b) --- rust/src/dns/dns.rs | 1 + rust/src/dns/log.rs | 5 +++++ src/output-json-dns.c | 5 +++++ 3 files changed, 11 insertions(+) diff --git a/rust/src/dns/dns.rs b/rust/src/dns/dns.rs index 31add6dd640e..6307fa091abd 100644 --- a/rust/src/dns/dns.rs +++ b/rust/src/dns/dns.rs @@ -85,6 +85,7 @@ pub const DNS_RECORD_TYPE_TLSA : u16 = 52; pub const DNS_RECORD_TYPE_HIP : u16 = 55; pub const DNS_RECORD_TYPE_CDS : u16 = 59; pub const DNS_RECORD_TYPE_CDNSKEY : u16 = 60; +pub const DNS_RECORD_TYPE_HTTPS : u16 = 65; pub const DNS_RECORD_TYPE_SPF : u16 = 99; // Obsolete pub const DNS_RECORD_TYPE_TKEY : u16 = 249; pub const DNS_RECORD_TYPE_TSIG : u16 = 250; diff --git a/rust/src/dns/log.rs b/rust/src/dns/log.rs index 27abab245afc..5e8e54d8c8c9 100644 --- a/rust/src/dns/log.rs +++ b/rust/src/dns/log.rs @@ -86,6 +86,7 @@ pub const LOG_URI : u64 = BIT_U64!(59); pub const LOG_FORMAT_GROUPED : u64 = BIT_U64!(60); pub const LOG_FORMAT_DETAILED : u64 = BIT_U64!(61); +pub const LOG_HTTPS : u64 = BIT_U64!(62); fn dns_log_rrtype_enabled(rtype: u16, flags: u64) -> bool { @@ -250,6 +251,9 @@ fn dns_log_rrtype_enabled(rtype: u16, flags: u64) -> bool DNS_RECORD_TYPE_CDNSKEY => { return flags & LOG_CDNSKEY != 0; } + DNS_RECORD_TYPE_HTTPS => { + return flags & LOG_HTTPS != 0; + } DNS_RECORD_TYPE_SPF => { return flags & LOG_SPF != 0; } @@ -324,6 +328,7 @@ pub fn dns_rrtype_string(rrtype: u16) -> String { DNS_RECORD_TYPE_HIP => "HIP", DNS_RECORD_TYPE_CDS => "CDS", DNS_RECORD_TYPE_CDNSKEY => "CDSNKEY", + DNS_RECORD_TYPE_HTTPS => "HTTPS", DNS_RECORD_TYPE_MAILA => "MAILA", DNS_RECORD_TYPE_URI => "URI", DNS_RECORD_TYPE_MB => "MB", diff --git a/src/output-json-dns.c b/src/output-json-dns.c index cf9043bc05ce..6d376c631f63 100644 --- a/src/output-json-dns.c +++ b/src/output-json-dns.c @@ -119,6 +119,7 @@ #define LOG_FORMAT_GROUPED BIT_U64(60) #define LOG_FORMAT_DETAILED BIT_U64(61) +#define LOG_HTTPS BIT_U64(62) #define LOG_FORMAT_ALL (LOG_FORMAT_GROUPED|LOG_FORMAT_DETAILED) #define LOG_ALL_RRTYPES (~(uint64_t)(LOG_QUERIES|LOG_ANSWERS|LOG_FORMAT_DETAILED|LOG_FORMAT_GROUPED)) @@ -176,6 +177,7 @@ typedef enum { DNS_RRTYPE_HIP, DNS_RRTYPE_CDS, DNS_RRTYPE_CDNSKEY, + DNS_RRTYPE_HTTPS, DNS_RRTYPE_SPF, DNS_RRTYPE_TKEY, DNS_RRTYPE_TSIG, @@ -196,6 +198,7 @@ static struct { const char *config_rrtype; uint64_t flags; } dns_rrtype_fields[] = { + // clang-format off { "a", LOG_A }, { "ns", LOG_NS }, { "md", LOG_MD }, @@ -248,12 +251,14 @@ static struct { { "hip", LOG_HIP }, { "cds", LOG_CDS }, { "cdnskey", LOG_CDNSKEY }, + { "https", LOG_HTTPS }, { "spf", LOG_SPF }, { "tkey", LOG_TKEY }, { "tsig", LOG_TSIG }, { "maila", LOG_MAILA }, { "any", LOG_ANY }, { "uri", LOG_URI } + // clang-format on }; typedef struct LogDnsFileCtx_ { From 0dc3e24d9858a1c08f2b17bdecc9b8040d985f56 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 10 Aug 2023 17:47:35 +0530 Subject: [PATCH 2/7] af-packet: terminate on same interface & copyiface If the interface and copy-iface are same for an af-packet IPS device setting then fataly exit else it leads to a segfault in later stages. Bug 5870 (cherry picked from commit d4dd53c95f5fe30a0b2a1e71ab185c06c71a1afe) --- src/runmode-af-packet.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/runmode-af-packet.c b/src/runmode-af-packet.c index 27a6bd26c2c1..8019bd063d73 100644 --- a/src/runmode-af-packet.c +++ b/src/runmode-af-packet.c @@ -201,6 +201,11 @@ static void *ParseAFPConfig(const char *iface) if (ConfGetChildValueWithDefault(if_root, if_default, "copy-iface", &out_iface) == 1) { if (strlen(out_iface) > 0) { aconf->out_iface = out_iface; + if (strcmp(iface, out_iface) == 0) { + FatalError(SC_ERR_FATAL, + "Invalid config: interface (%s) and copy-iface (%s) can't be the same", + iface, out_iface); + } } } From 1b2814bb818a1f37b5ede0bdc93d6ce6876ee373 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 2 Aug 2023 08:37:45 +0200 Subject: [PATCH 3/7] var-names: reimplement var name handling Implement a new design for handling var name id's. The old logic was aware of detection engine versions and generally didn't work well for multi-tenancy cases. Other than memory leaks and crashes, logging of var names worked or failed based on which tenant was loaded last. This patch implements a new approach, where there is a global store of vars and their id's for the lifetime of the program. Overall Design: Base Store: "base" Used during keyword registration. Operates under lock. Base is shared between all detect engines, detect engine versions and tenants. Each variable name is ref counted. During the freeing of a detect engine / tenant, unregistration decreases the ref cnt. Base has both a string to id and a id to string hash table. String to id is used during parsing/registration. id to string during unregistration. Active Store Pointer (atomic) The "active" store atomic pointer points to the active lookup store. The call to `VarNameStoreActivate` will build a new lookup store and hot swap the pointer. Ensuring memory safety. During the hot swap, the pointer is replaced, so any new call to the lookup functions will automatically use the new store. This leaves the case of any lookup happening concurrently with the pointer swap. For this case we add the old store to a free list. It gets a timestamp before which it cannot be freed. Free List The free list contains old stores that are waiting to get removed. They contain a timestamp that is checked before they are freed. Bug: #6044. Bug: #6201. (cherry picked from commit b130234b2639842619da4c156ce5164a652202ec) --- src/detect-engine-build.c | 2 +- src/detect-engine.c | 5 +- src/detect-flowbits.c | 27 +- src/detect-flowint.c | 3 +- src/detect-flowvar.c | 6 +- src/detect-flowvar.h | 6 +- src/detect-hostbits.c | 7 +- src/detect-lua.c | 131 ++++----- src/detect-pcre.c | 40 +-- src/detect-pktvar.c | 3 +- src/detect-xbits.c | 3 +- src/suricata.c | 14 +- src/util-error.c | 1 + src/util-error.h | 1 + src/util-var-name.c | 552 +++++++++++++++++++------------------- src/util-var-name.h | 16 +- 16 files changed, 420 insertions(+), 397 deletions(-) diff --git a/src/detect-engine-build.c b/src/detect-engine-build.c index 3c1d9acde381..df9e75a7e6ae 100644 --- a/src/detect-engine-build.c +++ b/src/detect-engine-build.c @@ -1967,7 +1967,7 @@ int SigGroupBuild(DetectEngineCtx *de_ctx) ThresholdHashAllocate(de_ctx); if (!DetectEngineMultiTenantEnabled()) { - VarNameStoreActivateStaging(); + VarNameStoreActivate(); } return 0; } diff --git a/src/detect-engine.c b/src/detect-engine.c index 8360041ec101..e3319d79aa86 100644 --- a/src/detect-engine.c +++ b/src/detect-engine.c @@ -2039,7 +2039,6 @@ static DetectEngineCtx *DetectEngineCtxInitReal(enum DetectEngineType type, cons } de_ctx->version = DetectEngineGetVersion(); - VarNameStoreSetupStaging(de_ctx->version); SCLogDebug("dectx with version %u", de_ctx->version); return de_ctx; error: @@ -2170,8 +2169,6 @@ void DetectEngineCtxFree(DetectEngineCtx *de_ctx) DetectPortCleanupList(de_ctx, de_ctx->udp_whitelist); DetectBufferTypeFreeDetectEngine(de_ctx); - /* freed our var name hash */ - VarNameStoreFree(de_ctx->version); SCFree(de_ctx); //DetectAddressGroupPrintMemory(); @@ -3768,7 +3765,7 @@ int DetectEngineMultiTenantSetup(void) goto error; } - VarNameStoreActivateStaging(); + VarNameStoreActivate(); } else { SCLogDebug("multi-detect not enabled (multi tenancy)"); diff --git a/src/detect-flowbits.c b/src/detect-flowbits.c index f1805b084d00..871b0e18762a 100644 --- a/src/detect-flowbits.c +++ b/src/detect-flowbits.c @@ -109,7 +109,7 @@ static int FlowbitOrAddData(DetectEngineCtx *de_ctx, DetectFlowbitsData *cd, cha if (unlikely(cd->or_list == NULL)) return -1; for (uint8_t j = 0; j < cd->or_list_size ; j++) { - cd->or_list[j] = VarNameStoreSetupAdd(strarr[j], VAR_TYPE_FLOW_BIT); + cd->or_list[j] = VarNameStoreRegister(strarr[j], VAR_TYPE_FLOW_BIT); de_ctx->max_fb_id = MAX(cd->or_list[j], de_ctx->max_fb_id); } @@ -310,7 +310,7 @@ static int DetectFlowbitParse( } cd->cmd = cmd; } else { - cd->idx = VarNameStoreSetupAdd(name, VAR_TYPE_FLOW_BIT); + cd->idx = VarNameStoreRegister(name, VAR_TYPE_FLOW_BIT); de_ctx->max_fb_id = MAX(cd->idx, de_ctx->max_fb_id); cd->cmd = cmd; cd->or_list_size = 0; @@ -386,8 +386,13 @@ void DetectFlowbitFree (DetectEngineCtx *de_ctx, void *ptr) DetectFlowbitsData *fd = (DetectFlowbitsData *)ptr; if (fd == NULL) return; - if (fd->or_list != NULL) + VarNameStoreUnregister(fd->idx, VAR_TYPE_FLOW_BIT); + if (fd->or_list != NULL) { + for (uint8_t i = 0; i < fd->or_list_size; i++) { + VarNameStoreUnregister(fd->or_list[i], VAR_TYPE_FLOW_BIT); + } SCFree(fd->or_list); + } SCFree(fd); } @@ -590,7 +595,7 @@ int DetectFlowbitsAnalyze(DetectEngineCtx *de_ctx) /* walk array to see if all bits make sense */ for (uint32_t i = 0; i < array_size; i++) { - char *varname = VarNameStoreSetupLookup(i, VAR_TYPE_FLOW_BIT); + const char *varname = VarNameStoreSetupLookup(i, VAR_TYPE_FLOW_BIT); if (varname == NULL) continue; @@ -643,7 +648,6 @@ int DetectFlowbitsAnalyze(DetectEngineCtx *de_ctx) "stateful rules that set flowbit %s", s->id, varname); } } - SCFree(varname); } if (rule_engine_analysis_set) { @@ -673,7 +677,7 @@ static void DetectFlowbitsAnalyzeDump(const DetectEngineCtx *de_ctx, jb_open_array(js, "flowbits"); for (uint32_t x = 0; x < elements; x++) { - char *varname = VarNameStoreSetupLookup(x, VAR_TYPE_FLOW_BIT); + const char *varname = VarNameStoreSetupLookup(x, VAR_TYPE_FLOW_BIT); if (varname == NULL) continue; @@ -733,7 +737,6 @@ static void DetectFlowbitsAnalyzeDump(const DetectEngineCtx *de_ctx, } jb_close(js); } - SCFree(varname); jb_close(js); } jb_close(js); // array @@ -911,8 +914,8 @@ static int FlowBitsTestSig04(void) s = de_ctx->sig_list = SigInit(de_ctx,"alert ip any any -> any any (msg:\"isset option\"; flowbits:isset,fbt; content:\"GET \"; sid:1;)"); FAIL_IF_NULL(s); - idx = VarNameStoreSetupAdd("fbt", VAR_TYPE_FLOW_BIT); - FAIL_IF(idx != 1); + idx = VarNameStoreRegister("fbt", VAR_TYPE_FLOW_BIT); + FAIL_IF(idx == 0); SigGroupBuild(de_ctx); DetectEngineCtxFree(de_ctx); @@ -994,7 +997,7 @@ static int FlowBitsTestSig06(void) s = de_ctx->sig_list = SigInit(de_ctx,"alert ip any any -> any any (msg:\"Flowbit set\"; flowbits:set,myflow; sid:10;)"); FAIL_IF_NULL(s); - idx = VarNameStoreSetupAdd("myflow", VAR_TYPE_FLOW_BIT); + idx = VarNameStoreRegister("myflow", VAR_TYPE_FLOW_BIT); SigGroupBuild(de_ctx); DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx); @@ -1068,7 +1071,7 @@ static int FlowBitsTestSig07(void) s = s->next = SigInit(de_ctx,"alert ip any any -> any any (msg:\"Flowbit unset\"; flowbits:unset,myflow2; sid:11;)"); FAIL_IF_NULL(s); - idx = VarNameStoreSetupAdd("myflow", VAR_TYPE_FLOW_BIT); + idx = VarNameStoreRegister("myflow", VAR_TYPE_FLOW_BIT); SigGroupBuild(de_ctx); DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx); @@ -1144,7 +1147,7 @@ static int FlowBitsTestSig08(void) s = s->next = SigInit(de_ctx,"alert ip any any -> any any (msg:\"Flowbit unset\"; flowbits:toggle,myflow2; sid:11;)"); FAIL_IF_NULL(s); - idx = VarNameStoreSetupAdd("myflow", VAR_TYPE_FLOW_BIT); + idx = VarNameStoreRegister("myflow", VAR_TYPE_FLOW_BIT); SigGroupBuild(de_ctx); DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx); diff --git a/src/detect-flowint.c b/src/detect-flowint.c index db97ee9f49d7..9c3fc9aa3582 100644 --- a/src/detect-flowint.c +++ b/src/detect-flowint.c @@ -329,7 +329,7 @@ static DetectFlowintData *DetectFlowintParse(DetectEngineCtx *de_ctx, const char SCLogError(SC_ERR_MEM_ALLOC, "malloc from strdup failed"); goto error; } - sfd->idx = VarNameStoreSetupAdd(varname, VAR_TYPE_FLOW_INT); + sfd->idx = VarNameStoreRegister(varname, VAR_TYPE_FLOW_INT); SCLogDebug("sfd->name %s id %u", sfd->name, sfd->idx); sfd->modifier = modifier; @@ -416,6 +416,7 @@ void DetectFlowintFree(DetectEngineCtx *de_ctx, void *tmp) { DetectFlowintData *sfd =(DetectFlowintData*) tmp; if (sfd != NULL) { + VarNameStoreUnregister(sfd->idx, VAR_TYPE_FLOW_INT); if (sfd->name != NULL) SCFree(sfd->name); if (sfd->targettype == FLOWINT_TARGET_VAR) diff --git a/src/detect-flowvar.c b/src/detect-flowvar.c index 8db8a14cd1fe..92e7f0b01819 100644 --- a/src/detect-flowvar.c +++ b/src/detect-flowvar.c @@ -78,6 +78,9 @@ static void DetectFlowvarDataFree(DetectEngineCtx *de_ctx, void *ptr) SCReturn; DetectFlowvarData *fd = (DetectFlowvarData *)ptr; + /* leave unregistration to pcre keyword */ + if (!fd->post_match) + VarNameStoreUnregister(fd->idx, VAR_TYPE_FLOW_VAR); if (fd->name) SCFree(fd->name); @@ -172,7 +175,7 @@ static int DetectFlowvarSetup (DetectEngineCtx *de_ctx, Signature *s, const char fd->name = SCStrdup(varname); if (unlikely(fd->name == NULL)) goto error; - fd->idx = VarNameStoreSetupAdd(varname, VAR_TYPE_FLOW_VAR); + fd->idx = VarNameStoreRegister(varname, VAR_TYPE_FLOW_VAR); /* Okay so far so good, lets get this into a SigMatch * and put it in the Signature. */ @@ -267,6 +270,7 @@ int DetectFlowvarPostMatchSetup(DetectEngineCtx *de_ctx, Signature *s, uint32_t /* we only need the idx */ fv->idx = idx; + fv->post_match = true; sm = SigMatchAlloc(); if (unlikely(sm == NULL)) diff --git a/src/detect-flowvar.h b/src/detect-flowvar.h index f9af869216ac..b2988a63517a 100644 --- a/src/detect-flowvar.h +++ b/src/detect-flowvar.h @@ -28,8 +28,10 @@ typedef struct DetectFlowvarData_ { char *name; uint32_t idx; uint8_t *content; - uint8_t content_len; - uint8_t flags; + uint16_t content_len; + /** set to true if used in a post-match */ + bool post_match; + uint32_t flags; } DetectFlowvarData; /* prototypes */ diff --git a/src/detect-hostbits.c b/src/detect-hostbits.c index 8db5c1076003..e826c11a1e92 100644 --- a/src/detect-hostbits.c +++ b/src/detect-hostbits.c @@ -387,7 +387,7 @@ int DetectHostbitSetup (DetectEngineCtx *de_ctx, Signature *s, const char *rawst if (unlikely(cd == NULL)) goto error; - cd->idx = VarNameStoreSetupAdd(fb_name, VAR_TYPE_HOST_BIT); + cd->idx = VarNameStoreRegister(fb_name, VAR_TYPE_HOST_BIT); cd->cmd = fb_cmd; cd->tracker = hb_dir; cd->type = VAR_TYPE_HOST_BIT; @@ -443,6 +443,7 @@ void DetectHostbitFree (DetectEngineCtx *de_ctx, void *ptr) if (fd == NULL) return; + VarNameStoreUnregister(fd->idx, VAR_TYPE_HOST_BIT); SCFree(fd); } @@ -760,8 +761,8 @@ static int HostBitsTestSig04(void) s = de_ctx->sig_list = SigInit(de_ctx,"alert ip any any -> any any (msg:\"isset option\"; hostbits:isset,fbt; content:\"GET \"; sid:1;)"); FAIL_IF_NULL(s); - idx = VarNameStoreSetupAdd("fbt", VAR_TYPE_HOST_BIT); - FAIL_IF(idx != 1); + idx = VarNameStoreRegister("fbt", VAR_TYPE_HOST_BIT); + FAIL_IF(idx == 0); SigGroupBuild(de_ctx); DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx); diff --git a/src/detect-lua.c b/src/detect-lua.c index f7087b4580a1..75b548a28334 100644 --- a/src/detect-lua.c +++ b/src/detect-lua.c @@ -810,7 +810,7 @@ static int DetectLuaSetupPrime(DetectEngineCtx *de_ctx, DetectLuaData *ld) goto error; } - uint32_t idx = VarNameStoreSetupAdd((char *)value, VAR_TYPE_FLOW_VAR); + uint32_t idx = VarNameStoreRegister(value, VAR_TYPE_FLOW_VAR); ld->flowvar[ld->flowvars++] = idx; SCLogDebug("script uses flowvar %u with script id %u", idx, ld->flowvars - 1); } @@ -832,7 +832,7 @@ static int DetectLuaSetupPrime(DetectEngineCtx *de_ctx, DetectLuaData *ld) goto error; } - uint32_t idx = VarNameStoreSetupAdd((char *)value, VAR_TYPE_FLOW_INT); + uint32_t idx = VarNameStoreRegister(value, VAR_TYPE_FLOW_INT); ld->flowint[ld->flowints++] = idx; SCLogDebug("script uses flowint %u with script id %u", idx, ld->flowints - 1); } @@ -1156,6 +1156,13 @@ static void DetectLuaFree(DetectEngineCtx *de_ctx, void *ptr) if (lua->filename) SCFree(lua->filename); + for (uint16_t i = 0; i < lua->flowints; i++) { + VarNameStoreUnregister(lua->flowint[i], VAR_TYPE_FLOW_INT); + } + for (uint16_t i = 0; i < lua->flowvars; i++) { + VarNameStoreUnregister(lua->flowvar[i], VAR_TYPE_FLOW_VAR); + } + DetectUnregisterThreadCtxFuncs(de_ctx, NULL, lua, "lua"); SCFree(lua); @@ -1299,11 +1306,11 @@ static int LuaMatchTest01(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_str.value_len != 1) { printf("%u != %u: ", fv->data.fv_str.value_len, 1); @@ -1460,11 +1467,11 @@ static int LuaMatchTest01a(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_str.value_len != 1) { printf("%u != %u: ", fv->data.fv_str.value_len, 1); @@ -1595,11 +1602,11 @@ static int LuaMatchTest02(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_str.value_len != 1) { printf("%u != %u: ", fv->data.fv_str.value_len, 1); @@ -1728,11 +1735,11 @@ static int LuaMatchTest02a(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_str.value_len != 1) { printf("%u != %u: ", fv->data.fv_str.value_len, 1); @@ -1861,11 +1868,11 @@ static int LuaMatchTest03(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_str.value_len != 1) { printf("%u != %u: ", fv->data.fv_str.value_len, 1); @@ -1994,11 +2001,11 @@ static int LuaMatchTest03a(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_str.value_len != 1) { printf("%u != %u: ", fv->data.fv_str.value_len, 1); @@ -2152,11 +2159,11 @@ static int LuaMatchTest04(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_int.value != 2) { printf("%u != %u: ", fv->data.fv_int.value, 2); @@ -2307,11 +2314,11 @@ static int LuaMatchTest04a(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_int.value != 2) { printf("%u != %u: ", fv->data.fv_int.value, 2); @@ -2455,11 +2462,11 @@ static int LuaMatchTest05(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_int.value != 2) { printf("%u != %u: ", fv->data.fv_int.value, 2); @@ -2604,11 +2611,11 @@ static int LuaMatchTest05a(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_int.value != 2) { printf("%u != %u: ", fv->data.fv_int.value, 2); @@ -2758,11 +2765,11 @@ static int LuaMatchTest06(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_int.value != 0) { printf("%u != %u: ", fv->data.fv_int.value, 0); @@ -2912,11 +2919,11 @@ static int LuaMatchTest06a(void) goto end; } - FlowVar *fv = FlowVarGet(&f, 1); - if (fv == NULL) { - printf("no flowvar: "); - goto end; - } + uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT); + FAIL_IF(id == 0); + + FlowVar *fv = FlowVarGet(&f, id); + FAIL_IF_NULL(fv); if (fv->data.fv_int.value != 0) { printf("%u != %u: ", fv->data.fv_int.value, 0); diff --git a/src/detect-pcre.c b/src/detect-pcre.c index 0c7b1aa70a12..af8447b3b22c 100644 --- a/src/detect-pcre.c +++ b/src/detect-pcre.c @@ -749,12 +749,14 @@ static int DetectPcreParseCapture(const char *regexstr, DetectEngineCtx *de_ctx, return -1; } else if (strncmp(name_array[name_idx], "flow:", 5) == 0) { - pd->capids[pd->idx] = VarNameStoreSetupAdd(name_array[name_idx]+5, VAR_TYPE_FLOW_VAR); + pd->capids[pd->idx] = + VarNameStoreRegister(name_array[name_idx] + 5, VAR_TYPE_FLOW_VAR); pd->captypes[pd->idx] = VAR_TYPE_FLOW_VAR; pd->idx++; } else if (strncmp(name_array[name_idx], "pkt:", 4) == 0) { - pd->capids[pd->idx] = VarNameStoreSetupAdd(name_array[name_idx]+4, VAR_TYPE_PKT_VAR); + pd->capids[pd->idx] = + VarNameStoreRegister(name_array[name_idx] + 4, VAR_TYPE_PKT_VAR); pd->captypes[pd->idx] = VAR_TYPE_PKT_VAR; SCLogDebug("id %u type %u", pd->capids[pd->idx], pd->captypes[pd->idx]); pd->idx++; @@ -813,12 +815,12 @@ static int DetectPcreParseCapture(const char *regexstr, DetectEngineCtx *de_ctx, } if (strcmp(type_str, "pkt") == 0) { - pd->capids[pd->idx] = VarNameStoreSetupAdd((char *)capture_str, VAR_TYPE_PKT_VAR); + pd->capids[pd->idx] = VarNameStoreRegister((char *)capture_str, VAR_TYPE_PKT_VAR); pd->captypes[pd->idx] = VAR_TYPE_PKT_VAR; SCLogDebug("id %u type %u", pd->capids[pd->idx], pd->captypes[pd->idx]); pd->idx++; } else if (strcmp(type_str, "flow") == 0) { - pd->capids[pd->idx] = VarNameStoreSetupAdd((char *)capture_str, VAR_TYPE_FLOW_VAR); + pd->capids[pd->idx] = VarNameStoreRegister((char *)capture_str, VAR_TYPE_FLOW_VAR); pd->captypes[pd->idx] = VAR_TYPE_FLOW_VAR; pd->idx++; } @@ -971,6 +973,11 @@ static void DetectPcreFree(DetectEngineCtx *de_ctx, void *ptr) DetectPcreData *pd = (DetectPcreData *)ptr; DetectParseFreeRegex(&pd->parse_regex); + DetectUnregisterThreadCtxFuncs(de_ctx, NULL, pd, "pcre"); + + for (uint8_t i = 0; i < pd->idx; i++) { + VarNameStoreUnregister(pd->capids[i], pd->captypes[i]); + } SCFree(pd); return; @@ -1059,7 +1066,7 @@ static int DetectPcreParseTest04 (void) FAIL_IF_NULL(pd); FAIL_IF_NOT(alproto == ALPROTO_UNKNOWN); - DetectPcreFree(NULL, pd); + DetectPcreFree(de_ctx, pd); DetectEngineCtxFree(de_ctx); return result; } @@ -1081,7 +1088,7 @@ static int DetectPcreParseTest05 (void) FAIL_IF_NULL(pd); FAIL_IF_NOT(alproto == ALPROTO_UNKNOWN); - DetectPcreFree(NULL, pd); + DetectPcreFree(de_ctx, pd); DetectEngineCtxFree(de_ctx); return result; } @@ -1103,7 +1110,7 @@ static int DetectPcreParseTest06 (void) FAIL_IF_NULL(pd); FAIL_IF_NOT(alproto == ALPROTO_UNKNOWN); - DetectPcreFree(NULL, pd); + DetectPcreFree(de_ctx, pd); DetectEngineCtxFree(de_ctx); return result; } @@ -1125,7 +1132,7 @@ static int DetectPcreParseTest07 (void) FAIL_IF_NULL(pd); FAIL_IF_NOT(alproto == ALPROTO_HTTP); - DetectPcreFree(NULL, pd); + DetectPcreFree(de_ctx, pd); DetectEngineCtxFree(de_ctx); return result; } @@ -1147,7 +1154,7 @@ static int DetectPcreParseTest08 (void) FAIL_IF_NULL(pd); FAIL_IF_NOT(alproto == ALPROTO_UNKNOWN); - DetectPcreFree(NULL, pd); + DetectPcreFree(de_ctx, pd); DetectEngineCtxFree(de_ctx); return result; } @@ -1168,7 +1175,7 @@ static int DetectPcreParseTest09 (void) pd = DetectPcreParse(de_ctx, teststring, &list, NULL, 0, false, &alproto); FAIL_IF_NULL(pd); - DetectPcreFree(NULL, pd); + DetectPcreFree(de_ctx, pd); DetectEngineCtxFree(de_ctx); PASS; } @@ -3515,7 +3522,7 @@ static int DetectPcreParseHttpHost(void) DetectPcreData *pd = DetectPcreParse(de_ctx, "/domain\\.com/W", &list, NULL, 0, false, &alproto); FAIL_IF(pd == NULL); - DetectPcreFree(NULL, pd); + DetectPcreFree(de_ctx, pd); list = DETECT_SM_LIST_NOTSET; pd = DetectPcreParse(de_ctx, "/dOmain\\.com/W", &list, NULL, 0, false, &alproto); @@ -3525,7 +3532,7 @@ static int DetectPcreParseHttpHost(void) list = DETECT_SM_LIST_NOTSET; pd = DetectPcreParse(de_ctx, "/domain\\D+\\.com/W", &list, NULL, 0, false, &alproto); FAIL_IF(pd == NULL); - DetectPcreFree(NULL, pd); + DetectPcreFree(de_ctx, pd); /* This should not parse as the first \ escapes the second \, then * we have a D. */ @@ -3562,10 +3569,11 @@ static int DetectPcreParseCaptureTest(void) SigGroupBuild(de_ctx); - uint32_t capid = VarNameStoreLookupByName("somecapture", VAR_TYPE_FLOW_VAR); - FAIL_IF (capid != 1); - capid = VarNameStoreLookupByName("anothercap", VAR_TYPE_PKT_VAR); - FAIL_IF (capid != 2); + uint32_t capid1 = VarNameStoreLookupByName("somecapture", VAR_TYPE_FLOW_VAR); + FAIL_IF(capid1 == 0); + uint32_t capid2 = VarNameStoreLookupByName("anothercap", VAR_TYPE_PKT_VAR); + FAIL_IF(capid2 == 0); + FAIL_IF(capid1 == capid2); DetectEngineCtxFree(de_ctx); PASS; diff --git a/src/detect-pktvar.c b/src/detect-pktvar.c index 9715f2770f42..045cebeeb348 100644 --- a/src/detect-pktvar.c +++ b/src/detect-pktvar.c @@ -80,6 +80,7 @@ static void DetectPktvarFree(DetectEngineCtx *de_ctx, void *ptr) { DetectPktvarData *data = ptr; if (data != NULL) { + VarNameStoreUnregister(data->id, VAR_TYPE_PKT_VAR); SCFree(data->content); SCFree(data); } @@ -144,7 +145,7 @@ static int DetectPktvarSetup (DetectEngineCtx *de_ctx, Signature *s, const char cd->content = content; cd->content_len = len; - cd->id = VarNameStoreSetupAdd(varname, VAR_TYPE_PKT_VAR); + cd->id = VarNameStoreRegister(varname, VAR_TYPE_PKT_VAR); pcre_free(varname); /* Okay so far so good, lets get this into a SigMatch diff --git a/src/detect-xbits.c b/src/detect-xbits.c index 4de72d708aac..b8d6792396e4 100644 --- a/src/detect-xbits.c +++ b/src/detect-xbits.c @@ -297,7 +297,7 @@ static int DetectXbitParse(DetectEngineCtx *de_ctx, if (unlikely(cd == NULL)) return -1; - cd->idx = VarNameStoreSetupAdd(name, var_type); + cd->idx = VarNameStoreRegister(name, var_type); cd->cmd = cmd; cd->tracker = track; cd->type = var_type; @@ -363,6 +363,7 @@ static void DetectXbitFree (DetectEngineCtx *de_ctx, void *ptr) if (fd == NULL) return; + VarNameStoreUnregister(fd->idx, fd->type); SCFree(fd); } diff --git a/src/suricata.c b/src/suricata.c index 6ebf24ba293f..45d9d5d742b9 100644 --- a/src/suricata.c +++ b/src/suricata.c @@ -47,6 +47,9 @@ #include "util-atomic.h" #include "util-spm.h" +#ifdef BUILD_HYPERSCAN +#include "util-mpm-hs.h" +#endif #include "util-cpu.h" #include "util-action.h" #include "util-pidfile.h" @@ -170,9 +173,11 @@ #include "tmqh-packetpool.h" #include "util-proto-name.h" -#include "util-mpm-hs.h" -#include "util-storage.h" -#include "host-storage.h" +#include "util-running-modes.h" +#include "util-signal.h" +#include "util-time.h" +#include "util-validate.h" +#include "util-var-name.h" #include "util-lua.h" @@ -442,6 +447,8 @@ static void GlobalsDestroy(SCInstance *suri) SCPidfileRemove(suri->pid_filename); SCFree(suri->pid_filename); suri->pid_filename = NULL; + + VarNameStoreDestroy(); } /** \brief make sure threads can stop the engine by calling this @@ -2821,6 +2828,7 @@ int InitGlobal(void) { /* Initialize the configuration module. */ ConfInit(); + VarNameStoreInit(); return 0; } diff --git a/src/util-error.c b/src/util-error.c index b39e05a907ee..975e12ae9eef 100644 --- a/src/util-error.c +++ b/src/util-error.c @@ -380,6 +380,7 @@ const char * SCErrorToString(SCError err) CASE_CODE(SC_WARN_CHOWN); CASE_CODE(SC_ERR_HASH_ADD); CASE_CODE(SC_ERR_SIGNAL); + CASE_CODE(SC_WARN_REFCNT); CASE_CODE (SC_ERR_MAX); } diff --git a/src/util-error.h b/src/util-error.h index 3387c74ae83a..398aee9eebe4 100644 --- a/src/util-error.h +++ b/src/util-error.h @@ -370,6 +370,7 @@ typedef enum { SC_WARN_CHOWN, SC_ERR_HASH_ADD, SC_ERR_SIGNAL, + SC_WARN_REFCNT, SC_ERR_MAX } SCError; diff --git a/src/util-var-name.c b/src/util-var-name.c index 3cc1231026a7..93fbb74554c5 100644 --- a/src/util-var-name.c +++ b/src/util-var-name.c @@ -1,4 +1,4 @@ -/* Copyright (C) 2007-2016 Open Information Security Foundation +/* Copyright (C) 2007-2023 Open Information Security Foundation * * You can copy, redistribute or modify this Program under the terms of * the GNU General Public License version 2 as published by the Free @@ -25,365 +25,351 @@ #include "suricata-common.h" #include "detect.h" +#include "util-hash-string.h" #include "util-hashlist.h" #include "util-var-name.h" +#include "util-validate.h" -/* the way this can be used w/o locking lookups: - * - Lookups use only g_varnamestore_current which is read only - * - Detection setups a new ctx in staging, which will include the 'current' - * entries keeping ID's stable - * - Detection hot swaps staging into current after a new detect engine was - * created. Current remains available through 'old'. - * - When detect reload is complete (threads are all moved over), 'old' can - * be freed. +/* Overall Design: + * + * Base Store: "base" + * + * Used during keyword registration. Operates under lock. Base is shared + * between all detect engines, detect engine versions and tenants. + * Each variable name is ref counted. + * + * During the freeing of a detect engine / tenant, unregistration decreases + * the ref cnt. + * + * Base has both a string to id and a id to string hash table. String to + * id is used during parsing/registration. id to string during unregistration. + * + * + * Active Store Pointer (atomic) + * + * The "active" store atomic pointer points to the active store. The call + * to `VarNameStoreActivate` will build a new lookup store and hot swap + * the pointer. + * + * Ensuring memory safety. During the hot swap, the pointer is replaced, so + * any new call to the lookup functions will automatically use the new store. + * This leaves the case of any lookup happening concurrently with the pointer + * swap. For this case we add the old store to a free list. It gets a timestamp + * before which it cannot be freed. + * + * + * Free List + * + * The free list contains old stores that are waiting to get removed. They + * contain a timestamp that is checked before they are freed. + * */ - typedef struct VarNameStore_ { HashListTable *names; HashListTable *ids; uint32_t max_id; - uint32_t de_ctx_version; /**< de_ctx version 'owning' this */ + struct timeval free_after; + TAILQ_ENTRY(VarNameStore_) next; } VarNameStore; - -static int initialized = 0; -/* currently VarNameStore that is READ ONLY. This way lookups can - * be done w/o locking or synchronization */ -SC_ATOMIC_DECLARE(VarNameStore *, g_varnamestore_current); - -/* old VarNameStore on the way out */ -static VarNameStore *g_varnamestore_old = NULL; - -/* new VarNameStore that is being prepared. Multiple DetectLoader threads - * may be updating it so a lock is used for synchronization. */ -static VarNameStore *g_varnamestore_staging = NULL; -static SCMutex g_varnamestore_staging_m = SCMUTEX_INITIALIZER; +typedef VarNameStore *VarNameStorePtr; /** \brief Name2idx mapping structure for flowbits, flowvars and pktvars. */ typedef struct VariableName_ { char *name; - uint8_t type; /* flowbit, pktvar, etc */ - uint32_t idx; + enum VarTypes type; /* flowbit, pktvar, etc */ + uint32_t id; + uint32_t ref_cnt; } VariableName; #define VARNAME_HASHSIZE 0x1000 #define VARID_HASHSIZE 0x1000 -static uint32_t VariableNameHash(HashListTable *ht, void *buf, uint16_t buflen) -{ - VariableName *fn = (VariableName *)buf; - uint32_t hash = strlen(fn->name) + fn->type; - uint16_t u; - - for (u = 0; u < buflen; u++) { - hash += fn->name[u]; - } +static SCMutex base_lock = SCMUTEX_INITIALIZER; +static VarNameStore base = { .names = NULL, .ids = NULL, .max_id = 0 }; +static TAILQ_HEAD(, VarNameStore_) free_list = TAILQ_HEAD_INITIALIZER(free_list); +static SC_ATOMIC_DECLARE(VarNameStorePtr, active); - return (hash % VARNAME_HASHSIZE); -} +static uint32_t VariableNameHash(HashListTable *ht, void *buf, uint16_t buflen); +static char VariableNameCompare(void *buf1, uint16_t len1, void *buf2, uint16_t len2); +static uint32_t VariableIdHash(HashListTable *ht, void *ptr, uint16_t _unused); +static char VariableIdCompare(void *ptr1, uint16_t _unused1, void *ptr2, uint16_t _unused2); +static void VariableNameFree(void *data); -static char VariableNameCompare(void *buf1, uint16_t len1, void *buf2, uint16_t len2) +void VarNameStoreInit(void) { - VariableName *fn1 = (VariableName *)buf1; - VariableName *fn2 = (VariableName *)buf2; - - if (fn1->type != fn2->type) - return 0; - - if (strcmp(fn1->name,fn2->name) == 0) - return 1; - - return 0; -} - -static uint32_t VariableIdxHash(HashListTable *ht, void *buf, uint16_t buflen) -{ - VariableName *fn = (VariableName *)buf; - uint32_t hash = fn->idx + fn->type; - return (hash % VARID_HASHSIZE); -} - -static char VariableIdxCompare(void *buf1, uint16_t len1, void *buf2, uint16_t len2) -{ - VariableName *fn1 = (VariableName *)buf1; - VariableName *fn2 = (VariableName *)buf2; - - if (fn1->type != fn2->type) - return 0; - - if (fn1->idx == fn2->idx) - return 1; - - return 0; -} - -static void VariableNameFree(void *data) -{ - VariableName *fn = (VariableName *)data; - - if (fn == NULL) - return; - - if (fn->name != NULL) { - SCFree(fn->name); - fn->name = NULL; + SCMutexLock(&base_lock); + base.names = HashListTableInit( + VARNAME_HASHSIZE, VariableNameHash, VariableNameCompare, VariableNameFree); + if (base.names == NULL) { + FatalError(SC_ERR_HASH_TABLE_INIT, "failed to initialize variable name hash (names)"); } - SCFree(fn); + /* base.names owns the allocation, so use a NULL Free pointer here */ + base.ids = HashListTableInit(VARID_HASHSIZE, VariableIdHash, VariableIdCompare, NULL); + if (base.ids == NULL) { + FatalError(SC_ERR_HASH_TABLE_INIT, "failed to initialize variable name hash (names)"); + } + SC_ATOMIC_INITPTR(active); + SCMutexUnlock(&base_lock); } -/** \brief Initialize the Name idx hash. - */ -static VarNameStore *VarNameStoreInit(void) +void VarNameStoreDestroy(void) { - VarNameStore *v = SCCalloc(1, sizeof(*v)); - if (v == NULL) - return NULL; - - v->names = HashListTableInit(VARNAME_HASHSIZE, VariableNameHash, VariableNameCompare, VariableNameFree); - if (v->names == NULL) { - SCFree(v); - return NULL; + SCMutexLock(&base_lock); + VarNameStore *s = SC_ATOMIC_GET(active); + if (s) { + HashListTableFree(s->names); + HashListTableFree(s->ids); + SCFree(s); + s = NULL; } + SC_ATOMIC_SET(active, NULL); - v->ids = HashListTableInit(VARID_HASHSIZE, VariableIdxHash, VariableIdxCompare, NULL); - if (v->ids == NULL) { - HashListTableFree(v->names); - SCFree(v); - return NULL; + while ((s = TAILQ_FIRST(&free_list))) { + TAILQ_REMOVE(&free_list, s, next); + HashListTableFree(s->names); + HashListTableFree(s->ids); + SCFree(s); } - v->max_id = 0; - return v; -} - -static void VarNameStoreDoFree(VarNameStore *v) -{ - if (v) { - HashListTableFree(v->names); - HashListTableFree(v->ids); - SCFree(v); + for (HashListTableBucket *b = HashListTableGetListHead(base.names); b != NULL; + b = HashListTableGetListNext(b)) { + VariableName *vn = HashListTableGetListData(b); + DEBUG_VALIDATE_BUG_ON(vn->ref_cnt > 0); + if (vn->ref_cnt > 0) { + SCLogWarning(SC_WARN_REFCNT, "%s (type %u, id %u) still has ref_cnt %u", vn->name, + vn->type, vn->id, vn->ref_cnt); + } } + HashListTableFree(base.ids); + base.ids = NULL; + HashListTableFree(base.names); + base.names = NULL; + base.max_id = 0; + SCMutexUnlock(&base_lock); } - -/** \brief Get a name idx for a name. If the name is already used reuse the idx. - * \param name nul terminated string with the name - * \param type variable type - * \retval 0 in case of error - * \retval idx the idx or 0 +/** + * \retval id or 0 on error */ -static uint32_t VariableNameGetIdx(VarNameStore *v, const char *name, enum VarTypes type) +uint32_t VarNameStoreRegister(const char *name, const enum VarTypes type) { - uint32_t idx = 0; - - VariableName *fn = SCMalloc(sizeof(VariableName)); - if (unlikely(fn == NULL)) - goto error; + SCMutexLock(&base_lock); + uint32_t id = 0; - memset(fn, 0, sizeof(VariableName)); - - fn->type = type; - fn->name = SCStrdup(name); - if (fn->name == NULL) - goto error; - - VariableName *lookup_fn = (VariableName *)HashListTableLookup(v->names, (void *)fn, 0); - if (lookup_fn == NULL) { - v->max_id++; - - idx = fn->idx = v->max_id; - HashListTableAdd(v->names, (void *)fn, 0); - HashListTableAdd(v->ids, (void *)fn, 0); - SCLogDebug("new registration %s id %u type %u", fn->name, fn->idx, fn->type); + SCLogDebug("registering: name %s type %u", name, type); + VariableName lookup = { .type = type, .name = (char *)name }; + VariableName *found = (VariableName *)HashListTableLookup(base.names, (void *)&lookup, 0); + if (found == NULL) { + VariableName *vn = SCCalloc(1, sizeof(VariableName)); + if (likely(vn != NULL)) { + vn->type = type; + vn->name = SCStrdup(name); + if (vn->name != NULL) { + vn->ref_cnt = 1; + id = vn->id = ++base.max_id; + HashListTableAdd(base.names, (void *)vn, 0); + HashListTableAdd(base.ids, (void *)vn, 0); + SCLogDebug( + "new registration %s id %u type %u -> %u", vn->name, vn->id, vn->type, id); + } else { + SCFree(vn); + } + } } else { - idx = lookup_fn->idx; - VariableNameFree(fn); + id = found->id; + found->ref_cnt++; + SCLogDebug("existing registration %s ref_cnt %u -> %u", name, found->ref_cnt, id); } - - return idx; -error: - VariableNameFree(fn); - return 0; + SCMutexUnlock(&base_lock); + return id; } -/** \brief Get a name from the idx. - * \param idx index of the variable whose name is to be fetched - * \param type variable type - * \retval NULL in case of error - * \retval name of the variable if successful. - * \todo no alloc on lookup - */ -static char *VariableIdxGetName(VarNameStore *v, uint32_t idx, enum VarTypes type) +const char *VarNameStoreSetupLookup(const uint32_t id, const enum VarTypes type) { - VariableName *fn = SCMalloc(sizeof(VariableName)); - if (unlikely(fn == NULL)) - goto error; - - char *name = NULL; - memset(fn, 0, sizeof(VariableName)); - - fn->type = type; - fn->idx = idx; - - VariableName *lookup_fn = (VariableName *)HashListTableLookup(v->ids, (void *)fn, 0); - if (lookup_fn != NULL) { - name = SCStrdup(lookup_fn->name); - if (unlikely(name == NULL)) - goto error; - - VariableNameFree(fn); - } else { - goto error; + const char *name = NULL; + SCMutexLock(&base_lock); + VariableName lookup = { .type = type, .id = id }; + VariableName *found = (VariableName *)HashListTableLookup(base.ids, (void *)&lookup, 0); + if (found) { + name = found->name; } - + SCMutexUnlock(&base_lock); return name; -error: - VariableNameFree(fn); - return NULL; } -/** \brief setup staging store. Include current store if there is one. - */ -int VarNameStoreSetupStaging(uint32_t de_ctx_version) +void VarNameStoreUnregister(const uint32_t id, const enum VarTypes type) { - SCMutexLock(&g_varnamestore_staging_m); - - if (!initialized) { - SC_ATOMIC_INITPTR(g_varnamestore_current); - initialized = 1; + SCMutexLock(&base_lock); + VariableName lookup = { .type = type, .id = id }; + VariableName *found = (VariableName *)HashListTableLookup(base.ids, (void *)&lookup, 0); + if (found) { + SCLogDebug("found %s ref_cnt %u", found->name, found->ref_cnt); + DEBUG_VALIDATE_BUG_ON(found->ref_cnt == 0); + found->ref_cnt--; } + SCMutexUnlock(&base_lock); +} - if (g_varnamestore_staging != NULL && - g_varnamestore_staging->de_ctx_version == de_ctx_version) { - SCMutexUnlock(&g_varnamestore_staging_m); - return 0; - } +int VarNameStoreActivate(void) +{ + int result = 0; + SCMutexLock(&base_lock); + SCLogDebug("activating new lookup store"); + + VarNameStore *new_active = NULL; + + // create lookup hash for id to string, strings should point to base + for (HashListTableBucket *b = HashListTableGetListHead(base.names); b != NULL; + b = HashListTableGetListNext(b)) { + VariableName *vn = HashListTableGetListData(b); + BUG_ON(vn == NULL); + SCLogDebug("base: %s/%u/%u", vn->name, vn->id, vn->ref_cnt); + if (vn->ref_cnt == 0) + continue; + + if (new_active == NULL) { + new_active = SCCalloc(1, sizeof(*new_active)); + if (new_active == NULL) { + result = -1; + goto out; + } + + new_active->names = HashListTableInit( + VARNAME_HASHSIZE, VariableNameHash, VariableNameCompare, NULL); + if (new_active->names == NULL) { + SCFree(new_active); + result = -1; + goto out; + } + new_active->ids = + HashListTableInit(VARID_HASHSIZE, VariableIdHash, VariableIdCompare, NULL); + if (new_active->ids == NULL) { + HashListTableFree(new_active->names); + SCFree(new_active); + result = -1; + goto out; + } + } - VarNameStore *nv = VarNameStoreInit(); - if (nv == NULL) { - SCMutexUnlock(&g_varnamestore_staging_m); - return -1; + /* memory is still owned by "base" */ + HashListTableAdd(new_active->names, (void *)vn, 0); + HashListTableAdd(new_active->ids, (void *)vn, 0); } - g_varnamestore_staging = nv; - nv->de_ctx_version = de_ctx_version; - VarNameStore *current = SC_ATOMIC_GET(g_varnamestore_current); - if (current) { - /* add all entries from the current hash into this new one. */ - HashListTableBucket *b = HashListTableGetListHead(current->names); - while (b) { - VariableName *var = HashListTableGetListData(b); - - VariableName *newvar = SCCalloc(1, sizeof(*newvar)); - BUG_ON(newvar == NULL); - memcpy(newvar, var, sizeof(*newvar)); - newvar->name = SCStrdup(var->name); - BUG_ON(newvar->name == NULL); - - HashListTableAdd(nv->names, (void *)newvar, 0); - HashListTableAdd(nv->ids, (void *)newvar, 0); - nv->max_id = MAX(nv->max_id, newvar->idx); - SCLogDebug("xfer %s id %u type %u", newvar->name, newvar->idx, newvar->type); - - b = HashListTableGetListNext(b); + if (new_active) { + VarNameStore *old_active = SC_ATOMIC_GET(active); + if (old_active) { + struct timeval ts, add; + memset(&ts, 0, sizeof(ts)); + memset(&add, 0, sizeof(add)); + gettimeofday(&ts, NULL); + add.tv_sec = 60; + timeradd(&ts, &add, &ts); + old_active->free_after = ts; + + TAILQ_INSERT_TAIL(&free_list, old_active, next); + SCLogDebug("old active is stored in free list"); } - } - SCLogDebug("set up staging with detect engine ver %u", nv->de_ctx_version); - SCMutexUnlock(&g_varnamestore_staging_m); - return 0; + SC_ATOMIC_SET(active, new_active); + SCLogDebug("new store active"); + + struct timeval now; + memset(&now, 0, sizeof(now)); + gettimeofday(&now, NULL); + + VarNameStore *s = NULL; + while ((s = TAILQ_FIRST(&free_list))) { + char timebuf[64]; + CreateIsoTimeString(&s->free_after, timebuf, sizeof(timebuf)); + + if (!timercmp(&now, &s->free_after, >)) { + SCLogDebug("not yet freeing store %p before %s", s, timebuf); + break; + } + SCLogDebug("freeing store %p with time %s", s, timebuf); + TAILQ_REMOVE(&free_list, s, next); + HashListTableFree(s->names); + HashListTableFree(s->ids); + SCFree(s); + } + } +out: + SCLogDebug("activating new lookup store: complete %d", result); + SCMutexUnlock(&base_lock); + return result; } +/** \brief find name for id+type at packet time. */ const char *VarNameStoreLookupById(const uint32_t id, const enum VarTypes type) { - VarNameStore *current = SC_ATOMIC_GET(g_varnamestore_current); - BUG_ON(current == NULL); - VariableName lookup = { NULL, type, id }; - VariableName *found = (VariableName *)HashListTableLookup(current->ids, (void *)&lookup, 0); - if (found == NULL) { - return NULL; + const char *name = NULL; + + const VarNameStore *current = SC_ATOMIC_GET(active); + if (current) { + VariableName lookup = { .type = type, .id = id }; + const VariableName *found = HashListTableLookup(current->ids, (void *)&lookup, 0); + if (found) { + return found->name; + } } - return found->name; + + return name; } +/** \brief find name for id+type at packet time. */ uint32_t VarNameStoreLookupByName(const char *name, const enum VarTypes type) { - VarNameStore *current = SC_ATOMIC_GET(g_varnamestore_current); - BUG_ON(current == NULL); - VariableName lookup = { (char *)name, type, 0 }; - VariableName *found = (VariableName *)HashListTableLookup(current->names, (void *)&lookup, 0); - if (found == NULL) { - return 0; + const VarNameStore *current = SC_ATOMIC_GET(active); + if (current) { + VariableName lookup = { .name = (char *)name, .type = type }; + const VariableName *found = HashListTableLookup(current->names, (void *)&lookup, 0); + if (found) { + return found->id; + } } - SCLogDebug("found %u for %s type %u", found->idx, name, type); - return found->idx; -} -/** \brief add to staging or return existing id if already in there */ -uint32_t VarNameStoreSetupAdd(const char *name, const enum VarTypes type) -{ - uint32_t id; - SCMutexLock(&g_varnamestore_staging_m); - id = VariableNameGetIdx(g_varnamestore_staging, name, type); - SCMutexUnlock(&g_varnamestore_staging_m); - return id; + return 0; } -char *VarNameStoreSetupLookup(uint32_t idx, const enum VarTypes type) +static uint32_t VariableNameHash(HashListTable *ht, void *buf, uint16_t buflen) { - SCMutexLock(&g_varnamestore_staging_m); - char *name = VariableIdxGetName(g_varnamestore_staging, idx, type); - SCMutexUnlock(&g_varnamestore_staging_m); - return name; + VariableName *vn = (VariableName *)buf; + uint32_t hash = StringHashDjb2((uint8_t *)vn->name, strlen(vn->name)) + vn->type; + return (hash % VARNAME_HASHSIZE); } -void VarNameStoreActivateStaging(void) +static char VariableNameCompare(void *buf1, uint16_t len1, void *buf2, uint16_t len2) { - SCMutexLock(&g_varnamestore_staging_m); - if (g_varnamestore_old) { - VarNameStoreDoFree(g_varnamestore_old); - g_varnamestore_old = NULL; - } - g_varnamestore_old = SC_ATOMIC_GET(g_varnamestore_current); - SC_ATOMIC_SET(g_varnamestore_current, g_varnamestore_staging); - g_varnamestore_staging = NULL; - SCMutexUnlock(&g_varnamestore_staging_m); + VariableName *vn1 = (VariableName *)buf1; + VariableName *vn2 = (VariableName *)buf2; + return (vn1->type == vn2->type && strcmp(vn1->name, vn2->name) == 0); } -void VarNameStoreFreeOld(void) +static uint32_t VariableIdHash(HashListTable *ht, void *ptr, uint16_t _unused) { - SCMutexLock(&g_varnamestore_staging_m); - SCLogDebug("freeing g_varnamestore_old %p", g_varnamestore_old); - if (g_varnamestore_old) { - VarNameStoreDoFree(g_varnamestore_old); - g_varnamestore_old = NULL; - } - SCMutexUnlock(&g_varnamestore_staging_m); + VariableName *vn = (VariableName *)ptr; + uint32_t hash = vn->id << vn->type; + return (hash % VARID_HASHSIZE); } -void VarNameStoreFree(uint32_t de_ctx_version) +static char VariableIdCompare(void *ptr1, uint16_t _unused1, void *ptr2, uint16_t _unused2) { - SCLogDebug("freeing detect engine version %u", de_ctx_version); - SCMutexLock(&g_varnamestore_staging_m); - if (g_varnamestore_old && g_varnamestore_old->de_ctx_version == de_ctx_version) { - VarNameStoreDoFree(g_varnamestore_old); - g_varnamestore_old = NULL; - SCLogDebug("freeing detect engine version %u: old done", de_ctx_version); - } + VariableName *vn1 = (VariableName *)ptr1; + VariableName *vn2 = (VariableName *)ptr2; - /* if at this point we have a staging area which matches our version - * we didn't complete the setup and are cleaning up the mess. */ - if (g_varnamestore_staging && g_varnamestore_staging->de_ctx_version == de_ctx_version) { - VarNameStoreDoFree(g_varnamestore_staging); - g_varnamestore_staging = NULL; - SCLogDebug("freeing detect engine version %u: staging done", de_ctx_version); - } + return (vn1->id == vn2->id && vn1->type == vn2->type); +} - VarNameStore *current = SC_ATOMIC_GET(g_varnamestore_current); - if (current && current->de_ctx_version == de_ctx_version) { - VarNameStoreDoFree(current); - SC_ATOMIC_SET(g_varnamestore_current, NULL); - SCLogDebug("freeing detect engine version %u: current done", de_ctx_version); +static void VariableNameFree(void *data) +{ + VariableName *vn = (VariableName *)data; + if (vn == NULL) + return; + if (vn->name != NULL) { + SCFree(vn->name); + vn->name = NULL; } - SCMutexUnlock(&g_varnamestore_staging_m); + SCFree(vn); } diff --git a/src/util-var-name.h b/src/util-var-name.h index 5b0b18d716c7..5f21ea3fa4ca 100644 --- a/src/util-var-name.h +++ b/src/util-var-name.h @@ -24,14 +24,16 @@ #ifndef __UTIL_VAR_NAME_H__ #define __UTIL_VAR_NAME_H__ -int VarNameStoreSetupStaging(uint32_t de_ctx_version); +void VarNameStoreInit(void); +void VarNameStoreDestroy(void); + +uint32_t VarNameStoreRegister(const char *name, const enum VarTypes type); +const char *VarNameStoreSetupLookup(const uint32_t id, const enum VarTypes type); +void VarNameStoreUnregister(const uint32_t id, const enum VarTypes type); +int VarNameStoreActivate(void); + const char *VarNameStoreLookupById(const uint32_t id, const enum VarTypes type); -uint32_t VarNameStoreLookupByName(const char *name, const enum VarTypes type); -uint32_t VarNameStoreSetupAdd(const char *name, const enum VarTypes type); -char *VarNameStoreSetupLookup(uint32_t idx, const enum VarTypes type); -void VarNameStoreActivateStaging(void); -void VarNameStoreFreeOld(void); -void VarNameStoreFree(uint32_t de_ctx_version); +uint32_t VarNameStoreLookupByName(const char *, const enum VarTypes type); #endif From fe2b8aab17d8dd6056862638a3a4ef4eec33c4d7 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 12 Jul 2023 08:25:28 +0200 Subject: [PATCH 4/7] detect/filemagic: fix thread ctx registration; reloads Make sure thread ctx registration happens and id remains correct in case of reloads. To do so, move id var into the detect ctx. (cherry picked from commit 2cac440f7d062aa54dbff54712087eecce5c7437) --- src/detect-engine.c | 1 + src/detect-filemagic.c | 20 +++++++++----------- src/detect.h | 3 +++ 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/detect-engine.c b/src/detect-engine.c index e3319d79aa86..b176fedc867e 100644 --- a/src/detect-engine.c +++ b/src/detect-engine.c @@ -1986,6 +1986,7 @@ static DetectEngineCtx *DetectEngineCtxInitReal(enum DetectEngineType type, cons TAILQ_INIT(&de_ctx->sig_stat.failed_sigs); de_ctx->sigerror = NULL; de_ctx->type = type; + de_ctx->filemagic_thread_ctx_id = -1; if (type == DETECT_ENGINE_TYPE_DD_STUB || type == DETECT_ENGINE_TYPE_MT_STUB) { de_ctx->version = DetectEngineGetVersion(); diff --git a/src/detect-filemagic.c b/src/detect-filemagic.c index 607f650cd803..7ca7cb2a9c57 100644 --- a/src/detect-filemagic.c +++ b/src/detect-filemagic.c @@ -92,8 +92,6 @@ static int DetectEngineInspectFilemagic( const Signature *s, Flow *f, uint8_t flags, void *alstate, void *txv, uint64_t tx_id); -static int g_magic_thread_ctx_id = -1; - /** * \brief Registration function for keyword: filemagic */ @@ -251,10 +249,10 @@ static int DetectFilemagicSetup (DetectEngineCtx *de_ctx, Signature *s, const ch de_ctx, s, NULL, DETECT_FILE_MAGIC, g_file_magic_buffer_id, s->alproto) < 0) return -1; - if (g_magic_thread_ctx_id == -1) { - g_magic_thread_ctx_id = DetectRegisterThreadCtxFuncs( + if (de_ctx->filemagic_thread_ctx_id == -1) { + de_ctx->filemagic_thread_ctx_id = DetectRegisterThreadCtxFuncs( de_ctx, "filemagic", DetectFilemagicThreadInit, NULL, DetectFilemagicThreadFree, 1); - if (g_magic_thread_ctx_id == -1) + if (de_ctx->filemagic_thread_ctx_id == -1) return -1; } return 0; @@ -276,11 +274,10 @@ static int DetectFilemagicSetupSticky(DetectEngineCtx *de_ctx, Signature *s, con if (DetectBufferSetActiveList(s, g_file_magic_buffer_id) < 0) return -1; - if (g_magic_thread_ctx_id == -1) { - g_magic_thread_ctx_id = DetectRegisterThreadCtxFuncs(de_ctx, "filemagic", - DetectFilemagicThreadInit, NULL, - DetectFilemagicThreadFree, 1); - if (g_magic_thread_ctx_id == -1) + if (de_ctx->filemagic_thread_ctx_id == -1) { + de_ctx->filemagic_thread_ctx_id = DetectRegisterThreadCtxFuncs( + de_ctx, "filemagic", DetectFilemagicThreadInit, NULL, DetectFilemagicThreadFree, 1); + if (de_ctx->filemagic_thread_ctx_id == -1) return -1; } return 0; @@ -301,7 +298,8 @@ static InspectionBuffer *FilemagicGetDataCallback(DetectEngineThreadCtx *det_ctx if (cur_file->magic == NULL) { DetectFilemagicThreadData *tfilemagic = - (DetectFilemagicThreadData *)DetectThreadCtxGetKeywordThreadCtx(det_ctx, g_magic_thread_ctx_id); + (DetectFilemagicThreadData *)DetectThreadCtxGetKeywordThreadCtx( + det_ctx, det_ctx->de_ctx->filemagic_thread_ctx_id); if (tfilemagic == NULL) { return NULL; } diff --git a/src/detect.h b/src/detect.h index 2aadd7fbf63f..86d32939fe5a 100644 --- a/src/detect.h +++ b/src/detect.h @@ -807,6 +807,9 @@ typedef struct DetectEngineCtx_ { uint16_t mpm_matcher; /**< mpm matcher this ctx uses */ uint16_t spm_matcher; /**< spm matcher this ctx uses */ + /* registration id for per thread ctx for the filemagic/file.magic keywords */ + int filemagic_thread_ctx_id; + /* spm thread context prototype, built as spm matchers are constructed and * later used to construct thread context for each thread. */ SpmGlobalThreadCtx *spm_global_thread_ctx; From c7fbaa72093d550411cac538f05bc3c9163e8523 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 9 Aug 2023 08:00:09 +0200 Subject: [PATCH 5/7] stat: add wrappers to isolate OS_WIN32 specifics (cherry picked from commit 269f751d36e8c485b6a31bd40242749056f49bc5) --- src/detect-engine.c | 10 ++---- src/runmode-unix-socket.c | 44 +++++------------------ src/source-pcap-file-directory-helper.c | 15 +++----- src/suricata.c | 48 +++++++++++++++---------- src/util-conf.c | 20 ++++------- src/util-path.h | 10 ++++++ 6 files changed, 62 insertions(+), 85 deletions(-) diff --git a/src/detect-engine.c b/src/detect-engine.c index b176fedc867e..2a3c7f4c7648 100644 --- a/src/detect-engine.c +++ b/src/detect-engine.c @@ -75,6 +75,7 @@ #include "util-spm.h" #include "util-device.h" #include "util-var-name.h" +#include "util-path.h" #include "util-profiling.h" #include "util-validate.h" @@ -3301,13 +3302,8 @@ static int DetectEngineMultiTenantLoadTenant(uint32_t tenant_id, const char *fil snprintf(prefix, sizeof(prefix), "multi-detect.%u", tenant_id); -#ifdef OS_WIN32 - struct _stat st; - if(_stat(filename, &st) != 0) { -#else - struct stat st; - if(stat(filename, &st) != 0) { -#endif /* OS_WIN32 */ + SCStat st; + if (SCStatFn(filename, &st) != 0) { SCLogError(SC_ERR_FOPEN, "failed to stat file %s", filename); goto error; } diff --git a/src/runmode-unix-socket.c b/src/runmode-unix-socket.c index 8dbe1de3e24b..66c02e49e593 100644 --- a/src/runmode-unix-socket.c +++ b/src/runmode-unix-socket.c @@ -27,6 +27,8 @@ #include "util-time.h" #include "util-cpu.h" #include "util-affinity.h" +#include "util-var-name.h" +#include "util-path.h" #include "unix-manager.h" #include "detect-engine.h" @@ -304,11 +306,7 @@ static TmEcode UnixSocketAddPcapFileImpl(json_t *cmd, json_t* answer, void *data bool should_delete = false; time_t delay = 30; time_t poll_interval = 5; -#ifdef OS_WIN32 - struct _stat st; -#else - struct stat st; -#endif /* OS_WIN32 */ + SCStat st; json_t *jarg = json_object_get(cmd, "filename"); if (!json_is_string(jarg)) { @@ -318,11 +316,7 @@ static TmEcode UnixSocketAddPcapFileImpl(json_t *cmd, json_t* answer, void *data return TM_ECODE_FAILED; } filename = json_string_value(jarg); -#ifdef OS_WIN32 - if (_stat(filename, &st) != 0) { -#else - if (stat(filename, &st) != 0) { -#endif /* OS_WIN32 */ + if (SCStatFn(filename, &st) != 0) { json_object_set_new(answer, "message", json_string("filename does not exist")); return TM_ECODE_FAILED; @@ -346,11 +340,7 @@ static TmEcode UnixSocketAddPcapFileImpl(json_t *cmd, json_t* answer, void *data return TM_ECODE_FAILED; } -#ifdef OS_WIN32 - if (_stat(output_dir, &st) != 0) { -#else - if (stat(output_dir, &st) != 0) { -#endif /* OS_WIN32 */ + if (SCStatFn(output_dir, &st) != 0) { json_object_set_new(answer, "message", json_string("output-dir does not exist")); return TM_ECODE_FAILED; @@ -909,11 +899,7 @@ TmEcode UnixSocketUnregisterTenantHandler(json_t *cmd, json_t* answer, void *dat TmEcode UnixSocketRegisterTenant(json_t *cmd, json_t* answer, void *data) { const char *filename; -#ifdef OS_WIN32 - struct _stat st; -#else - struct stat st; -#endif /* OS_WIN32 */ + SCStat st; if (!(DetectEngineMultiTenantEnabled())) { SCLogInfo("error: multi-tenant support not enabled"); @@ -936,11 +922,7 @@ TmEcode UnixSocketRegisterTenant(json_t *cmd, json_t* answer, void *data) return TM_ECODE_FAILED; } filename = json_string_value(jarg); -#ifdef OS_WIN32 - if (_stat(filename, &st) != 0) { -#else - if (stat(filename, &st) != 0) { -#endif /* OS_WIN32 */ + if (SCStatFn(filename, &st) != 0) { json_object_set_new(answer, "message", json_string("file does not exist")); return TM_ECODE_FAILED; } @@ -985,11 +967,7 @@ static int reload_cnt = 1; TmEcode UnixSocketReloadTenant(json_t *cmd, json_t* answer, void *data) { const char *filename; -#ifdef OS_WIN32 - struct _stat st; -#else - struct stat st; -#endif /* OS_WIN32 */ + SCStat st; if (!(DetectEngineMultiTenantEnabled())) { SCLogInfo("error: multi-tenant support not enabled"); @@ -1012,11 +990,7 @@ TmEcode UnixSocketReloadTenant(json_t *cmd, json_t* answer, void *data) return TM_ECODE_FAILED; } filename = json_string_value(jarg); -#ifdef OS_WIN32 - if (_stat(filename, &st) != 0) { -#else - if (stat(filename, &st) != 0) { -#endif /* OS_WIN32 */ + if (SCStatFn(filename, &st) != 0) { json_object_set_new(answer, "message", json_string("file does not exist")); return TM_ECODE_FAILED; } diff --git a/src/source-pcap-file-directory-helper.c b/src/source-pcap-file-directory-helper.c index 7cdad3625470..b0045a891b76 100644 --- a/src/source-pcap-file-directory-helper.c +++ b/src/source-pcap-file-directory-helper.c @@ -26,6 +26,8 @@ #include "source-pcap-file-directory-helper.h" #include "runmode-unix-socket.h" #include "util-mem.h" +#include "util-time.h" +#include "util-path.h" #include "source-pcap-file.h" static void GetTime(struct timespec *tm); @@ -227,23 +229,14 @@ TmEcode PcapDetermineDirectoryOrFile(char *filename, DIR **directory) int PcapDirectoryGetModifiedTime(char const *file, struct timespec *out) { -#ifdef OS_WIN32 - struct _stat buf; -#else - struct stat buf; -#endif /* OS_WIN32 */ + SCStat buf; int ret; if (file == NULL) return -1; -#ifdef OS_WIN32 - if((ret = _stat(file, &buf)) != 0) - return ret; -#else - if ((ret = stat(file, &buf)) != 0) + if ((ret = SCStatFn(file, &buf)) != 0) return ret; -#endif #ifdef OS_DARWIN out->tv_sec = buf.st_mtimespec.tv_sec; diff --git a/src/suricata.c b/src/suricata.c index 45d9d5d742b9..9a08655fa238 100644 --- a/src/suricata.c +++ b/src/suricata.c @@ -172,6 +172,25 @@ #include "tmqh-packetpool.h" +#include "util-byte.h" +#include "util-conf.h" +#include "util-coredump-config.h" +#include "util-cpu.h" +#include "util-daemon.h" +#include "util-device.h" +#include "util-ebpf.h" +#include "util-exception-policy.h" +#include "util-host-os-info.h" +#include "util-ioctl.h" +#include "util-luajit.h" +#include "util-macset.h" +#include "util-misc.h" +#include "util-mpm-hs.h" +#include "util-path.h" +#include "util-pidfile.h" +#include "util-plugin.h" +#include "util-privs.h" +#include "util-profiling.h" #include "util-proto-name.h" #include "util-running-modes.h" #include "util-signal.h" @@ -518,19 +537,17 @@ static void SetBpfStringFromFile(char *filename) char *bpf_comment_tmp = NULL; char *bpf_comment_start = NULL; uint32_t bpf_len = 0; -#ifdef OS_WIN32 - struct _stat st; -#else - struct stat st; -#endif /* OS_WIN32 */ + SCStat st; FILE *fp = NULL; size_t nm = 0; -#ifdef OS_WIN32 - if(_stat(filename, &st) != 0) { -#else - if(stat(filename, &st) != 0) { -#endif /* OS_WIN32 */ + fp = fopen(filename, "r"); + if (fp == NULL) { + SCLogError(SC_ERR_FOPEN, "Failed to open file %s", filename); + exit(EXIT_FAILURE); + } + + if (SCFstatFn(fileno(fp), &st) != 0) { SCLogError(SC_ERR_FOPEN, "Failed to stat file %s", filename); exit(EXIT_FAILURE); } @@ -1828,14 +1845,9 @@ static TmEcode ParseCommandLine(int argc, char** argv, SCInstance *suri) PrintUsage(argv[0]); return TM_ECODE_FAILED; } -#ifdef OS_WIN32 - struct _stat buf; - if(_stat(optarg, &buf) != 0) { -#else - struct stat buf; - if (stat(optarg, &buf) != 0) { -#endif /* OS_WIN32 */ - SCLogError(SC_ERR_INITIALIZATION, "ERROR: Pcap file does not exist\n"); + SCStat buf; + if (SCStatFn(optarg, &buf) != 0) { + SCLogError(SC_ERR_INITIALIZATION, "pcap file '%s': %s", optarg, strerror(errno)); return TM_ECODE_FAILED; } if (ConfSetFinal("pcap-file.file", optarg) != 1) { diff --git a/src/util-conf.c b/src/util-conf.c index da337c6b4bfb..916a0a8ba718 100644 --- a/src/util-conf.c +++ b/src/util-conf.c @@ -26,6 +26,8 @@ #include "conf.h" #include "runmodes.h" #include "util-conf.h" +#include "util-debug.h" +#include "util-path.h" TmEcode ConfigSetLogDirectory(const char *name) { @@ -53,13 +55,8 @@ const char *ConfigGetLogDirectory(void) TmEcode ConfigCheckLogDirectoryExists(const char *log_dir) { SCEnter(); -#ifdef OS_WIN32 - struct _stat buf; - if (_stat(log_dir, &buf) != 0) { -#else - struct stat buf; - if (stat(log_dir, &buf) != 0) { -#endif /* OS_WIN32 */ + SCStat buf; + if (SCStatFn(log_dir, &buf) != 0) { SCReturnInt(TM_ECODE_FAILED); } SCReturnInt(TM_ECODE_OK); @@ -101,13 +98,8 @@ const char *ConfigGetDataDirectory(void) TmEcode ConfigCheckDataDirectory(const char *data_dir) { SCEnter(); -#ifdef OS_WIN32 - struct _stat buf; - if (_stat(data_dir, &buf) != 0) { -#else - struct stat buf; - if (stat(data_dir, &buf) != 0) { -#endif /* OS_WIN32 */ + SCStat buf; + if (SCStatFn(data_dir, &buf) != 0) { SCReturnInt(TM_ECODE_FAILED); } SCReturnInt(TM_ECODE_OK); diff --git a/src/util-path.h b/src/util-path.h index 6f788a8f2513..b8a5dd25939d 100644 --- a/src/util-path.h +++ b/src/util-path.h @@ -25,6 +25,16 @@ #ifndef __UTIL_PATH_H__ #define __UTIL_PATH_H__ +#ifdef OS_WIN32 +typedef struct _stat SCStat; +#define SCFstatFn(fd, statbuf) _fstat((fd), (statbuf)) +#define SCStatFn(pathname, statbuf) _stat((pathname), (statbuf)) +#else +typedef struct stat SCStat; +#define SCFstatFn(fd, statbuf) fstat((fd), (statbuf)) +#define SCStatFn(pathname, statbuf) stat((pathname), (statbuf)) +#endif + #ifndef HAVE_NON_POSIX_MKDIR #define SCMkDir(a, b) mkdir(a, b) #else From 3d285406011cec3e530c9b0c6f6ec1149b062d22 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Thu, 10 Aug 2023 10:07:22 +0200 Subject: [PATCH 6/7] path: new funcs to merge paths Take windows directory separators into account. Path is not checked or "resolved". (cherry picked from commit 228caa640b3f961fd13ca8744cbbee789116bd30) --- src/util-error.c | 2 ++ src/util-error.h | 2 ++ src/util-path.c | 80 +++++++++++++++++++++++++++++++++++++++--------- src/util-path.h | 10 +++++- 4 files changed, 78 insertions(+), 16 deletions(-) diff --git a/src/util-error.c b/src/util-error.c index 975e12ae9eef..d516aca57d89 100644 --- a/src/util-error.c +++ b/src/util-error.c @@ -381,6 +381,8 @@ const char * SCErrorToString(SCError err) CASE_CODE(SC_ERR_HASH_ADD); CASE_CODE(SC_ERR_SIGNAL); CASE_CODE(SC_WARN_REFCNT); + CASE_CODE(SC_ERR_PATH_JOIN); + CASE_CODE(SC_ERR_PATH_RESOLVE); CASE_CODE (SC_ERR_MAX); } diff --git a/src/util-error.h b/src/util-error.h index 398aee9eebe4..841cc128a935 100644 --- a/src/util-error.h +++ b/src/util-error.h @@ -371,6 +371,8 @@ typedef enum { SC_ERR_HASH_ADD, SC_ERR_SIGNAL, SC_WARN_REFCNT, + SC_ERR_PATH_JOIN, + SC_ERR_PATH_RESOLVE, SC_ERR_MAX } SCError; diff --git a/src/util-path.c b/src/util-path.c index 22d5e94e0b22..7063aba595b5 100644 --- a/src/util-path.c +++ b/src/util-path.c @@ -1,4 +1,4 @@ -/* Copyright (C) 2007-2012 Open Information Security Foundation +/* Copyright (C) 2007-2023 Open Information Security Foundation * * You can copy, redistribute or modify this Program under the terms of * the GNU General Public License version 2 as published by the Free @@ -72,36 +72,86 @@ int PathIsRelative(const char *path) return PathIsAbsolute(path) ? 0 : 1; } +int PathMerge(char *out_buf, size_t buf_size, const char *const dir, const char *const fname) +{ + char path[PATH_MAX]; + if (dir == NULL || strlen(dir) == 0) + return -1; + + size_t r = strlcpy(path, dir, sizeof(path)); + if (r >= sizeof(path)) { + return -1; + } + +#if defined OS_WIN32 || defined __CYGWIN__ + if (path[strlen(path) - 1] != '\\') + r = strlcat(path, "\\\\", sizeof(path)); +#else + if (path[strlen(path) - 1] != '/') + r = strlcat(path, "/", sizeof(path)); +#endif + if (r >= sizeof(path)) { + return -1; + } + r = strlcat(path, fname, sizeof(path)); + if (r >= sizeof(path)) { + return -1; + } + r = strlcpy(out_buf, path, buf_size); + if (r >= buf_size) { + return -1; + } + + return 0; +} + +char *PathMergeAlloc(const char *const dir, const char *const fname) +{ + char path[PATH_MAX]; + if (PathMerge(path, sizeof(path), dir, fname) != 0) + return NULL; + + char *ret = SCStrdup(path); + if (ret == NULL) + return NULL; + + return ret; +} + /** * \brief Wrapper to join a directory and filename and resolve using realpath * _fullpath is used for WIN32 * * \param out_buf output buffer. Up to PATH_MAX will be written. Unchanged on exit failure. - * \param buf_len length of output buffer + * \param buf_size length of output buffer, must be PATH_MAX * \param dir the directory * \param fname the filename * - * \retval TM_ECODE_OK on success - * \retval TM_ECODE_FAILED on failure + * \retval 0 on success + * \retval -1 on failure */ -TmEcode PathJoin (char *out_buf, uint16_t buf_len, const char *const dir, const char *const fname) +int PathJoin(char *out_buf, size_t buf_size, const char *const dir, const char *const fname) { SCEnter(); - uint16_t max_path_len = MAX(buf_len, PATH_MAX); - int bytes_written = snprintf(out_buf, max_path_len, "%s%c%s", dir, DIRECTORY_SEPARATOR, fname); - if (bytes_written <= 0) { - SCLogError(SC_ERR_SPRINTF, "Could not join filename to path"); - SCReturnInt(TM_ECODE_FAILED); + if (buf_size != PATH_MAX) { + return -1; + } + if (PathMerge(out_buf, buf_size, dir, fname) != 0) { + SCLogError(SC_ERR_PATH_JOIN, "Could not join filename to path"); + return -1; } char *tmp_buf = SCRealPath(out_buf, NULL); if (tmp_buf == NULL) { - SCLogError(SC_ERR_SPRINTF, "Error resolving path: %s", strerror(errno)); - SCReturnInt(TM_ECODE_FAILED); + SCLogError(SC_ERR_PATH_RESOLVE, "Error resolving path: %s", strerror(errno)); + return -1; } - memset(out_buf, 0, buf_len); - strlcpy(out_buf, tmp_buf, max_path_len); + memset(out_buf, 0, buf_size); + size_t ret = strlcpy(out_buf, tmp_buf, buf_size); free(tmp_buf); - SCReturnInt(TM_ECODE_OK); + if (ret >= buf_size) { + return -1; + } + return 0; } /** diff --git a/src/util-path.h b/src/util-path.h index b8a5dd25939d..141b6fc2472e 100644 --- a/src/util-path.h +++ b/src/util-path.h @@ -35,6 +35,12 @@ typedef struct stat SCStat; #define SCStatFn(pathname, statbuf) stat((pathname), (statbuf)) #endif +#if defined OS_WIN32 || defined __CYGWIN__ +#define PATH_SEPARATOR_SIZE 2 +#else +#define PATH_SEPARATOR_SIZE 1 +#endif + #ifndef HAVE_NON_POSIX_MKDIR #define SCMkDir(a, b) mkdir(a, b) #else @@ -43,7 +49,9 @@ typedef struct stat SCStat; int PathIsAbsolute(const char *); int PathIsRelative(const char *); -TmEcode PathJoin (char *out_buf, uint16_t buf_len, const char *const dir, const char *const fname); +int PathMerge(char *out_buf, size_t buf_size, const char *const dir, const char *const fname); +char *PathMergeAlloc(const char *const dir, const char *const fname); +int PathJoin(char *out_buf, size_t buf_len, const char *const dir, const char *const fname); int SCDefaultMkDir(const char *path); int SCCreateDirectoryTree(const char *path, const bool final); bool SCPathExists(const char *path); From cf22cdb268466919875f0ad5d88548db024bf70d Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Sun, 20 Aug 2023 17:32:47 +0200 Subject: [PATCH 7/7] community-id: Fix IPv6 address sorting not respecting byte order When comparing IPv6 addresses based on uint32_t chunks, one needs to apply ntohl() conversion to the individual parts, otherwise on little endian systems individual bytes are compared in the wrong order. Avoid this all and leverage memcmp(), it'll short circuit on the first differing byte and its return values tells us which address sorts lower. Bug: #6276 (cherry picked from commit ccefbd80268a3fe947b43cb9facd613173e02b68) --- src/output-json.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/src/output-json.c b/src/output-json.c index f26297257fa1..347697b4497e 100644 --- a/src/output-json.c +++ b/src/output-json.c @@ -679,18 +679,6 @@ static bool CalculateCommunityFlowIdv4(const Flow *f, return false; } -static inline bool FlowHashRawAddressIPv6LtU32(const uint32_t *a, const uint32_t *b) -{ - for (int i = 0; i < 4; i++) { - if (a[i] < b[i]) - return true; - if (a[i] > b[i]) - break; - } - - return false; -} - static bool CalculateCommunityFlowIdv6(const Flow *f, const uint16_t seed, unsigned char *base64buf) { @@ -714,9 +702,8 @@ static bool CalculateCommunityFlowIdv6(const Flow *f, dp = htons(dp); ipv6.seed = htons(seed); - if (FlowHashRawAddressIPv6LtU32(f->src.addr_data32, f->dst.addr_data32) || - ((memcmp(&f->src, &f->dst, sizeof(f->src)) == 0) && sp < dp)) - { + int cmp_r = memcmp(&f->src, &f->dst, sizeof(f->src)); + if ((cmp_r < 0) || (cmp_r == 0 && sp < dp)) { memcpy(&ipv6.src, &f->src.addr_data32, 16); memcpy(&ipv6.dst, &f->dst.addr_data32, 16); ipv6.sp = sp;