Sorry to resurrect this commit, but I happened to be poking around in this code and noticed a potential null pointer dereference in it. Some comments mixed in with the patch below...
On Tue, Feb 26, 2013 at 7:08 PM, John McCall <[email protected]> wrote: > Author: rjmccall > Date: Tue Feb 26 18:08:19 2013 > New Revision: 176146 > > URL: http://llvm.org/viewvc/llvm-project?rev=176146&view=rev > Log: > Don't crash when diagnosing path-constrained protected > access to a private member to which we have special access. > > rdar://12926092 > > Modified: > cfe/trunk/lib/Sema/SemaAccess.cpp > cfe/trunk/test/CXX/class.access/class.access.base/p5.cpp > > Modified: cfe/trunk/lib/Sema/SemaAccess.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAccess.cpp?rev=176146&r1=176145&r2=176146&view=diff > ============================================================================== > --- cfe/trunk/lib/Sema/SemaAccess.cpp (original) > +++ cfe/trunk/lib/Sema/SemaAccess.cpp Tue Feb 26 18:08:19 2013 > @@ -217,6 +217,15 @@ struct AccessTarget : public AccessedEnt > return DeclaringClass; > } > > + /// The "effective" naming class is the canonical non-anonymous > + /// class containing the actual naming class. > + const CXXRecordDecl *getEffectiveNamingClass() const { > + const CXXRecordDecl *namingClass = getNamingClass(); > + while (namingClass->isAnonymousStructOrUnion()) > + namingClass = cast<CXXRecordDecl>(namingClass->getParent()); > + return namingClass->getCanonicalDecl(); > + } > + > private: > void initialize() { > HasInstanceContext = (isMemberAccess() && > @@ -1023,8 +1032,7 @@ static bool TryDiagnoseProtectedAccess(S > > assert(Target.isMemberAccess()); > > - const CXXRecordDecl *NamingClass = Target.getNamingClass(); > - NamingClass = NamingClass->getCanonicalDecl(); > + const CXXRecordDecl *NamingClass = Target.getEffectiveNamingClass(); > > for (EffectiveContext::record_iterator > I = EC.Records.begin(), E = EC.Records.end(); I != E; ++I) { > @@ -1089,129 +1097,173 @@ static bool TryDiagnoseProtectedAccess(S > return false; > } > > -/// Diagnose the path which caused the given declaration or base class > -/// to become inaccessible. > -static void DiagnoseAccessPath(Sema &S, > - const EffectiveContext &EC, > - AccessTarget &Entity) { > - AccessSpecifier Access = Entity.getAccess(); > +/// We are unable to access a given declaration due to its direct > +/// access control; diagnose that. > +static void diagnoseBadDirectAccess(Sema &S, > + const EffectiveContext &EC, > + AccessTarget &entity) { > + assert(entity.isMemberAccess()); > + NamedDecl *D = entity.getTargetDecl(); > + > + if (D->getAccess() == AS_protected && > + TryDiagnoseProtectedAccess(S, EC, entity)) > + return; > + > + // Find an original declaration. > + while (D->isOutOfLine()) { > + NamedDecl *PrevDecl = 0; > + if (VarDecl *VD = dyn_cast<VarDecl>(D)) > + PrevDecl = VD->getPreviousDecl(); > + else if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) > + PrevDecl = FD->getPreviousDecl(); > + else if (TypedefNameDecl *TND = dyn_cast<TypedefNameDecl>(D)) > + PrevDecl = TND->getPreviousDecl(); > + else if (TagDecl *TD = dyn_cast<TagDecl>(D)) { > + if (isa<RecordDecl>(D) && cast<RecordDecl>(D)->isInjectedClassName()) > + break; > + PrevDecl = TD->getPreviousDecl(); > + } > + if (!PrevDecl) break; > + D = PrevDecl; > + } > > - NamedDecl *D = (Entity.isMemberAccess() ? Entity.getTargetDecl() : 0); > - const CXXRecordDecl *DeclaringClass = Entity.getDeclaringClass(); > + CXXRecordDecl *DeclaringClass = FindDeclaringClass(D); > + Decl *ImmediateChild; > + if (D->getDeclContext() == DeclaringClass) > + ImmediateChild = D; > + else { > + DeclContext *DC = D->getDeclContext(); > + while (DC->getParent() != DeclaringClass) > + DC = DC->getParent(); > + ImmediateChild = cast<Decl>(DC); > + } > > - // Easy case: the decl's natural access determined its path access. > - // We have to check against AS_private here in case Access is AS_none, > - // indicating a non-public member of a private base class. > - if (D && (Access == D->getAccess() || D->getAccess() == AS_private)) { > - switch (HasAccess(S, EC, DeclaringClass, D->getAccess(), Entity)) { > - case AR_inaccessible: { > - if (Access == AS_protected && > - TryDiagnoseProtectedAccess(S, EC, Entity)) > - return; > - > - // Find an original declaration. > - while (D->isOutOfLine()) { > - NamedDecl *PrevDecl = 0; > - if (VarDecl *VD = dyn_cast<VarDecl>(D)) > - PrevDecl = VD->getPreviousDecl(); > - else if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) > - PrevDecl = FD->getPreviousDecl(); > - else if (TypedefNameDecl *TND = dyn_cast<TypedefNameDecl>(D)) > - PrevDecl = TND->getPreviousDecl(); > - else if (TagDecl *TD = dyn_cast<TagDecl>(D)) { > - if (isa<RecordDecl>(D) && > cast<RecordDecl>(D)->isInjectedClassName()) > - break; > - PrevDecl = TD->getPreviousDecl(); > - } > - if (!PrevDecl) break; > - D = PrevDecl; > - } > + // Check whether there's an AccessSpecDecl preceding this in the > + // chain of the DeclContext. > + bool isImplicit = true; > + for (CXXRecordDecl::decl_iterator > + I = DeclaringClass->decls_begin(), E = DeclaringClass->decls_end(); > + I != E; ++I) { > + if (*I == ImmediateChild) break; > + if (isa<AccessSpecDecl>(*I)) { > + isImplicit = false; > + break; > + } > + } > > - CXXRecordDecl *DeclaringClass = FindDeclaringClass(D); > - Decl *ImmediateChild; > - if (D->getDeclContext() == DeclaringClass) > - ImmediateChild = D; > - else { > - DeclContext *DC = D->getDeclContext(); > - while (DC->getParent() != DeclaringClass) > - DC = DC->getParent(); > - ImmediateChild = cast<Decl>(DC); > - } > - > - // Check whether there's an AccessSpecDecl preceding this in the > - // chain of the DeclContext. > - bool Implicit = true; > - for (CXXRecordDecl::decl_iterator > - I = DeclaringClass->decls_begin(), E = > DeclaringClass->decls_end(); > - I != E; ++I) { > - if (*I == ImmediateChild) break; > - if (isa<AccessSpecDecl>(*I)) { > - Implicit = false; > - break; > - } > - } > + S.Diag(D->getLocation(), diag::note_access_natural) > + << (unsigned) (D->getAccess() == AS_protected) > + << isImplicit; > +} > > - S.Diag(D->getLocation(), diag::note_access_natural) > - << (unsigned) (Access == AS_protected) > - << Implicit; > - return; > - } > +/// Diagnose the path which caused the given declaration or base class > +/// to become inaccessible. > +static void DiagnoseAccessPath(Sema &S, > + const EffectiveContext &EC, > + AccessTarget &entity) { > + // Save the instance context to preserve invariants. > + AccessTarget::SavedInstanceContext _ = entity.saveInstanceContext(); > + > + // This basically repeats the main algorithm but keeps some more > + // information. > + > + // The natural access so far. > + AccessSpecifier accessSoFar = AS_public; > + > + // Check whether we have special rights to the declaring class. > + if (entity.isMemberAccess()) { > + NamedDecl *D = entity.getTargetDecl(); > + accessSoFar = D->getAccess(); > + const CXXRecordDecl *declaringClass = entity.getDeclaringClass(); > + > + switch (HasAccess(S, EC, declaringClass, accessSoFar, entity)) { > + // If the declaration is accessible when named in its declaring > + // class, then we must be constrained by the path. > + case AR_accessible: > + accessSoFar = AS_public; > + entity.suppressInstanceContext(); > + break; > > - case AR_accessible: break; > + case AR_inaccessible: > + if (accessSoFar == AS_private || > + declaringClass == entity.getEffectiveNamingClass()) > + return diagnoseBadDirectAccess(S, EC, entity); > + break; > > case AR_dependent: > - llvm_unreachable("can't diagnose dependent access failures"); > + llvm_unreachable("cannot diagnose dependent access"); > } > } > > - CXXBasePaths Paths; > - CXXBasePath &Path = *FindBestPath(S, EC, Entity, AS_public, Paths); > - > - CXXBasePath::iterator I = Path.end(), E = Path.begin(); > - while (I != E) { > - --I; > - > - const CXXBaseSpecifier *BS = I->Base; > - AccessSpecifier BaseAccess = BS->getAccessSpecifier(); > - > - // If this is public inheritance, or the derived class is a friend, > - // skip this step. > - if (BaseAccess == AS_public) > - continue; > + CXXBasePaths paths; > + CXXBasePath &path = *FindBestPath(S, EC, entity, accessSoFar, paths); > + assert(path.Access != AS_public); > + > + CXXBasePath::iterator i = path.end(), e = path.begin(); > + CXXBasePath::iterator constrainingBase = i; > + while (i != e) { > + --i; > + > + assert(accessSoFar != AS_none && accessSoFar != AS_private); > + > + // Is the entity accessible when named in the deriving class, as > + // modified by the base specifier? > + const CXXRecordDecl *derivingClass = i->Class->getCanonicalDecl(); > + const CXXBaseSpecifier *base = i->Base; > + > + // If the access to this base is worse than the access we have to > + // the declaration, remember it. > + AccessSpecifier baseAccess = base->getAccessSpecifier(); > + if (baseAccess > accessSoFar) { > + constrainingBase = i; > + accessSoFar = baseAccess; > + } > > - switch (GetFriendKind(S, EC, I->Class)) { > - case AR_accessible: continue; > + switch (HasAccess(S, EC, derivingClass, accessSoFar, entity)) { > case AR_inaccessible: break; > + case AR_accessible: > + accessSoFar = AS_public; > + entity.suppressInstanceContext(); > + constrainingBase = 0; Shouldn't this be constrainingBase = path.end(); ? > + break; > case AR_dependent: > - llvm_unreachable("can't diagnose dependent access failures"); > + llvm_unreachable("cannot diagnose dependent access"); > } > > - // Check whether this base specifier is the tighest point > - // constraining access. We have to check against AS_private for > - // the same reasons as above. > - if (BaseAccess == AS_private || BaseAccess >= Access) { > - > - // We're constrained by inheritance, but we want to say > - // "declared private here" if we're diagnosing a hierarchy > - // conversion and this is the final step. > - unsigned diagnostic; > - if (D) diagnostic = diag::note_access_constrained_by_path; > - else if (I + 1 == Path.end()) diagnostic = diag::note_access_natural; > - else diagnostic = diag::note_access_constrained_by_path; > - > - S.Diag(BS->getSourceRange().getBegin(), diagnostic) > - << BS->getSourceRange() > - << (BaseAccess == AS_protected) > - << (BS->getAccessSpecifierAsWritten() == AS_none); > - > - if (D) > - S.Diag(D->getLocation(), diag::note_field_decl); > - > - return; > + // If this was private inheritance, but we don't have access to > + // the deriving class, we're done. > + if (accessSoFar == AS_private) { > + assert(baseAccess == AS_private); > + assert(constrainingBase == i); > + break; > } > } > > - llvm_unreachable("access not apparently constrained by path"); > + // If we don't have a constraining base, the access failure must be > + // due to the original declaration. > + if (constrainingBase == path.end()) Because this test is not checking for null. > + return diagnoseBadDirectAccess(S, EC, entity); > + > + // We're constrained by inheritance, but we want to say > + // "declared private here" if we're diagnosing a hierarchy > + // conversion and this is the final step. > + unsigned diagnostic; > + if (entity.isMemberAccess() || > + constrainingBase + 1 != path.end()) { > + diagnostic = diag::note_access_constrained_by_path; > + } else { > + diagnostic = diag::note_access_natural; > + } > + > + const CXXBaseSpecifier *base = constrainingBase->Base; But we are using constrainingBase here after it has potentially been null'ed out. > + > + S.Diag(base->getSourceRange().getBegin(), diagnostic) > + << base->getSourceRange() > + << (base->getAccessSpecifier() == AS_protected) > + << (base->getAccessSpecifierAsWritten() == AS_none); > + > + if (entity.isMemberAccess()) > + S.Diag(entity.getTargetDecl()->getLocation(), diag::note_field_decl); > } > > static void DiagnoseBadAccess(Sema &S, SourceLocation Loc, > @@ -1273,10 +1325,7 @@ static AccessResult IsAccessible(Sema &S > const EffectiveContext &EC, > AccessTarget &Entity) { > // Determine the actual naming class. > - CXXRecordDecl *NamingClass = Entity.getNamingClass(); > - while (NamingClass->isAnonymousStructOrUnion()) > - NamingClass = cast<CXXRecordDecl>(NamingClass->getParent()); > - NamingClass = NamingClass->getCanonicalDecl(); > + const CXXRecordDecl *NamingClass = Entity.getEffectiveNamingClass(); > > AccessSpecifier UnprivilegedAccess = Entity.getAccess(); > assert(UnprivilegedAccess != AS_public && "public access not weeded out"); > > Modified: cfe/trunk/test/CXX/class.access/class.access.base/p5.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/class.access/class.access.base/p5.cpp?rev=176146&r1=176145&r2=176146&view=diff > ============================================================================== > --- cfe/trunk/test/CXX/class.access/class.access.base/p5.cpp (original) > +++ cfe/trunk/test/CXX/class.access/class.access.base/p5.cpp Tue Feb 26 > 18:08:19 2013 > @@ -72,4 +72,27 @@ namespace test3 { > }; > } > > +// Don't crash. <rdar://12926092> > +// Note that 'field' is indeed a private member of X but that access > +// is indeed ultimately constrained by the protected inheritance from Y. > +// If someone wants to put the effort into improving this diagnostic, > +// they can feel free; even explaining it in person would be a pain. > +namespace test4 { > + class Z; > + class X { > + public: > + void f(Z *p); > + > + private: > + int field; // expected-note {{member is declared here}} > + }; > + > + class Y : public X { }; > + class Z : protected Y { }; // expected-note 2 {{constrained by protected > inheritance here}} > + > + void X::f(Z *p) { > + p->field = 0; // expected-error {{cannot cast 'test4::Z' to its > protected base class 'test4::X'}} expected-error {{'field' is a private > member of 'test4::X'}} > + } > +} > + > // TODO: flesh out these cases > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
