Is it unwise to have a static method be used for security validation in ASP.net?

Question

I have a static method like this

public static string DoSomethingToString(string UntrustedString)
{
//parse format and change string here.
return newString.
}

Since I know that multiple calls to:

static int myInt=0;
public static int AddNumber()
{
lock(someObject)
{
myInt++;
}
return myInt;
}

will return an ever increasing number (that is shared among pages), I'm not sure how local the variables will be treated in DoSomethingToString();

I'd like to learn the conditions in which a static method can safely/unsafely be used in ASP.net just to put my multi-threaded brain to rest on this topic.

UPDATE:

Most of the discussion has been around value types. How do I know when it's safe to call methods (explicitly or implicitly) that change my reference types? Is it enough to looking at MSDN documentation and only use methods where it says "threadSafe"?

One example if what I call an implicit change is using String.Split() since it's part of the same class. I think there is a likelihood that some security characteristics are shared and there is less worry/diligence needed. (?)

What I'm calling an explicit change (for lack of a better word right now) is calling another method to do work... which can be static or an instance class. I think that more background/research work needs to be done to ensure that each object and method is ThreadSafe.

For the sake of discussion assume I have a method with this signature:

ValidateStringAndContext(string untrustedString, Object myCustomUserContext)

and it has a static method that references the following object

public SecurityChecker
{
public static object CheckSecurityStatic(string DirtyData)
{
//do string.split
//maybe call a database, see if it's a token replay 
//

//OR  - alternate implementation
SecurityChecker sc = new SecurityChecker();
 if (sc.CheckSecurity(DirtyData))
  {
  myCustomUserContext.Property1 = new GUID()
  }
return myCustomUserContext;
}


public class bool CheckSecurity(string DirtyData)
{
//do string.split
//maybe call a database, see if it's a token replay 
// return true if OK return false if not
}
}

Revised Question

Would I run into concurrency issues (variables overwriting each other) if the static "utilities" class I create were to create an instance of another object and then call the method --versus-- simply calling a static method directly?

Answer 1

Check this site out to get a better understanding of Threading

Threading Explained

Answer 2

Each thread calling your static method has its own call stack. Every call to DoSomethingToString() has its own copy of the method's variables, completely independant of any other call to that method on another thread (unless you declare a variable as static, thats what you may be getting confused with - a static variable has only one instance that is accessible to multiple threads in your program).

As always, you need to consider concurrency when multiple threads are accessing a shared resource. When a resource only exists within the context of a method call, concurrency is a non-issue.

See also: http://stackoverflow.com/questions/511378/net-static-methods-and-its-effects-on-concurrencyt

Answer 3

There are some very important points that have more to do with the code that is not visible than the code shown in the question...

The DoSomeThingToString method is static, and any variables declared within that method will be local to that thread's call stack. If the variables in use are defined outside the function, you will have race conditions on that memory. Be sure it looks like this, using only local variables:

public static string DoSomethingToString(string UntrustedString)
{
    var newString = UntrustedString;
    // operations on newString...
    return newString;
}

The AddNumber method could be subject to other problems that might not be obvious. If this is really what you are trying to do, to add a number, do it like this:

System.Threading.Interlocked.Increment(ref myInt);

The Interlocked.Increment method guarantees that the operation will be completed in one clock cycle.

Otherwise, the use of the lock keyword can be tricky in some rare situations. Here are some rules of thumb. Always lock on an object that you both create and hold the reference to, and even better, that you can never change. That means: readonly, and assigning the memory address upon creation of the class. That looks like this:

static int myInt=0;
static readonly object aGoodLock = new object();
public static int MoreComplexIntStuff()
{
    lock(aGoodLock)
    {
        // Do stuff with myInt...
    }
    return myInt;
}

Also, this isn't the whole story. Another gotcha is that anytime the variable myInt is accessed, even if it's in another part of this class - or if it's public and used somewhere else, you need to wrap it with a lock. And not just any lock, but the same one that you're using, aGoodLock.

The best way to help your fellow developers (and perhaps your own long-term memory) with this is to make the variable and the lock that wraps it private, and to expose myInt using a Property where you would be careful to use the lock in the get and set.

Answer 4

I get the feeling that you are thinking that variables can be only instance or static, and forgetting that local variables are different to either. Consider:

public class MyClass
{
    public int InstanceInt;
    public static int StaticInt;
    public int InstanceMethod()
    {
        int i = new Random().Next(1, 50);
        return i;
    }
    public static int StaticMethod()
    {
        int j = new Random().Next(1, 50);
        return j;
    }
}

Here InstanceInt is an instance variable, and StaticInt a static variable. Both of these require locking if they are going to be accessed by different threads, however, InstanceInt is only going to be accessed by different threads if an instance of MyClass is accessed by different threads. This could happen with static MyClass AllThreadsSeeMe = new MyClass(), storage in a static collection, explicitly passing it to another thread, or so on, but otherwise will not. StaticInt meanwhile is inherently accessible to all threads running in the application, even if you take care to make sure that no instance of MyClass is shared between threads.

Meanwhile, i and j are both local to their functions. Each thread calling the function will be given their own copy of i or j, irregardless to how many threads could call them. Hence they are inherently thread-safe. It is only if either of these methods alter a static or instance variable, or read a mutable static or instance variable that could be altered by a different method, that they are not thread-safe (immutable - readonly - variables are also threadsafe because there's no way another thread can change their value, given that no thread at all can change it).