PHP Unlocker

PHP-Unlocker is a static analysis tool that detects potential, unintended DB table locks for PHP applications using ADOdb. It searches your code for improper usage of StartTrans() and CompleteTrans() methods.

So why write this tool? It’s to scratch our own need, because we had an application with bad coding practices. We’d constantly see errors in production about timeouts because DB write operation could not obtain a lock on a table.

The Problem

This is how problematic code looks like:

public function someMethod($argument) {

    Connections::$dbConn->StartTrans();

    $result = $this->doSomeStuff($argument);
    $this->writeInTheDB($result);        

    Connections::$dbConn->CompleteTrans();

    return $result;
}

Imagine what would happen when doSomeStuff() throws an exception. If there is no try/catch block (that also needs to properly handle the error) somewhere up the call stack, CompleteTrans() will never be called.

Now, depending on how your transactions are set and database is configured, a lock on a table may be in effect that does not allow write operations until CompleteTrans() is called or until certain timeout has expired.

For example, this situation happens when the transaction access mode is set to ‘READ ONLY’ and innodb_lock_wait_timeout is set to any value (by default, for MySQL it’s set to 50 seconds).

The Solution

OK, so what is the solution? The whole DB operation needs to be wrapped in try/catch block that properly cleans after itself in case of an exception. Here is how the above code should look like:

public function someMethod($argument) {

     try {
         Connections::$dbConn->StartTrans();

         $result = $this->doSomeStuff($argument);
         $this->writeInTheDB($result);        

         Connections::$dbConn->CompleteTrans();

         return $result;
     } catch(Exception $exception) {
         Connections::$dbConn->FailTrans();
         Connections::$dbConn->CompleteTrans();
         return $exception->getMessage();   
     }         
}

Sometimes, because of ignorance, laziness or just because of pressure, the developers skip these ‘extra’ details. In case of an exception, the transaction is properly handled in the catch block, first by calling FailTrans() and then by calling CompleteTrans().

What PHP-Unlocker does is, to check where in the code there are calls to StartTrans(). Then it goes back to the beginning of the method and checks if this call is wrapped in try/catch statement. If it is wrapped in try/catch, then the tool checks the catch block. In the catch block, there needs to be at least two calls: FailTrans() and CompleteTrans(). If any of the above mentioned conditions are false, then PHP-Unlocker will flag the line of code as problematic.

Usage

Check the github page for usage instructions.

Miscellaneous

What if a line is a false positive? I’ve never seen this happen in our codebase so far (160,000 KLOC - no empty lines, no commented lines, no tests code). If it happens, let me know.

I’ve written another tool that uses the same PHP parser - PHP-Reaper, that detects SQL injection in ADOdb code. There is an option in that tool to ignore a false positive, by adding a specific line comment. However PHP-Reaper is more complicated (e.g. tries to figure out where the variable used in a string concatenation is coming from). For the same codebase we’ve noticed some false positives and that’s why we added the silencing option to PHP-Reaper.

PHP-Unlocker is more simple and more reliable. I also think that if you have problematic code construction like the one above you should just fix it with away.

Future Work

Currently we’re moving away from PHP and gradually migrating functionalities to Java. I probably will not be able to support this project much longer. But if you have a burning need let me know.

On that note, Java has a better tooling around static code analysis. Findbugs has warnings about closing a resources in case of an exception. Also with the introduction in Java 7 of try-with-resources the manual labour to close resources in case of an exception becomes thing of the past.