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

Submitted by Pali Rohár on Nov. 26, 2018, 9:17 p.m.

Details

Message ID 20181126211724.1041-1-pali.rohar@gmail.com
State New
Series "Fix checking for valid argument of --dpi"
Headers show

Commit Message

Pali Rohár Nov. 26, 2018, 9:17 p.m.
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@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(-)

Patch hide | download patch | download mbox

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];
@@ -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)

Comments

Pali Rohár Dec. 29, 2018, 5:45 p.m.
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@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];
> @@ -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?
Walter Harms Dec. 30, 2018, 12:51 p.m.
> Pali Rohár <pali.rohar@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@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 ? 
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.rohar@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 Dec. 30, 2018, 1:11 p.m.
On Sunday 30 December 2018 13:51:10 Walter Harms wrote:
> 
> 
> > Pali Rohár <pali.rohar@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@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.

> 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@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