0 Comments

Sometimes when maintaining legacy software, you come across a piece of code and you just wonder;

But….why?

If you’re lucky the reasoning might end up being sound (i.e. this code is structured this way because of the great {insert-weird-issue-here} of 1979), but its probably more likely that it was just a plain old lack of knowledge or experience.

Its always a journey of discovery though, so enjoy the following post, sponsored by some legacy VB6 code that interacts with a database.

Building On Shaky Foundations

As is typically the precursor to finding weird stuff, it all started with a bug.

During the execution of a user workflow, the presence of a ‘ character in a name caused an error, which in turn caused the workflow to fail, which in turn led to some nasty recovery steps. I’m not going to get into the recovery steps in this post, but suffice to say, this workflow was not correctly wrapped in a transaction scope and we’ll just leave it at that.

Now, the keen-eyed among you might already be able to guess what the issue was. Maybe because you’ve already fought this battle before.

Deep in the bowels of that particular piece of code, an SQL statement was being manually constructed using string concatenation.

Private Function Owner_LetPrepFees_CreateTransaction(ByVal psTransAudit As String, ByVal plTransLink As Long, ByVal plTransPropertyID As Long, _
                                                     ByVal plTransOwnerID As Long, ByVal psTransFromRef As String, ByVal psTransFromText As String, _
                                                     ByVal plTransCreditorID As Long, ByVal psTransToRef As String, ByVal psTransToText As String, _
                                                     ByVal psTransReference As String, ByVal psTransType As String, ByVal psTransDetails As String, _
                                                     ByVal pbGSTActive As Boolean, ByVal pnTransAmount As Currency, ByVal psTransLedgerType As String, _
                                                     ByVal pbLetFee As Boolean, ByVal pbPrepFee As Boolean, ByVal plCount As Long) As Long
    Dim sInsertFields           As String
    Dim sInsertValues           As String
    Dim lCursorLocation         As Long

    ' replaces single "'" with "''" to avoid the "'" issue in insert statement
    psTransFromRef = SQL_ValidateString(psTransFromRef)
    psTransFromText = SQL_ValidateString(psTransFromText)
    psTransToRef = SQL_ValidateString(psTransToRef)
    psTransToText = SQL_ValidateString(psTransToText)
    psTransReference = SQL_ValidateString(psTransReference)
    psTransDetails = SQL_ValidateString(psTransDetails)

    sInsertFields = "AccountID,Created,UserId,Audit,Date"
    sInsertValues = "" & GetAccountID() & ",GetDate()," & User.ID & ",'" & psTransAudit & "','" & Format(UsingDate, "yyyyMMdd") & "'"
    If plTransLink <> 0& Then
        sInsertFields = sInsertFields & ",Link"
        sInsertValues = sInsertValues & "," & plTransLink
    End If
    If plTransPropertyID <> 0 Then
        sInsertFields = sInsertFields & ",PropertyId"
        sInsertValues = sInsertValues & "," & plTransPropertyID
    End If
    If plCount = 1& Then
        sInsertFields = sInsertFields & ",OwnerID,FromRef,FromText"
        sInsertValues = sInsertValues & "," & plTransOwnerID & ",'" & Left$(psTransFromRef, 8&) & "','" & Left$(psTransFromText, 50&) & "'"
    Else
        sInsertFields = sInsertFields & ",CreditorId,ToRef,ToText"
        sInsertValues = sInsertValues & "," & plTransCreditorID & ",'" & Left$(psTransToRef, 8&) & "','" & Left$(psTransToText, 50&) & "'"
    End If
    sInsertFields = sInsertFields & ",Reference,TransType,Details,Amount,LedgerType"
    sInsertValues = sInsertValues & ",'" & psTransReference & "','" & psTransType & "','" & IIf(pbGSTActive, Left$(Trim$(psTransDetails), 50&), Left$(psTransDetails, 50&)) & "'," & pnTransAmount & ",'" & psTransLedgerType & "'"
    If pbLetFee And psTransType = "CR" And psTransLedgerType = "JC" Then
        sInsertFields = sInsertFields & ",DisburseType"
        sInsertValues = sInsertValues & "," & dtLetFeePayment
    ElseIf pbPrepFee And psTransType = "CR" And psTransLedgerType = "JC" Then
        sInsertFields = sInsertFields & ",DisburseType"
        sInsertValues = sInsertValues & "," & dtPrepFeePayment
    End If
    If pbGSTActive Then
        sInsertFields = sInsertFields & ",GST"
        sInsertValues = sInsertValues & "," & 1&
    End If
    
    lCursorLocation = g_cnUser.CursorLocation
    g_cnUser.CursorLocation = adUseClient
    Owner_LetPrepFees_CreateTransaction = RSQueryFN(g_cnUser, "SET NOCOUNT ON; INSERT INTO Transactions (" & sInsertFields & ") VALUES (" & sInsertValues & "); SET NOCOUNT OFF; SELECT SCOPE_IDENTITY() ID")
    g_cnUser.CursorLocation = lCursorLocation

End Function

Hilariously enough, I don’t even have a VB6 Syntax Highlighter available on this blog, so enjoy the glory of formatter:none.

Clearly we had run into the same problem at some point in the past, because the code was attempting to sanitize the input (looking for special characters and whatnot) before concatenating the strings together to make the SQL statement. Unfortunately, input sanitization can be a hairy beast at the best of times, and this particular sanitization function was not quite doing the job.

Leave It Up To The Professionals

Now, most sane people don’t go about constructing SQL Statements by concatenating strings, especially when it comes to values that are coming from other parts of the system.

Ignoring SQL injection attacks for a second (its an on-premises application, you would already have to be inside the system to do any damage in that way), you are still taking on a bunch of unnecessary complexity in handling all of the user input sanitization yourself. You might not even get it right, as you can see in the example above.

Even in VB6 there exists libraries that do exactly that sort of thing for you, allowing you to use parameterized queries.

No muss, no fuss, just construct the text of the query (which is still built, but is built from controlled strings representing column names and parameter placeholders), jam some parameter values in and then execute. It doesn’t matter what’s in the parameters at that point, the library takes care of the rest.

Private Function Owner_LetPrepFees_CreateTransaction(ByVal psTransAudit As String, _
            ByVal plTransLink As Long, ByVal plTransPropertyID As Long, _
            ByVal plTransOwnerID As Long, ByVal psTransFromRef As String, ByVal psTransFromText As String, _
            ByVal plTransCreditorID As Long, ByVal psTransToRef As String, ByVal psTransToText As String, _
            ByVal plTransReference As Long, ByVal psTransType As String, ByVal psTransDetails As String, _
            ByVal pbGSTActive As Boolean, ByVal pnTransAmount As Currency, ByVal psTransLedgerType As String, _
            ByVal pbLetFee As Boolean, ByVal pbPrepFee As Boolean, ByVal plCount As Long) As Long            


    'Trims the parameters to fit inside the sql tables columns'
    psTransFromRef = Left$(Trim$(psTransFromRef), 8&)
    psTransFromText = Left$(Trim$(psTransFromText), 50&)
    psTransToRef = Left$(Trim$(psTransToRef), 8&)
    psTransToText = Left$(Trim$(psTransToText), 50&)
    psTransDetails = Left$(Trim$(psTransDetails), 50&)

    Dim sCommand    As String
    Dim sFieldList  As String
    Dim sValueList  As String
    Dim oCommand    As ADODB.Command
    Dim oParameter  As ADODB.Parameter

    Set oCommand = New Command
    oCommand.ActiveConnection = g_cnUser
    oCommand.CommandType = adCmdText
    oCommand.CommandTimeout = Val(GetRegistry("Settings", "SQL Command Timeout Override", "3600"))

    sFieldList = "AccountID, Created, UserId, Audit, Date"
    sValueList = "?" & ", GetDate()" & ", ?" & ", ?" & ", ?"
    oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , GetAccountID())
    oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , User.ID)
    oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 10, psTransAudit)
    oCommand.Parameters.Append oCommand.CreateParameter(, adDate, adParamInput, , UsingDate)

    If plTransLink <> 0& Then
        sFieldList = sFieldList & ", Link"
        sValueList = sValueList & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , plTransLink)
    End If

    If plTransPropertyID <> 0 Then
        sFieldList = sFieldList & ", PropertyId"
        sValueList = sValueList & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , plTransPropertyID)
    End If

    If plCount = 1& Then
        sFieldList = sFieldList & ", OwnerID" & ", FromRef" & ", FromText"
        sValueList = sValueList & ", ?" & ", ?" & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , plTransOwnerID)
        oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 8, psTransFromRef)
        oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 50, psTransFromText)
    Else
        sFieldList = sFieldList & ", CreditorId" & ", ToRef" & ", ToText"
        sValueList = sValueList & ", ?" & ", ?" & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , plTransCreditorID)
        oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 8, psTransToRef)
        oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 50, psTransToText)
    End If

    sFieldList = sFieldList & ", Reference" & ", TransType" & ", Details" & ", Amount" & ", LedgerType"
    sValueList = sValueList & ", ?" & ", ?" & ", ?" & ", ?" & ", ?"
    oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, 8, plTransReference)
    oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 2, psTransType)
    oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 100, psTransDetails)
    oCommand.Parameters.Append oCommand.CreateParameter(, adCurrency, adParamInput, , pnTransAmount)
    oCommand.Parameters.Append oCommand.CreateParameter(, adVarChar, adParamInput, 2, psTransLedgerType)

    If pbLetFee And psTransType = "CR" And psTransLedgerType = "JC" Then
        sFieldList = sFieldList & ", DisburseType"
        sValueList = sValueList & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , dtLetFeePayment)
    ElseIf pbPrepFee And psTransType = "CR" And psTransLedgerType = "JC" Then
        sFieldList = sFieldList & ", DisburseType"
        sValueList = sValueList & ",?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adInteger, adParamInput, , dtPrepFeePayment)
    End If

    If pbGSTActive Then
        sFieldList = sFieldList & ", GST"
        sValueList = sValueList & ", ?"
        oCommand.Parameters.Append oCommand.CreateParameter(, adBoolean, adParamInput, , pbGSTActive)
    End If

    Set oParameter = oCommand.CreateParameter(, adInteger, adParamReturnValue)
    oCommand.Parameters.Append oParameter
    sCommand = "INSERT INTO Transactions (" & sFieldList & ") VALUES (" & sValueList & "); SELECT ?=SCOPE_IDENTITY()"
    oCommand.CommandText = sCommand
    oCommand.Execute , , adExecuteNoRecords
    Exit Function    
    
    Owner_LetPrepFees_CreateTransaction = oParameter.Value
    Set oCommand = Nothing
    
End Function

Of course, because VB6 is a positively ancient language, its not quite as easy as all that.

Normally, in .NET or something similar, if you are constructing a parameterized query yourself, you’d probably use a query builder of some sort, and you would name your parameters so that the resulting query was easy to read as it goes through profilers and logs and whatnot.

No such luck here, the only parameter placeholder that seems to be supported is the classic ?, which means the parameters are positional. This can be dangerous when the code is edited in the future, but if we’re lucky, maybe that won’t happen.

Its still better than jamming some strings together though.

Conclusion

I think of this as a textbook case for never just “fixing the bug” as its stated. Sure, we could have just edited the SQL_ValidateString function to be more robust in the face of a particular arrangement of special characters, but that’s just putting a bandaid on a bad solution.

Instead we spent the time to work with the language and the constructs available to put a solution in place that is a lot more robust in the face of unexpected input.

And lets be honest, at this point, the only input we get is the unexpected kind.