core: Accept a b/k/m suffix in size-valued directive parameters

include/httpd.h

@@ -2364,13 +2364,16 @@ AP_DECLARE(apr_status_t) ap_timeout_parameter_parse(
                                                const char *default_time_unit);
 
 /**
- * Parse a configuration parameter as a base 10 apr_off_t. The whole parameter
- * has to be a number, there must not be any other character after it. What
+ * Parse a configuration parameter as a base 10 apr_off_t. The number may be
+ * followed by a single optional suffix, 'b'/'B' (bytes), 'k'/'K' (multiply
+ * by 1024), 'm'/'M' (multiply by 1024*1024) or 'g'/'G' (multiply by
+ * 1024*1024*1024); there must not be any other character after it. What
  * values are acceptable is up to the caller.
  * @param parameter The string containing the number.
  * @param value The parsed value, only set on success.
- * @return APR_SUCCESS, APR_EINVAL if the parameter is not a number, or
- *         APR_ERANGE if the number does not fit in an apr_off_t.
+ * @return APR_SUCCESS, APR_EINVAL if the parameter is not a number with
+ *         optional suffix, or APR_ERANGE if the value does not fit in an
+ *         apr_off_t.
  */
 AP_DECLARE(apr_status_t) ap_size_parameter_parse(const char *parameter,
                                                  apr_off_t *value);

modules/aaa/mod_auth_digest.c

@@ -555,35 +555,15 @@ static const char *set_uri_list(cmd_parms *cmd, void *config, const char *uri)
 static const char *set_shmem_size(cmd_parms *cmd, void *config,
                                   const char *size_str)
 {
-    char *endptr;
     apr_off_t size;
     apr_size_t min;
+    apr_status_t rv;
 
-    if (apr_strtoff(&size, size_str, &endptr, 10) != APR_SUCCESS || size < 0) {
-        return apr_pstrcat(cmd->pool, "Invalid size in AuthDigestShmemSize: ",
-                          size_str, NULL);
-    }
-    if (*endptr == 'k' || *endptr == 'K') {
-        if (size > APR_INT64_MAX / 1024) {
-            return "AuthDigestShmemSize value is too large";
-        }
-        size *= 1024;
-        endptr++;
-    }
-    else if (*endptr == 'm' || *endptr == 'M') {
-        if (size > APR_INT64_MAX / (1024 * 1024)) {
-            return "AuthDigestShmemSize value is too large";
-        }
-        size *= 1024 * 1024;
-        endptr++;
-    }
-    else if (*endptr == 'b' || *endptr == 'B') {
-        endptr++;
-    }
-    while (apr_isspace(*endptr)) {
-        endptr++;
+    rv = ap_size_parameter_parse(size_str, &size);
+    if (rv == APR_ERANGE) {
+        return "AuthDigestShmemSize value is too large";
     }
-    if (*endptr != '\0') {
+    if (rv != APR_SUCCESS || size < 0) {
         return apr_pstrcat(cmd->pool, "Invalid size in AuthDigestShmemSize: ",
                           size_str, NULL);
     }

modules/filters/mod_substitute.c

@@ -773,40 +773,17 @@ static const char *set_pattern(cmd_parms *cmd, void *cfg, const char *line)
     return NULL;
 }
 
-#define KBYTE         1024
-#define MBYTE         1048576
-#define GBYTE         1073741824
 
 static const char *set_max_line_length(cmd_parms *cmd, void *cfg, const char *arg)
 {
     subst_dir_conf *dcfg = (subst_dir_conf *)cfg;
     apr_off_t max;
-    char *end;
     apr_status_t rv;
 
-    rv = apr_strtoff(&max, arg, &end, 10);
-    if (rv == APR_SUCCESS) {
-        if ((*end == 'K' || *end == 'k') && !end[1]) {
-            if (max > APR_INT64_MAX / KBYTE)
-                return "SubstituteMaxLineLength value too large";
-            max *= KBYTE;
-        }
-        else if ((*end == 'M' || *end == 'm') && !end[1]) {
-            if (max > APR_INT64_MAX / MBYTE)
-                return "SubstituteMaxLineLength value too large";
-            max *= MBYTE;
-        }
-        else if ((*end == 'G' || *end == 'g') && !end[1]) {
-            if (max > APR_INT64_MAX / GBYTE)
-                return "SubstituteMaxLineLength value too large";
-            max *= GBYTE;
-        }
-        else if (*end && /* neither empty nor [Bb] */
-                 ((*end != 'B' && *end != 'b') || end[1])) {
-            rv = APR_EGENERAL;
-        }
+    rv = ap_size_parameter_parse(arg, &max);
+    if (rv == APR_ERANGE) {
+        return "SubstituteMaxLineLength value too large";
     }
-
     if (rv != APR_SUCCESS || max < 0)
     {
         return "SubstituteMaxLineLength must be a non-negative integer optionally "

server/util.c

@@ -2777,7 +2777,10 @@ AP_DECLARE(apr_status_t) ap_timeout_parameter_parse(
 AP_DECLARE(apr_status_t) ap_size_parameter_parse(const char *parameter,
                                                  apr_off_t *value)
 {
-    apr_off_t parsed;
+    /* The maximum apr_off_t value. */
+    const apr_off_t off_max = sizeof(apr_off_t) == 8 ? APR_INT64_MAX
+                                                     : APR_INT32_MAX;
+    apr_off_t parsed, multiplier = 1;
     apr_status_t rv;
     char *end;
 
@@ -2790,11 +2793,32 @@ AP_DECLARE(apr_status_t) ap_size_parameter_parse(const char *parameter,
     if (rv != APR_SUCCESS) {
         return APR_ERANGE;
     }
+
+    if (*end == 'k' || *end == 'K') {
+        multiplier = 1024;
+        end++;
+    }
+    else if (*end == 'm' || *end == 'M') {
+        multiplier = 1024 * 1024;
+        end++;
+    }
+    else if (*end == 'g' || *end == 'G') {
+        multiplier = 1024 * 1024 * 1024;
+        end++;
+    }
+    else if (*end == 'b' || *end == 'B') {
+        end++;
+    }
     if (*end != '\0') {
         return APR_EINVAL;
     }
 
-    *value = parsed;
+    if (parsed > off_max / multiplier
+        || parsed < (-off_max - 1) / multiplier) {
+        return APR_ERANGE;
+    }
+
+    *value = parsed * multiplier;
     return APR_SUCCESS;
 }
 #undef CHECK_OVERFLOW

test/modules/core/test_012_config_off_t.py

@@ -28,15 +28,18 @@ def check_config(self, env, directive, value):
                         "-f", os.path.join(env.server_dir, 'conf', 'httpd.conf')])
 
     @pytest.mark.parametrize("directive", DIRECTIVES)
-    @pytest.mark.parametrize("value", ["0", "1", "123", "1000000", "00123"])
+    @pytest.mark.parametrize("value", ["0", "1", "123", "1000000", "00123",
+                                       "512b", "512B", "1k", "1K", "2m", "2M",
+                                       "1g", "1G"])
     def test_core_012_001(self, env, directive, value):
         r = self.check_config(env, directive, value)
         assert r.exit_code == 0, f"{r.stderr}"
 
     # only a whole decimal number is valid
     @pytest.mark.parametrize("directive", DIRECTIVES)
     @pytest.mark.parametrize("value", [
-        "abc", "abc123", "123abc", "123foo456", "12.3", "0x10", "1K", "+", "-",
+        "abc", "abc123", "123abc", "123foo456", "12.3", "0x10", "1kb", "k",
+        "+", "-",
     ])
     def test_core_012_002(self, env, directive, value):
         r = self.check_config(env, directive, value)
@@ -45,7 +48,8 @@ def test_core_012_002(self, env, directive, value):
 
     # a number the directive does not accept, or too large to be stored
     @pytest.mark.parametrize("directive", DIRECTIVES)
-    @pytest.mark.parametrize("value", ["-1", "9223372036854775808", "9" * 40])
+    @pytest.mark.parametrize("value", ["-1", "9223372036854775808", "9" * 40,
+                                       "8589934592g"])
     def test_core_012_003(self, env, directive, value):
         r = self.check_config(env, directive, value)
         assert r.exit_code != 0, f"{value} was accepted"

test/unit/util.c

@@ -149,6 +149,75 @@ HTTPD_START_LOOP_TEST(check_escape_quotes, ap_test_escape_quotes_cases_len)
 END_TEST
 
 
+/*
+ * ap_size_parameter_parse()
+ */
+
+struct ap_size_parameter_parse_case {
+    const char *input;
+    apr_status_t expected_rv;
+    apr_int64_t expected;       /* for a 64-bit apr_off_t */
+};
+
+const struct ap_size_parameter_parse_case ap_size_parameter_parse_cases[] = {
+    { "0",                    APR_SUCCESS, 0 },
+    { "1",                    APR_SUCCESS, 1 },
+    { "-1",                   APR_SUCCESS, -1 },
+    { "1024",                 APR_SUCCESS, 1024 },
+    { "0b",                   APR_SUCCESS, 0 },
+    { "1b",                   APR_SUCCESS, 1 },
+    { "512B",                 APR_SUCCESS, 512 },
+    { "1k",                   APR_SUCCESS, 1024 },
+    { "1K",                   APR_SUCCESS, 1024 },
+    { "10k",                  APR_SUCCESS, 10240 },
+    { "1m",                   APR_SUCCESS, 1024 * 1024 },
+    { "2M",                   APR_SUCCESS, 2 * 1024 * 1024 },
+    { "1g",                   APR_SUCCESS, 1024 * 1024 * 1024 },
+    { "4G",                   APR_SUCCESS, APR_INT64_C(4) * 1024 * 1024 * 1024 },
+    { "-2k",                  APR_SUCCESS, -2048 },
+    { "",                     APR_EINVAL,  0 },
+    { "k",                    APR_EINVAL,  0 },
+    { "-",                    APR_EINVAL,  0 },
+    { "1t",                   APR_EINVAL,  0 },
+    { "1kb",                  APR_EINVAL,  0 },
+    { "1 k",                  APR_EINVAL,  0 },
+    { "1k ",                  APR_EINVAL,  0 },
+    { "2 ",                   APR_EINVAL,  0 },
+    { "0x10",                 APR_EINVAL,  0 },
+    { "1.5k",                 APR_EINVAL,  0 },
+    /* overflows whether apr_off_t is 32 or 64 bits wide */
+    { "99999999999999999999", APR_ERANGE,  0 },
+    { "9223372036854775807k", APR_ERANGE,  0 },
+    { "9007199254740992m",    APR_ERANGE,  0 },
+    { "-9007199254740992m",   APR_ERANGE,  0 },
+    { "8589934592g",          APR_ERANGE,  0 },
+};
+
+const size_t ap_size_parameter_parse_cases_len =
+    sizeof(ap_size_parameter_parse_cases) /
+    sizeof(ap_size_parameter_parse_cases[0]);
+
+HTTPD_START_LOOP_TEST(size_parameter_parse_handles_suffixes, ap_size_parameter_parse_cases_len)
+{
+    const struct ap_size_parameter_parse_case *c = &ap_size_parameter_parse_cases[_i];
+    apr_status_t expected_rv = c->expected_rv;
+    apr_off_t value = 0;
+    apr_status_t rv;
+
+    /* A value which does not fit a narrower apr_off_t is a range error. */
+    if (expected_rv == APR_SUCCESS && (apr_off_t)c->expected != c->expected) {
+        expected_rv = APR_ERANGE;
+    }
+
+    rv = ap_size_parameter_parse(c->input, &value);
+    ck_assert_int_eq(rv, expected_rv);
+    if (expected_rv == APR_SUCCESS) {
+        ck_assert_int_eq(value, (apr_off_t)c->expected);
+    }
+}
+END_TEST
+
+
 /*
  * Test Case Boilerplate
  */