Succeeded!



  • Someone from accounting came to my cube, asking me to investigate why he could not void a check in our system. He says he's never had a problem before, and there was no indication of a problem this time either, except that when he looked at another report, just this one check was not voided.

    OK, then. When debugging, I found the following code. Note that this is an ASP.NET application in C# with SQL Server 2005 backend. I'll provide what happens when he clicks the button in the UI to void the checks, then all relevant functions from there (mildly anonymized).

    [code language="C#"]
    // In the code-behind:
    

    protected void btnVoidChecks_Click(object sender, EventArgs e)
    {
    try
    {
    lblMsg.Text = "";

        SomeWcfService service = new SomeWcfService();
        service.VoidChecks(Convert.ToInt32(cboSomeDropDownList.SelectedItem.Value), txtFromCheckNo.Text, txtToCheckNo.Text, this.UserProfile.UserId.Value);
        lblMsg.Text = "Succeeded!";
    }
    catch (Exception ex)
    {
        lblMsg.Text = ex.Message.ToString();
    }
    

    }

    // In SomeWcfService:

    public int VoidChecks(int branchId, string checkNumbersFrom, string checkNumbersTo, int voidedBy)
    {
    return _repository.VoidChecks(branchId, checkNumbersFrom, checkNumbersTo, voidedBy);
    }

    // _repository is an interface type -- let's skip to the implementation it's using and the VoidChecks function there:

    public int VoidChecks(int branchId, string checkNumbersFrom, string checkNumbersTo, int voidedBy)
    {
    // Need to make sure that the first 3 digits of checkNumbersFrom and checkNumbersTo match
    // Example: Void checks from 103968 to 1032016 that have 103 as a prefix
    if (checkNumbersFrom.Substring(0,3) != checkNumbersTo.Substring(0, 3))
    {
    throw new Exception("The first 3 digits of the Check numbers must match!");
    }
    else if (checkNumbersFrom.Length < 3)
    {
    throw new Exception("The Check number length must be 3 or greater!");
    }

    using (SqlConnection connection = new SqlConnection(this.ConnectionString))
    {
        using (var command = new SqlCommand("CheckVoid", connection))
        {
            command.CommandType = CommandType.StoredProcedure;
            command.CommandTimeout = 500000;
    
            command.Parameters.AddWithValue("BeginCheckNumber", checkNumbersFrom);
            command.Parameters.AddWithValue("EndCheckNumber", checkNumbersTo);
            command.Parameters.AddWithValue("BranchID", branchId);
            command.Parameters.AddWithValue("UserID", voidedBy);
    
            connection.Open();
            object returnValue = command.ExecuteScalar();
    
            if (returnValue == null)
            {
                return 0;
            }
            else
            {
                try
                {
                    return (int)returnValue;
                }
                catch (Exception)
                {
                    return -1;
                }
            }
        }
    }
    

    }
    [/code]

    I won't bore you with the stored procedure, but let's say that it has the possibility of violating a primary key constraint, which is what caused the problem. That's a WTF in itself that I won't cover right here.

    Now, the litany of WTFs:

    1. The same stupid anti-pattern shows up seemingly at random throughout the code, including here, in which instead of propagating an exception, a method returns a certain value. It's as though we didn't have Exceptions in C#.
    2. If that isn't bad enough, callers of these methods nearly always discard the value and treat the method as void. In this case, that means
    3. Why set the label to an empty string just to reset it again in all circumstances?
    4. That's a great command timeout for something that should be pretty much instant. How about a delay indicating a problem?
    5. ex.Message is already a string, so ToString() is worthless, not that we'll ever get to it anyway as long as the service is responding.
    6. Check numbers are compared in a range, using string comparison. There are some business rules in place that might make that not completely problematic here, but still...


  • You sure you copied it properly? try { return ... } catch (...) { return ... } is a great WTF pattern, but any exception in ExecuteScalar would be passed on to the caller.



  • @corgimonster said:

    <font face="Lucida Console" size="2">    ...
    if (checkNumbersFrom.Substring(0,3) != checkNumbersTo.Substring(0, 3))
    {
    throw new Exception("The first 3 digits of the Check numbers must match!");
    }
    else if (checkNumbersFrom.Length < 3)
    {
    throw new Exception("The Check number length must be 3 or greater!");
    }
    ...</font>

    7: Length check is never executed, since if checkNumbersFrom.Length was < 3, the Substring method would throw an exception.



  • 7) What if I want to void check numbers <font size="2"><font face="Lucida Console">103998 through 104002? </font></font>

     



  • @RichP said:

    7) What if I want to void check numbers <font size="2"><font face="Lucida Console">103998 through 104002? </font></font>

     


    8) How about checks 1009 to 10097?



    1. Or checks 1009 to 1001


  • I have less problem with the UI stuff than I have with the webservice code and interface.

    1. Passing a number in a string. Yes I know the input comes from a textbox, but it should be parsed and checked on the UI and passed as a number.
      If anyone out there thinks this isn't a problem, then by inference we should pass all parameters as strings, including dates, numerics, images, arrays etc.
    2. The string prefix check is an example of 'good intention, totally incompetent execution'. The intention of the coder was 'don't allow the user to void more than 1000 checks'. Instead they check the first three characters. Imagine our user wants to void checks 103001 to 103002, but accidentally types 103001 and 1030002 (note the extra zero). This passes the code prefix test, and then voids 927,000 checks.

      A malicious user could also bypass this test by prefixing the ranges with three zeros and the number would be passed, checked and deemed valid, then possibly void every check in the system.
    3. Returning a value not used may be okay if other code somewhere else needs to use the value. However the value isn't used correctly: a value -1 is an exception and will be reported as success in the UI. Worse still, catching an exception and returning -1 hides the exception and thus the cause. I've been guilty of doing this myself in the past and it's very bad practice. Imagine the -1 was handled correctly, and output "Sorry the void check operation failed". The person debugging would immediately want to know "But why did it fail?!?"


  • @Quango said:

    Passing a number in a string. Yes I know the input comes from a textbox, but it should be parsed and checked on the UI and passed as a number.
    If anyone out there thinks this isn't a problem, then by inference we should pass all parameters as strings, including dates, numerics, images, arrays etc.

    This is how our offshore team operates.  Strings for everything.  We've got methods that take 22 string parameters, some of which are used as booleans ("YES", "TRUE", etc.).  Good times.


Log in to reply