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

[Bryce Harrington]

On Mon, Jul 11, 2016 at 09:28:16AM +1000, Peter Hutterer wrote:
> 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.

I've pushed this specific cleanup with Eric's R-b but will follow up
with a proposal for this helper function later.  I think there may be a
few more cleanups to do first.

Bryce
 
> 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
> >