This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Fix a null pointer reference crash.
ClosedPublic

Authored by hokein on Sep 23 2021, 2:02 AM.

Diff Detail

Event Timeline

hokein requested review of this revision.Sep 23 2021, 2:02 AM
hokein created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2021, 2:02 AM
hokein added inline comments.Sep 23 2021, 2:06 AM
clang/lib/Sema/SemaDecl.cpp
14496

I have a reproduce test case, and wait for the creduce to minimize it (will add it once creduce finishes)

I think the bug is obvious, by reading the code on the line 14495, FD could be a nullptr.

sammccall accepted this revision.Sep 23 2021, 6:00 AM
sammccall added a subscriber: mibintc.

LG because not crashing here is surely better than crashing, but I'm not sure whether the behaviour is actually right.
If we can't be sure, maybe leave a comment?

clang/lib/Sema/SemaDecl.cpp
14496

Yes, the bug is clear.
It's not obvious to me that the fix is right, because I don't know:

  • when dcl can be null
  • when/if dcl can be non-null but neither a function nor function template
  • how the FP attr should apply in such cases

cc @mibintc who may know the answer to at least the 3rd question, and I guess your testcase will give an example of either 1 or 2.

I'm *fairly* sure that not applying the strictfpattr is better than crashing here though.

This revision is now accepted and ready to land.Sep 23 2021, 6:00 AM
hokein updated this revision to Diff 374538.Sep 23 2021, 7:21 AM

upload a minimized testcase.

hokein added inline comments.Sep 23 2021, 7:31 AM
clang/lib/Sema/SemaDecl.cpp
14496

my understanding is that dcl is null if clang fails to to parse the function declarator in some way, but still can recover to parse the function body.
The testcase shows it happens for a broken function template. I think it is fine to not apply attr, since the ast node is broken and likely discarded afterwards

I'm going to land this patch now as this issue blocks one of our internal tools, @mibintc feel free to polish it.

This revision was landed with ongoing or failed builds.Sep 23 2021, 7:37 AM
This revision was automatically updated to reflect the committed changes.