Page MenuHomePhabricator

[clang] Fix a crash on passing C structure of incompatible type to function with reference parameter
ClosedPublic

Authored by ArcsinX on Mon, Jun 29, 2:10 PM.

Details

Summary

__builtin_va_*() and __builtin_ms_va_*() declared as functions with reference parameters.

This patch fix clang crashes at using these functions in C and passing incompatible structures as parameters in case of __builtin_va_list/__builtin_ms_va_list are structures.

Diff Detail

Event Timeline

ArcsinX created this revision.Mon, Jun 29, 2:10 PM
ArcsinX edited the summary of this revision. (Show Details)Mon, Jun 29, 2:11 PM
riccibruno added inline comments.
clang/lib/Sema/SemaInit.cpp
4562

I cannot reproduce this with the provided test case. Can you explain why T2RecordDecl would not be a CXXRecordDecl since we should only get there with c++?

ArcsinX updated this revision to Diff 274250.Mon, Jun 29, 2:56 PM

Fix format

ArcsinX marked an inline comment as done.Mon, Jun 29, 3:18 PM
ArcsinX added inline comments.
clang/lib/Sema/SemaInit.cpp
4562

You can reproduce it with

bin/clang -xc --target=arm-unknown-gnu -c ../clang/test/Sema/init-ref-c.c

We can get there with C in case of using builtins declared as functions with reference parameters (e.g. __builtin_va_end())

ArcsinX updated this revision to Diff 274338.Mon, Jun 29, 10:49 PM
riccibruno added inline comments.Tue, Jun 30, 6:38 AM
clang/lib/Sema/SemaInit.cpp
4562

Okay I see it now. Thank you.

I think it would be better to only call TryRefInitWithConversionFunction in C++ mode since conversion functions don't exist in C (only the first call on line 4773).

4707

Can you add a comment here explaining that we can get there in C with some builtins?

4774

Here

clang/test/Sema/init-ref-c.c
8

I think it would be better to name this test varargs-arm.c since this bug can only happen with the varargs-related builtins (__builtin_addressof takes a reference but has custom type-checking).

Additional test case:

const __builtin_va_list cva;
__builtin_va_end(cva); // expected-error {{binding reference of type '__builtin_va_list' to value of type 'const __builtin_va_list' drops 'const' qualifier}}
ArcsinX marked an inline comment as done.Tue, Jun 30, 7:38 AM
ArcsinX added inline comments.
clang/test/Sema/init-ref-c.c
8

It's not only about ARM, its about __builtin_va_list type which depends on architecture. E.g. on MIPS it also crashes.

Also it's not about __builtin_va_*() and __builtin_ms_va_*(), its about builtins with reference parameters, when we pass incompatible C structures to these builtin calls. May be there could be other builtins with reference parameters in the future.

We could not pass incompatible structure to __builtin_addressof() that is the reason why we could not reproduce the problem with this builtin.

riccibruno added inline comments.Tue, Jun 30, 8:27 AM
clang/test/Sema/init-ref-c.c
8

It's not only about ARM, its about __builtin_va_list type which depends on architecture.
E.g. on MIPS it also crashes.

Sure.

Also it's not about __builtin_va_*() and __builtin_ms_va_*(), its about built-ins with
reference parameters, when we pass incompatible C structures to these builtin calls.
May be there could be other built-ins with reference parameters in the future.

Right, but currently only these built-ins and __builtin_addressof() have a parameter of reference type, and as you said __builtin_addressof() doesn't have this problem.

That said it's not a big deal so I think it's fine if you want to leave it here.

ArcsinX updated this revision to Diff 274789.Wed, Jul 1, 6:42 AM

Try conversion function only in C++.

ArcsinX marked 3 inline comments as done.Wed, Jul 1, 6:46 AM
ArcsinX added inline comments.
clang/lib/Sema/SemaInit.cpp
4562

Agree. Fixed.

4707

Added extra description for TryReferenceInitializationCore() with explanation that we can get here in C.

4774

Done.

riccibruno accepted this revision.EditedWed, Jul 8, 8:26 AM

This LGTM, with a few wording nits.

This patch fix clang crashes at using these functions in C and passing incompatible structures as parameters in case of builtin_va_list/builtin_ms_va_list are structures.

This patch fixes a crash when using these functions in C where an argument of structure type is incompatible with the parameter type.

Do you want me to commit it for you? If so I need a name and an email for the attribution.

clang/lib/Sema/SemaInit.cpp
4698

s/call builtin/call a builtin/
s/with reference arguments/with a parameter of reference type/

The second wording change is important since "argument" != "parameter".

This revision is now accepted and ready to land.Wed, Jul 8, 8:26 AM

Do you want me to commit it for you?

Yes, thank you.

If so I need a name and an email for the attribution.

Aleksandr Platonov <platonov.aleksandr@huawei.com>

This revision was automatically updated to reflect the committed changes.