Discussion:
[PATCH app/xrandr] Fix checking for valid argument of --dpi
Pali Rohár
2018-04-12 18:52:21 UTC
Permalink
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 and zero DPI is invalid
too.

Update also error message about invalid argument.

Signed-off-by: Pali Rohár <***@gmail.com>
---
xrandr.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xrandr.c b/xrandr.c
index 7f1e867..1960bbf 100644
--- a/xrandr.c
+++ b/xrandr.c
@@ -3115,9 +3115,10 @@ 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]);
+ if (++i >= argc || !argv[i][0]) argerr ("%s requires an argument\n", argv[i-1]);
+ errno = 0;
dpi = strtod(argv[i], &strtod_error);
- if (argv[i] == strtod_error)
+ if (*strtod_error || errno || dpi == 0)
{
dpi = 0.0;
dpi_output_name = argv[i];
@@ -3567,7 +3568,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)
--
2.11.0

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Pali Rohár
2018-10-17 16:36:42 UTC
Permalink
Gentle reminder for a patch which I sent 5 months ago...
Hello, can you review my patch below?
Post by Pali Rohár
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 and zero DPI is invalid
too.
Update also error message about invalid argument.
---
xrandr.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/xrandr.c b/xrandr.c
index 7f1e867..1960bbf 100644
--- a/xrandr.c
+++ b/xrandr.c
@@ -3115,9 +3115,10 @@ 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]);
+ if (++i >= argc || !argv[i][0]) argerr ("%s requires an argument\n", argv[i-1]);
+ errno = 0;
dpi = strtod(argv[i], &strtod_error);
- if (argv[i] == strtod_error)
+ if (*strtod_error || errno || dpi == 0)
{
dpi = 0.0;
dpi_output_name = argv[i];
@@ -3567,7 +3568,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)
--
Pali Rohár
***@gmail.com
Giuseppe Bilotta
2018-10-18 07:03:27 UTC
Permalink
Hello,
- if (++i >= argc) argerr ("%s requires an argument\n", argv[i-1]);
+ if (++i >= argc || !argv[i][0]) argerr ("%s requires an argument\n", argv[i-1]);
I don't think this change is necessary, if arg[i][0] is NULL it means
there _was_ an argument, but it was empty. Getting different error
messages from `xrandr --dpi ''` and `xrandr --dpi ' '` doesn't seem
like a good idea to me.
+ errno = 0;
dpi = strtod(argv[i], &strtod_error);
- if (argv[i] == strtod_error)
+ if (*strtod_error || errno || dpi == 0)
While we're at it, I would make the check for dpi <= 0, since negative
values aren't valid either (in fact, negative values are effectively a
no-op, since they set the DPI from the current framebuffer settings,
and then set the virtual framebuffer physical dimensions from the
DPI).

Cheers,

Giuseppe Bilotta
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/ma
Pali Rohár
2018-10-18 07:36:45 UTC
Permalink
Hello,
- if (++i >= argc) argerr ("%s requires an argument\n", argv[i-1]);
+ if (++i >= argc || !argv[i][0]) argerr ("%s requires an argument\n", argv[i-1]);
I don't think this change is necessary, if arg[i][0] is NULL it means
there _was_ an argument, but it was empty. Getting different error
messages from `xrandr --dpi ''` and `xrandr --dpi ' '` doesn't seem
like a good idea to me.
strtod() does not signal error for empty string argument:

char *endptr = NULL;
double ret;
errno = 0;
ret = strtod("", &endptr);
printf("ret=%lf errno=%d end=%x\n", ret, errno, endptr[0]);

ret=0.000000 errno=0 end=0

But with explicit check for zero or negative return value, it is not
really needed.
+ errno = 0;
dpi = strtod(argv[i], &strtod_error);
- if (argv[i] == strtod_error)
+ if (*strtod_error || errno || dpi == 0)
While we're at it, I would make the check for dpi <= 0, since negative
values aren't valid either (in fact, negative values are effectively a
no-op, since they set the DPI from the current framebuffer settings,
and then set the virtual framebuffer physical dimensions from the
DPI).
Cheers,
Giuseppe Bilotta
--
Pali Rohár
***@gmail.com
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x
Pali Rohár
2018-11-26 21:17:24 UTC
Permalink
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 <***@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)
--
2.11.0

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: ht
Loading...