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

[Stanislav Kinsburskiy]

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");
>>>
>>> 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.

>>
>>
>>>> +		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?
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?

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