Re: [Qemu-devel] [PATCH 1/7] string-input-visitor: Fix uint64 parsing

[Markus Armbruster]

Andreas Färber <address@hidden> writes:

> All integers would get parsed by strtoll(), not handling the case of
> UINT64 properties with the most significient bit set.

This mess is part of a bigger mess, I'm afraid.

The major ways integers get parsed are:

* QMP: parse_literal() in qmp/qobject/json-parser.c

  This is what parses QMP off the wire.

  RFC 7159 does not prescribe range or precision of JSON numbers.  Our
  implementation accepts the union of int64_t and double.

  If the lexer recognizes a floating-point number, we convert it with
  strtod() and represent it as double.

  If the lexer recognizes a decimal integer, and strtoll() can convert
  it, we represent it in int64_t.  Else, we convert it with strtod() and
  represent it as double.  Unclean: code assumes int64_t is long long.

  Yes, that means QMP can't currently support the full range of QAPI's
  uint64 type.

* QemuOpts: parse_option_number() in util/qemu-option.c

  This is what parses key=value,... strings for command line and other
  places.

  QemuOpts can be used in two ways.  If you fill out QemuOptDesc desc[],
  it rejects unknown keys and parses values of known keys.  If you leave
  it empty, it accepts all keys, and doesn't parse values.  Either way,
  it also stores raw string values.

  QemuOpts' parser only supports unsigned numbers, in decimal, octal and
  hex.  Error checking is very poor.  In particular, it considers
  negative input valid, and silently casts it to uint64_t.  I wouldn't
  be surprised if some code depended on that.

* String input visitor: parse_str() in qapi/string-input-visitor.c

  This appears to be used only by QOM so far:

  - object_property_get_enum()
  - object_property_get_uint16List()
  - object_property_parse()

  parse_str() appears to parse some fancy list syntax.  Comes from
  commit 659268f.  The commit message is useless.  I can't see offhand
  how this interacts with the visitor core.

  Anyway, if we ignore the fancy crap and just look at the parsing of a
  single integer, we see that it supports int64_t in decimal, octal and
  hex, it fails to check for ERANGE, and assumes int64_t is long long.

* Options visitor: opts_type_int() in opts qapi/opts-visitor.c

  This one is for converting QemuOpts to QAPI-defined C types.  It uses
  the raw string values, not the parsed ones.  The QemuOpts parser is
  neither needed nor wanted here.  You should use the options visitor
  with an empty desc[] array to bypass it.  Example: numa.c.

  We got fancy list syntax again.  This one looks like I could
  understand it with a bit of effort.  But let's look just at the
  parsing of a single integer.  It supports uint64_t in decimal, octal
  and hex, and *surprise* checks for errors carefully.

Fixing just a part of a mess can be okay.  I just don't want to lever
the bigger mess unmentioned.

> Implement a .type_uint64 visitor callback, reusing the existing
> parse_str() code through a new argument, using strtoull().

I'm afraid you're leaving the bug in the visitor core unfixed.

The (essentially undocumented) Visitor abstraction has the following
methods for integers:

* Mandatory: type_int()

  Interface uses int64_t for the value.  The implementation should
  ensure it fits into int64_t.

* Optional: type_int{8,16,32}()

  These use int{8,16,32}_t for the value.

  If present, it should ensure the value fits into the data type.

  If missing, the core falls back to type_int() plus appropriate range
  checking.

* Optional: type_int64()

  Same interface as type_int().

  If present, it should ensure the value fits into int64_t.

  If missing, the core falls back to type_int().

  Aside: setting type_int64() would be useful only when you want to
  distinguish QAPI types int and int64.  So far, nobody does.  In fact,
  nobody uses QAPI type int64!  I'm tempted to define QAPI type int as a
  mere alias for int64 and drop the redundant stuff.

* Optional: type_uint{8,16,32}()

  These use uint{8,16,32}_t for the value.

  If present, it should ensure the value fits into the data type.

  If missing, the core falls back to type_int() plus appropriate range
  checking.

* Optional: type_uint64()

  Now it gets interesting.  Interface uses uint64_t for the value.

  If present, it should ensure the value fits into uint64_t.

  If missing, the core falls back to type_int().  No range checking.  If
  type_int() performs range checking as it should, then uint64_t values
  not representable in int64_t get rejected (wrong), and negative values
  representable in int64_t get cast to uint64_t (also wrong).

  I think we need to make type_uint64() mandatory, and drop the
  fallback.

* Optional: type_size()

  Same interface as type_uint64().

  If present, it should ensure the value fits into uint64_t.

  If missing, the core first tries falling back to type_uint64() and
  then to type_int().  Falling back to type_int() is as wrong here as it
  is in type_uint64().

> As a bug fix, ignore warnings about preference of qemu_strto[u]ll().

I'm not sure I get this sentence.

> Cc: address@hidden
> Signed-off-by: Andreas Färber <address@hidden>

On the actual patch, I have nothing to add over Eric's review right now.