number conversion in lvm

number conversion in lvm

[Mikulas Patocka]

Hi

I found some strange code in _get_int_arg in ./tools/lvmcmdline.c:

v = strtol(val, ptr, 10);

if (*ptr == val)
        return 0;

av->i_value = (int32_t) v;
av->ui_value = (uint32_t) v;
av->i64_value = (int64_t) v;
av->ui64_value = (uint64_t) v;

It is taking 32-bit long value and then casting it to 64 bits. Should 
there be strtoull instead of strtol?

BTW. how is that function supposed to handle integer overflow - it doesn't 
seem to do that.

Mikulas

number conversion in lvm

[Zdenek Kabelac]

Dne 13.7.2013 01:11, Mikulas Patocka napsal(a):
> Hi
>
> I found some strange code in _get_int_arg in ./tools/lvmcmdline.c:
>
> v = strtol(val, ptr, 10);
>
> if (*ptr == val)
>          return 0;
>
> av->i_value = (int32_t) v;
> av->ui_value = (uint32_t) v;
> av->i64_value = (int64_t) v;
> av->ui64_value = (uint64_t) v;
>
> It is taking 32-bit long value and then casting it to 64 bits. Should
> there be strtoull instead of strtol?

It used to read only 32bit values.
For 64bit other functions are used - i.e.  _size_arg

>
> BTW. how is that function supposed to handle integer overflow - it doesn't
> seem to do that.
>

it returns  LONG_MIN/MAX for underflow/overflow - but assuming errno could
be checked for ERANGE and fail command for this case instead.

Zdenek

number conversion in lvm

[Mikulas Patocka]

On Sat, 13 Jul 2013, Zdenek Kabelac wrote:

> Dne 13.7.2013 01:11, Mikulas Patocka napsal(a):
> > Hi
> > 
> > I found some strange code in _get_int_arg in ./tools/lvmcmdline.c:
> > 
> > v = strtol(val, ptr, 10);
> > 
> > if (*ptr == val)
> >          return 0;
> > 
> > av->i_value = (int32_t) v;
> > av->ui_value = (uint32_t) v;
> > av->i64_value = (int64_t) v;
> > av->ui64_value = (uint64_t) v;
> > 
> > It is taking 32-bit long value and then casting it to 64 bits. Should
> > there be strtoull instead of strtol?
> 
> It used to read only 32bit values.
> For 64bit other functions are used - i.e.  _size_arg

In tools/pvck.c there is "arg_uint64_value(cmd, labelsector_ARG,", but 
labelsector_ARG is declared with "arg(labelsector_ARG, '\0', 
"labelsector", int_arg, 0)", so there is loss of values that do not fit 
into long type.

> > BTW. how is that function supposed to handle integer overflow - it doesn't
> > seem to do that.
> > 
> 
> it returns  LONG_MIN/MAX for underflow/overflow - but assuming errno could
> be checked for ERANGE and fail command for this case instead.
> 
> Zdenek

The major problem is that the same function _get_int_arg parses both 
32-bit and 64-bit arguments. It would be natural to return error on 
integer overflow but you can't because you don't know if the caller will 
use the 32-bit value or the 64-bit value.

Another question is why there is av->i_value and av->ui_value when the 
values are the same and not dependent on the sign? What if the value fits 
into av->ui_value and doesn't fit into av->i_value - the overflow is never 
detected. This argument parser just seems totally confusing.

Mikulas

number conversion in lvm

[Zdenek Kabelac]

Dne 15.7.2013 22:10, Mikulas Patocka napsal(a):
>
>
> On Sat, 13 Jul 2013, Zdenek Kabelac wrote:
>
>> Dne 13.7.2013 01:11, Mikulas Patocka napsal(a):
>>> Hi
>>>
>>> I found some strange code in _get_int_arg in ./tools/lvmcmdline.c:
>>>
>>> v = strtol(val, ptr, 10);
>>>
>>> if (*ptr == val)
>>>           return 0;
>>>
>>> av->i_value = (int32_t) v;
>>> av->ui_value = (uint32_t) v;
>>> av->i64_value = (int64_t) v;
>>> av->ui64_value = (uint64_t) v;
>>>
>>> It is taking 32-bit long value and then casting it to 64 bits. Should
>>> there be strtoull instead of strtol?
>>
>> It used to read only 32bit values.
>> For 64bit other functions are used - i.e.  _size_arg
>
> In tools/pvck.c there is "arg_uint64_value(cmd, labelsector_ARG,", but
> labelsector_ARG is declared with "arg(labelsector_ARG, '\0',
> "labelsector", int_arg, 0)", so there is loss of values that do not fit
> into long type.
>

Yep - could be some inconsistency which may need implementation of
function like   sector_arg which will accept full 64bit value.
(See args.h for labelsector_ARG)
int_arg should be left to accept only 32bit values.

Seems like noone so far tried to specify sectors reaching past 1TB when 
seeking for lvm header ;)

>>> BTW. how is that function supposed to handle integer overflow - it doesn't
>>> seem to do that.
>>>
>>
>> it returns  LONG_MIN/MAX for underflow/overflow - but assuming errno could
>> be checked for ERANGE and fail command for this case instead.
>>
>> Zdenek
>
> The major problem is that the same function _get_int_arg parses both
> 32-bit and 64-bit arguments. It would be natural to return error on
> integer overflow but you can't because you don't know if the caller will
> use the 32-bit value or the 64-bit value.
>
> Another question is why there is av->i_value and av->ui_value when the
> values are the same and not dependent on the sign? What if the value fits

There are separate function for checking sign and accepting signed ints 
(int_arg_with_sign) - otherwise if SIGN_MINUS is detect - log error
is reported.

Zdenek

number conversion in lvm

[Mikulas Patocka]

> > > > BTW. how is that function supposed to handle integer overflow - it
> > > > doesn't
> > > > seem to do that.
> > > > 
> > > 
> > > it returns  LONG_MIN/MAX for underflow/overflow - but assuming errno could
> > > be checked for ERANGE and fail command for this case instead.
> > > 
> > > Zdenek
> > 
> > The major problem is that the same function _get_int_arg parses both
> > 32-bit and 64-bit arguments. It would be natural to return error on
> > integer overflow but you can't because you don't know if the caller will
> > use the 32-bit value or the 64-bit value.
> > 
> > Another question is why there is av->i_value and av->ui_value when the
> > values are the same and not dependent on the sign? What if the value fits
> 
> There are separate function for checking sign and accepting signed ints
> (int_arg_with_sign) - otherwise if SIGN_MINUS is detect - log error
> is reported.
> 
> Zdenek

Why are there two values av->i_value and av->ui_value if they have the 
same value and the sign flag is stored separately in av->sign?

Mikulas