[PATCH weston v2 2/2] Re-apply "config-parser: Catch negative numbers assigned to unsigned config values"

[Bryce Harrington]

On Thu, Jul 14, 2016 at 06:28:04PM -0700, Bryce Harrington wrote:
> [With hexadecimal color values now handled via their own routine,
> re-introduce the negative unsigned numbers fix.]
> 
> strtoul() has a side effect that when given a string representing a
> negative number, it treats it as a high value hexadecimal.  IOW,
> strtoul("-42", &val) sets val to 0xffffffd6.  This could potentially
> result in unintended surprise behaviors.
> 
> 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 <bryce at osg.samsung.com>

Eric R-b'd this one off-list as well.
I renumbered the test cases to bring up to date with trunk and pushed
both patches in this series.

   cbfde13..d0716f4  master -> master


> ---
>  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 33d5e36..d5bbb8d 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 5a5a380..7f30b22 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"
>  	"[colors]\n"
> @@ -578,6 +579,36 @@ ZUC_TEST_F(config_test_t1, test026, data)
>  	ZUC_ASSERT_EQ(EINVAL, errno);
>  }
>  
> +ZUC_TEST_F(config_test_t1, test025, 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, test026, 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;
> -- 
> 1.9.1