ANSWER: POP QUIZ: What’s wrong with this code – part 1


So here was the code we asked about:

using System;
using System.Data;
using System.Configuration;
using System.Collections;
using System.Web;
using System.Web.Security;
using System.Web.UI;
using System.Web.UI.WebControls;
using System.Web.UI.WebControls.WebParts;
using System.Web.UI.HtmlControls;

public partial class Login : System.Web.UI.Page
{
    protected void Page_Load(object sender, EventArgs e)
    {

    }

    void Logon_Click(object sender, EventArgs e)
    {
        string username = UserNameTextBox.Text;
        string password = UserPassTextBox.Text;

        if (AuthenticateRequest(username, password))
        {

            FormsAuthenticationTicket ticket = new FormsAuthenticationTicket(1,
              username,
              DateTime.Now,
              DateTime.Now.AddMinutes(30),
              isPersistent,
              userData,
              FormsAuthentication.FormsCookiePath);

            // Encrypt the ticket.
            string encTicket = FormsAuthentication.Encrypt(ticket);

            // Create the cookie.
            Response.Cookies.Add(new 
              HttpCookie(FormsAuthentication.FormsCookieName, 
              encTicket));

            FormsAuthentication.RedirectFromLoginPage
               (username, false); // Problem 2 below
        }
        else
        {
            throw new System.Exception(
                 "Error authenticating the user"); // Problem 1 below
        }
    }

    bool AuthenticateRequest(string username, string password)
    {
        // Do authentication here
    }
}

Looking at this, the two lines in red are where the problems are. 

  1. I’ll start with the exception.  So what is wrong with this?  Well, the main problem here is that when a user doesn’t log in, we will throw an exception.  That is fine until we have the case of someone trying to break into our site.  In that case, they are trying over and over to log in and will end up throwing many exceptions.  This can cause ASP.NET to run out of memory.  We shouldn’t throw an exception when an application is in normal use.  Failing to authenticate is normal, so we shouldn’t throw an exception in this case.
  2. The first line in red is a little more tricky.  Here we have created our own FormsAuthentication ticket and added it to the cookie collection.  But when we make the call in red to RedirectFromLoginPage, it will create a ticket and add it to the cookie collection also.  So you just removed the real ticket that you wanted to create.  The code we should have used, which just redirects the user back to the original page, is:
      ' Redirect back to original URL.
      Response.Redirect(FormsAuthentication.GetRedirectUrl(
        username, false))

Note: There was a typo in the posting originally where Problem 2 was pulling information out of parameters that didn’t exist.  That was unintentional and removed.

Thanks for all the comments and I try to post one each week moving forward.

Comments (3)

  1. Lee Culver says:

    Why does throwing an exception here leak memory?

  2. So it isn’t a “traditional” memory leak.  But it is very expensive to create an exception object.  Creating the callstack, the message and everything around the actual object itself.  All of that will be stored in memory until the next GC.  I have actually seen dumps from customers where they had over 5000 exceptions in memory and all of them were from failed logins.  You can imagine if each of them took up 10k, just as an example, that would be 50MB of memory just from these.  There is more helpful information on exception handling here:

    http://msdn2.microsoft.com/en-us/library/seyhszts(vs.71).aspx