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 Laszlo
