On 07/18/2014 03:14 PM, John Snow wrote: > visit_type_uint32 has a boundary check where it makes sure that the > value given to it is within its range, though it will still convert > negatives "automatically" and depending on the negative given, it might > pass this range check. > visit_type_uint64 by contrast cannot perform a check since everything is > within range by definition.
Yes, for that very reason, libvirt recently switched from two wrappers per width (parse signed, parse unsigned) to three wrappers (parse signed, parse unsigned and allow negative wraparound, parse unsigned and reject negative wraparound). By the way, did you realize that strtoull is required to parse 50% more input than strtoll, all because of negative wraparound? > This would fix the semantic issue that Markus has pointed out wherein we > requested an unsigned integer, but we may have already been passed a > signed integer. Tightening the integer parsing /might/ help make parsing > more semantically meaningful. When we tightened the parsing by adding the variant that rejects negative wraparound, we had to audit a lot of code to decide which callers still want to allow wraparound as a convenience (for example, it's much easier to say "copy up to -1 bytes of a file" than it is to say "copy up to 9223372036854775807 bytes of a file", when the semantics of copy automatically stop when EOF is hit no matter how much larger the end bound is when treated as unsigned). I also want to make sure we don't break QMP. The JSON parser is a bit finicky about numbers larger than LLONG_MAX, and so there are existing cases where for 64-bit unsigned parameters, libvirt has code to pass in the negative 2s complement counterpart in order to not upset the parser. > > Another option might be to somehow leave a breadcrumb somewhere in the > StringInputVisitor object that informs the parse_type_int/parse_str > functions ultimately that we'd like to be strict about enforcing a "no > sign modifiers" policy for this parsing. I am not well familiar enough > with the properties regime as a whole to really recommend where we might > inject such a bool. > > Of course, this would have to be a 2.2+ fix. For 2.1, I might just stick > with my original plan and make the error message nicer. Absolutely. For 2.1, only the bare minimum for this one message is okay; any dramatic cleanups to reject negative wraparound is 2.2 material. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
