[PATCH] parse_int: fix overflow detection

[Michael Adler]

The previous comparison with LONG_{MIN,MAX} was (almost) always false
because the variable for the comparison had already been casted from
long to int.

Signed-off-by: Michael Adler <michae...@siemens.com>
---
tools/bg_setenv.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index d26eeed..043c0aa 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -221,16 +221,17 @@ static error_t set_uservars(char *arg)
static int parse_int(char *arg)
{
char *tmp;
- int i;
+ long i;

errno = 0;
i = strtol(arg, &tmp, 10);
if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
+ (i > INT_MAX || i < INT_MIN) ||
(errno != 0 && i == 0) || (tmp == arg)) {
errno = EINVAL;
return -1;
}
- return i;
+ return (int) i;
}

static error_t parse_opt(int key, char *arg, struct argp_state *state)
--
2.33.0

[Jan Kiszka]

On 08.10.21 09:51, Michael Adler wrote:
> The previous comparison with LONG_{MIN,MAX} was (almost) always false
> because the variable for the comparison had already been casted from
> long to int.
>
> Signed-off-by: Michael Adler <michae...@siemens.com>
> ---
> tools/bg_setenv.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
> index d26eeed..043c0aa 100644
> --- a/tools/bg_setenv.c
> +++ b/tools/bg_setenv.c
> @@ -221,16 +221,17 @@ static error_t set_uservars(char *arg)
> static int parse_int(char *arg)
> {
> char *tmp;
> - int i;
> + long i;
>
> errno = 0;
> i = strtol(arg, &tmp, 10);
> if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
> + (i > INT_MAX || i < INT_MIN) ||

Can't we drop the LONG_* tests then?

Jan

> (errno != 0 && i == 0) || (tmp == arg)) {
> errno = EINVAL;
> return -1;
> }
> - return i;
> + return (int) i;
> }
>
> static error_t parse_opt(int key, char *arg, struct argp_state *state)
>

--

Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Michael Adler]

> Can't we drop the LONG_* tests then?

I don't think so, INT_MAX could be the same value as LONG_MAX (same for _MIN).

--
Michael Adler

Siemens AG
T RDA IOT SES-DE
Otto-Hahn-Ring 6
81739 München, Deutschland

Siemens Aktiengesellschaft: Vorsitzender des Aufsichtsrats: Jim Hagemann Snabe; Vorstand: Roland Busch, Vorsitzender; Klaus Helmrich, Cedrik Neike, Matthias Rebellius, Ralf P. Thomas, Judith Wiese; Sitz der Gesellschaft: Berlin und München, Deutschland; Registergericht: Berlin-Charlottenburg, HRB 12300, München, HRB 6684; WEEE-Reg.-Nr. DE 23691322

[Jan Kiszka]

On 08.10.21 10:07, Michael Adler wrote:
>> Can't we drop the LONG_* tests then?
>
> I don't think so, INT_MAX could be the same value as LONG_MAX (same for _MIN).
>

Maybe time to rethink what we are checking here: Is LONG_MAX/MIN
supposed to catch overflows? Those are already reported by errno.

Jan

[Michael Adler]

Hi Jan,

> Maybe time to rethink what we are checking here: Is LONG_MAX/MIN
> supposed to catch overflows? Those are already reported by errno.

you are right, good point. There was another corner case missing which I have also added.

Since the patch is so small I won't start a new thread for v2, I hope that's fine with you.

Michael Adler (1):
parse_int: fix overflow detection

tools/bg_setenv.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

--
2.33.0

[Michael Adler]

The previous comparison with LONG_{MIN,MAX} was (almost) always false
because the variable for the comparison had already been casted from
long to int.

Signed-off-by: Michael Adler <michae...@siemens.com>
---

tools/bg_setenv.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index d26eeed..08beecc 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -221,16 +221,19 @@ static error_t set_uservars(char *arg)

static int parse_int(char *arg)
{
char *tmp;
- int i;
+ long i;

errno = 0;
i = strtol(arg, &tmp, 10);

- if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
- (errno != 0 && i == 0) || (tmp == arg)) {
+ if (errno == ERANGE /* out of range */
+ || (errno != 0 && i == 0) /* no conversion was performed */
+ || (tmp == arg || *tmp != '\0') /* invalid input */
+ || (i < INT_MIN || i > INT_MAX) /* not a valid int */
+ ) {

errno = EINVAL;
return -1;
}
- return i;
+ return (int) i;
}

static error_t parse_opt(int key, char *arg, struct argp_state *state)
--

2.33.0

[Jan Kiszka]

Thanks, applied. I've only reformatted things a bit, please see next.