From: Fupeng Zhao <fupeng.zhao@foxmail.com>
Date: Thu, 19 Jun 2025 05:48:07 +0000 (+0800)
Subject: detect: replace strtoul with ByteExtractStringUint32
X-Git-Tag: suricata-8.0.0~22
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e740307cd5228150851a729dca1b112fe51932d9;p=thirdparty%2Fsuricata.git

detect: replace strtoul with ByteExtractStringUint32

Also added and updated unit tests to ensure correctness.

Ticket: #7212
---

diff --git a/src/detect-gid.c b/src/detect-gid.c
index e131b53591..9f346f1050 100644
--- a/src/detect-gid.c
+++ b/src/detect-gid.c
@@ -33,6 +33,7 @@
 #include "decode-events.h"
 
 #include "detect-gid.h"
+#include "util-byte.h"
 #include "util-unittest.h"
 #include "util-debug.h"
 
@@ -71,21 +72,13 @@ void DetectGidRegister (void)
  */
 static int DetectGidSetup (DetectEngineCtx *de_ctx, Signature *s, const char *rawstr)
 {
-    unsigned long gid = 0;
-    char *endptr = NULL;
-    errno = 0;
-    gid = strtoul(rawstr, &endptr, 10);
-    if (errno == ERANGE || endptr == NULL || *endptr != '\0') {
-        SCLogError("invalid character as arg "
-                   "to gid keyword");
-        goto error;
-    }
-    if (gid >= UINT_MAX) {
-        SCLogError("gid value to high, max %u", UINT_MAX);
+    uint32_t gid = 0;
+    if (ByteExtractStringUint32(&gid, 10, strlen(rawstr), rawstr) <= 0) {
+        SCLogError("invalid input as arg to gid keyword");
         goto error;
     }
 
-    s->gid = (uint32_t)gid;
+    s->gid = gid;
 
     return 0;
 
diff --git a/src/detect-mark.c b/src/detect-mark.c
index 985865cedd..4c25f261f2 100644
--- a/src/detect-mark.c
+++ b/src/detect-mark.c
@@ -35,6 +35,7 @@
 #include "detect-parse.h"
 
 #include "util-unittest.h"
+#include "util-byte.h"
 #include "util-debug.h"
 
 #define PARSE_REGEX "([0x]*[0-9a-f]+)/([0x]*[0-9a-f]+)"
@@ -81,7 +82,6 @@ static void * DetectMarkParse (const char *rawstr)
     size_t pcre2_len;
     const char *str_ptr = NULL;
     char *ptr = NULL;
-    char *endptr = NULL;
     uint32_t mark;
     uint32_t mask;
     DetectMarkData *data;
@@ -105,20 +105,8 @@ static void * DetectMarkParse (const char *rawstr)
     if (ptr == NULL)
         goto error;
 
-    errno = 0;
-    mark = strtoul(ptr, &endptr, 0);
-    if (errno == ERANGE) {
-        SCLogError("Numeric value out of range");
-        pcre2_substring_free((PCRE2_UCHAR8 *)ptr);
-        goto error;
-    }     /* If there is no numeric value in the given string then strtoull(), makes
-             endptr equals to ptr and return 0 as result */
-    else if (endptr == ptr && mark == 0) {
-        SCLogError("No numeric value");
-        pcre2_substring_free((PCRE2_UCHAR8 *)ptr);
-        goto error;
-    } else if (endptr == ptr) {
-        SCLogError("Invalid numeric value");
+    if (ByteExtractStringUint32(&mark, 0, strlen(ptr), ptr) <= 0) {
+        SCLogError("invalid input as arg to nfq_set_mark keyword");
         pcre2_substring_free((PCRE2_UCHAR8 *)ptr);
         goto error;
     }
@@ -143,21 +131,8 @@ static void * DetectMarkParse (const char *rawstr)
         return data;
     }
 
-    errno = 0;
-    mask = strtoul(ptr, &endptr, 0);
-    if (errno == ERANGE) {
-        SCLogError("Numeric value out of range");
-        pcre2_substring_free((PCRE2_UCHAR8 *)ptr);
-        goto error;
-    }     /* If there is no numeric value in the given string then strtoull(), makes
-             endptr equals to ptr and return 0 as result */
-    else if (endptr == ptr && mask == 0) {
-        SCLogError("No numeric value");
-        pcre2_substring_free((PCRE2_UCHAR8 *)ptr);
-        goto error;
-    }
-    else if (endptr == ptr) {
-        SCLogError("Invalid numeric value");
+    if (ByteExtractStringUint32(&mask, 0, strlen(ptr), ptr) <= 0) {
+        SCLogError("invalid input as arg to nfq_set_mark keyword");
         pcre2_substring_free((PCRE2_UCHAR8 *)ptr);
         goto error;
     }
@@ -272,6 +247,8 @@ static int MarkTestParse01 (void)
     data = DetectMarkParse("1/1");
 
     FAIL_IF_NULL(data);
+    FAIL_IF(data->mark != 1);
+    FAIL_IF(data->mask != 1);
 
     DetectMarkDataFree(NULL, data);
     PASS;
@@ -302,6 +279,8 @@ static int MarkTestParse03 (void)
     DetectMarkData *data;
 
     data = DetectMarkParse("0x10/0xff");
+    FAIL_IF(data->mark != 0x10);
+    FAIL_IF(data->mask != 0xff);
 
     FAIL_IF_NULL(data);
 
diff --git a/src/detect-rev.c b/src/detect-rev.c
index 6f0f94b130..6b247561d5 100644
--- a/src/detect-rev.c
+++ b/src/detect-rev.c
@@ -25,11 +25,18 @@
 
 #include "suricata-common.h"
 #include "detect.h"
+#include "detect-engine.h"
+#include "detect-parse.h"
 #include "detect-rev.h"
+#include "util-byte.h"
 #include "util-debug.h"
 #include "util-error.h"
+#include "util-unittest.h"
 
 static int DetectRevSetup (DetectEngineCtx *, Signature *, const char *);
+#ifdef UNITTESTS
+static void DetectRevRegisterTests(void);
+#endif
 
 void DetectRevRegister (void)
 {
@@ -37,21 +44,16 @@ void DetectRevRegister (void)
     sigmatch_table[DETECT_REV].desc = "set version of the rule";
     sigmatch_table[DETECT_REV].url = "/rules/meta.html#rev-revision";
     sigmatch_table[DETECT_REV].Setup = DetectRevSetup;
+#ifdef UNITTESTS
+    sigmatch_table[DETECT_REV].RegisterTests = DetectRevRegisterTests;
+#endif
 }
 
 static int DetectRevSetup (DetectEngineCtx *de_ctx, Signature *s, const char *rawstr)
 {
-    unsigned long rev = 0;
-    char *endptr = NULL;
-    errno = 0;
-    rev = strtoul(rawstr, &endptr, 10);
-    if (errno == ERANGE || endptr == NULL || *endptr != '\0') {
-        SCLogError("invalid character as arg "
-                   "to rev keyword");
-        goto error;
-    }
-    if (rev >= UINT_MAX) {
-        SCLogError("rev value to high, max %u", UINT_MAX);
+    uint32_t rev = 0;
+    if (ByteExtractStringUint32(&rev, 10, strlen(rawstr), rawstr) <= 0) {
+        SCLogError("invalid input as arg to rev keyword");
         goto error;
     }
     if (rev == 0) {
@@ -63,10 +65,85 @@ static int DetectRevSetup (DetectEngineCtx *de_ctx, Signature *s, const char *ra
         goto error;
     }
 
-    s->rev = (uint32_t)rev;
-
+    s->rev = rev;
     return 0;
 
- error:
+error:
     return -1;
- }
+}
+
+#ifdef UNITTESTS
+/**
+ * \test RevTestParse01 is a test for a valid rev value
+ */
+static int RevTestParse01(void)
+{
+    DetectEngineCtx *de_ctx = DetectEngineCtxInit();
+    FAIL_IF_NULL(de_ctx);
+
+    Signature *s =
+            DetectEngineAppendSig(de_ctx, "alert tcp 1.2.3.4 any -> any any (sid:1; rev:1;)");
+
+    FAIL_IF_NULL(s);
+    FAIL_IF(s->rev != 1);
+
+    DetectEngineCtxFree(de_ctx);
+    PASS;
+}
+
+/**
+ * \test RevTestParse02 is a test for an invalid rev value
+ */
+static int RevTestParse02(void)
+{
+    DetectEngineCtx *de_ctx = DetectEngineCtxInit();
+    FAIL_IF_NULL(de_ctx);
+
+    FAIL_IF_NOT_NULL(
+            DetectEngineAppendSig(de_ctx, "alert tcp 1.2.3.4 any -> any any (sid:1; rev:a;)"));
+
+    DetectEngineCtxFree(de_ctx);
+    PASS;
+}
+
+/**
+ * \test RevTestParse03 is a test for a rev containing of a single quote.
+ */
+static int RevTestParse03(void)
+{
+    DetectEngineCtx *de_ctx = DetectEngineCtxInit();
+    FAIL_IF_NULL(de_ctx);
+
+    FAIL_IF_NOT_NULL(DetectEngineAppendSig(
+            de_ctx, "alert tcp any any -> any any (content:\"ABC\"; rev:\";)"));
+
+    DetectEngineCtxFree(de_ctx);
+    PASS;
+}
+
+/**
+ * \test RevTestParse04 is a test for a rev value of 0
+ */
+static int RevTestParse04(void)
+{
+    DetectEngineCtx *de_ctx = DetectEngineCtxInit();
+    FAIL_IF_NULL(de_ctx);
+
+    FAIL_IF_NOT_NULL(DetectEngineAppendSig(
+            de_ctx, "alert tcp any any -> any any (content:\"ABC\"; rev:0;)"));
+
+    DetectEngineCtxFree(de_ctx);
+    PASS;
+}
+
+/**
+ * \brief this function registers unit tests for Rev
+ */
+static void DetectRevRegisterTests(void)
+{
+    UtRegisterTest("RevTestParse01", RevTestParse01);
+    UtRegisterTest("RevTestParse02", RevTestParse02);
+    UtRegisterTest("RevTestParse03", RevTestParse03);
+    UtRegisterTest("RevTestParse04", RevTestParse04);
+}
+#endif /* UNITTESTS */
diff --git a/src/detect-sid.c b/src/detect-sid.c
index be017e2507..1d65f56938 100644
--- a/src/detect-sid.c
+++ b/src/detect-sid.c
@@ -28,6 +28,7 @@
 #include "detect-engine.h"
 #include "detect-parse.h"
 #include "detect-sid.h"
+#include "util-byte.h"
 #include "util-debug.h"
 #include "util-error.h"
 #include "util-unittest.h"
@@ -52,17 +53,9 @@ void DetectSidRegister (void)
 
 static int DetectSidSetup (DetectEngineCtx *de_ctx, Signature *s, const char *sidstr)
 {
-    unsigned long id = 0;
-    char *endptr = NULL;
-    errno = 0;
-    id = strtoul(sidstr, &endptr, 10);
-    if (errno == ERANGE || endptr == NULL || *endptr != '\0') {
-        SCLogError("invalid character as arg "
-                   "to sid keyword");
-        goto error;
-    }
-    if (id >= UINT_MAX) {
-        SCLogError("sid value too high, max %u", UINT_MAX);
+    uint32_t id = 0;
+    if (ByteExtractStringUint32(&id, 10, strlen(sidstr), sidstr) <= 0) {
+        SCLogError("invalid input as arg to sid keyword");
         goto error;
     }
     if (id == 0) {
@@ -74,7 +67,7 @@ static int DetectSidSetup (DetectEngineCtx *de_ctx, Signature *s, const char *si
         goto error;
     }
 
-    s->id = (uint32_t)id;
+    s->id = id;
     return 0;
 
  error: