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

[Bryce Harrington]

On Tue, Jul 12, 2016 at 01:17:20PM +0100, Eric Engestrom wrote:
> On Mon, Jul 11, 2016 at 05:55:15PM -0700, Bryce Harrington wrote:
> > 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", ) 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 
> 
> Looks good to me.
> Reviewed-by: Eric Engestrom 

Thanks, pushed:
   5ba41eb..6351fb0  master -> master
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Bryce Harrington]

On Tue, Jul 12, 2016 at 10:58:05AM -0700, Bill Spitzak wrote:
> I tested this and at least for libc on linux it returns 0x1-n, ie
> "-1" is 0x.
> 
> This is actually pretty useful when the unsigned value is bitflags or you
> want to guarantee you typed in the largest number possible. I am not sure
> you really want to disable it, especially if it prevents entry for 1/2 the
> possible numbers.

Sure, but nothing using this routine requires either of those
capabilities.

Bryce

> On Tue, Jul 12, 2016 at 5:17 AM, Eric Engestrom 
> wrote:
> 
> > On Mon, Jul 11, 2016 at 05:55:15PM -0700, Bryce Harrington wrote:
> > > 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", ) 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 
> >
> > Looks good to me.
> > Reviewed-by: Eric Engestrom 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Eric Engestrom]

On Mon, Jul 11, 2016 at 05:55:15PM -0700, Bryce Harrington wrote:
> 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", ) 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 

Looks good to me.
Reviewed-by: Eric Engestrom 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Bryce Harrington]

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", ) 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 
---
 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 151ae9c..247e880 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, , 0);
+   ret = strtol(entry->value, , 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 735da4e..f88e89b 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", , 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", , 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel