CodeSOD: An Incremental Replacement

Jon sends us an entire function, but I'm just going start with the comment, after which you'll understand everything bad about this without me explaining further. Don't worry, I will, but the comment says it all:

/** * Returns the next ID of a table. This function is a replacement for MySQL's * auto-increment so that we don't need it anymore. * * @param string $table The name of the table * @param string $id The name of the ID column * @return integer */

"This function is a replacement for MySQL's auto-increment so that we don't need it anymore."

Replacing built-in functions of your database is a great strategy, I can't possibly see anything going wrong. But let's look at the implementation, which is actually worse than the comment indicates:

public function nextID($table, $id){ $select = sprintf(" SELECT MAX(%s) AS current_id FROM %s", $id, $table); $result = $this->query($select); $currentID = mysql_result($result, 0, 'current_id'); return ($currentID + 1);}

Now sure, we all knew there was going to be a SQL injection vulnerability in this code, before we even looked at it. I have to give them credit, though, for doing it via sprintf, instead of just plain-old string concatenation, though. Either they read the docs or they started life as a C developer.

In any case, this finds the MAX value of any arbitrary column in any table, and then returns the increment of that MAX. As the name implies, it's supposed to accept id columns, but there's nothing enforcing that.

You know what else isn't enforced? Transactions and error handling. This function is incredibly unreliable, because two simultaneous queries can easily end up in a race condition, since there's nothing atomic about this.

At least they don't need the built-in auto-increment anymore, though.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!