From 60ad9d29c5e9ce77836da4893829b5e73c42a698 Mon Sep 17 00:00:00 2001 From: Steve Grubb Date: Sun, 24 Jan 2010 14:30:36 -0500 Subject: [PATCH] Memory leak cleanup in detectors Hello, I ran the code through an analysis program and found several leaks that should be cleaned up. *In src/detect-engine-address-ipv4.c at line 472, the test for ag == NULL will never be true since that is the loop entry test. *In src/detect-engine-port.c at line 1133, the test for p == NULL will never be true since that is the loop entry test. *In src/detect-engine-mpm.c at line 263 is a return without freeing fast_pattern *In src/detect-ack.c at line 80 and 85, data catches the return from malloc. One of them should be deleted. *In src/detect-seq.c at line 81 and 86, data catches the return from malloc. One of them should be deleted. *In src/detect-content.c at line 749, many of the paths that lead to the error exit still has temp pointing to allocated memory. To clean this up, temp should be set to NULL if not immediately assigning and new value. *In src/detect-uricontent.c at line 319, both cd and str needto be freed. At lines 344, str needs to be freed. And at line 347 str and temp need to be freed. *In src/detect-flowbits.c at line 231 and 235, str was not being freed. cd was not being freed at line 235. *In src/detect-flowvar.c at line 127, str was not being freed. At line 194, cd and str were not being freed. *In src/detect-flowint.c at line 277, sfd was not being freed. At line 315, str was not being freed. *In src/detect-pktvar.c at line 121, str was not being freed. At line 188, str and cd was not being freed. *In src/detect-pcre.c at line 389, there is an extra free of "re" that should be deleted. *In src/detect-depth.c at line 42 & 48, str has not been freed. *In src/detect-distance.c at line 49 and 55, str has not been freed *In src/detect-offset.c at line 45, str has not been freed. The patch below fixes these issues. -Steve --- src/detect-ack.c | 2 +- src/detect-content.c | 5 ++++- src/detect-depth.c | 2 ++ src/detect-distance.c | 2 ++ src/detect-engine-address-ipv4.c | 2 -- src/detect-engine-mpm.c | 4 +++- src/detect-engine-port.c | 3 --- src/detect-flowbits.c | 7 ++++--- src/detect-flowint.c | 2 ++ src/detect-flowvar.c | 9 +++++++-- src/detect-offset.c | 2 ++ src/detect-pcre.c | 1 - src/detect-pktvar.c | 9 +++++++-- src/detect-seq.c | 2 +- src/detect-uricontent.c | 13 +++++++++---- 15 files changed, 44 insertions(+), 21 deletions(-) diff --git a/src/detect-ack.c b/src/detect-ack.c index 55a1cae54d..41a415d8f9 100644 --- a/src/detect-ack.c +++ b/src/detect-ack.c @@ -77,7 +77,7 @@ static int DetectAckMatch(ThreadVars *t, DetectEngineThreadCtx *det_ctx, static int DetectAckSetup(DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, char *optstr) { - DetectAckData *data = malloc(sizeof(DetectAckData)); + DetectAckData *data; SigMatch *sm = NULL; //printf("DetectAckSetup: \'%s\'\n", optstr); diff --git a/src/detect-content.c b/src/detect-content.c index f205ca0bc9..2ba130c1cf 100644 --- a/src/detect-content.c +++ b/src/detect-content.c @@ -591,7 +591,7 @@ DetectContentData *DetectContentParse (char *contentstr) goto error; if (strlen(temp) == 0) { - if (temp) free(temp); + free(temp); return NULL; } @@ -625,6 +625,7 @@ DetectContentData *DetectContentParse (char *contentstr) } free(temp); + temp = NULL; if (str[0] == '!') { if (cd->negated == 1) { @@ -637,6 +638,7 @@ DetectContentData *DetectContentParse (char *contentstr) goto error; cd->negated = 1; free(temp); + temp = NULL; } } @@ -741,6 +743,7 @@ DetectContentData *DetectContentParse (char *contentstr) error: free(str); + free(temp); if (cd != NULL) { if (cd->content != NULL) free(cd->content); diff --git a/src/detect-depth.c b/src/detect-depth.c index f1f3c21ae4..7fdc75b0cb 100644 --- a/src/detect-depth.c +++ b/src/detect-depth.c @@ -39,12 +39,14 @@ int DetectDepthSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, char * SigMatch *pm = DetectContentFindPrevApplicableSM(m); if (pm == NULL) { SCLogError(SC_ERR_DEPTH_MISSING_CONTENT, "depth needs a preceeding content option"); + if (dubbed) free(str); return -1; } DetectContentData *cd = (DetectContentData *)pm->ctx; if (cd == NULL) { SCLogError(SC_INVALID_ARGUMENT, "invalid argument"); + if (dubbed) free(str); return -1; } diff --git a/src/detect-distance.c b/src/detect-distance.c index c3100abf1e..39d29f7610 100644 --- a/src/detect-distance.c +++ b/src/detect-distance.c @@ -46,12 +46,14 @@ int DetectDistanceSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, cha pm = DetectContentFindPrevApplicableSM(m); if (pm == NULL || DetectContentHasPrevSMPattern(pm) == NULL) { SCLogError(SC_ERR_DISTANCE_MISSING_CONTENT, "distance needs two preceeding content options"); + if (dubbed) free(str); return -1; } DetectContentData *cd = (DetectContentData *)pm->ctx; if (cd == NULL) { printf("DetectDistanceSetup: Unknown previous keyword!\n"); + if (dubbed) free(str); return -1; } diff --git a/src/detect-engine-address-ipv4.c b/src/detect-engine-address-ipv4.c index 9748fbc894..7205650fef 100644 --- a/src/detect-engine-address-ipv4.c +++ b/src/detect-engine-address-ipv4.c @@ -469,8 +469,6 @@ int DetectAddressIsCompleteIPSpaceIPv4(DetectAddress *ag) ag = ag->next; for ( ; ag != NULL; ag = ag->next) { - if (ag == NULL) - return 0; if (ag->ip[0] != next_ip) return 0; diff --git a/src/detect-engine-mpm.c b/src/detect-engine-mpm.c index 8954e59f0b..5be50801cc 100644 --- a/src/detect-engine-mpm.c +++ b/src/detect-engine-mpm.c @@ -259,8 +259,10 @@ static int PatternMatchPreprarePopulateMpm(DetectEngineCtx *de_ctx, SigGroupHead memset(fast_pattern, 0, sgh->sig_cnt * sizeof(uint32_t)); HashTable *ht = HashTableInit(4096, ContentHashFunc, ContentHashCompareFunc, ContentHashFree); - if (ht == NULL) + if (ht == NULL) { + free(fast_pattern); return -1; + } /* add all the contents to a counting hash */ for (sig = 0; sig < sgh->sig_cnt; sig++) { diff --git a/src/detect-engine-port.c b/src/detect-engine-port.c index 381ff04013..ae03dd5822 100644 --- a/src/detect-engine-port.c +++ b/src/detect-engine-port.c @@ -1130,9 +1130,6 @@ int DetectPortIsCompletePortSpace(DetectPort *p) { p = p->next; for ( ; p != NULL; p = p->next) { - if (p == NULL) - return 0; - if (p->port != next_port) return 0; diff --git a/src/detect-flowbits.c b/src/detect-flowbits.c index df5ae4a07e..50786f60c4 100644 --- a/src/detect-flowbits.c +++ b/src/detect-flowbits.c @@ -228,12 +228,13 @@ int DetectFlowbitSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, char SigMatchAppend(s,m,sm); - if (dubbed) free(str); + free(str); return 0; error: - if (dubbed) free(str); - if (sm) free(sm); + free(str); + free(cd); + free(sm); return -1; } diff --git a/src/detect-flowint.c b/src/detect-flowint.c index 8dc2a9044f..e5f9a24e2a 100644 --- a/src/detect-flowint.c +++ b/src/detect-flowint.c @@ -274,6 +274,7 @@ DetectFlowintData *DetectFlowintParse(DetectEngineCtx *de_ctx, varval =(char *) str_ptr; if (res < 0 || strcmp(varval,"") == 0) { SCLogDebug("DetectFlowintParse: pcre_get_substring failed"); + free(sfd); return NULL; } @@ -312,6 +313,7 @@ DetectFlowintData *DetectFlowintParse(DetectEngineCtx *de_ctx, return sfd; error: if (sfd != NULL) free(sfd); + free(str); return NULL; } diff --git a/src/detect-flowvar.c b/src/detect-flowvar.c index e4161dc6a9..6e3e154054 100644 --- a/src/detect-flowvar.c +++ b/src/detect-flowvar.c @@ -123,8 +123,10 @@ int DetectFlowvarSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, char } len = strlen(str); - if (len == 0) + if (len == 0) { + if (dubbed) free(str); return -1; + } cd = malloc(sizeof(DetectFlowvarData)); if (cd == NULL) { @@ -190,8 +192,11 @@ int DetectFlowvarSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, char } cd->content = malloc(len); - if (cd->content == NULL) + if (cd->content == NULL) { + if (dubbed) free(str); + free(cd); return -1; + } cd->name = strdup(varname); cd->idx = VariableNameGetIdx(de_ctx,varname,DETECT_FLOWVAR); diff --git a/src/detect-offset.c b/src/detect-offset.c index 04b629f403..ae3f70c177 100644 --- a/src/detect-offset.c +++ b/src/detect-offset.c @@ -42,12 +42,14 @@ int DetectOffsetSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, char SigMatch *pm = DetectContentFindPrevApplicableSM(m); if (pm == NULL) { SCLogError(SC_ERR_OFFSET_MISSING_CONTENT, "offset needs a preceeding content option"); + if (dubbed) free(str); return -1; } DetectContentData *cd = (DetectContentData *)pm->ctx; if (cd == NULL) { SCLogError(SC_INVALID_ARGUMENT, "invalid argument"); + if (dubbed) free(str); return -1; } diff --git a/src/detect-pcre.c b/src/detect-pcre.c index 748809bfe8..eeac6d78d4 100644 --- a/src/detect-pcre.c +++ b/src/detect-pcre.c @@ -386,7 +386,6 @@ error: if (op_ptr != NULL) free(op_ptr); if (pd != NULL && pd->re != NULL) pcre_free(pd->re); if (pd != NULL && pd->sd != NULL) pcre_free(pd->sd); - if (dubbed) free(re); if (pd) free(pd); return NULL; } diff --git a/src/detect-pktvar.c b/src/detect-pktvar.c index 0cea25ae60..6eaa250c6e 100644 --- a/src/detect-pktvar.c +++ b/src/detect-pktvar.c @@ -117,8 +117,10 @@ int DetectPktvarSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, char } len = strlen(str); - if (len == 0) + if (len == 0) { + if (dubbed) free(str); return -1; + } cd = malloc(sizeof(DetectPktvarData)); if (cd == NULL) { @@ -184,8 +186,11 @@ int DetectPktvarSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, char } cd->content = malloc(len); - if (cd->content == NULL) + if (cd->content == NULL) { + free(cd); + if (dubbed) free(str); return -1; + } cd->name = strdup(varname); memcpy(cd->content, str, len); diff --git a/src/detect-seq.c b/src/detect-seq.c index 519b11e5b8..20971b6813 100644 --- a/src/detect-seq.c +++ b/src/detect-seq.c @@ -78,7 +78,7 @@ static int DetectSeqMatch(ThreadVars *t, DetectEngineThreadCtx *det_ctx, static int DetectSeqSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, char *optstr) { - DetectSeqData *data = malloc(sizeof(DetectSeqData)); + DetectSeqData *data; SigMatch *sm = NULL; //printf("DetectSeqSetup: \'%s\'\n", optstr); diff --git a/src/detect-uricontent.c b/src/detect-uricontent.c index ba1f1cd50c..e9420101d5 100644 --- a/src/detect-uricontent.c +++ b/src/detect-uricontent.c @@ -220,7 +220,7 @@ int DetectUricontentSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, c goto error; if (strlen(temp) == 0) { - if (temp) free(temp); + free(temp); return -1; } @@ -252,6 +252,7 @@ int DetectUricontentSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, c } free(temp); + temp = NULL; len = strlen(str); //printf("DetectUricontentSetup: \"%s\", len %" PRIu32 "\n", str, len); @@ -315,8 +316,11 @@ int DetectUricontentSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, c SCLogDebug("len %" PRIu32 "", len); cd->uricontent = malloc(len); - if (cd->uricontent == NULL) + if (cd->uricontent == NULL) { + free(cd); + free(str); return -1; + } memcpy(cd->uricontent, str, len); cd->uricontent_len = len; @@ -340,11 +344,12 @@ int DetectUricontentSetup (DetectEngineCtx *de_ctx, Signature *s, SigMatch *m, c cd->id = de_ctx->uricontent_max_id; de_ctx->uricontent_max_id++; - if (dubbed) free(str); + free(str); return 0; error: - if (dubbed) free(str); + free(str); + free(temp); if (cd) free(cd); if (sm) free(sm); return -1;