Bug 14417 - xrandr loose argument parsing and error checking, with partial patches
Summary: xrandr loose argument parsing and error checking, with partial patches
Status: CLOSED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: App/xrandr (show other bugs)
Version: unspecified
Hardware: All All
: medium minor
Assignee: Keith Packard
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: xorg-7.6
  Show dependency treegraph
 
Reported: 2008-02-07 18:04 UTC by Bart Massey
Modified: 2009-09-14 09:22 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Add some argument parsing checks (7.45 KB, patch)
2008-02-07 18:04 UTC, Bart Massey
no flags Details | Splinter Review
Give a warning at the end for each --output that was not found on the server (1.79 KB, patch)
2008-02-07 18:07 UTC, Bart Massey
no flags Details | Splinter Review
revised version of previous patch (7.59 KB, patch)
2009-09-04 10:54 UTC, Bart Massey
no flags Details | Splinter Review

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.