[clang] Check null TypeSourceInfo in CreateUnaryExprOrTypeTraitExpr #112111

sookach · 2024-10-12T23:56:16Z

Fixes #111594. The crash is caused by the following call
https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ComputeDependence.cpp#L81-L82

We already check for a null TypeInfo when creating a UnaryExprOrTypeTraitExpr here
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExpr.cpp#L4616-L4617

but the following lines can, and in the case of the code in the issue, nullify the TypeInfo
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExpr.cpp#L4616-L4617

Thus, adding the additional check for nullptr prevents the erroneous memory access.

@shafik Thoughts? Thanks

llvmbot · 2024-10-12T23:56:49Z

@llvm/pr-subscribers-clang

Author: Andrew Sukach (sookach)

Changes

Fixes #111594. The crash is caused by the following call
https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ComputeDependence.cpp#L81-L82

We already check for a null TypeInfo when creating a UnaryExprOrTypeTraitExpr here
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExpr.cpp#L4616-L4617

but the following lines can, and in the case of the code in the issue, nullify the TypeInfo
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExpr.cpp#L4616-L4617

Thus, adding the additional check for nullptr prevents the erroneous memory access.

@shafik Thoughts? Thanks

Full diff: https://github.com/llvm/llvm-project/pull/112111.diff

1 Files Affected:

(modified) clang/lib/Sema/SemaExpr.cpp (+3)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 4e37385710af5e..b0bd216c5dc101 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -4629,6 +4629,9 @@ ExprResult Sema::CreateUnaryExprOrTypeTraitExpr(TypeSourceInfo *TInfo,
       TInfo->getType()->isVariablyModifiedType())
     TInfo = TransformToPotentiallyEvaluated(TInfo);
 
+  if (!TInfo)
+    return ExprError();
+
   // C99 6.5.3.4p4: the type (an unsigned integer type) is size_t.
   return new (Context) UnaryExprOrTypeTraitExpr(
       ExprKind, TInfo, Context.getSizeType(), OpLoc, R.getEnd());

Sirraide

The implementation lgtm, but this still needs a test and a release note.

clang/docs/ReleaseNotes.rst

Sirraide · 2024-10-15T14:54:17Z

clang/test/SemaCXX/unary-expr-or-type-trait-invalid.cpp

+// expected-error@3 {{a type specifier is required for all declarations}}
+// expected-error@3 {{use of undeclared identifier 'tree'; did you mean 'true'?}}
+// expected-error@3 {{member reference type 'bool' is not a pointer}}
+// expected-error@3 {{expected ';' after expression}}
+// expected-error@3 {{use of undeclared identifier 'next'; did you mean 'new'?}}
+// expected-error@3 {{expected expression}}
+// expected-error@3 {{expected ';' after expression}}
+// expected-error@26 {{expected '}'}}
+// expected-note@3 {{to match this '{'}}
+// expected-error@26 {{expected ')'}}
+// expected-note@3 {{to match this '('}}
+// expected-error@26 {{expected ']'}}
+// expected-note@3 {{to match this '['}}
+// expected-error@26 {{expected ')'}}
+// expected-note@3 {{to match this '('}}
+// expected-error@3 {{using declaration 'exp' instantiates to an empty pack}}
+// expected-error@3 {{variable has incomplete type 'struct b'}}
+// expected-note@3 {{forward declaration of 'b'}}
+// expected-error@3 {{expected ';' at end of declaration}}
+// expected-error@26 {{expected '}'}}
+// expected-note@3 {{to match this '{'}}
+// expected-warning@3 {{expression result unused}}


CC @Endilll @erichkeane Do we have a cleaner way of writing fuzzer tests? Because this is rather atrocious to look at.

OOof, that is awful. Is that really the minimal example that causes the crash? I don't really have a great idea. I might disable the warning with a -Wno flag, but most of those are not actually important to what we want to do here. The whole point is to ensure it doesn't crash/assert, not to make sure we emit all those diagnostics.

We should come up with a way to test "did not crash" instead of this.

Instead of running -verify on the test, can we just run FileCheck and ensure there are no strings like PLEASE submit a bug report or something?

Not really, not all of the test configurations use the normal crash handler... I would hope the author could use the test as an example to come up with a minimal example that does the bare minimum to cause this issue rather than using the fuzz itself: I don't think the fuzz generated code should actually be in our test suite, just the actual use case that is required to get into this situation.

Definitely agree that the test file is pretty ugly. I tried to remove some parts of the original code but none of my reductions kept the crash, so that's why I left it in full. I think ideally there would just be a way to make sure that there is no crash without checking the specific diagnostics, but it seems that option isn't available.

See #78898 for the previous discussion on the matter of tests based on crash reports.

In most scenarios, we really do want to use -verify to see what the diagnostic behavior is because we want it to be very obvious when diagnostic behavior needs to be improved. (Similar for -emit-llvm when doing codegen tests.) However, we only care about the behavior of code that a real user is likely to write, which is not typical for fuzzer-generated crashing issues. I think it would be useful to have a special -verify mode that ignores all diagnostics (whether any are generated or not) and only tests that there was not a crash. Something like // expected-no-crash and we diagnose if any other // expected-whatever comments appear anywhere in the file. That should make it more clear "we're not interested in behavior beyond not crashing" for anyone reading the test.

// expected-no-crash would be a good name for the directive, but I'm not sure that frontend is a good place for such check layering-wise.

Yeah, it seems like more of a lit layer thing than a frontend layer thing.

So long as it's obvious that the intent of the test is "test this doesn't crash", I'm not too hung up on the syntax.

At this point it seems like this is a separate issue, maybe we should open an issue for this and we can move the discussion there?

…raitExpr

shafik

The fix looks good to me but I guess everyone needs to be happy w/ the test first.

shafik · 2025-01-28T21:32:19Z

ping, any chance we can make progress here?

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 12, 2024

Sirraide reviewed Oct 14, 2024

View reviewed changes

sookach force-pushed the issue-111594 branch 2 times, most recently from f502117 to d7a37cb Compare October 14, 2024 21:42

sookach requested a review from Sirraide October 14, 2024 21:49

Sirraide reviewed Oct 15, 2024

View reviewed changes

sookach force-pushed the issue-111594 branch 4 times, most recently from a478b5e to 384e4eb Compare October 15, 2024 18:20

[clang] Check for null TypeSourceInfo in Sema::CreateUnaryExprOrTypeT…

d3b9b6e

…raitExpr

sookach force-pushed the issue-111594 branch from 384e4eb to d3b9b6e Compare October 15, 2024 18:21

sookach requested a review from Sirraide October 15, 2024 18:22

shafik reviewed Oct 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] Check null TypeSourceInfo in CreateUnaryExprOrTypeTraitExpr #112111

[clang] Check null TypeSourceInfo in CreateUnaryExprOrTypeTraitExpr #112111

Uh oh!

sookach commented Oct 12, 2024

Uh oh!

llvmbot commented Oct 12, 2024

Uh oh!

Sirraide left a comment

Uh oh!

Uh oh!

Sirraide Oct 15, 2024

Uh oh!

erichkeane Oct 15, 2024

Uh oh!

zyn0217 Oct 15, 2024

Uh oh!

erichkeane Oct 15, 2024

Uh oh!

sookach Oct 15, 2024

Uh oh!

Endilll Oct 15, 2024

Uh oh!

AaronBallman Oct 17, 2024

Uh oh!

Endilll Oct 17, 2024

Uh oh!

AaronBallman Oct 17, 2024

Uh oh!

shafik Oct 30, 2024

Uh oh!

shafik left a comment

Uh oh!

shafik commented Jan 28, 2025

Uh oh!

Uh oh!

[clang] Check null TypeSourceInfo in CreateUnaryExprOrTypeTraitExpr #112111

Are you sure you want to change the base?

[clang] Check null TypeSourceInfo in CreateUnaryExprOrTypeTraitExpr #112111

Uh oh!

Conversation

sookach commented Oct 12, 2024

Uh oh!

llvmbot commented Oct 12, 2024

Uh oh!

Sirraide left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

shafik commented Jan 28, 2025

Uh oh!

Uh oh!