[Qemu-devel] qtest protocol: should memset/read/write etc of a size of 0 bytes be permitted?

[Qemu-devel] qtest protocol: should memset/read/write etc of a size of 0 bytes be permitted?

[Peter Maydell]

I've upgraded to a more recent version of clang, which now produces
undefined-behaviour warnings for passing NULL pointers to some library
functions. One of the things it has shown up is that some of the
qtest tests ask for "memset" with size zero. In our current implementation
this results in qtest.c calling g_malloc(0), which returns NULL, and
then calling memset(NULL, chr, 0), which is UB.

So should we:
(1) declare the qtest protocol commands 'memset', 'read', 'write'
etc which operate on a lump of guest memory of specified size to
support size == 0 as meaning "do nothing"
(2) declare that size == 0 is not valid and make it return a failure
code back down the qtest pipe (and fix the offending tests)

?

The offending tests are i386/ahci/flush/simple and i386/ahci/max
(because ahci_io() calls qmemset() with a zero size.)

thanks
-- PMM

Re: [Qemu-devel] qtest protocol: should memset/read/write etc of a size of 0 bytes be permitted?

[John Snow]

On 08/04/2016 02:46 PM, Peter Maydell wrote:
> I've upgraded to a more recent version of clang, which now produces
> undefined-behaviour warnings for passing NULL pointers to some library
> functions. One of the things it has shown up is that some of the
> qtest tests ask for "memset" with size zero. In our current implementation
> this results in qtest.c calling g_malloc(0), which returns NULL, and
> then calling memset(NULL, chr, 0), which is UB.
>
> So should we:
> (1) declare the qtest protocol commands 'memset', 'read', 'write'
> etc which operate on a lump of guest memory of specified size to
> support size == 0 as meaning "do nothing"

This would be easy to do.

> (2) declare that size == 0 is not valid and make it return a failure
> code back down the qtest pipe (and fix the offending tests)
>

This is probably the nicer thing to do -- if memset of length 0 is 
undefined, probably qmemset and friends should also be undefined by 
extension.

I reserve the right to change my mind depending on how gnarly it is to 
untangle.

I assume you're hoping for 2.7.

> ?
>
> The offending tests are i386/ahci/flush/simple and i386/ahci/max
> (because ahci_io() calls qmemset() with a zero size.)
>
> thanks
> -- PMM
>

--js

Re: [Qemu-devel] qtest protocol: should memset/read/write etc of a size of 0 bytes be permitted?

[Peter Maydell]

On 4 August 2016 at 19:49, John Snow <jsnow@redhat.com> wrote:
>
>
> On 08/04/2016 02:46 PM, Peter Maydell wrote:
>> (2) declare that size == 0 is not valid and make it return a failure
>> code back down the qtest pipe (and fix the offending tests)
>>
>
> This is probably the nicer thing to do -- if memset of length 0 is
> undefined, probably qmemset and friends should also be undefined by
> extension.

That was my thought.

> I reserve the right to change my mind depending on how gnarly it is to
> untangle.
>
> I assume you're hoping for 2.7.

I dunno. If the patch turns out easy we can put it in 2.7.
But it's only test code and it's been working fine for
a long time til we noticed it, so I don't think it matters
if we let it slide til 2.8.

thanks
-- PMM

Re: [Qemu-devel] qtest protocol: should memset/read/write etc of a size of 0 bytes be permitted?

[Eric Blake]

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

On 08/04/2016 12:46 PM, Peter Maydell wrote:
> I've upgraded to a more recent version of clang, which now produces
> undefined-behaviour warnings for passing NULL pointers to some library
> functions. One of the things it has shown up is that some of the
> qtest tests ask for "memset" with size zero. In our current implementation
> this results in qtest.c calling g_malloc(0), which returns NULL, and

I never understood why glib made that choice on g_malloc(0). I would
much prefer it to ALWAYS return something, just as glibc malloc(0) does.

> then calling memset(NULL, chr, 0), which is UB.

Indeed, although I really wish POSIX could be loosened to say that the
source pointer is untouched if the length is 0 (I've debated about
filing a POSIX bug report to that effect, but have not done so yet), so
that the UB only happens when passing NULL with a non-zero size.

> 
> So should we:
> (1) declare the qtest protocol commands 'memset', 'read', 'write'
> etc which operate on a lump of guest memory of specified size to
> support size == 0 as meaning "do nothing"

My preference - even if we have to special case things to avoid UB at
the lower level, presenting well-defined behavior at the upper level is
easier to think about.

> (2) declare that size == 0 is not valid and make it return a failure
> code back down the qtest pipe (and fix the offending tests)

Doable, but not as fun to audit, and not my preference.

-- 
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 --]

Re: [Qemu-devel] qtest protocol: should memset/read/write etc of a size of 0 bytes be permitted?

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 08/04/2016 12:46 PM, Peter Maydell wrote:
>> I've upgraded to a more recent version of clang, which now produces
>> undefined-behaviour warnings for passing NULL pointers to some library
>> functions. One of the things it has shown up is that some of the
>> qtest tests ask for "memset" with size zero. In our current implementation
>> this results in qtest.c calling g_malloc(0), which returns NULL, and
>
> I never understood why glib made that choice on g_malloc(0). I would
> much prefer it to ALWAYS return something, just as glibc malloc(0) does.

I guess it made the choice because unlike malloc(0), it can't cause
confusion.  malloc() returning null can mean either error or nothing.
The caller should pick nothing when it passed zero.  g_malloc()
returning null can only mean nothing.

>> then calling memset(NULL, chr, 0), which is UB.
>
> Indeed, although I really wish POSIX could be loosened to say that the
> source pointer is untouched if the length is 0 (I've debated about
> filing a POSIX bug report to that effect, but have not done so yet), so
> that the UB only happens when passing NULL with a non-zero size.

For me, this is one of those pointless UBs, whose only effect on the
real world is wasting programmer time.  Seriously, what else could a
sane memset(NULL, 0, 0) do?  What could anyone gain from an insane
memset() other than the glee of telling people "your program is wrong,
and you're <insert derogatory word>!".

The mission of this standard is to codify existing practice.  Show me a
real-world implementation of memset() that does something other than
nothing when passed a zero size, and whose maintainers don't consider
that a bug in need of fixing.

>> So should we:
>> (1) declare the qtest protocol commands 'memset', 'read', 'write'
>> etc which operate on a lump of guest memory of specified size to
>> support size == 0 as meaning "do nothing"
>
> My preference - even if we have to special case things to avoid UB at
> the lower level, presenting well-defined behavior at the upper level is
> easier to think about.
>
>> (2) declare that size == 0 is not valid and make it return a failure
>> code back down the qtest pipe (and fix the offending tests)
>
> Doable, but not as fun to audit, and not my preference.

Concur.  We shouldn't slavishly replicate an underlying interface's
complexities when we can just as well provide a more regular interface
instead.

Re: [Qemu-devel] qtest protocol: should memset/read/write etc of a size of 0 bytes be permitted?

[Peter Maydell]

On 5 August 2016 at 07:46, Markus Armbruster <armbru@redhat.com> wrote:
> For me, this is one of those pointless UBs, whose only effect on the
> real world is wasting programmer time.  Seriously, what else could a
> sane memset(NULL, 0, 0) do?

Since the function call is annotated in the system headers
to indicate that the first argument isn't NULL, it gives
the compiler more information which it can then use to
delete clearly-never-reachable "if (foo == NULL) { ... }"
codepaths for you :-)

thanks
-- PMM