|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:|
|i915 platform:||i915 features:|
|Bug Depends on:|
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