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 >
