[PATCH v2] Fix checking for valid argument of --dpi

[Pali Rohár]

On Wednesday 24 April 2019 16:29:25 Pali Rohár wrote:
> On Sunday 30 December 2018 14:11:13 Pali Rohár wrote:
> > On Sunday 30 December 2018 13:51:10 Walter Harms wrote:
> > > 
> > > 
> > > > Pali Rohár <pali.rohar at gmail.com> hat am 29. Dezember 2018 um 18:45
> > > > geschrieben:
> > > > 
> > > > 
> > > > On Monday 26 November 2018 22:17:24 Pali Rohár wrote:
> > > > > Function strtod() sets strtod_error to the pointer of the first invalid
> > > > > character and therefore it does not have to be first character from input.
> > > > > When input is valid then it points to nul byte. Conversion error is
> > > > > indicated by setted errno. Zero-length argument, non-positive DPI or DPI
> > > > > with trailing or leading whitespaces is invalid too.
> > > > > 
> > > > > Update also error message about invalid argument.
> > > > > 
> > > > > Signed-off-by: Pali Rohár <pali.rohar at gmail.com>
> > > > > 
> > > > > --
> > > > > 
> > > > > Changes since v1:
> > > > > * Get same error message for `xrandr --dpi ''` and `xrandr --dpi ' '`
> > > > > * Make the check for dpi <= 0
> > > > > * Do not accept leading whitespaces (trailing were already disallowed)
> > > > > ---
> > > > >  xrandr.c | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/xrandr.c b/xrandr.c
> > > > > index ce3cd91..4baa075 100644
> > > > > --- a/xrandr.c
> > > > > +++ b/xrandr.c
> > > > > @@ -40,6 +40,7 @@
> > > > >  #include <inttypes.h>
> > > > >  #include <stdarg.h>
> > > > >  #include <math.h>
> > > > > +#include <ctype.h>
> > > > >  
> > > > >  #ifdef HAVE_CONFIG_H
> > > > >  #include "config.h"
> > > > > @@ -3118,8 +3119,9 @@ main (int argc, char **argv)
> > > > >  	if (!strcmp ("--dpi", argv[i])) {
> > > > >  	    char *strtod_error;
> > > > >  	    if (++i >= argc) argerr ("%s requires an argument\n", argv[i-1]);
> > > > > +	    errno = 0;
> > > > >  	    dpi = strtod(argv[i], &strtod_error);
> > > > > -	    if (argv[i] == strtod_error)
> > > > > +	    if (!argv[i][0] || isspace(argv[i][0]) || *strtod_error || errno ||
> > > > > dpi <= 0)
> > > > >  	    {
> > > > >  		dpi = 0.0;
> > > > >  		dpi_output_name = argv[i];
> > > 
> > > 
> > > Why spending so much effort ? 
> > 
> > Which "much effort"?
> > 
> > This is to ensure that incorrect param or garbage supplied by user is
> > not interpreted as number.
> > 
> > > Using atoi(argv[i]) and checking for an invalid value seems fine.
> > 
> > atoi does not signal when garbage is on input. And dpi can be
> > fractional, so atoi cannot be used for it.
> > 
> > > Special cases like NAN do not matter here.
> > > 
> > > BTW: leading whitespaces are ignored with strtod() (so far i know)
> > 
> > But strtod does not signal that they were ignored. Moreover trailing are
> > not accepted. So I chose implementation which is consistent for trailing
> > and leading whitespaces.
> 
> Any other review comments for this patch?
> 
> I would like to know if this patch could be applied or if there are some
> other problems which needs to be resolved.

ping, just a reminder for this my patch...

> > > just my 2 cents
> > > re,
> > >  wh
> > > 
> > > 
> > > 
> > > > > @@ -3569,7 +3571,7 @@ main (int argc, char **argv)
> > > > >  	    XRROutputInfo	*output_info;
> > > > >  	    XRRModeInfo	*mode_info;
> > > > >  	    if (!dpi_output)
> > > > > -		fatal ("Cannot find output %s\n", dpi_output_name);
> > > > > +		fatal ("%s is not valid DPI nor valid output\n", dpi_output_name);
> > > > >  	    output_info = dpi_output->output_info;
> > > > >  	    mode_info = dpi_output->mode_info;
> > > > >  	    if (output_info && mode_info && output_info->mm_height)
> > > > 
> > > > Hello, can you review this v2 patch?
> 

-- 
Pali Rohár
pali.rohar at gmail.com