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

2018-12-30 Thread Pali Rohár
On Sunday 30 December 2018 13:51:10 Walter Harms wrote:
> 
> 
> > Pali Rohár  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 
> > > 
> > > --
> > > 
> > > 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 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  #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], _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.

> 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.ro...@gmail.com
> > ___
> > xorg-devel@lists.x.org: X.Org development
> > Archives: http://lists.x.org/archives/xorg-devel
> > Info: https://lists.x.org/mailman/listinfo/xorg-devel

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

2018-12-30 Thread Walter Harms


> Pali Rohár  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 
> > 
> > --
> > 
> > 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 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #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], _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 ? 
Using atoi(argv[i]) and checking for an invalid value seems fine.
Special cases like NAN do not matter here.

BTW: leading whitespaces are ignored with strtod() (so far i know)

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.ro...@gmail.com
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel