xbits: clean up parsing and tests

src/detect-xbits.c

@@ -180,10 +180,16 @@ int DetectXbitMatch (ThreadVars *t, DetectEngineThreadCtx *det_ctx, Packet *p, S
     return 0;
 }
 
-int DetectXbitSetup (DetectEngineCtx *de_ctx, Signature *s, char *rawstr)
+/** \internal
+ *  \brief parse xbits rule options
+ *  \retval 0 ok
+ *  \retval -1 bad
+ *  \param[out] cdout return DetectXbitsData structure or NULL if noalert
+ */
+static int DetectXbitParse(DetectEngineCtx *de_ctx,
+        const char *rawstr, DetectXbitsData **cdout)
 {
     DetectXbitsData *cd = NULL;
-    SigMatch *sm = NULL;
     uint8_t fb_cmd = 0;
     uint8_t hb_dir = 0;
 #define MAX_SUBSTRINGS 30
@@ -192,7 +198,7 @@ int DetectXbitSetup (DetectEngineCtx *de_ctx, Signature *s, char *rawstr)
     char fb_cmd_str[16] = "", fb_name[256] = "";
     char hb_dir_str[16] = "";
     enum VarTypes var_type = VAR_TYPE_NOT_SET;
-    int expire = 30;
+    int expire = DETECT_XBITS_EXPIRE_DEFAULT;
 
     ret = pcre_exec(parse_regex, parse_regex_study, rawstr, strlen(rawstr), 0, 0, ov, MAX_SUBSTRINGS);
     if (ret != 2 && ret != 3 && ret != 4 && ret != 5) {
@@ -210,13 +216,13 @@ int DetectXbitSetup (DetectEngineCtx *de_ctx, Signature *s, char *rawstr)
         res = pcre_copy_substring((char *)rawstr, ov, MAX_SUBSTRINGS, 2, fb_name, sizeof(fb_name));
         if (res < 0) {
             SCLogError(SC_ERR_PCRE_GET_SUBSTRING, "pcre_copy_substring failed");
-            goto error;
+            return -1;
         }
         if (ret >= 4) {
             res = pcre_copy_substring((char *)rawstr, ov, MAX_SUBSTRINGS, 3, hb_dir_str, sizeof(hb_dir_str));
             if (res < 0) {
                 SCLogError(SC_ERR_PCRE_GET_SUBSTRING, "pcre_copy_substring failed");
-                goto error;
+                return -1;
             }
             SCLogDebug("hb_dir_str %s", hb_dir_str);
             if (strlen(hb_dir_str) > 0) {
@@ -231,7 +237,7 @@ int DetectXbitSetup (DetectEngineCtx *de_ctx, Signature *s, char *rawstr)
                     var_type = VAR_TYPE_IPPAIR_BIT;
                 } else {
                     // TODO
-                    goto error;
+                    return -1;
                 }
             }
 
@@ -240,10 +246,19 @@ int DetectXbitSetup (DetectEngineCtx *de_ctx, Signature *s, char *rawstr)
                 res = pcre_copy_substring((char *)rawstr, ov, MAX_SUBSTRINGS, 4, expire_str, sizeof(expire_str));
                 if (res < 0) {
                     SCLogError(SC_ERR_PCRE_GET_SUBSTRING, "pcre_copy_substring failed");
-                    goto error;
+                    return -1;
                 }
                 SCLogDebug("expire_str %s", expire_str);
                 expire = atoi(expire_str);
+                if (expire < 0) {
+                    SCLogError(SC_ERR_INVALID_VALUE, "expire must be positive. "
+                            "Got %d (\"%s\")", expire, expire_str);
+                    return -1;
+                }
+                if (expire == 0) {
+                    SCLogError(SC_ERR_INVALID_VALUE, "expire must be bigger than 0");
+                    return -1;
+                }
                 SCLogDebug("expire %d", expire);
             }
         }
@@ -262,16 +277,18 @@ int DetectXbitSetup (DetectEngineCtx *de_ctx, Signature *s, char *rawstr)
     } else if (strcmp(fb_cmd_str,"toggle") == 0) {
         fb_cmd = DETECT_XBITS_CMD_TOGGLE;
     } else {
-        SCLogError(SC_ERR_UNKNOWN_VALUE, "ERROR: flowbits action \"%s\" is not supported.", fb_cmd_str);
+        SCLogError(SC_ERR_UNKNOWN_VALUE, "xbits action \"%s\" is not supported.", fb_cmd_str);
-        goto error;
+        return -1;
     }
 
     switch (fb_cmd) {
-        case DETECT_XBITS_CMD_NOALERT:
+        case DETECT_XBITS_CMD_NOALERT: {
             if (strlen(fb_name) != 0)
-                goto error;
+                return -1;
-            s->flags |= SIG_FLAG_NOALERT;
+            /* return ok, cd is NULL. Flag sig. */
+            *cdout = NULL;
             return 0;
+        }
         case DETECT_XBITS_CMD_ISNOTSET:
         case DETECT_XBITS_CMD_ISSET:
         case DETECT_XBITS_CMD_SET:
@@ -279,13 +296,13 @@ int DetectXbitSetup (DetectEngineCtx *de_ctx, Signature *s, char *rawstr)
         case DETECT_XBITS_CMD_TOGGLE:
         default:
             if (strlen(fb_name) == 0)
-                goto error;
+                return -1;
             break;
     }
 
     cd = SCMalloc(sizeof(DetectXbitsData));
     if (unlikely(cd == NULL))
-        goto error;
+        return -1;
 
     cd->idx = VariableNameGetIdx(de_ctx, fb_name, var_type);
     cd->cmd = fb_cmd;
@@ -296,6 +313,24 @@ int DetectXbitSetup (DetectEngineCtx *de_ctx, Signature *s, char *rawstr)
     SCLogDebug("idx %" PRIu32 ", cmd %s, name %s",
         cd->idx, fb_cmd_str, strlen(fb_name) ? fb_name : "(none)");
 
+    *cdout = cd;
+    return 0;
+}
+
+int DetectXbitSetup (DetectEngineCtx *de_ctx, Signature *s, char *rawstr)
+{
+    SigMatch *sm = NULL;
+    DetectXbitsData *cd = NULL;
+
+    int result = DetectXbitParse(de_ctx, rawstr, &cd);
+    if (result < 0) {
+        return -1;
+    /* noalert doesn't use a cd/sm struct. It flags the sig. We're done. */
+    } else if (result == 0 && cd == NULL) {
+        s->flags |= SIG_FLAG_NOALERT;
+        return 0;
+    }
+
     /* Okay so far so good, lets get this into a SigMatch
      * and put it in the Signature. */
     sm = SigMatchAlloc();
@@ -305,7 +340,7 @@ int DetectXbitSetup (DetectEngineCtx *de_ctx, Signature *s, char *rawstr)
     sm->type = DETECT_XBITS;
     sm->ctx = (void *)cd;
 
-    switch (fb_cmd) {
+    switch (cd->cmd) {
         /* case DETECT_XBITS_CMD_NOALERT can't happen here */
 
         case DETECT_XBITS_CMD_ISNOTSET:
@@ -327,8 +362,6 @@ int DetectXbitSetup (DetectEngineCtx *de_ctx, Signature *s, char *rawstr)
 error:
     if (cd != NULL)
         SCFree(cd);
-    if (sm != NULL)
-        SCFree(sm);
     return -1;
 }
 
@@ -361,11 +394,59 @@ static void XBitsTestShutdown(void)
     StorageCleanup();
 }
 
+
+static int XBitsTestParse01(void)
+{
+    DetectEngineCtx *de_ctx = NULL;
+    de_ctx = DetectEngineCtxInit();
+    FAIL_IF_NULL(de_ctx);
+    de_ctx->flags |= DE_QUIET;
+    DetectXbitsData *cd = NULL;
+
+#define BAD_INPUT(str) \
+    FAIL_IF_NOT(DetectXbitParse(de_ctx, (str), &cd) == -1);
+
+    BAD_INPUT("alert");
+    BAD_INPUT("n0alert");
+    BAD_INPUT("nOalert");
+    BAD_INPUT("set,abc,track nonsense, expire 3600");
+    BAD_INPUT("set,abc,track ip_source, expire 3600");
+    BAD_INPUT("set,abc,track ip_src, expire -1");
+    BAD_INPUT("set,abc,track ip_src, expire 0");
+
+#undef BAD_INPUT
+
+#define GOOD_INPUT(str, command, trk, typ, exp)             \
+    FAIL_IF_NOT(DetectXbitParse(de_ctx, (str), &cd) == 0);  \
+    FAIL_IF_NULL(cd);                                       \
+    FAIL_IF_NOT(cd->cmd == (command));                      \
+    FAIL_IF_NOT(cd->tracker == (trk));                      \
+    FAIL_IF_NOT(cd->type == (typ));                         \
+    FAIL_IF_NOT(cd->expire == (exp));                       \
+    DetectXbitFree(cd);                                     \
+    cd = NULL;
+
+    GOOD_INPUT("set,abc,track ip_pair",
+            DETECT_XBITS_CMD_SET,
+            DETECT_XBITS_TRACK_IPPAIR, VAR_TYPE_IPPAIR_BIT,
+            DETECT_XBITS_EXPIRE_DEFAULT);
+    GOOD_INPUT("set,abc,track ip_pair, expire 3600",
+            DETECT_XBITS_CMD_SET,
+            DETECT_XBITS_TRACK_IPPAIR, VAR_TYPE_IPPAIR_BIT,
+            3600);
+    GOOD_INPUT("set,abc,track ip_src, expire 1234",
+            DETECT_XBITS_CMD_SET,
+            DETECT_XBITS_TRACK_IPSRC, VAR_TYPE_HOST_BIT,
+            1234);
+
+#undef GOOD_INPUT
+
+    DetectEngineCtxFree(de_ctx);
+    PASS;
+}
+
 /**
- * \test HostBitsTestSig01 is a test for a valid noalert flowbits option
+ * \test
- *
- *  \retval 1 on succces
- *  \retval 0 on failure
  */
 
 static int XBitsTestSig01(void)
@@ -376,13 +457,11 @@ static int XBitsTestSig01(void)
                     "\r\n";
     uint16_t buflen = strlen((char *)buf);
     Packet *p = SCMalloc(SIZE_OF_PACKET);
-    if (unlikely(p == NULL))
+    FAIL_IF_NULL(p);
-        return 0;
     Signature *s = NULL;
     ThreadVars th_v;
     DetectEngineThreadCtx *det_ctx = NULL;
     DetectEngineCtx *de_ctx = NULL;
-    int result = 0;
 
     memset(&th_v, 0, sizeof(th_v));
     memset(p, 0, SIZE_OF_PACKET);
@@ -395,46 +474,23 @@ static int XBitsTestSig01(void)
     XBitsTestSetup();
 
     de_ctx = DetectEngineCtxInit();
-
+    FAIL_IF_NULL(de_ctx);
-    if (de_ctx == NULL) {
-        printf("bad de_ctx: ");
-        goto end;
-    }
-
     de_ctx->flags |= DE_QUIET;
 
     s = DetectEngineAppendSig(de_ctx,
             "alert ip any any -> any any (xbits:set,abc,track ip_pair; content:\"GET \"; sid:1;)");
-    if (s == NULL) {
+    FAIL_IF_NULL(s);
-        printf("bad sig: ");
-        goto end;
-    }
 
     SigGroupBuild(de_ctx);
     DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx);
-
     SigMatchSignatures(&th_v, de_ctx, det_ctx, p);
-
+    DetectEngineThreadCtxDeinit(&th_v, (void *)det_ctx);
-    result = 1;
+    DetectEngineCtxFree(de_ctx);
-
-end:
-    if (de_ctx != NULL) {
-        SigGroupCleanup(de_ctx);
-        SigCleanSignatures(de_ctx);
-    }
-
-    if (det_ctx != NULL) {
-        DetectEngineThreadCtxDeinit(&th_v, (void *)det_ctx);
-    }
-
-    if (de_ctx != NULL) {
-        DetectEngineCtxFree(de_ctx);
-    }
-
     XBitsTestShutdown();
-
     SCFree(p);
-    return result;
+    StatsThreadCleanup(&th_v);
+    StatsReleaseResources();
+    PASS;
 }
 
 /**
@@ -447,67 +503,33 @@ end:
 static int XBitsTestSig02(void)
 {
     Signature *s = NULL;
-    ThreadVars th_v;
     DetectEngineCtx *de_ctx = NULL;
-    int result = 0;
-    int error_count = 0;
-
-    memset(&th_v, 0, sizeof(th_v));
-
     de_ctx = DetectEngineCtxInit();
-    if (de_ctx == NULL) {
+    FAIL_IF_NULL(de_ctx);
-        goto end;
-    }
-
     de_ctx->flags |= DE_QUIET;
 
     s = DetectEngineAppendSig(de_ctx,
             "alert ip any any -> any any (xbits:isset,abc,track ip_src; content:\"GET \"; sid:1;)");
-    if (s == NULL) {
+    FAIL_IF_NULL(s);
-        error_count++;
-    }
 
     s = DetectEngineAppendSig(de_ctx,
             "alert ip any any -> any any (xbits:isnotset,abc,track ip_dst; content:\"GET \"; sid:2;)");
-    if (s == NULL) {
+    FAIL_IF_NULL(s);
-        error_count++;
-    }
 
     s = DetectEngineAppendSig(de_ctx,
             "alert ip any any -> any any (xbits:set,abc,track ip_pair; content:\"GET \"; sid:3;)");
-    if (s == NULL) {
+    FAIL_IF_NULL(s);
-        error_count++;
-    }
 
     s = DetectEngineAppendSig(de_ctx,
             "alert ip any any -> any any (xbits:unset,abc,track ip_src; content:\"GET \"; sid:4;)");
-    if (s == NULL) {
+    FAIL_IF_NULL(s);
-        error_count++;
-    }
 
     s = DetectEngineAppendSig(de_ctx,
             "alert ip any any -> any any (xbits:toggle,abc,track ip_dst; content:\"GET \"; sid:5;)");
-    if (s == NULL) {
+    FAIL_IF_NULL(s);
-        error_count++;
-    }
 
-    if (error_count != 0)
-        goto end;
-
-    result = 1;
-
-    SigGroupCleanup(de_ctx);
-    SigCleanSignatures(de_ctx);
     DetectEngineCtxFree(de_ctx);
-    return result;
+    PASS;
-end:
-    if (de_ctx != NULL) {
-        SigGroupCleanup(de_ctx);
-        SigCleanSignatures(de_ctx);
-        DetectEngineCtxFree(de_ctx);
-    }
-
-    return result;
 }
 
 #endif /* UNITTESTS */
@@ -518,6 +540,7 @@ end:
 void XBitsRegisterTests(void)
 {
 #ifdef UNITTESTS
+    UtRegisterTest("XBitsTestParse01", XBitsTestParse01);
     UtRegisterTest("XBitsTestSig01", XBitsTestSig01);
     UtRegisterTest("XBitsTestSig02", XBitsTestSig02);
 #endif /* UNITTESTS */

src/detect-xbits.h

@@ -37,6 +37,8 @@
 #define DETECT_XBITS_TRACK_IPPAIR 2
 #define DETECT_XBITS_TRACK_FLOW   3
 
+#define DETECT_XBITS_EXPIRE_DEFAULT 30
+
 typedef struct DetectXbitsData_ {
     uint16_t idx;
     uint8_t cmd;