r176146 - Don't crash when diagnosing path-constrained protected
Aaron Ballman
aaron at aaronballman.com
Mon Nov 18 13:45:18 PST 2013
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 <rjmccall at apple.com> 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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
~Aaron
More information about the cfe-commits
mailing list