[Qemu-devel] test-qapi-visit causes clang -fsanitize=undefined warning

[Qemu-devel] test-qapi-visit causes clang -fsanitize=undefined warning

[Peter Maydell]

I've noticed that the tests/test-qapi-visit.c code provokes the following
complaint from clang's -fsanitize=undefined undefined-behaviour
checker when you run 'make check':

tests/test-qapi-visit.c:462:33: runtime error: member access within
null pointer of type 'UserDefA' (aka 'struct UserDefA')

which is the line
    visit_type_bool(m, &(*obj)->boolean, "boolean", &err);
in static void visit_type_UserDefA_fields(Visitor *m, UserDefA ** obj,
Error **errp).

It's presumably complaining because we've passed in an obj which
points to NULL (ie *obj == NULL). The callsite in visit_type_UserDefA()
checks for this and doesn't call the visit..fields function. The callsite
in visit_type_UserDefFlatUnion doesn't.

Unfortunately this is all autogenerated C so I'm not sure where exactly
the bug should be fixed. Could one of you have a look at it?

thanks
-- PMM

Re: [Qemu-devel] test-qapi-visit causes clang -fsanitize=undefined warning

[Peter Maydell]

On 8 March 2014 12:39, Peter Maydell <peter.maydell@linaro.org> wrote:
> I've noticed that the tests/test-qapi-visit.c code provokes the following
> complaint from clang's -fsanitize=undefined undefined-behaviour
> checker when you run 'make check':
>
> tests/test-qapi-visit.c:462:33: runtime error: member access within
> null pointer of type 'UserDefA' (aka 'struct UserDefA')

There's also this clang compile warning which is probably not
related but is also in code dealing with unions:

  CC    tests/test-qmp-output-visitor.o
/home/petmay01/linaro/qemu-for-merges/tests/test-qmp-output-visitor.c:452:17:
warning: implicit conversion from enumeration type
      'enum UserDefUnionKind' to different enumeration type
'UserDefFlatUnionKind' (aka 'enum UserDefFlatUnionKind')
      [-Wenum-conversion]
    tmp->kind = USER_DEF_UNION_KIND_A;
              ~ ^~~~~~~~~~~~~~~~~~~~~
1 warning generated.

> thanks
> -- PMM

Re: [Qemu-devel] test-qapi-visit causes clang -fsanitize=undefined warning

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 8 March 2014 12:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I've noticed that the tests/test-qapi-visit.c code provokes the following
>> complaint from clang's -fsanitize=undefined undefined-behaviour
>> checker when you run 'make check':
>>
>> tests/test-qapi-visit.c:462:33: runtime error: member access within
>> null pointer of type 'UserDefA' (aka 'struct UserDefA')
>
> There's also this clang compile warning which is probably not
> related but is also in code dealing with unions:
>
>   CC    tests/test-qmp-output-visitor.o
> /home/petmay01/linaro/qemu-for-merges/tests/test-qmp-output-visitor.c:452:17:
> warning: implicit conversion from enumeration type
>       'enum UserDefUnionKind' to different enumeration type
> 'UserDefFlatUnionKind' (aka 'enum UserDefFlatUnionKind')
>       [-Wenum-conversion]
>     tmp->kind = USER_DEF_UNION_KIND_A;
>               ~ ^~~~~~~~~~~~~~~~~~~~~
> 1 warning generated.

Thanks, I'll look into both.

Re: [Qemu-devel] test-qapi-visit causes clang -fsanitize=undefined warning

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> I've noticed that the tests/test-qapi-visit.c code provokes the following
> complaint from clang's -fsanitize=undefined undefined-behaviour
> checker when you run 'make check':
>
> tests/test-qapi-visit.c:462:33: runtime error: member access within
> null pointer of type 'UserDefA' (aka 'struct UserDefA')
>
> which is the line
>     visit_type_bool(m, &(*obj)->boolean, "boolean", &err);
> in static void visit_type_UserDefA_fields(Visitor *m, UserDefA ** obj,
> Error **errp).
>
> It's presumably complaining because we've passed in an obj which
> points to NULL (ie *obj == NULL). The callsite in visit_type_UserDefA()
> checks for this and doesn't call the visit..fields function. The callsite
> in visit_type_UserDefFlatUnion doesn't.
>
> Unfortunately this is all autogenerated C so I'm not sure where exactly
> the bug should be fixed. Could one of you have a look at it?

My local clang doesn't complain.  May I have your clang version, exact
invocation and output?

Re: [Qemu-devel] test-qapi-visit causes clang -fsanitize=undefined warning

[Peter Maydell]

On 10 March 2014 12:21, Markus Armbruster <armbru@redhat.com> wrote:
> My local clang doesn't complain.  May I have your clang version, exact
> invocation and output?

clang version 3.3 (tags/RELEASE_33/final)

mkdir build/clang
(cd build/clang && '../../configure' '--cc=clang' '--cxx=clang++'
'--enable-gtk' '--target-list=arm-softmmu'
'--extra-cflags=-fsanitize=undefined')
make -C build/clang -j4
cd build/clang
tests/test-qmp-input-strict

Output is:

/visitor/input-strict/pass/struct: OK
/visitor/input-strict/pass/struct-nested: OK
/visitor/input-strict/pass/list: OK
/visitor/input-strict/pass/union: OK
/visitor/input-strict/pass/union-flat: OK
/visitor/input-strict/pass/union-anon: OK
/visitor/input-strict/fail/struct: OK
/visitor/input-strict/fail/struct-nested: OK
/visitor/input-strict/fail/list: OK
/visitor/input-strict/fail/union: OK
/visitor/input-strict/fail/union-flat: tests/test-qapi-visit.c:462:33:
runtime error: member access within null pointer of type 'UserDefA'
(aka 'struct UserDefA')
OK
/visitor/input-strict/fail/union-anon: OK


thanks
-- PMM

Re: [Qemu-devel] test-qapi-visit causes clang -fsanitize=undefined warning

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 March 2014 12:21, Markus Armbruster <armbru@redhat.com> wrote:
>> My local clang doesn't complain.  May I have your clang version, exact
>> invocation and output?
>
> clang version 3.3 (tags/RELEASE_33/final)
>
> mkdir build/clang
> (cd build/clang && '../../configure' '--cc=clang' '--cxx=clang++'
> '--enable-gtk' '--target-list=arm-softmmu'
> '--extra-cflags=-fsanitize=undefined')
> make -C build/clang -j4
> cd build/clang
> tests/test-qmp-input-strict
>
> Output is:
>
> /visitor/input-strict/pass/struct: OK
> /visitor/input-strict/pass/struct-nested: OK
> /visitor/input-strict/pass/list: OK
> /visitor/input-strict/pass/union: OK
> /visitor/input-strict/pass/union-flat: OK
> /visitor/input-strict/pass/union-anon: OK
> /visitor/input-strict/fail/struct: OK
> /visitor/input-strict/fail/struct-nested: OK
> /visitor/input-strict/fail/list: OK
> /visitor/input-strict/fail/union: OK
> /visitor/input-strict/fail/union-flat: tests/test-qapi-visit.c:462:33:
> runtime error: member access within null pointer of type 'UserDefA'
> (aka 'struct UserDefA')
> OK
> /visitor/input-strict/fail/union-anon: OK

Turns out my clang installation doesn't support -fsanitize=undefined: it
lacks libclang_rt.san-x86_64.a.

Test works fine without -fsanitize=undefined.  I set a breakpoint on
visit_type_UserDefA_fields, and there's no null pointer to be found.

Looks like I have to upgrade clang before I can make progress.

Re: [Qemu-devel] test-qapi-visit causes clang -fsanitize=undefined warning

[Peter Maydell]

On 10 March 2014 13:36, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> Turns out my clang installation doesn't support -fsanitize=undefined: it
> lacks libclang_rt.san-x86_64.a.
>
> Test works fine without -fsanitize=undefined.  I set a breakpoint on
> visit_type_UserDefA_fields, and there's no null pointer to be found.

Yes, there is. This is gdb on a gcc-compiled version of
the test:

/visitor/input-strict/fail/union-flat:
Breakpoint 1, visit_type_UserDefA_fields (m=0x5555557a8ed0,
obj=0x5555557a8d88, errp=0x7fffffffdfb0)
    at tests/test-qapi-visit.c:460
460     {
(gdb) print *obj
$3 = (UserDefA *) 0x0
(gdb) bt
#0  visit_type_UserDefA_fields (m=0x5555557a8ed0, obj=0x5555557a8d88,
errp=0x7fffffffdfb0)
    at tests/test-qapi-visit.c:460
#1  0x000055555555b2a2 in visit_type_UserDefFlatUnion
(m=0x5555557a8ed0, obj=0x7fffffffdfd8, name=0x0, errp=0x0)
    at tests/test-qapi-visit.c:654
#2  0x0000555555558c0b in qapi_free_UserDefFlatUnion
(obj=0x5555557a8d80) at tests/test-qapi-types.c:368
#3  0x000055555555cc47 in test_validate_fail_union_flat
(data=0x5555557a8de0, unused=0x7fffffffe3f0)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/tests/test-qmp-input-strict.c:241

*obj is NULL, which is what clang is complaining about.

thanks
-- PMM

Re: [Qemu-devel] test-qapi-visit causes clang -fsanitize=undefined warning

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 March 2014 13:36, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> Turns out my clang installation doesn't support -fsanitize=undefined: it
>> lacks libclang_rt.san-x86_64.a.
>>
>> Test works fine without -fsanitize=undefined.  I set a breakpoint on
>> visit_type_UserDefA_fields, and there's no null pointer to be found.
>
> Yes, there is. This is gdb on a gcc-compiled version of
> the test:
>
> /visitor/input-strict/fail/union-flat:
> Breakpoint 1, visit_type_UserDefA_fields (m=0x5555557a8ed0,
> obj=0x5555557a8d88, errp=0x7fffffffdfb0)
>     at tests/test-qapi-visit.c:460
> 460     {
> (gdb) print *obj
> $3 = (UserDefA *) 0x0
> (gdb) bt
> #0  visit_type_UserDefA_fields (m=0x5555557a8ed0, obj=0x5555557a8d88,
> errp=0x7fffffffdfb0)
>     at tests/test-qapi-visit.c:460
> #1  0x000055555555b2a2 in visit_type_UserDefFlatUnion
> (m=0x5555557a8ed0, obj=0x7fffffffdfd8, name=0x0, errp=0x0)
>     at tests/test-qapi-visit.c:654
> #2  0x0000555555558c0b in qapi_free_UserDefFlatUnion
> (obj=0x5555557a8d80) at tests/test-qapi-types.c:368
> #3  0x000055555555cc47 in test_validate_fail_union_flat
> (data=0x5555557a8de0, unused=0x7fffffffe3f0)
>     at
> /home/petmay01/linaro/qemu-from-laptop/qemu/tests/test-qmp-input-strict.c:241
>
> *obj is NULL, which is what clang is complaining about.

I think what happens here is the visitor fails half-way through
constructing the type, and returns a UserDefFlatUnion with kind
USER_DEF_UNION_KIND_A and data.a null, violating the data type's
invariant.

We either have to relax the invariant, or fix the error paths.  I'm
looking into the latter.

Re: [Qemu-devel] test-qapi-visit causes clang -fsanitize=undefined warning

[Kevin Wolf]

Am 10.03.2014 um 19:21 hat Markus Armbruster geschrieben:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On 10 March 2014 13:36, Markus Armbruster <armbru@redhat.com> wrote:
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >> Turns out my clang installation doesn't support -fsanitize=undefined: it
> >> lacks libclang_rt.san-x86_64.a.
> >>
> >> Test works fine without -fsanitize=undefined.  I set a breakpoint on
> >> visit_type_UserDefA_fields, and there's no null pointer to be found.
> >
> > Yes, there is. This is gdb on a gcc-compiled version of
> > the test:
> >
> > /visitor/input-strict/fail/union-flat:
> > Breakpoint 1, visit_type_UserDefA_fields (m=0x5555557a8ed0,
> > obj=0x5555557a8d88, errp=0x7fffffffdfb0)
> >     at tests/test-qapi-visit.c:460
> > 460     {
> > (gdb) print *obj
> > $3 = (UserDefA *) 0x0
> > (gdb) bt
> > #0  visit_type_UserDefA_fields (m=0x5555557a8ed0, obj=0x5555557a8d88,
> > errp=0x7fffffffdfb0)
> >     at tests/test-qapi-visit.c:460
> > #1  0x000055555555b2a2 in visit_type_UserDefFlatUnion
> > (m=0x5555557a8ed0, obj=0x7fffffffdfd8, name=0x0, errp=0x0)
> >     at tests/test-qapi-visit.c:654
> > #2  0x0000555555558c0b in qapi_free_UserDefFlatUnion
> > (obj=0x5555557a8d80) at tests/test-qapi-types.c:368
> > #3  0x000055555555cc47 in test_validate_fail_union_flat
> > (data=0x5555557a8de0, unused=0x7fffffffe3f0)
> >     at
> > /home/petmay01/linaro/qemu-from-laptop/qemu/tests/test-qmp-input-strict.c:241
> >
> > *obj is NULL, which is what clang is complaining about.
> 
> I think what happens here is the visitor fails half-way through
> constructing the type, and returns a UserDefFlatUnion with kind
> USER_DEF_UNION_KIND_A and data.a null, violating the data type's
> invariant.

This is one of the cases that I found during my blockdev-add testing. I
wasn't sure whether introduing something like a KIND_UNDEFINED or adding
a new visitor callback like .struct_abort was the better way of fixing
this. And then I ran out of time.

Please keep me CCed when you send patches for this.

> We either have to relax the invariant, or fix the error paths.  I'm
> looking into the latter.

Yes, I think that's preferable.

Kevin