[CRIU] [PATCH v2 1/2] util: xatol() and xatoi() helpers introduced

[Andrei Vagin]

On Tue, Sep 26, 2017 at 11:16:14AM +0200, Stanislav Kinsburskiy wrote:
> 
> 
> 25.09.2017 20:19, Andrei Vagin пишет:
> > On Mon, Sep 25, 2017 at 11:07:11AM +0200, Stanislav Kinsburskiy wrote:
> >>
> >>
> >> 23.09.2017 00:30, Andrei Vagin пишет:
> >>> On Wed, Sep 13, 2017 at 04:48:50PM +0300, Stanislav Kinsburskiy wrote:
> >>>> These helpers are safe versions of atol() and atoi() respectively.
> >>>> And they check for overflow and NAN errors
> >>>>
> >>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>
> >>>> ---
> >>>>  criu/include/util.h |    3 +++
> >>>>  criu/util.c         |   38 ++++++++++++++++++++++++++++++++++++++
> >>>>  2 files changed, 41 insertions(+)
> >>>>
> >>>> diff --git a/criu/include/util.h b/criu/include/util.h
> >>>> index 1fc20db..20684cc 100644
> >>>> --- a/criu/include/util.h
> >>>> +++ b/criu/include/util.h
> >>>> @@ -280,6 +280,9 @@ void tcp_cork(int sk, bool on);
> >>>>  
> >>>>  const char *ns_to_string(unsigned int ns);
> >>>>  
> >>>> +int xatol(const char *string, long *number);
> >>>> +int xatoi(const char *string, int *number);
> >>>> +
> >>>>  char *xstrcat(char *str, const char *fmt, ...)
> >>>>  	__attribute__ ((__format__ (__printf__, 2, 3)));
> >>>>  char *xsprintf(const char *fmt, ...)
> >>>> diff --git a/criu/util.c b/criu/util.c
> >>>> index b5a161f..7d60617 100644
> >>>> --- a/criu/util.c
> >>>> +++ b/criu/util.c
> >>>> @@ -58,6 +58,44 @@
> >>>>  
> >>>>  #define VMA_OPT_LEN	128
> >>>>  
> >>>> +static int xatol_base(const char *string, long *number, int base)
> >>>> +{
> >>>> +	char *endptr;
> >>>> +	long nr;
> >>>> +
> >>>> +	errno = 0;
> >>>> +	nr = strtol(string, &endptr, base);
> >>>> +	if ((errno == ERANGE && (nr == LONG_MAX || nr == LONG_MIN))
> >>>> +			|| (errno != 0 && nr == 0)) {
> >>>> +		pr_perror("failed to convert string");

pr_perror("failed to convert string: %s", string);

> >>>
> >>> It is better to print this string in a error message
> >>>
> >>
> >>
> >> Well,on the other hand how we can distinguish between this error case above and EINVAL, returned below, when string is not a number?
> > 
> > I am not sure that I understand what you mean. Error messages are
> > different, is it not enough?
> > 
> 
> They are not. Function strtol can return EINVAL.
> And EINVAL is also returned below on endptr check.

I sugested to print a string in a error message, why you started talking
about error codes? Why don't you return ERANGE here?

> 
> >>
> >>
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	if ((endptr == string) || (*endptr != '\0')) {
> >>>> +		pr_err("String is not a number: '%s'\n", string);
> >>>> +		return -EINVAL;
> 
> ^^^^^^^^^^^^^^^^^^^^^^^^^^ here.
> 
> But here it can be converted to some other error though... ENOTSUP for instance, if you insist on moving the print outside the helper.
> 
> 
> >>>> +	}
> >>>> +	*number = nr;
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +int xatol(const char *string, long *number)
> >>>> +{
> >>>> +	return xatol_base(string, number, 10);
> >>>> +}
> >>>> +
> >>>> +
> >>>> +int xatoi(const char *string, int *number)
> >>>> +{
> >>>> +	long tmp;
> >>>> +	int err;
> >>>> +
> >>>> +	err = xatol(string, &tmp);
> >>>
> >>> Should we check that tmp is in [INT_MIN, INT_MAX]?
> >>
> >>
> >> Well, looks like we shouldn't. Atoi will return interger even if string contains long.
> > 
> > So why do we need to check that a value is in [LONG_MIN, LONG_MAX] for
> > xatol? Why do we need these hellpers?
> > 
> 
> 1) In case of xatol we check the string can be converted in arch-size variable.
> 2) In case of xatoi it's not needed, because number can be converted to int by simply zeroing high 32 bits. You don't check number for int, when casting like this "(int)long_number", don't you?

It is not convince me. xatoi and xtol have to be symetrical, if one of
them check a range, another one should check a range too (IMHO)

> 3) These helpers do convert strings to numbers *properly* and taking errors into account.
> I understand, that CRIU relies on the fact that kernel is much less error-prone and if we know, that is has to return int (long) we can rely on it. But I don't think that it's a safe approach. Do you?

I am agree and this point contradicts with your explanation.

If the kernel has to return int, but it returns a value bigger than
INT_MAX, xatoi has to return an error, doesn't it?

> 
> >>
> >>
> >>>> +	if (!err)
> >>>> +		*number = (int)tmp;
> >>>> +	return err;
> >>>> +}
> >>>> +
> >>>>  /*
> >>>>   * This function reallocates passed str pointer.
> >>>>   * It means:
> >>>>
> >>>> _______________________________________________
> >>>> CRIU mailing list
> >>>> CRIU at openvz.org
> >>>> https://lists.openvz.org/mailman/listinfo/criu