Freigeben über

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

  • Artikel

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.

Comments

  • Anonymous
    January 14, 2010
    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

  • Anonymous
    January 14, 2010
    The comment has been removed

  • Anonymous
    January 14, 2010
    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.

  • Anonymous
    January 14, 2010
    Well, if you have the instances.... you could still just call that method via reflection. tips black hat and disappears into the night

  • Anonymous
    January 14, 2010
    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.

  • Anonymous
    January 14, 2010
    Good post.  This is one of the reasons inheritance is inherently evil.

  • Anonymous
    January 15, 2010
    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...

  • Anonymous
    January 17, 2010
    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.

  • Anonymous
    January 17, 2010
    The comment has been removed

  • Anonymous
    January 17, 2010
    The comment has been removed

  • Anonymous
    January 18, 2010
    The comment has been removed

  • Anonymous
    January 19, 2010
    The comment has been removed

  • Anonymous
    January 19, 2010
    @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.

  • Anonymous
    January 20, 2010
    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

  • Anonymous
    January 20, 2010
    "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.

  • Anonymous
    January 20, 2010
    The comment has been removed

  • Anonymous
    January 20, 2010
    Eric: Maybe your car has its hood sealed shut, but mine doesn't. Just because the manufacturer didn't explicitly design each part of my car for extensibility doesn't mean I can't extend it. While I understand that there are parts that must be sealed for performance or security reasons (airbags, odometer, alarm system), just about every other part of my car can be changed whether intended or not. Can you imagine if the only parts you could change on your car were ones the manufacturer explicitly allowed? "Sorry, sir, but you can't put snow tires on our convertible models because we didn't design them to be used in the winter." Anyway, I suppose there are two schools of thought. One says that extensibility should only be permitted where it is designed; the other says that extensibility should be disallowed only when there's a good reason. People who design libraries and keep getting bitten by implementors using their libraries in unexpected ways prefer the first school, while implementors who keep running into walls where the functionality they need is blocked by interal/private/sealed prefer the second school. I read Raymond's blog, so I understand why people are in the first school. However if you read my first comment on this post, it should be obvious why I'm in the second school.

  • Anonymous
    January 21, 2010
    The comment has been removed

  • Anonymous
    January 21, 2010
    David Nelson: Since I live in the US, the Magnuson-Moss Warranty Act (http://en.wikipedia.org/wiki/Magnuson%E2%80%93Moss_Warranty_Act) means that the manufacturer of my car cannot require that I only use their parts in the engine or anywhere else in the car. So in fact, I signed no such paperwork. Anyway, let's consider the ArrayList example. Assume that I am using some library that uses ArrayLists and has some algorithm that calls IndexOf a lot. Since IndexOf is currently O(n), I can speed things up by orders of magnitude with a fast IndexOf() method. With the right things being virtual, I can derive from ArrayList, add an index, and override the necessary methods (Add, Remove, IndexOf) to use and maintain my index. Then I just have to pass MyFastArrayList objects into the library and everything will be orders of magnitude faster. If ArrayList were sealed, I could only create MyFastArrayList by completely reimplementing ArrayList and modifying the few methods that need to maintain the index. Of course, that would be completely useless because there's no way to pass MyFastArrayList instances to the library that expects ArrayList instances. My only recourse here is to REIMPLEMENT the WHOLE library. This scenario certainly puts less burden on the maintainers of ArrayList and the 3rd-party library, but only because I can't use them anymore. In fact, perhaps the 3rd-party library maintainers would prefer the first scenario because at least then I would be their customer. Ideally, there would be some IArrayList which is implemented by SealedArrayList and VirtualArrayList. That way all libraries would just take IArrayList instances and most users who don't care about extending it would use the SealedArrayList that can get performance revs, while users who need an index can extend VirtualArrayList. Now, imagine a more complex version of scenario 2: I want to use some library of yours but I need to override something that isn't virtual, so I go into Reflector, copy the classes I need, paste them into my code, and change the function that I need to override. What happens when you discover a security flaw in the part of your library that I needed to change? That fix isn't going to automatically propogate to my code because I'm using a copy of it. Is this any better of a maintenance scenario?

  • Anonymous
    January 22, 2010
    The comment has been removed

  • Anonymous
    January 27, 2010
    "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." I tend to agree with you, but it leads me to the question, why aren't all classes sealed by default?  I mean, why do we have a "sealed" keyword instead of an "unsealed" or "extendable" keyword?  It would seem that the language design is at direct odds with the standard guidance in this scenario. Obviously the ship has sailed on doing that at the language level.  But the New Class... template in VS could certainly be modified to declare the new class as sealed, which would have the same effect of requiring the programmer to make a conscious choice to unseal a class. Another useful feature to support the guidance might be a keyword that means "only extendable within its own assembly."  Designing a class to be extendable by the general public is certainly more work than designing it to be extendable only by people who have the ability to modify the class itself.

  • Anonymous
    May 24, 2011
    I don't get why does the DoTransfer Method in BankAccount have code, if it is an abstract method. I have read in the Stack Overflow's Web phorum that abstract methods do not have code , but it's body is implemented by the classes that derive that class.