开发者

preg_replace & mysql_real_escape_string problem cleaning SQL

check out the method below. If entered value in text box is \ mysql_real_escape_string will return duble backslash but preg_replace will return SQL with only one backslash. Im not that good with regular expression so plz help.

$sql = "INSERT INTO tbl SET val='?'";
$params = array('someval');

public function execute($sql, array $params){
    $keys = a开发者_StackOverflow中文版rray();

    foreach ($params as $key => $value) {
        $keys[] = '/[?]/';
        if (get_magic_quotes_gpc()) {
            $value = stripslashes($value);
        }
        $paramsEscaped[$key] = mysql_real_escape_string(trim($value));
    }

    $sql = preg_replace($keys, $paramsEscaped, $sql, 1, $count);
    return $this->query($sql);

}


For me it basically looks like you're re-inventing the wheel and your concept has some serious flaws:

  • It assumes get_magic_quotes_gpc could be switched on. This feature is broken. You should not code against it. Instead make your application require that it is switched off.
  • mysql_real_escape_string needs a database link identifier to properly work. You are not providing any. This is a serious issue, you should change your concept.
  • You're actually not using prepared statements, but you mimic the syntax of those. This is fooling other developers who might think that it is safe to use the code while it is not. This is highly discouraged.

However let's do it, but just don't use preg_replace for the job. That's for various reasons, but especially, as the first pattern of ? results in replacing everything with the first parameter. It's inflexible to deal with the error-cases like too less or too many parameters/placeholders. And additionally imagine a string you insert contains a ? character as well. It would break it. Instead, the already processed part as well as the replacement needs to be skipped (Demo of such).

For that you need to go through, take it apart and process it:

public function execute($sql, array $params)
{
    $params = array_map(array($this, 'filter_value'), $params);

    $sql = $this->expand_placeholders($sql, $params);

    return $this->query($sql);
}

public function filter_value($value)
{
    if (get_magic_quotes_gpc())
    {
       $value = stripslashes($value);
    }
    $value = trim($value);
    $value = mysql_real_escape_string($value);
    return $value;
}

public function expand_placeholders($sql, array $params)
{
    $sql = (string) $sql;
    $params = array_values($params);
    $offset = 0;
    foreach($params as $param)
    {
        $place = strpos($sql, '?', $offset);
        if ($place === false)
        {
            throw new InvalidArgumentException('Parameter / Placeholder count mismatch. Not enough placeholders for all parameters.');
        }
        $sql = substr_replace($sql, $param, $place, 1);
        $offset = $place + strlen($param);
    }
    $place = strpos($sql, '?', $offset);
    if ($place === false)
    {
        throw new InvalidArgumentException('Parameter / Placeholder count mismatch. Too many placeholders.');
    }
    return $sql;
}

The benefit with already existing prepared statements is, that they actually work. You should really consider to use those. For playing things like that is nice, but you need to deal with much more cases in the end and it's far easier to re-use an existing component tested by thousand of other users.


It's better to use prepared statement. See more info http://www.php.net/manual/en/pdo.prepared-statements.php

0

上一篇:

下一篇:

精彩评论

暂无评论...
验证码 换一张
取 消

最新问答

问答排行榜