current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

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, ...*/)
{
    // get all arguments passed to this function
    $args = func_get_args();

    // check whether first parameter is a link
    if (is_resource($args[0]))
        $link = array_shift($args);

    // get $format argument passed
    $query = array_shift($args);
    $offset = 0;

    // for each placeholder at $offset do the following
    while (FALSE !== ($offset = strpos($query, '?', $offset)))
    {
        // fetch next parameter passed
        $param = array_shift($args);

        // if next parameter is a string, we must escape it
        if (is_string($param))
            $param = "X'" . bin2hex($param) . "'";
        // datatype NULL should became unquoted NULL string
        else if (is_null($param))
            $param = "NULL";

        // now just replace the placeholder with actual value
        $query = substr_replace($query, strval($param), $offset, 1);
    }

    // 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;
    }

    // if we haven't got a link
    if (!isset($link))
        return mysql_real_query($query);

    // we got the link
    return mysql_real_query($link, $query);
}
share|improve this question

1 Answer 1

up vote 4 down vote accepted
+50

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;
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.