Bug 14417

Summary:

xrandr loose argument parsing and error checking, with partial patches

Product:

xorg

Reporter:

Bart Massey <x>

Component:

App/xrandr

Assignee:

Keith Packard <keithp>

Status:

CLOSED FIXED

QA Contact:

Severity:

minor

Priority:

medium

CC:

esigra, mat

Version:

unspecified

Hardware:

All

OS:

All

Whiteboard:

i915 platform:

i915 features:

Bug Depends on:

Bug Blocks:

23614

Attachments:

Description	Flags
Add some argument parsing checks	none
Give a warning at the end for each --output that was not found on the server	none
revised version of previous patch	none

Description Bart Massey 2008-02-07 18:04:29 UTC

Created attachment 14210 [details] [review]
Add some argument parsing checks

xrandr's argument parsing is done very loosely with a lot of scanf()s and atoi()s.  As a result, trailing garbage is accepted on almost any numeric argument, and sometimes altogether garbage is accepted as a numeric argument of 0.  The first attached patch fixes much of this, but more work would be needed to fix it all.

More importantly, the name given to --output is not checked in any way.  If a nonexistent output is specified, xrandr silently does nothing.  The second attached patch fixes this, so that a warning is issued after execution if one or more of the specified outputs was not found.

Comment 1 Bart Massey 2008-02-07 18:07:12 UTC

Created attachment 14211 [details] [review]
Give a warning at the end for each --output that was not found on the server

Comment 2 Daniel Stone 2009-08-31 17:34:16 UTC

probably too late for 7.6, unless you want to merge it and do a new stable release, matthias? if so, please go ahead.

Comment 3 Matthias Hopf 2009-09-04 06:39:10 UTC

Some changes remove a reasonable warning, e.g.

-	    screen = atoi (argv[i]);
-	    if (screen < 0) usage();
+	    screen = check_strtol(argv[i]);

It would be worthwhile to verify whether check_strtol() could validate for positive or 0 values always.

Also, I'd love to have the two fixes (double vs. float and check_strto*()) in separate commits. But that is no showstopper.

Bart, can you fix the patch accordingly?


I like the second patch, though I think 'mark' is a misnomer, 'found' is probably better. I'll apply it.


David, what hast to be done to apply this to 7.6? A release, yes, what else?

Comment 4 Bart Massey 2009-09-04 10:54:14 UTC

Created attachment 29220 [details] [review]
revised version of previous patch

Comment 5 Bart Massey 2009-09-04 10:55:13 UTC

(In reply to comment #3)
> Some changes remove a reasonable warning, e.g.
> 
> -           screen = atoi (argv[i]);
> -           if (screen < 0) usage();
> +           screen = check_strtol(argv[i]);

Good point.  Fixed.

> Also, I'd love to have the two fixes (double vs. float and check_strto*()) in
> separate commits. But that is no showstopper.

IIRC (and it's been a while) the only reason to move the floats to doubles was to avoid writing check_strtof().  So I just changed the commit message. :-)

> Bart, can you fix the patch accordingly?

Attached.

Comment 6 Matthias Hopf 2009-09-07 04:05:49 UTC

Committed. Thanks!

Any more patches to be applied? Otherwise I'll do another release.

Comment 7 Bart Massey 2009-09-07 22:17:55 UTC

No, I'm good for now.  Thanks for dealing with it.

At some point someone should replace the whole argument parsing mechanism for this program with something better.  However, that's a big job and I'm afraid I don't really feel like taking it on right this second.

Comment 8 Matthias Hopf 2009-09-14 09:22:33 UTC

Released.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.