Amazon.com Widgets

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; }

    }

 

 

 

Published 09 September 04 06:49 by BradA
Filed under:

Comments

# Jeroen Frijters said on September 9, 2004 7:39 AM:
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.
# Brad Abrams [MSFT] said on September 9, 2004 7:59 AM:
OK -- I am game... what do you think is a good use of protected statics?
# Nathan Neitzke said on September 9, 2004 9:04 AM:
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.
# Joe said on September 9, 2004 10:05 AM:
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.


# Nicholas Allen said on September 9, 2004 1:46 PM:
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.
# Steven Padfield said on September 10, 2004 11:46 AM:
> 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.
# Jeroen Frijters said on September 10, 2004 12:07 PM:
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
# Jeroen Frijters said on September 10, 2004 12:10 PM:
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
# Keith Patrick said on September 10, 2004 6:58 PM:
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?
# Steven Padfield said on September 13, 2004 5:02 PM:
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.
# Eric Newton said on September 13, 2004 6:27 PM:
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>
# Eric Newton said on September 13, 2004 6:28 PM:
ah crap, i screwed up the first set of MyOrders should read Orders... sorry guys! grrr
# Eric Newton said on September 13, 2004 6:31 PM:
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...
# Steven Padfield said on September 14, 2004 12:02 PM:
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;
}
}
# Matthew W. Jackson said on September 14, 2004 11:38 PM:
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.
# Steven Padfield said on September 15, 2004 5:22 PM:
http://67.174.60.122/blog/archive/2004/09/15/155.aspx
# Steven Padfield said on September 16, 2004 11:53 AM:
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
# protected static » Blog Archive » This just in! said on May 26, 2006 12:18 AM:
PingBack from http://www.blog.protectedstatic.com/?p=5
# Brad Abrams New Design Guideline Avoid Protected Static | patio umbrella said on June 17, 2009 11:59 PM:

PingBack from http://patioumbrellasource.info/story.php?id=312

New Comments to this post are disabled

Search

This Blog

Syndication

Page view tracker