Re: [Qemu-devel] [PATCH 00/16] introduce OptsVisitor, rebase -net/-netdev parsing

[Michael Roth]

On Wed, Jun 06, 2012 at 05:30:03PM +0200, Laszlo Ersek wrote:
> On 06/06/12 17:16, Michael Roth wrote:
> > On Wed, Jun 06, 2012 at 04:10:44PM +0200, Paolo Bonzini wrote:
> 
> >> The uintXX visitors do not fail if you pass a negative value.  I'm fine
> >> with including the patch with the small bug and fixing it as a
> >> follow-up, there's plenty of time before 1.2.
> > 
> > How would we implement such a check?
> > 
> > In the case of uint64_t, the field we're visiting is passed in as a
> > uint64_t*, so -1 is indistinguishable from the unsigned interpretation
> > of the field, which is within the valid range. 
> > 
> > For uintXX_t where XX < 64, a negative value would exceed the UINTXX_MAX
> > check, so those cases are already handled.
> > 
> > Or am I missing something?
> 
> I found three instances of the patch on the list:
> 
>   http://lists.nongnu.org/archive/html/qemu-devel/2012-04/msg00333.html
>   http://lists.nongnu.org/archive/html/qemu-devel/2012-04/msg01292.html
>   http://lists.nongnu.org/archive/html/qemu-devel/2012-04/msg04068.html
> 
> looking at the third one, all of
> 
> - visit_type_uint8()
> - visit_type_uint16()
> - visit_type_uint32()
> - visit_type_uint64()
> 
> seem to define "value" as an int64_t. Thus when we fall back to
> (*v->type_int)(), the comparison is still done against an int64_t. Since
> "int" is equivalent to "int32_t" on the platforms I can think of, and
> "int64_t" to "long long", the comparisons are evaluated as follows:
> 
>   value > UINT8_MAX
>   value > UINT16_MAX
> 
> First the right hand sides are promoted to "int" (with unchanged value),
> and then "int" is converted to "long long" (both signed, different
> conversion rank).
> 
>   value > UINT32_MAX
> 
> The right hand side is directly converted to "long long" (signed vs.
> unsigned, signed has greater rank and can represent all values of the
> lower-rank unsigned type).
> 
> I propose
> 
>   value < 0 || value > UINT8_MAX
>   value < 0 || value > UINT16_MAX
>   value < 0 || value > UINT32_MAX

>   value < 0

I think this last one will cause problems though, since uint64_t's
within the valid range for visit_type_uint64() will fail due to being
interpreted as < 0 when cast to int64_t. With the others we can detect
these cases since the max unsigned value doesn't exceed the max signed
value of the intermediate type we're storing to.

> 
> Laszlo
>