HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

mysql_safe_query()

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mysql_safe_querystackoverflowprogramming

Problem

I have been thinking of a sql-injection free implementation in dynamic languages. Here's what I came with. All the code was written just for fun and learning purposes.

I would like to share it and get some feedback about it.

Mainly about performance, efficiency and usability.

All the information you need is directly written to documentation inside the script:

```
<?php

/// @license public domain
/// @brief This function works exactly like mysql_real_query, but it escapes all data for you.
/// This function is an example how dynamic languages can deal with sql injection.
/// And was created mainly for learning purpose.
/// There are two prototypes for this function:
/// mysql_safe_query(string $format[, ...])
/// mysql_safe_query(mysql $mysql, string $format [, ...])
/// Example:
/// mysql_safe_query("SELECT * FROM test WHERE a = ? AND b = ? LIMIT 0, ?", 20.21, "mal'form'ed", 12);
/// Output:
/// SELECT * FROM test WHERE a = 20.21 AND b = X'6d616c27666f726d276564' LIMIT 0, 12
/// @param resource $mysql Optional - pass a $mysql link
/// @param string $format Format of the query with placeholders ('?') instead of values
/// @param mixed ... values/data to be queried with
/// @return A result from mysql_real_query, or false with an error generated when
/// number of parameters is larger than the number of placeholder.
/// @warning In case that there's more placeholders than parameters, they will be replaced with NULL
/// @note You don't have to call mysql_set_charset before invoking this function, since this
/// function is not using mysql_real_escape_string but bin2hex instead:
/// @see https://dev.mysql.com/doc/refman/5.0/en/hexadecimal-literals.html
/// @note You can't bind keywords, as all strings are escaped
/// @note Although mysql_* functions are deprecated they are used by this function to maintain
/// MySQL C API compatibilty. Feel free to change it to whatever api you are using.
function mysql_safe_query(/$mysql, $format, .../)
{
//

Solution

Validating the arguments

You validate the arguments a bit late,
after processing the query placeholders:

// if we got any unprocessed arguments, its an error
if (count($args))
{
    // emit error
    trigger_error("Count of bound parameters doesnt equal to count of passed parameters!", E_USER_ERROR);
    return false;
}


For one thing, the error message is not accurate:
it is triggered when there are more arguments than query placeholders.
If there are too few arguments, then any excess placeholders are replaced with NULL, as duly noted in the documentation.

It would be somewhat cleaner to check the parameter count and the ? count as early as possible.
Admittedly, that would incur an additional string search step compared to your current solution, and as such less efficient.

Is the return false necessary? On my system the execution stops at trigger_error.

Limitations

It's of concern, and at least should be mentioned in the documentation that a query cannot have ? characters in SQL string values. For example this won't work as expected:

mysql_safe_query("UPDATE Posts SET question = 'How are you?' WHERE id = ?", 12);


Ideally ? symbols within SQL strings (enclosed within pairs of single quotes) should not be treated as placeholders,
but obviously that would make the implementation significantly more complex.

Nitpicks

It's generally recommended to have one statement per line.
This kind of loop condition is considered tricky, hard to read, error-prone:

while (FALSE !== ($offset = strpos($query, '?', $offset)))


Although slightly longer, but less confusing to write this way:

while (true)
{
    $offset = strpos($query, '?', $offset);
    if (FALSE === $offset) break;

Code Snippets

// if we got any unprocessed arguments, its an error
if (count($args))
{
    // emit error
    trigger_error("Count of bound parameters doesnt equal to count of passed parameters!", E_USER_ERROR);
    return false;
}
mysql_safe_query("UPDATE Posts SET question = 'How are you?' WHERE id = ?", 12);
while (FALSE !== ($offset = strpos($query, '?', $offset)))
while (true)
{
    $offset = strpos($query, '?', $offset);
    if (FALSE === $offset) break;

Context

StackExchange Code Review Q#98138, answer score: 3

Revisions (0)

No revisions yet.