[PATCH weston] config-parser: Improve error checks for strtol/strtoul calls

[Peter Hutterer]

On Thu, Jul 07, 2016 at 02:24:39PM -0700, Bryce Harrington wrote:
> On Thu, Jul 07, 2016 at 02:08:28PM -0700, Bryce Harrington wrote:
> > Check errno, which is set of over/underflow, out of range, etc.  Also
> > check for empty strings (the usages covered in this patch already also
> > cover the case where there are non-digits present).  Set errno to 0
> > before making the strto*l call in case of pre-existing errors
> > (i.e. ENOTTY when running under the testsuite).
> > 
> > This follows the error checking style used in Wayland
> > (c.f. wayland-client.c and scanner.c).
> > 
> > In tests, also check errno, and add testcases for parsing '0'.
> > 
> > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> 
> If this approach looks ok to folks, there are other irregular uses of
> strto*l I'll clean up as well subsequently.

it'd be best to have a safe_strtol helper function in a weston-util.h
header and just enforce use of that. because most of the time all we care
about is "has it parsed and what's the result", you can get that with a
    bool safe_strtol(const char *string, long *result);

or safe_atoi, or whatever you want to call it.

Cheers,
   Peter



> 
> > ---
> >  shared/config-parser.c     |  6 ++++--
> >  tests/config-parser-test.c | 34 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 38 insertions(+), 2 deletions(-)
> > 
> > diff --git a/shared/config-parser.c b/shared/config-parser.c
> > index 2256469..151ae9c 100644
> > --- a/shared/config-parser.c
> > +++ b/shared/config-parser.c
> > @@ -169,8 +169,9 @@ weston_config_section_get_int(struct weston_config_section *section,
> >  		return -1;
> >  	}
> >  
> > +	errno = 0;
> >  	*value = strtol(entry->value, &end, 0);
> > -	if (*end != '\0') {
> > +	if (errno != 0 || end == entry->value || *end != '\0') {
> >  		*value = default_value;
> >  		errno = EINVAL;
> >  		return -1;
> > @@ -195,8 +196,9 @@ weston_config_section_get_uint(struct weston_config_section *section,
> >  		return -1;
> >  	}
> >  
> > +	errno = 0;
> >  	*value = strtoul(entry->value, &end, 0);
> > -	if (*end != '\0') {
> > +	if (errno != 0 || end == entry->value || *end != '\0') {
> >  		*value = default_value;
> >  		errno = EINVAL;
> >  		return -1;
> > diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
> > index 5dcafc4..735da4e 100644
> > --- a/tests/config-parser-test.c
> > +++ b/tests/config-parser-test.c
> > @@ -116,6 +116,7 @@ static struct zuc_fixture config_test_t1 = {
> >  	"[bar]\n"
> >  	"# more comments\n"
> >  	"number=5252\n"
> > +	"zero=0\n"
> >  	"flag=false\n"
> >  	"\n"
> >  	"[stuff]\n"
> > @@ -263,6 +264,7 @@ ZUC_TEST_F(config_test_t1, test006, data)
> >  
> >  	ZUC_ASSERT_EQ(0, r);
> >  	ZUC_ASSERT_EQ(5252, n);
> > +	ZUC_ASSERT_EQ(0, errno);
> >  }
> >  
> >  ZUC_TEST_F(config_test_t1, test007, data)
> > @@ -289,8 +291,10 @@ ZUC_TEST_F(config_test_t1, test008, data)
> >  
> >  	section = weston_config_get_section(config, "bar", NULL, NULL);
> >  	r = weston_config_section_get_uint(section, "number", &u, 600);
> > +
> >  	ZUC_ASSERT_EQ(0, r);
> >  	ZUC_ASSERT_EQ(5252, u);
> > +	ZUC_ASSERT_EQ(0, errno);
> >  }
> >  
> >  ZUC_TEST_F(config_test_t1, test009, data)
> > @@ -427,6 +431,36 @@ ZUC_TEST_F(config_test_t1, test017, data)
> >  	ZUC_ASSERT_EQ(5, i);
> >  }
> >  
> > +ZUC_TEST_F(config_test_t1, test018, 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, "zero", &n, 600);
> > +
> > +	ZUC_ASSERT_EQ(0, r);
> > +	ZUC_ASSERT_EQ(0, n);
> > +	ZUC_ASSERT_EQ(0, errno);
> > +}
> > +
> > +ZUC_TEST_F(config_test_t1, test019, 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, "zero", &n, 600);
> > +
> > +	ZUC_ASSERT_EQ(0, r);
> > +	ZUC_ASSERT_EQ(0, n);
> > +	ZUC_ASSERT_EQ(0, 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
>