New Design Guideline: Avoid Protected Static


Here is a minor update to the design guidelines around subclassing.  It is based on this quiz I did a last week. Please let me know if you have any questions or comments.  

As always, you can check out the base design guidelines and my incremental updates.

 

 

Avoid Protected Statics.   Carefully consider what information you expose in and use from static protected data because parallel subclass can access it.  Consider the following example. Even though Child and Malicious are parallel and unrelated, Malicious is able to access state the Child sets.  The core issue here is that sensitive data should not be exposed as protected as any arbitrary subclass can use access it.  FxCop rule <tbd> flags any protected static member for review of this issue. 

 

    public class Base

    {

        static protected String _password;

    }

    public class Child : Base

    {

        static Child()

        {

            _password = “Darb”;

        }

    }

    public class Malicious : Base

    {

        public static String StealPassword()

        { return _password; }

    }  

 

Notice, any subclass of Base can access _password, the Child class is not required. 

 

 

Annotation (BradA): Notice that, unlike C++, the runtime does eliminate this problem for protected instance data as a subclass is only allowed to access instance data through its own type.  In the example below Malicious is not allowed to access _password through an instance of Child. 

 

    public class Base

    {

         protected String _password;

    }

    public class Child : Base

    {

        public Child(string password)

        {

            _password = password;

       }

    }

    public class Malicious : Base

    {

        public String StealPassword(Child child)

        { return child._password; }

    }

 

 

 

Comments (19)

  1. This is one of those "I got carried away" nonsense guidelines. There is nothing wrong with protected statics, you just have to know what you’re doing. The example is obviously stupid, but that doesn’t mean you should never use protected statics.

  2. OK — I am game… what do you think is a good use of protected statics?

  3. Nathan Neitzke says:

    I have a comment about your example. It is with the annotation that instanced data could not be accessed.

    What if Malicious inherited from Child? Would it then succeed even in the CLR?

    Thanks.

  4. Joe says:

    I agree with Jeroen.

    "protected static" means you explicitly want all subclasses to be able to access the value. Of course a subclass should and would not store its own sensitive data in a protected static property.

    As for a use of "protected static", the most obvious thing would be a readonly value supplied by a base class and used by all subclasses, such as configuration data.

  5. Nicholas Allen says:

    I’ve never actually seen this problem Brad describes in real world code. What I have seen several times though is a field that is protected, static, but not read only. The field is usually then written to by several subclasses with the obvious, sometimes fatal, result.

    I use protected statics quite a bit, primarily to keep the namespace clean. I don’t care if someone gets access; I just want to improve the API usability by not publishing to the documentation, code editors, or a reader some elements that they don’t need to think about when using the class. This gives a clear separation about what’s important to external users as opposed to extenders.

  6. > OK — I am game… what do you think is a good use of protected statics?

    > 9/9/2004 7:59 AM | Brad Abrams [MSFT]

    Personally, I agree with Brad. I’ve never used protected statics and I probably never will. And if I ever see them in someone’s code, I’ll immediately begin thinking about how to refactor them away. If Microsoft chooses to remove them from the CLS, I probably won’t miss them.

    If you have to provide subclass access to a private static, provide a protected instance accessor.

  7. My response was a little knee-jerk, because design guidelines are no substitute for actually knowing what you’re doing. I feel that anybody that doesn’t understand that the code in the example is a bad idea really has no business writing code, but I’m a bit of a snob that way 😉

    As to practical use for protected static, I was thinking of readonly fields (constants that aren’t primitive, or maybe you don’t want the versioning semantics of constants) that are only useful for subclasses.

    As to examples, just search the BCL:

    mscorlib:

    System.Threading.WaitHandle::InvalidHandle

    System.Security.Util.SiteString::m_separators

    System.Security.Util.StringExpressionSet::m_emptyList

    System.Security.Util.StringExpressionSet::m_separators

    System.Security.Util.StringExpressionSet::m_trimChars

    System.Security.Util.StringExpressionSet::m_directorySeparator

    System.Security.Util.StringExpressionSet::m_alternateDirectorySeparator

    System.Security.Util.DirectoryString::m_illegalDirectoryCharacters

    System.Runtime.Remoting.Identity::IDFLG_DISCONNECTED_FULL

    System.Runtime.Remoting.Identity::IDFLG_DISCONNECTED_REM

    System.Runtime.Remoting.Identity::IDFLG_IN_IDTABLE

    System.Runtime.Remoting.Identity::IDFLG_CONTEXT_BOUND

    System.Runtime.Remoting.Identity::IDFLG_WELLKNOWN

    System.Runtime.Remoting.Identity::IDFLG_SERVER_SINGLECALL

    System.Runtime.Remoting.Identity::IDFLG_SERVER_SINGLETON

    System.Windows.Forms:

    System.Windows.Forms.ScrollableControl::ScrollStateAutoScrolling

    System.Windows.Forms.ScrollableControl::ScrollStateHScrollVisible

    System.Windows.Forms.ScrollableControl::ScrollStateVScrollVisible

    System.Windows.Forms.ScrollableControl::ScrollStateUserHasScrolled

    System.Windows.Forms.ScrollableControl::ScrollStateFullDrag

    System.Windows.Forms.DataGridRow::xOffset

    System.Windows.Forms.DataGridRow::yOffset

    System.Windows.Forms.DateTimePicker::DefaultTitleBackColor

    System.Windows.Forms.DateTimePicker::DefaultTitleForeColor

    System.Windows.Forms.DateTimePicker::DefaultMonthBackColor

    System.Windows.Forms.DateTimePicker::DefaultTrailingForeColor

    System.Windows.Forms.FileDialog::EventFileOk

    System.Windows.Forms.FontDialog::EventApply

    System.Windows.Forms.PropertyGridInternal.GridEntry::InvalidPoint

    System.Windows.Forms.PropertyGridInternal.GridEntry::DisplayNameComparer

    System.Windows.Forms.PropertyGridInternal.GridEntry::NOTIFY_RESET

    System.Windows.Forms.PropertyGridInternal.GridEntry::NOTIFY_CAN_RESET

    System.Windows.Forms.PropertyGridInternal.GridEntry::NOTIFY_DBL_CLICK

    System.Windows.Forms.PropertyGridInternal.GridEntry::NOTIFY_SHOULD_PERSIST

    System.Windows.Forms.PropertyGridInternal.GridEntry::NOTIFY_RETURN

    System.Windows.Forms.PropertyGridInternal.GridEntry::OUTLINE_ICON_PADDING

    System.Windows.Forms.PropertyGridInternal.DocComment::CBORDER

    System.Windows.Forms.PropertyGridInternal.DocComment::CXDEF

    System.Windows.Forms.PropertyGridInternal.DocComment::CYDEF

    System.Windows.Forms.PropertyGridInternal.DocComment::MIN_LINES

    System.Windows.Forms.PropertyGridInternal.PropertyGridCommands::wfcMenuGroup

    System.Windows.Forms.PropertyGridInternal.PropertyGridCommands::wfcMenuCommand

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::InvalidPoint

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::InvalidPosition

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::EDIT_INDENT

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::OUTLINE_INDENT

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::OUTLINE_SIZE

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::PAINT_WIDTH

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::PAINT_INDENT

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::ROWLABEL

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::ROWVALUE

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::MAX_LISTBOX_HEIGHT

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::ERROR_NONE

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::ERROR_THROWN

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::ERROR_MSGBOX_UP

    System.Windows.Forms.ComponentModel.Com2Interop.Com2IDispatchConverter::none

  8. Oops. Some of those are actually constants.

    Here is the correct list:

    mscorlib:

    System.Threading.WaitHandle::InvalidHandle

    System.Security.Util.SiteString::m_separators

    System.Security.Util.StringExpressionSet::m_emptyList

    System.Security.Util.StringExpressionSet::m_separators

    System.Security.Util.StringExpressionSet::m_trimChars

    System.Security.Util.StringExpressionSet::m_directorySeparator

    System.Security.Util.StringExpressionSet::m_alternateDirectorySeparator

    System.Security.Util.DirectoryString::m_illegalDirectoryCharacters

    System.Windows.Forms:

    System.Windows.Forms.DateTimePicker::DefaultTitleBackColor

    System.Windows.Forms.DateTimePicker::DefaultTitleForeColor

    System.Windows.Forms.DateTimePicker::DefaultMonthBackColor

    System.Windows.Forms.DateTimePicker::DefaultTrailingForeColor

    System.Windows.Forms.FileDialog::EventFileOk

    System.Windows.Forms.FontDialog::EventApply

    System.Windows.Forms.PropertyGridInternal.GridEntry::InvalidPoint

    System.Windows.Forms.PropertyGridInternal.GridEntry::DisplayNameComparer

    System.Windows.Forms.PropertyGridInternal.PropertyGridCommands::wfcMenuGroup

    System.Windows.Forms.PropertyGridInternal.PropertyGridCommands::wfcMenuCommand

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::InvalidPoint

    System.Windows.Forms.PropertyGridInternal.PropertyGridView::InvalidPosition

    System.Windows.Forms.ComponentModel.Com2Interop.Com2IDispatchConverter::none

  9. Keith Patrick says:

    Does this apply exclusively to protected static properties/fields, or does it go for methods as well? If I have a method that doesn’t use instance data, even protected, I’ll often just tack a static to it, but should I not be doing this for some reason?

  10. Actually, I just found myself using a protected static method today.

    Originally, I had a class with a private static method. Then I had to refactor this into two separate subclasses with a common base abstract class. Yet both subclasses still needed access to that private static method. Here were my choices:

    1) Make the method public static. This violates encapsulation.

    2) Copy the method to each of the subclasses. This results in duplicated code.

    3) Make the method a protected instance method. Since the method uses no instance data, does not employ polymorphism, and conceptually has no relationship to specific instances of the class, this is a bad idea and could lead to confusion later.

    4) Make the method protected static. This is the decision I made, because it avoids the previous three pitfalls.

    Maybe the guideline would be better restated as "Avoid Protected Static FIELDS".

    Just my $0.02.

  11. Eric Newton says:

    Another irritation:

    internal protected modifiers dont do what you’d expect… the internal keyword takes precedence over the protected modifier, even though you might need for a subclass to be able to call, but you’d like to give the assembly a way to call into it as well…

    for example:

    <code>

    public class Order

    {

    private int _orderId; //orderId should never be settable from outside the assembly

    //but what if somebody subclasses the Order? It would be nice to let the subclass

    //be able to set it… AND let the data classes be able to set it when

    // Loading an order or Creating a new order [linking the instance to a record in the db

    public int OrderId

    { get { return this._orderId; } }

    internal protected void Set_OrderId(int value) { this._orderId=value; }

    }

    public class MyOrders //class handles database interaction

    {

    public MyOrder LoadMyOrder(int orderId)

    {

    MyOrder o = new MyOrder();

    // build a dbcommand to read a record into a datareader

    o.Set_OrderId( (int) dr["OrderId"] ); //impossible to do this without making OrderId public somehow

    }

    ///<summary>Inserts the order into the db</summary>

    ///<returns>the id of the new order. (OrderId property is also set.)</returns>

    public int Insert(MyOrder order)

    {

    //build a dbcommand to insert the order into the database…

    //set the OrderId to the new record?

    order.Set_OrderId( (int) dr["OrderId"] ); //another impossible call

    return order.OrderId;

    }

    }

    </code>

    —————————

    make a new project, referencing the above assembly

    <code>

    public class MyOrder : Order

    {

    public DateTime MyOrderSpecificProperty { get; set; }

    }

    public class MyOrders //class handles database interaction

    {

    public MyOrder LoadMyOrder(int orderId)

    {

    MyOrder o = new MyOrder();

    // build a dbcommand to read a record into a datareader

    o.Set_OrderId( (int) dr["OrderId"] ); //impossible to do this without making OrderId public somehow

    }

    ///<summary>Inserts the order into the db</summary>

    ///<returns>the id of the new order. (OrderId property is also set.)</returns>

    public int Insert(MyOrder order)

    {

    //build a dbcommand to insert the order into the database…

    //set the OrderId to the new record?

    order.Set_OrderId( (int) dr["OrderId"] ); //another impossible call

    return order.OrderId;

    }

    }

    </code>

  12. Eric Newton says:

    ah crap, i screwed up the first set of MyOrders should read Orders… sorry guys! grrr

  13. Eric Newton says:

    Another irritation:

    internal protected modifiers dont do what you’d expect… the internal keyword takes precedence over the protected modifier, even though you might need for a subclass to be able to call, but you’d like to give the assembly a way to call into it as well…

    for example:

    <code>

    public class Order

    {

    private int _orderId; //orderId should never be settable from outside the assembly

    //but what if somebody subclasses the Order? It would be nice to let the subclass

    //be able to set it… AND let the data classes be able to set it when

    // Loading an order or Creating a new order [linking the instance to a record in the db

    public int OrderId

    { get { return this._orderId; } }

    internal protected void Set_OrderId(int value) { this._orderId=value; }

    }

    public class Orders //class handles database interaction

    {

    public Order LoadOrder(int orderId)

    {

    Order o = new Order();

    // build a dbcommand to read a record into a datareader

    o.Set_OrderId( (int) dr["OrderId"] ); //can call because in same assembly

    }

    ///<summary>Inserts the order into the db</summary>

    ///<returns>the id of the new order. (OrderId property is also set.)</returns>

    public int Insert(MyOrder order)

    {

    //build a dbcommand to insert the order into the database…

    //set the OrderId to the new record?

    order.Set_OrderId( (int) dr["OrderId"] ); //another impossible call

    return order.OrderId;

    }

    }

    </code>

    —————————

    make a new project, referencing the above assembly

    <code>

    public class MyOrder : Order

    {

    public DateTime MyOrderSpecificProperty { get; set; }

    internal new void Set_OrderId(int orderId) //try to use the protected aspect of the member

    { base.Set_OrderId(orderId); /* again, impossible */ }

    }

    public class MyOrders //class handles database interaction

    {

    public MyOrder LoadMyOrder(int orderId)

    {

    MyOrder o = new MyOrder();

    // build a dbcommand to read a record into a datareader

    o.Set_OrderId( (int) dr["OrderId"] ); //impossible to do this without making OrderId public somehow

    }

    ///<summary>Inserts the order into the db</summary>

    ///<returns>the id of the new order. (OrderId property is also set.)</returns>

    public int Insert(MyOrder order)

    {

    //build a dbcommand to insert the order into the database…

    //set the OrderId to the new record?

    order.Set_OrderId( (int) dr["OrderId"] ); //another impossible call

    return order.OrderId;

    }

    }

    </code>

    The idea is to force users of Orders to go through a hoop to get a valid Order that represents an Order record in the db, hence no setting OrderId publically… but you run into an inheritance problem…

  14. Hey Eric,

    I would say you should have a protected property so that it can be accessed and mutated by subclasses, but then also have an alternative mutator method marked as internal for other classes within the assembly to set/get the property’s underlying data without needing to actually reference the protected property itself.

    public class Order

    {

    // private field

    private int _orderID;

    // anyone can read the value

    public int OrderID

    {

    get { return _orderID; }

    }

    // derived classes from any assembly use this property to set the order id

    protected int InternalOrderID

    {

    get { return _orderID; }

    set { _orderID = value; }

    }

    // non-derived classes within this assembly use this method to set the order id

    internal int SetOrderID(int value)

    {

    _orderID = value;

    }

    }

  15. Matthew W. Jackson says:

    Yeah, I tend to follow Steven’s pattern. I tend to avoid "protected internal", since it’s really kind of a screwball visibility (there are lots of "exceptions" to the normal rules to accomodate it). I prefer a protected method and an internal method, usually with the same name appended with "Internal".

    I could go for a few more visibilities in the language and runtime, though, since I don’t always like exposing internals to everybody else in my library. I’d like to add the "protected and internal" visibility to C#, but there’s not really a good syntax for it (except maybe an awkward "protected & internal"). I’d also like a visibility for nested classes that was only accessible to an outer class that contains it. But, what do you call such a thing?

    I suppose a true "friend" visibility would solve both of these problems.

  16. Hey Eric,

    I did some experimenting and I found that protected internal does indeed do what you want it to do: it only allows a member to be called from the same assembly or from a derived class. This is consistent with the MSDN documentation.

    I tried loading your example into the IDE, and after filling in some of the blanks, it compiles fine. I didn’t have to change any of your access modifiers.

    -Steven