Skip to content

Commit

Permalink
var-names: reimplement var name handling
Browse files Browse the repository at this point in the history 
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: OISF#6044.
Bug: OISF#6201.
  • Loading branch information
@victorjulien
victorjulien committed Aug 8, 2023
1 parent d7e8350 commit dc97a55
Show file tree
Hide file tree
Showing 14 changed files with 386 additions and 335 deletions.
2 changes: 1 addition & 1 deletion src/detect-engine-build.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2035,7 +2035,7 @@ int SigGroupBuild(DetectEngineCtx *de_ctx)
ThresholdHashAllocate(de_ctx);

if (!DetectEngineMultiTenantEnabled()) {
VarNameStoreActivateStaging();
VarNameStoreActivate();
}
return 0;
}
Expand Down
5 changes: 1 addition & 4 deletions src/detect-engine.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2530,7 +2530,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:
Expand Down Expand Up @@ -2658,8 +2657,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);
SCClassConfDeinit(de_ctx);
SCReferenceConfDeinit(de_ctx);

Expand Down Expand Up @@ -4277,7 +4274,7 @@ int DetectEngineMultiTenantSetup(const bool unix_socket)
goto error;
}

VarNameStoreActivateStaging();
VarNameStoreActivate();

} else {
SCLogDebug("multi-detect not enabled (multi tenancy)");
Expand Down
27 changes: 15 additions & 12 deletions src/detect-flowbits.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,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);
}

Expand Down Expand Up @@ -329,7 +329,7 @@ int DetectFlowbitSetup (DetectEngineCtx *de_ctx, Signature *s, const char *rawst
}
cd->cmd = fb_cmd;
} else {
cd->idx = VarNameStoreSetupAdd(fb_name, VAR_TYPE_FLOW_BIT);
cd->idx = VarNameStoreRegister(fb_name, VAR_TYPE_FLOW_BIT);
de_ctx->max_fb_id = MAX(cd->idx, de_ctx->max_fb_id);
cd->cmd = fb_cmd;
cd->or_list_size = 0;
Expand Down Expand Up @@ -383,8 +383,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);
}

Expand Down Expand Up @@ -581,7 +586,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;

Expand Down Expand Up @@ -634,7 +639,6 @@ int DetectFlowbitsAnalyze(DetectEngineCtx *de_ctx)
"stateful rules that set flowbit %s", s->id, varname);
}
}
SCFree(varname);
}

if (rule_engine_analysis_set) {
Expand Down Expand Up @@ -664,7 +668,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;

Expand Down Expand Up @@ -724,7 +728,6 @@ static void DetectFlowbitsAnalyzeDump(const DetectEngineCtx *de_ctx,
}
jb_close(js);
}
SCFree(varname);
jb_close(js);
}
jb_close(js); // array
Expand Down Expand Up @@ -903,8 +906,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);
Expand Down Expand Up @@ -986,7 +989,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);

Expand Down Expand Up @@ -1060,7 +1063,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);

Expand Down Expand Up @@ -1136,7 +1139,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);

Expand Down
3 changes: 2 additions & 1 deletion src/detect-flowint.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ static DetectFlowintData *DetectFlowintParse(DetectEngineCtx *de_ctx, const char
SCLogError("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;

Expand Down Expand Up @@ -422,6 +422,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)
Expand Down
6 changes: 5 additions & 1 deletion src/detect-flowvar.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -177,7 +180,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. */
Expand Down Expand Up @@ -272,6 +275,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))
Expand Down
2 changes: 2 additions & 0 deletions src/detect-flowvar.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ typedef struct DetectFlowvarData_ {
uint32_t idx;
uint8_t *content;
uint16_t content_len;
/** set to true if used in a post-match */
bool post_match;
uint32_t flags;
} DetectFlowvarData;

Expand Down
7 changes: 4 additions & 3 deletions src/detect-hostbits.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,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;
Expand Down Expand Up @@ -451,6 +451,7 @@ void DetectHostbitFree (DetectEngineCtx *de_ctx, void *ptr)

if (fd == NULL)
return;
VarNameStoreUnregister(fd->idx, VAR_TYPE_HOST_BIT);

SCFree(fd);
}
Expand Down Expand Up @@ -688,8 +689,8 @@ static int HostBitsTestSig03(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);
Expand Down
71 changes: 57 additions & 14 deletions src/detect-lua.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -794,7 +794,7 @@ static int DetectLuaSetupPrime(DetectEngineCtx *de_ctx, DetectLuaData *ld, const
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);
}
Expand All @@ -816,7 +816,7 @@ static int DetectLuaSetupPrime(DetectEngineCtx *de_ctx, DetectLuaData *ld, const
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);
}
Expand Down Expand Up @@ -1166,6 +1166,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, lua, "lua");

SCFree(lua);
Expand Down Expand Up @@ -1281,7 +1288,10 @@ static int LuaMatchTest01(void)

FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_str.value_len != 1);
Expand Down Expand Up @@ -1398,7 +1408,10 @@ static int LuaMatchTest01a(void)

FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_str.value_len != 1);
Expand Down Expand Up @@ -1503,7 +1516,10 @@ static int LuaMatchTest02(void)

FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_str.value_len != 1);
Expand Down Expand Up @@ -1606,7 +1622,10 @@ static int LuaMatchTest02a(void)

FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_str.value_len != 1);
Expand Down Expand Up @@ -1709,7 +1728,10 @@ static int LuaMatchTest03(void)

FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_str.value_len != 1);
Expand Down Expand Up @@ -1811,7 +1833,10 @@ static int LuaMatchTest03a(void)
SigMatchSignatures(&th_v, de_ctx, det_ctx, p2);
FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_VAR);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_str.value_len != 1);
Expand Down Expand Up @@ -1925,7 +1950,10 @@ static int LuaMatchTest04(void)

FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_int.value != 2);
Expand Down Expand Up @@ -2040,7 +2068,10 @@ static int LuaMatchTest04a(void)

FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_int.value != 2);
Expand Down Expand Up @@ -2148,7 +2179,10 @@ static int LuaMatchTest05(void)

FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_int.value != 2);
Expand Down Expand Up @@ -2256,7 +2290,10 @@ static int LuaMatchTest05a(void)

FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_int.value != 2);
Expand Down Expand Up @@ -2369,7 +2406,10 @@ static int LuaMatchTest06(void)

FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_int.value != 0);
Expand Down Expand Up @@ -2482,7 +2522,10 @@ static int LuaMatchTest06a(void)

FAIL_IF_NOT(PacketAlertCheck(p2, 1));

FlowVar *fv = FlowVarGet(&f, 1);
uint32_t id = VarNameStoreLookupByName("cnt", VAR_TYPE_FLOW_INT);
FAIL_IF(id == 0);

FlowVar *fv = FlowVarGet(&f, id);
FAIL_IF_NULL(fv);

FAIL_IF(fv->data.fv_int.value != 0);
Expand Down
Loading

0 comments on commit dc97a55

Please sign in to comment.