Why Can't I Access A Protected Member From A Derived Class? Part Six

Why Can't I Access A Protected Member From A Derived Class? Part Six

Rate This
  • Comments 22

Reader Jesse McGrew asks an excellent follow-up question to my 2005 post about why you cannot access a protected member from a derived class. (You probably want to re-read that post in order to make sense of this one.)

I want to be clear in my terminology, so I’m going to define some terms. Suppose we have a call foo.Bar() inside class C. The value of foo is the “receiver” of the call. The compile-time type of foo is the “compile time type of the receiver”. The “runtime type of the receiver” could potentially be more derived. The “call site type” is C.

The rule in C# is that if Bar is protected, then the compile-time type of the receiver must be the call site type, or a type derived from the call site type.  It cannot be a base type of the call site type, because then the runtime type of the receiver might have a “cousin” (or sibling, or uncle… but let’s not split hairs, let’s just call them all cousins) relationship to the call site type, rather than an “ancestor/descendant” relationship.

Jesse quite rightly points out that my original answer to the question was not really complete. There are two questions unanswered:

1) Why would it be a bad thing to allow calling a protected method from a receiver whose runtime type is a “cousin” class?

2) Supposing for the sake of argument that there is a good answer to (1) – why doesn’t that argument apply equally well to calling a protected method via a receiver whose compile-time type is the same as the call site type? The runtime type of the receiver still could be more derived.

I’ll answer the first question first. In this answer I’m going to use a humorous and exaggerated example to illustrate the problem; I want to emphasize that this is not how you should actually design applications that need “big S” Security, like bank accounts. What I want to illustrate here is simply that allowing protected methods to be called via “cousins” makes it difficult to maintain invariants and therefore difficult to write correct, robust code.

// Good.dll:

    public abstract class BankAccount
    {
      abstract protected void DoTransfer(
        BankAccount destinationAccount, 
        User authorizedUser,
        decimal amount);
    }
    public abstract class SecureBankAccount : BankAccount
    {
      protected readonly int accountNumber;
      public SecureBankAccount(int accountNumber)
      {
        this.accountNumber = accountNumber;
      }
      public void Transfer(
        BankAccount destinationAccount, 
        User authorizedUser,
        decimal amount)
      {
        if (!Authorized(user, accountNumber)) throw something;
        this.DoTransfer(destinationAccount, user, amount);
      }
    }
    public sealed class SwissBankAccount : SecureBankAccount
    {
      public SwissBankAccount(int accountNumber) : base(accountNumber) {}
      override protected void DoTransfer(
        BankAccount destinationAccount, 
        User authorizedUser,
        decimal amount)
      {
        // Code to transfer money from a Swiss bank account here.
        // This code can assume that authorizedUser is authorized.
        // We are guaranteed this because SwissBankAccount is sealed, and
        // all callers must go through public version of Transfer from base
        // class SecureBankAccount.
      }
    }
    // Evil.exe:
    class HostileBankAccount : BankAccount
    {
      override protected void Transfer(
        BankAccount destinationAccount, 
        User authorizedUser,
        decimal amount) {  }
      public static void Main()
      {
        User drEvil = new User("Dr. Evil");
        BankAccount yours = new SwissBankAccount(1234567);
        BankAccount mine = new SwissBankAccount(66666666);
        yours.DoTransfer(mine, drEvil, 1000000.00m); // compilation error
        // You don't have the right to access the protected member of
        // SwissBankAccount just because you are in a
        // type derived from BankAccount.
      }
    }

Dr. Evil's attempt to steal ONE... MILLION... DOLLARS... from your Swiss bank account has been foiled by the C# compiler.

Obviously this is a silly example, and obviously, fully-trusted code could do anything it wants to your types -- fully-trusted code can start up a debugger and change the code as its running. Full trust means full trust. Again, do not actually design a real security system this way! 

But my point is simply that the "attack" that is foiled here is someone attempting to do an end-run around the invariants set up by SecureBankAccount in order to access the code in SwissBankAccount directly.

The second question is "Why doesn't SecureBankAccount also have this restriction?"  In my example, SecureBankAccount says “this.DoTransfer(destinationAccount, user, amount);”

Clearly "this" is of type SecureBankAccount or something more derived. It could be any value of a more derived type, including a new SwissBankAccount. Doesn’t the same concern apply? Couldn't SecureBankAccount be doing an end-run around SwissBankAccount's invariants?

Yes, absolutely! And because of that, the authors of SwissBankAccount are required to understand and approve of everything that their base class does!  You can't just go deriving from some class willy-nilly and hope for the best. The implementation of your base class is allowed to call the set of protected methods exposed by the base class. If you want to derive from it then you are required to read the documentation for that class, or the code, and understand under what circumstances your protected methods will be called, and write your code accordingly. Derivation is a way of sharing implementation details; if you don't understand the implementation details of the thing you are deriving from then don't derive from it.

And besides, the base class is always written before the derived class. The base class isn't up and changing on you, and presumably you trust the author of the class to not attempt to break you sneakily with a future version. (Of course, a change to a base class can always cause problems; this is yet another version of the brittle base class problem.)

The difference between the two cases is that when you derive from a base class, you have the behaviour of one class of your choice to understand and approve of. That is a tractable amount of work. The authors of SwissBankAccount are required to precisely understand what SecureBankAccount guarantees to be invariant before the protected method is called. But they should not have to understand and trust every possible behaviour of every possible cousin class that just happens to be derived from the same base class. Those guys could be implemented by anyone and do anything. You would have no ability whatsoever to understand any of their pre-call invariants, and therefore you would have no ability to successfully write a working protected method. Therefore, we save you that bother and disallow that scenario.

And besides, we have to allow you to call protected methods on receivers of potentially more-derived classes. Suppose we didn't allow that and deduce something absurd. Under what circumstances could a protected method ever be called, if we disallowed calling protected methods on receivers of potentially-more-derived classes?  The only time you could ever call a protected method in that world is if you were calling your own protected method from a sealed class! Effectively, protected methods could almost never be called, and the implementation that was called would always be the most derived one. What's the point of "protected" in that case? Your "protected" means the same thing as "private, and can only be called from a sealed class". That would make them rather less useful.

So, the short answer to both questions is "because if we didn't do that, it would be impossible to use protected methods at all."  We restrict calls through less-derived receiver compile-time types because if we don't, it's impossible to safely implement any protected method that depends on an invariant.  We allow calls through potential subtypes because if we do not allow this, then we don't allow hardly any calls at all.

Finally, an unasked question: what if you are the person writing the base class with a protected method? Essentially you are in the same boat as the person writing any public or virtual method; then you have to accept that anyone can call your public method in any way it chooses, or that derived class can override your virtual method and make it do something completely different, at their discretion. If you write a protected method, you have to accept that any derived class can call that method in any way it chooses and write the base class code accordingly.

When you write a public method, you have to consider the consequences of bad callers; if there are ways that bad callers can misuse your public method, then you need to consider the cost to the user vs the cost of hardening the method against the potentially bad caller. The same is true of virtual methods; you have to consider the consequences of bad overriders. And the same is true of protected methods; you have to consider the consequence of bad derived classes calling your code.

Designing robust code that has public methods is hard. Designing code that is robust and has virtual or protected methods is even harder; designing for extendability is a difficult problem in general, and shouldn’t be taken on lightly. Consider sealing classes that aren’t designed for extension.

  • Good post!

    Regarding your last point, where you talk about bad derived classes when designing a base class. I've always thought about it this way: classes have both a public API and a protected API.

    This is why the vast majority of my classes are sealed, and why I always seal my classes by default when doing the design: because I'm lazy, and a good public API is hard enough without also designing a good protected API. :)

          -Steve

  • Inheritance chains make me want to run for the hills, composition and/or dependency injection ftw.

  • In my last team one of our coding guidelines was to require all classes to be marked sealed unless explicitly intended to be used as base classes. We had a boilerplate .cs file template everyone used for new class files which marked the class as sealed - if you wanted it unsealed you had to manually go and delete the 'sealed' keyword. I really wish sealed was the default, but I understand that might have been confusing/annoying for developers coming from C++ or Java.

    Inheritance is certainly an overused tool, but there are definitely cases where it is exactly what you need. I always started out code reviews by asking about the mapping between the classes and the corresponding "real world" concepts - decisions about whether and how to use inheritance tend to fall out quite naturally and quickly when you frame things up that way.

  • Well, if you have the instances.... you could still just call that method via reflection.

    *tips black hat and disappears into the night*

  • While I certainly understand the difficulty in creating public and/or extensible interfaces, I often curse sealed/internal/non-virtual methods/classes. I can't imagine trying to program in C# without Reflector to enable you to copy vast swaths of assemblies to change or add small bits of functionality to non-extensible classes. (Or you could work for MS, in which case you can just copy the original files and get the comments and correct variable names. It's readily apparent that many of the files in the Silverlight Toolkit are just copied from the SL sources.)

    For example, let's say you have a Silverlight app with a large DataGrid and you want this grid to be sortable. Everything will work fine with no C# code, but it will be slow because the default sort comparison function uses reflection. A list of 100,000 elements will make millions of reflection calls on average if there are multiple keys, so how do you get a DataGrid that can sort quickly?

    Well, it turns out that the DataGrid uses a ListCollectionView to do the sorting, and the ListCollectionView has an ActiveComparer property that would be easy to change with by making your own sort class. But of course ActiveComparer is protected and ListCollectionView is internal to Silverlight, so the only way to derive from it to be able to set ActiveComparer is to copy-and-paste the whole class from Reflector. Of course ListCollectionView derives from an internal class which itself then has to be copied.

    Now you just have to supply a faster comparison class. It just so happens that SortFieldComparer class already used by ListCollectionView does almost exactly what you need, only it's internal to Silverlight and the 3 lines of code you want to modify are in SortFieldComparer's nested private SortPropertyInfo struct. And SortFieldComparer requires another internal class, Comparer.

    So now you've copied over 1200 lines of code just so you can change 3 of them to use compiled lambdas instead of reflection. At least it's worth it, though, because your sort function is several orders of magnitude faster and sorting a list of 100,000 items takes milliseconds instead of minutes.

  • Good post.  This is one of the reasons inheritance is inherently evil.

  • Thank you, Eric. Another instant classic which is bound to be referenced a lot on StackOverflow and the likes.

    > This is one of the reasons inheritance is inherently evil.

    Inheritance isn't inherently evil, "protected" (and "virtual") are. Of course, inheritance is nowhere near as interesting without those two pieces...

  • I completely agree that inheritance is massively over used, but I really think that 'sealed by default' does FAR more harm than good.  To illustrate, which problem do you come across more often:

    A) Code that does *almost* exactly what you want, but that you cannot change due to the sealed marker

    B) Code that you are have extended to alter behavior that you don't bother to test before deploying.

    The argument that the deriver is going to 'do something bad' is irrational.  If the deriver is doing something bad it will be discovered during testing and fixed!  The 'sealed' keyword does nothing more than (force me to use reflection (ala Mr. Kopp)  || decompile/fix || rewrite the code and forgo reuse)

    Personally, I think that sealed should not exist and that private/protected should generate warnings.  Perhaps that's too radical, but 90% of the time I come across a private method it works perfectly well as a public method so I have to alter the code from private to public.  People just put private on because they don't trust their teammates.  Your code should be checking its error conditions, not trusting private to prevent people from doing things that they are clearly able to do.

    Private & Sealed are delusions.  Don't kid yourself that you've made the code more safe by using it.  All the sealed keyword does is annoy people who could have been happily re-using your code.

  • I can understand Eric's point of view, but in practice I more agree with John Somnez and drake42. Protecting your class from misuse is not feasible. And the harm that is done by trying so through sealed classes and non-virtual members is greater than the benefit of preventing misuse. IMHO is misuse mucht better and faster detected by automatic unit testing than by a compiler. But using sealed classes and non virtual methods by default makes the creation of test mocks/stubs much more difficult. Using interfaces everywhere is often overkill and if your 3rdparty component provider didn't do so you can't change it.

    Another case of harmful sealed are static classes that are sealed by default! Often a static class is just a container for helper methods like the XmlConvert or Convert class. The later is sealed, the former is non sealed. Therefore you can easiy derive from XmlConvert and add your own ToMySpecialType() conversion methods. In the case of the Convert class you need a new class and you custom code that converts  always has to use two classes doing the same thing.

    But we can't change it in C# now except perhaps the default sealing of static classes.

  • I am an advocate of sealed as default. I really don't see the problem, if you're intention is to create a derivable base class then by all means do so by stating that's what you want to do explicitly. C# should have taken the "safest" option as default.

    IMHO, if a class is obnoxiously sealed its due to three reasons:

    1. The developer knows what he is doing and even if the class seems extendible, there might be very important security or functional reasons for it to not to be, which are NOT OBVIOUS to a possible coder deriving from it. Of course you can use reflector and all that and recreate and extendible version and quite possibly you'll end up with a huge security hole somewhere in your code you're not aware of. Of course depending on how sensitive the code you're working on is this might not be an issue. But the "I know better than the base class programming team" approach is not the way to go at all.

    2. The class has really no use as a base class at all as it impements a very specific scenario.

    3. The developer has "no idea" about creating an extendible class and the class happens to be sealed because its the default option (if it were) or becuase he heard somewhere that its the safe thing to do. Do you really want to extend a class with such a background???

    My point is that one of the greatestevils of inheritance is that people are using it in places where its wrong. Only classes that have been purposefully created with inheritance in mind should be used. Having sealed as default enforces this much more than the way it works right now. Of course this is impossible to change as it would be the "mother of all breaking changes" but its something that should be fixed when the next programming language comes around.

  • It seems to me that the reason why sealed/non-virtual causes so much aggravation is because we have interface and implementation inheritance all mixed up.  If you merely want to extend a class with your own functionality, sealed/non-virtual won't prevent you from doing so - you can always wrap an existing instance and delegate calls. The problem is that your object won't formally implement the interface of the original class, and thus cannot be substituted.

    This problem disappears if you strictly use interfaces for contracts (so you only allow interface types for variables, method parameters etc), and classes are only allowed in "new" and "extends". Actually, you don't even need "extends", or even "virtual" and "protected" with such an arrangement, since delegation can just be used everywhere, though you'd probably want some syntactic sugar for it. More sugar can be added so that any class automagically defines an interface with the same name, disambiguated by context (e.g. in "Foo foo = new Foo()", first "Foo" is type of variable, so it's an interface, and second "Foo" is used in "new", so it's a class). And there you have the perfect OO world, where everything can be substituted (and thus mocked), but all classes are perfectly encapsulated with no mucking around with their internals from outside.

    I often wonder why such approach isn't used by OO languages more widely. Sather is the only one that I'm aware of - subtyping on interface level only, and ability to "mixin" classes for code reuse (http://www.icsi.berkeley.edu/~sather/Documentation/LanguageDescription/webmaker/DescriptionX2Erem-chapters-1.html) - but it is relatively obscure.

  • Grico makes a statement that I think sums up the entire debate:

       "Only classes that have been purposefully created with inheritance in mind should be used."

    I respectfully but strongly disagree.  The reason I disagree is that you cannot predict the future.  You may be writing a class today that would be the cat's pajamas if only we could alter this one method on it.  But that alteration is for a whole different product that wasn't even conceived of when you wrote your code.  Should future teams be punished because you can't see the future?  Of course not.  But sealing the class makes that future team go through wild gyrations that are completely not needed and reduces the ROI of your software because more money must be spent in order to re-use it.

    Just don't seal your code.  Assume that all classes will someday be used as a base class for something completely unpredictable and make your methods check their input state to ensure consistency.  Even things that are 'wrong' can be very useful.  

    So you're saying that the right thing to do is to spend real money now to enable an undesigned feature that serves no known customer scenario and might never do so, because it might be barely good enough for someone in the future? Suppose we designed cars that way. Someone, at some time in the future might decide that a sensor to detect something we haven't thought of could go in this part of the exhaust manifold. Or in the fuel line. Or in the distributor. Or in the tires. Or in the radio...  Now, 99.999% of our customers are never going to do any such thing, but we'll design the whole car so that anyone who wants to extend it in some way we haven't thought of can do so with minimal expense. Does that sound like it would lead to a rational car design? It sounds to me like it would lead to an expensive mess. Extensibility is a feature, and all features should be designed for particular business purposes and customer scenarios, not just thrown in for free because it's cheap and easy to do a bad job. -- Eric

  • @drake42:

    If its your own software then there shouldn't be any trouble. Unsealing a class is not a breaking change as far as I see it, you can always do so. Furthermore, being your own software, you have all the details, specs and design decisions at your disposal and you can analyze fully the implications of releasing the class as a non sealed class to the public.

    If it isnt you're software then I will still hold true to what I stated before. Lets study the two basic scenarios:

    1. Class is not part of a inheritance chain and was not thought to be extensible: Does not have any virtual methods and therefore should not be a candidate for inheritance. IMHO you should always go with a "has a" relationship if you want to extend this class. You'd be amazed how many times I've seen inheritance used in similar cases though. 50% or more of the classes out there fall in this category and are not sealed. Why? Because language defaults to non-sealed.

    2. Class is sealed and therefore is the last member of a inheritance chain: In this case I advocate trusting the coder. He's implementing a class part of a inheritance chain and decides to seal it. Why? I can't think of a serious developer sealing a class without a compelling reason to do so.

    I underline that sealing a class is not the same as not foreseeing future extensibility. When I seal a class it's because I have a very good reason to do so, not because I dont see any good reason to not do so. If there isn't a good reason then why should you? Keep the chain open.

  • I think that classes should be sealed only for performance reasons. That is, the compiler or runtime can avoid an indirect call or inline a method if it knows that there won't be an arbitrary number of possibilities.

    Your manager comes to you one day and says "we've had hackers attacking our customers by creating derived versions of our classes that were never designed for third-party extensibility, and passing them to our methods, which incorrectly assume that they're getting our implementation, not someone else's implementation, and then fail to an insecure path. And we've got other customers who have extended some of our other classes in unexpected ways, and have opened issues with our support staff, who are now overworked dealing with supporting scenarios that we never planned for, designed, tested or budgetted for. Make sure from now on that we don't make these mistakes again, and seal everything that is not specifically designed for and security tested for third-party extension."

    And you respond with "Ma'am, I refuse to do so. Your reasons are not good enough. The ONLY reason you should EVER seal a class is when doing so enables the runtime to sometimes, maybe, save ONE OR TWO NANOSECONDS on a call. No other reason is good enough for me."

    How well do you think that's going to go over on your next performance evaluation?

    Maybe you write software where the most important consideration is how many nanoseconds you can shave off of each call. Most of us don't. Most of us write software because doing so serves a business purpose. Unsealed types are a *feature*, just as much as every method of a type is a feature, and that feature has costs, risks and benefits associated with it. I don't care one little bit about whether the runtime can save a single nanosecond on the call; I care very much about whether every single feature of my product was carefully designed, tested and documented. I care very much about whether any feature is potentially causing security breaches, customer pain, increased support costs, or increased backwards compatibility burdens that slow down development of future features. -- Eric

  • "Only classes that have been purposefully created with inheritance in mind should be used." I think we could (too late now, obviously) argue that this implies that the default should be sealed.  I've had many times where I felt the urge to derive just so I could tweak a feature or otherwise do odd things, but I've mostly done this with classes included in the .Net Framework, which have a tendency to be designed for inheritance.

    I think the benefit of having nothing sealed is that a user can say, "I know the author didn't want me to do this and I know this will probably break random stuff, but I *really* want to replace Joe's special Frob-Compatible class with my Frob-Incompatible class because it works perfectly with Joe's, "happensToWorkWhenNotFrob-Compatible" function.  From a maintenance perspective, this is horrible.  Joe is probably feeling a strong urge to yell at whoever did it, especially if they ask Joe for help making it work.  However, from a, "spend 5 minutes making broken code that works 95% of the time (and fails silently returns totally wrong data that looks right until you use it and lose a big contract for corrupting a database) catastrophically 5% of the time) instead of two weeks doing it right" perspective, sometimes the coder just doesn't care.  Probably not a good attitude to have.

Page 1 of 2 (22 items) 12