qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Eric Blake <eblake@redhat.com>
To: John Snow <jsnow@redhat.com>, Markus Armbruster <armbru@redhat.com>
Cc: Amit Shah <amit.shah@redhat.com>,
	peter.maydell@linaro.org, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter
Date: Mon, 21 Jul 2014 14:31:20 -0600	[thread overview]
Message-ID: <53CD7898.10503@redhat.com> (raw)
In-Reply-To: <53CD7452.2060204@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2018 bytes --]

On 07/21/2014 02:13 PM, John Snow wrote:

> I can certainly grep through the code to find out who is using unsigned
> properties. In the case of uint32, -1 I believe will already wrap around
> but then overflow (because we parse as uint64_t) and throw an error, so
> I don't expect we will see anyone using -1 to signify "MAX" for less
> than 64bit properties. In the case of uint64, it may be more difficult
> to see who, if anyone, is abusing such behavior.

Actually, you may find that behavior on uint32 is MORE likely to be
confused, rather than less.  The _reason_ libvirt started tightening up
is because we hit a case where we were parsing an unsigned 32-bit
integer, but had different behavior on 32-bit hosts than on 64-bit
hosts, and it all boiled down to type promotion rules (basically,
strtoul("-1") on 32-bit platforms wrapped around to a 32-bit value,
which was still in range for uint32, while strtoul("-1") on 64-bit
platforms wrapped around to a 64-bit value which then appeared different
when truncated to uint32).  At least strtoull("-1") behaves the same on
both 32-bit and 64-bit hosts.

> 
> However, from a quick look-see it looks like DEFINE_PROP_UINT64 is used
> in 26 places. The fourth argument is "default value" and you can see
> many authors using -1 here, so either these authors expect wraparound or
> are trying to set the default value to something invalid that they will
> try to catch later on somehow.
> 
> CC'ing Eric Blake again for input, since he went through a similar
> ordeal recently and might have some input.

Tightening semantics is always a pain - in libvirt, we had to audit all
callers and make a case-by-case judgment call on whether the tighter
semantics of rejecting negatives made sense.  We ended up with very few
callers that still allowed wraparound, but there's no magic fix short of
just auditing the callers.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

      reply	other threads:[~2014-07-21 20:31 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-17 20:47 [Qemu-devel] [PATCH v2] virtio-rng: Add human-readable error message for negative max-bytes parameter John Snow
2014-07-18  6:27 ` Markus Armbruster
2014-07-18  7:46   ` Amit Shah
2014-07-18 11:15     ` Markus Armbruster
2014-07-18 11:27       ` Amit Shah
2014-07-18 11:54         ` Markus Armbruster
2014-07-18 12:14           ` Amit Shah
2014-07-18 13:16             ` Markus Armbruster
2014-07-18 16:22               ` John Snow
2014-07-21  7:38                 ` Markus Armbruster
2014-07-18 21:14               ` John Snow
2014-07-18 21:53                 ` Eric Blake
2014-07-21  7:48                 ` Markus Armbruster
2014-07-21 15:44                   ` John Snow
2014-07-21 17:33                     ` Markus Armbruster
2014-07-21 17:53                       ` John Snow
2014-07-21 19:15                         ` Markus Armbruster
2014-07-21 20:13                           ` John Snow
2014-07-21 20:31                             ` Eric Blake [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53CD7898.10503@redhat.com \
    --to=eblake@redhat.com \
    --cc=amit.shah@redhat.com \
    --cc=armbru@redhat.com \
    --cc=jsnow@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).