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

[Yong Bakos]

On Jul 13, 2016, at 2:10 PM, Bryce Harrington <bryce at osg.samsung.com> wrote:
> 
> 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?

If I understand correctly I would:

- apply the recent reversion of 6351fb08
- do (c) but just keep it to hex values
- reapply 6351fb08 and use (c) at appropriate call sites

Or, just apply the revision and live without the slight 'safety' benefit
that 6351fb08 provides.

yong


> 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
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel