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

[Michael Blumenkrantz]

On Wed, 13 Jul 2016 14:27:17 -0700
Yong Bakos <junk at humanoriented.com> wrote:

> 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

This seems like it would probably be a fine choice for 'realistic' use cases of a reference compositor.

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