[PATCH weston] Revert "config-parser: Catch negative numbers assigned to unsigned config values"

[Bryce Harrington]

On Wed, Jul 13, 2016 at 01:27:29PM -0700, Bryce Harrington wrote:
> The reduction in range limits does have an effect for color values,
> which are expressed as hexadecimal values from 0x00000000 to
> 0xFFFFFFFF.  By limiting the range to INT_MAX, color values of
> 0x80000000 and up are in fact lost.
> 
> This reverts commit 6351fb08c2e302f8696b2022830e5317e7219c39.
> 
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>

Here are some alternative solutions:

a.  Use strtoll instead of strtol

b.  Add weston_config_section_get_hex()

c.  Add weston_config_section_get_color(), that supports both
    hexadecimal and color names (ala rgb.txt or similar)

I'm leaning towards (b), and sounds like Peter was thinking along those
lines.  Splitting hexadecimal handling out of get_uint() would also
allow specifying base-10, and would allow stronger type checking
generally.

We could also do both (a) and (b).  But I don't know whether we actually
need the full range for unsigned ints other than for hexadecimal values.
I also don't know if there's any difference in overhead between strtoll
vs. strtol.

Option (c) I think would be spiffy but I'm skeptical that using color
names would really be all that useful in practice, and it would require
extra overhead for loading and/or maintaining a color map database of
some sort.

So, (b) seems to me to be the right way to go.  Other opinions?

Bryce

> ---
>  shared/config-parser.c     | 12 +-----------
>  tests/config-parser-test.c | 31 -------------------------------
>  2 files changed, 1 insertion(+), 42 deletions(-)
> 
> diff --git a/shared/config-parser.c b/shared/config-parser.c
> index 4c67220..1e08759 100644
> --- a/shared/config-parser.c
> +++ b/shared/config-parser.c
> @@ -186,7 +186,6 @@ 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;
>  
> @@ -198,22 +197,13 @@ weston_config_section_get_uint(struct weston_config_section *section,
>  	}
>  
>  	errno = 0;
> -	ret = strtol(entry->value, &end, 0);
> +	*value = strtoul(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 f88e89b..735da4e 100644
> --- a/tests/config-parser-test.c
> +++ b/tests/config-parser-test.c
> @@ -117,7 +117,6 @@ 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"
> @@ -462,36 +461,6 @@ 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;
> -- 
> 1.9.1