From 6351fb08c2e302f8696b2022830e5317e7219c39 Mon Sep 17 00:00:00 2001 From: Bryce Harrington Date: Mon, 11 Jul 2016 17:55:15 -0700 Subject: [PATCH] config-parser: Catch negative numbers assigned to unsigned config values strtoul() has a side effect that when given a string representing a negative number, it returns a negated version as the value, and does not flag an error. IOW, strtoul("-42", &val) sets val to 42. This could potentially result in unintended surprise behaviors, such as if one were to inadvertantly set a config param to -1 expecting that to disable it, but with the result of setting the param to 1 instead. Catch this by using strtol() and then manually check for the negative value. This logic is modelled after Wayland's strtouint(). Note that this change unfortunately reduces the range of parseable numbers from [0,UINT_MAX] to [0,INT_MAX]. The current users of weston_config_section_get_uint() are anticipating numbers far smaller than either of these limits, so the change is believed to have no impact in practice. Also add a test case for negative numbers that catches this error condition. Signed-off-by: Bryce Harrington Reviewed-by: Eric Engestrom --- shared/config-parser.c | 12 +++++++++++- tests/config-parser-test.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/shared/config-parser.c b/shared/config-parser.c index 151ae9ca..247e8801 100644 --- a/shared/config-parser.c +++ b/shared/config-parser.c @@ -186,6 +186,7 @@ weston_config_section_get_uint(struct weston_config_section *section, const char *key, uint32_t *value, uint32_t default_value) { + long int ret; struct weston_config_entry *entry; char *end; @@ -197,13 +198,22 @@ weston_config_section_get_uint(struct weston_config_section *section, } errno = 0; - *value = strtoul(entry->value, &end, 0); + ret = strtol(entry->value, &end, 0); if (errno != 0 || end == entry->value || *end != '\0') { *value = default_value; errno = EINVAL; return -1; } + /* check range */ + if (ret < 0 || ret > INT_MAX) { + *value = default_value; + errno = ERANGE; + return -1; + } + + *value = ret; + return 0; } diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c index 735da4e0..f88e89b8 100644 --- a/tests/config-parser-test.c +++ b/tests/config-parser-test.c @@ -117,6 +117,7 @@ static struct zuc_fixture config_test_t1 = { "# more comments\n" "number=5252\n" "zero=0\n" + "negative=-42\n" "flag=false\n" "\n" "[stuff]\n" @@ -461,6 +462,36 @@ ZUC_TEST_F(config_test_t1, test019, data) ZUC_ASSERT_EQ(0, errno); } +ZUC_TEST_F(config_test_t1, test020, data) +{ + int r; + int32_t n; + struct weston_config_section *section; + struct weston_config *config = data; + + section = weston_config_get_section(config, "bar", NULL, NULL); + r = weston_config_section_get_int(section, "negative", &n, 600); + + ZUC_ASSERT_EQ(0, r); + ZUC_ASSERT_EQ(-42, n); + ZUC_ASSERT_EQ(0, errno); +} + +ZUC_TEST_F(config_test_t1, test021, data) +{ + int r; + uint32_t n; + struct weston_config_section *section; + struct weston_config *config = data; + + section = weston_config_get_section(config, "bar", NULL, NULL); + r = weston_config_section_get_uint(section, "negative", &n, 600); + + ZUC_ASSERT_EQ(-1, r); + ZUC_ASSERT_EQ(600, n); + ZUC_ASSERT_EQ(ERANGE, errno); +} + ZUC_TEST_F(config_test_t2, doesnt_parse, data) { struct weston_config *config = data;