[PATCH weston] Require base-10 for strtol() calls

[Peter Hutterer]

On Tue, Jul 12, 2016 at 06:54:49PM -0700, Bryce Harrington wrote:
> On Wed, Jul 13, 2016 at 10:27:49AM +1000, Peter Hutterer wrote:
> > On Tue, Jul 12, 2016 at 04:51:27PM -0700, Bryce Harrington wrote:
> > > The third arg to strtol() specifies the base to assume for the number.
> > > When 0 is passed, as is currently done in option-parser.c, hexadecimal
> > > and octal numbers are permitted and automatically detected and
> > > converted.
> > > 
> > > This change is an expansion of f6051cbab84c0e577473b67f0585c0f329eb80fe
> > > to cover the remaining strtol() calls in Weston, where the routine is
> > > being used to read fds and pids - which are always expressed in base-10.
> > > It also changes the calls in config-parser, used by
> > > weston_config_section_get_int(), which in turn is being used to read
> > > scales, sizes, times, rates, and delays; these are all expressed in
> > > base-10 numbers only.
> > > 
> > > The benefit of limiting this to base-10 is to eliminate surprises when
> > > parsing numbers from the command line.  Also, by making the code
> > > consistent with other usages of strtol, it may make it possible to
> > > factor out the common code in the future.
> > > 
> > > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > 
> > code looks correct, but I didn't check all the callers, I'll just take
> > your word for the base-10 assumption :)
> > Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
> 
> Thanks, both patches pushed:
>    6351fb0..375759e  master -> master
> 
> For the strtol() calls the callers are pretty straightforward so this
> change should be just fine.
> 
> Where things get a bit tricker is the strtoul() call in
> weston_config_section_get_uint() which I'd also like to switch to
> base-10, however some of its callers are parsing colors - which are hex
> numbers and thus switching to base-10 would break those.  I haven't
> sorted out what to do there.  I could introduce a new routine like
> weston_config_section_get_hex() or even
> weston_config_section_get_color() to handle that case, thus allowing
> weston_config_section_get_uint() to be forced base-10; or I could just
> leave it as-is and let it autodetect decimal or hexadecimal as needed.
> Any opinions?

IMO anything that's explicitly only parsing hex should be a separate
function if for no other reason than obviousness in the caller.

Cheers,
   Peter

> > > ---
> > >  compositor/main.c      | 2 +-
> > >  libweston/compositor.c | 2 +-
> > >  shared/config-parser.c | 2 +-
> > >  xwayland/launcher.c    | 2 +-
> > >  4 files changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/compositor/main.c b/compositor/main.c
> > > index 6cf9194..8400d70 100644
> > > --- a/compositor/main.c
> > > +++ b/compositor/main.c
> > > @@ -1684,7 +1684,7 @@ int main(int argc, char *argv[])
> > >  	server_socket = getenv("WAYLAND_SERVER_SOCKET");
> > >  	if (server_socket) {
> > >  		weston_log("Running with single client\n");
> > > -		fd = strtol(server_socket, &end, 0);
> > > +		fd = strtol(server_socket, &end, 10);
> > >  		if (*end != '\0')
> > >  			fd = -1;
> > >  	} else {
> > > diff --git a/libweston/compositor.c b/libweston/compositor.c
> > > index 771f1c9..96eeb17 100644
> > > --- a/libweston/compositor.c
> > > +++ b/libweston/compositor.c
> > > @@ -4617,7 +4617,7 @@ weston_environment_get_fd(const char *env)
> > >  	e = getenv(env);
> > >  	if (!e)
> > >  		return -1;
> > > -	fd = strtol(e, &end, 0);
> > > +	fd = strtol(e, &end, 10);
> > >  	if (*end != '\0')
> > >  		return -1;
> > >  
> > > diff --git a/shared/config-parser.c b/shared/config-parser.c
> > > index 247e880..4c67220 100644
> > > --- a/shared/config-parser.c
> > > +++ b/shared/config-parser.c
> > > @@ -170,7 +170,7 @@ weston_config_section_get_int(struct weston_config_section *section,
> > >  	}
> > >  
> > >  	errno = 0;
> > > -	*value = strtol(entry->value, &end, 0);
> > > +	*value = strtol(entry->value, &end, 10);
> > >  	if (errno != 0 || end == entry->value || *end != '\0') {
> > >  		*value = default_value;
> > >  		errno = EINVAL;
> > > diff --git a/xwayland/launcher.c b/xwayland/launcher.c
> > > index 614ef5b..b7aee3b 100644
> > > --- a/xwayland/launcher.c
> > > +++ b/xwayland/launcher.c
> > > @@ -164,7 +164,7 @@ create_lockfile(int display, char *lockfile, size_t lsize)
> > >  			return -1;
> > >  		}
> > >  
> > > -		other = strtol(pid, &end, 0);
> > > +		other = strtol(pid, &end, 10);
> > >  		if (end != pid + 10) {
> > >  			weston_log("can't parse lock file %s\n",
> > >  				lockfile);
> > > -- 
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > wayland-devel mailing list
> > > wayland-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > >