Page MenuHomePhabricator

[clang] Don't segfault on incorrect using directive (PR41400)
ClosedPublic

Authored by Tyker on Apr 10 2019, 11:22 AM.

Diff Detail

Event Timeline

Tyker created this revision.Apr 10 2019, 11:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2019, 11:22 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Tyker added a comment.Apr 20 2019, 4:32 AM

awaiting feedback on this

lebedev.ri retitled this revision from [clang] Bugfixe for 41400 to [clang] Don't segfault on incorrect using directive (PR41400).Apr 20 2019, 4:48 AM
lebedev.ri edited the summary of this revision. (Show Details)

I will take a look at this tomorrow, I know that it is annoying to get no feedback!

riccibruno resigned from this revision.Apr 23 2019, 2:18 PM

I will take a look at this tomorrow, I know that it is annoying to get no feedback!

Sorry, I don't think I can judge whether this is the correct solution. I don't know if returning a type for the injected-class-name is the correct thing to do when there is no nested-name-specifier.

I can't really comment on correctness of your fix but had been willing to do the work I'd suggest making ASTContext::getDependentNameType and DependentNameType::DependentNameType interface more robust.

With current master (95c18c7beec) the crash is here (with your test):

thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)

  frame #0: 0x0000000105f0c2e0 clang` clang::NestedNameSpecifier::getKind(this=0x0000000000000000) const  + 16 at NestedNameSpecifier.cpp:143
  frame #1: 0x0000000105f0c659 clang` clang::NestedNameSpecifier::containsUnexpandedParameterPack(this=0x0000000000000000) const  + 25 at NestedNameSpecifier.cpp:253
* frame #2: 0x00000001059f2dc1 clang` clang::DependentNameType::DependentNameType(this=0x0000000116852b30, Keyword=ETK_None, NNS=0x0000000000000000, Name=0x000000011686eff8, CanonType=QualType @ 0x00007ffeefbf7818)  + 65 at Type.h:5238
  frame #3: 0x00000001059c1113 clang` clang::DependentNameType::DependentNameType(this=0x0000000116852b30, Keyword=ETK_None, NNS=0x0000000000000000, Name=0x000000011686eff8, CanonType=QualType @ 0x00007ffeefbf7858)  + 51 at Type.h:5239
  frame #4: 0x00000001059c0d9f clang` clang::ASTContext::getDependentNameType(this=0x0000000116815000, Keyword=ETK_None, NNS=0x0000000000000000, Name=0x000000011686eff8, Canon=QualType @ 0x00007ffeefbf7990) const  + 431 at ASTContext.cpp:4247
  frame #5: 0x000000010509f539 clang` clang::Sema::getConstructorName(this=0x0000000116846600, II=0x000000011686eff8, NameLoc=(ID = 35), S=0x000000011530bc40, SS=0x00007ffeefbf8730, EnteringContext=false)  + 377 at SemaExprCXX.cpp:94

The problem starts in frame #5 in getConstructorName where your fix is but

  • DependentNameType should also have a tighter interface and not accept NSS = nullptr in constructor since it obviously expects some value to be present (calling a method on it) - the parameter should be a reference.
  • ASTContext::getDependentNameType also seems to expect some non-null value - it should either make this obvious in the interface by using a reference or deal with absence of the value in a reasonable manner.
Tyker added a subscriber: jkooker.EditedApr 24 2019, 1:11 PM

@jkooker
I don't think it is possible for ASTContext::getDependentNameType to deal with NSS = nullptr except by reporting the error. we probably don't want to just report the error because the error could have been handled before invoking ASTContext::getDependentNameType. I agree that it should optimally be reflected in the interface of those functions because pointer are often used a optional references. but wouldn't an assert + doxygen comment suffice ? instead of changing the interface, implementation and all callsites. Pointers as argument that are required not to be null are used in a lot of places in LLVM. For example DependentNameType::DependentNameType takes NNS as a pointer even thought it requires it to be non-null(calls a method on it). Mixing pointers and references adds a lot of & and * which makes the code harder to read.

rsmith accepted this revision.Jun 7 2019, 10:48 AM
This revision is now accepted and ready to land.Jun 7 2019, 10:48 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2019, 1:25 AM