How to become an author
Search
Write a publication
Pull to refresh
0
Rating
Badoo
Big Dating

Автоматизированный рефакторинг в большом проекте

Reading time19 min
Views17K
Badoo corporate blogPHP*Designing and refactoring*
Если вы работаете в большой команде разработчиков над одним и тем же проектом, то рефакторинг становится очень сложной задачей. Приведем пример: мы хотим переименовать функцию do_something() в do_something_with_blackjack(). Мы переименовали все вхождения этой функции в своей ветке и отправили задачу на тестирование. В тот же момент кто-то другой добавил ещё один вызов функции, но со старым названием, тоже в своей ветке. По отдельности наборы изменений будут работать, а вот после слияния получится ошибка.

В статье будет рассмотрен приём, который можно назвать «автоматизированный рефакторинг» — использование самописных скриптов, которые делают нужную работу за вас, позволяя провести рефакторинг после слияния всех веток и перед непосредственной выкладкой на staging/production.

На примере phpBB будет показано, как можно «отрефакторить» вызовы SQL-запросов, чтобы они использовали автоматическое экранирование входных данных (и таким образом помочь в решении проблемы SQL-инъекций).

Описание подхода


Начнем с теории: опишем проблему и предлагаемые способы её решения.

Проблемы автоматического слияния изменений


Предположим, что мы занимаемся рефакторингом кода и хотим переименовать функцию (в примерах — код на PHP). Допустим, мы изменили код следующим образом (первый символ «-» означает удаление строки, а «+» означает добавление строки):

 <?php
-function do_something()
+function print_hello()
 {
     echo "Hello world!\n";
 }
 
 $a = 1;
-do_something();
+print_hello();
 $b = 2;
 $c = 3;

Пока мы это делали, наш коллега успел добавить использование старого имени функции в другой ветке:

 <?php
 function do_something()
 {
     echo "Hello world!\n";
 }

 $a = 1;
 do_something();
 $b = 2;
+do_something();
 $c = 3;

Поскольку изменения непротиворечивы (мы работали с разными строками кода), после автоматического слияния наших веток с изменениями получится следующий код:

<?php
function print_hello()
{
    echo "Hello world!\n";
}

$a = 1;
print_hello();
$b = 2;
do_something();
$c = 3;

Таким образом, в конечном варианте будет вызов более несуществующей функции (do_something). Такой код, очевидно, не будет работать. Некоторые приводят возможность такого поведения в качестве аргумента против использования «feature branches», когда под одну задачу используют одну ветку. Можно предлагать различные решения этой проблемы, но понятно одно: она не решается легко, и за возможность рефакторинга в активно развивающемся проекте приходится дорого платить.

Автоматизированный рефакторинг


Суть подхода заключается в том, что мы должны отложить выполнение большинства изменений в коде таким образом, чтобы замены выполнялись уже после того, как произошло слияние всех веток. То есть сначала нужно применить изменения, сделанные коллегами, и только потом приступить к своей работе и выполнить переименование после всех остальных изменений. Также нужно ввести механизм, который бы запрещал использование кода в старом стиле.

Следовательно, для выполнения рефакторинга нужно написание следующих скриптов:

  • проверка на использование старого кода;
  • выполнение автоматической замены;
  • сбор статистики использования старого и нового кода (опционально).

Подробнее о каждом из них.

Проверка на использование старого кода


Чтобы не допустить использования кода в старом стиле в процессе рефакторинга и после его завершения, нужно написать скрипт, который бы позволил однозначно и надежно установить, что код более не используется. Для нашего простого примера скрипт будет выглядеть примерно так:

#!/bin/sh
grep do_something file.php

Скрипт возвращает строки в файле, которые содержат старое имя функции — do_something.

Выполнение автоматической замены


Для нашего простенького примера будет достаточно использовать sed, чтобы заменить все вхождения do_something на print_hello. В более сложных случаях этого будет недостаточно и понадобится более аккуратная обработка (например, если подстрока do_something может присутствовать в проекте и не являться вызовом функции).

#!/bin/sh
sed -i 's/do_something/print_hello/g' file.php

Сбор статистики использования старого и нового кода


Наш скрипт для замены хоть и должен заменить все вхождения do_something на print_hello, но он ничего не сможет сделать, например, с таким кодом:

$func_prefix = "do_";
$func_name = $func_prefix . "something";

// да, PHP позволяет вызывать функцию по её имени, записанному в переменной
$func_name();

Такая проблема существует даже в статически типизированных языках, например С и Java. В C можно всегда написать #define и наделать макросов, а в Java существует механизм Reflection. Для этого введем новую версию функции do_something (нужно позаботиться о том, чтобы эта функция не была автоматически заменена скриптом выше):

function do_something()
{
    error_log(__FUNCTION__ . " found. Trace: " . (new Exception()));
    return print_hello();
}

В результате, после выполнения юнит-тестов или после выкладки этого кода в production (если у вас нет 100% покрытия), вы сможете найти пропущенные упоминания старой функции. Эти фрагменты можно проанализировать и выполнить соответствующую замену вручную.

Этапы применения написанных скриптов


После того как соответствующие скрипты написаны, нужно их правильно применить. Предлагается делать это следующим образом:

  1. выполнить слияние всех последних изменений разработчиков;
  2. выполнить скрипт по автоматической замене и зафиксировать изменения в системе контроля версий;
  3. проверить, что упоминаний старого кода больше нет;
  4. добавить логирование использования старого кода;
  5. прогнать юнит-тесты, выложить код на production;
  6. сразу после попадания кода в основную ветку разработки нужно настроить «хуки» своей системы контроля версий так, чтобы она более не пропускала использование старого кода;
  7. добавить проверку использования старого кода в скрипты, которые запускаются перед попаданием кода на production;
  8. (опционально) собрать с production логи использования старого кода и исправить соответствующие места вручную.
  9. Вы успешно выполнили рефакторинг!

Наличие пункта 7 важно, потому что в репозитории может присутствовать старый код в других ветках, которые ещё не были слиты с основной веткой разработки. Слияние таких веток может пройти без конфликтов, но в них могут присутствовать упоминания старого кода.

Важным моментом в описываемой методике является возможность применения изменений одномоментно, с помощью скриптов и небольшого количества ручной работы. Выполнение большого рефакторинга полностью вручную также возможно, но это очень тяжелый путь, по которому нужно идти, только если у вас не остается других вариантов.

Рефакторинг использования SQL в phpBB


Для иллюстрации подхода на более приближенных к реальности примерах мы бы хотели показать, как отрефакторить код phpBB, чтобы максимально избавить его от SQL-инъекций:

Работа с SQL в phpBB


Давайте разберёмся, как же устроена работа с SQL в phpBB и почему используемый механизм несовершенен.

Во-первых, реализация самих классов работы с базой данных находится в «includes/db», и в «common.php» создается глобальная переменная $db, которая содержит экземпляр соответствующего класса для работы с БД.

$ ls includes/db
db_tools.php    index.htm       mssqlnative.php oracle.php
dbal.php        mssql.php       mysql.php       postgres.php
firebird.php    mssql_odbc.php  mysqli.php      sqlite.php

Таким образом, директория «includes/db» должна исключаться из наших скриптов при автоматической замене — соответствующие замены мы проведем вручную.

Чтобы оценить масштаб проблемы, воспользуемся замечательным средством под названием grep:
$ grep -RF 'sql_query(' * | wc -l
    1611

То есть примерно 1 600 обращений к sql_query(). Полагаем, становится понятно, что вручную заменить такое количество мест — не самая лучшая идея. Вероятно, такое количество мест является одной из причин, почему разработчики phpBB до сих пор ничего не сделали с этими вызовами.

Давайте теперь всё же посмотрим, что с ними не так?

Вот определение sql_query:
function sql_query($query = '', $cache_ttl = 0) …

Отсюда мы можем легко видеть, что метод получает только лишь сам запрос, а экранирование значений должна выполнять вызывающая сторона самостоятельно.

Пример из viewtopic.php:

$sql = 'SELECT forum_id
FROM ' . TOPICS_TABLE . "
	WHERE topic_id = $topic_id";
$result = $db->sql_query($sql);

Поскольку $topic_id приходит извне и может не обрабатываться должным образом, существует возможность получить как ошибку SQL, так и настоящую SQL-инъекцию. Использование UNION может даже позволить нам вытащить данные из другой таблицы, поэтому стоит всеми силами избегать написания такого кода.

Вместо этого мы могли бы написать что-нибудь наподобие следующего:

$sql = 'SELECT forum_id
FROM ' . TOPICS_TABLE . '
	WHERE topic_id = ?d';
$result = $db->sql_query_escaped($sql, $topic_id);

То есть вместо «$topic_id» написать «?d», которое будет воспринято методом sql_query_escaped и будет всегда явно обработано как число.

SQL-запросы с автоматическим экранированием значений


Предлагается для примера сделать простейшую обёртку следующего вида и поместить её в dbal.php:

/**
* Execute escaped SQL query
* 
* First parameter, $query_template is the SQL template that has the following placeholders:
*  
*  ?d   - value is integer
*  ?s   - value is string (escaped by default)
*  ?a   - array of integers for usage like IN(?a)
*  ?r   - raw SQL (e.g. for use in dynamic part of SQL expression)
*
* Example:
* 
*   sql_query_escaped("SELECT * FROM table WHERE table_id = ?d", 42) will be executed as
*
*     SELECT * FROM table WHERE table_id = 42
*
*   sql_query_escaped("SELECT * FROM table WHERE table_id IN(?a)", array(1, 2, 3)) will be executed as
*
*     SELECT * FROM table WHERE table_id IN(1, 2, 3)
*
*/
function sql_query_escaped($query_template)
{
    $values = func_get_args();
    array_shift($values);

    $regexp = '/\\?[dsar]/s';
    preg_match_all($regexp, $query_template, $matches);
    foreach ($matches[0] as $i => $m) {
        if ($m == '?d') $values[$i] = intval($values[$i]);
        else if ($m == '?s') $values[$i] = "'" . $this->sql_escape($values[$i]) . "'";
        else if ($m == '?a') $values[$i] = implode(',', array_map('intval', $values[$i]));
    }
    
    $idx = 0;
    $replace_func = function($placeholder) use ($values, &$idx) {
        $placeholder = $placeholder[0];
        return $values[$idx++];
    };

    $query = preg_replace_callback($regexp, $replace_func, $query_template);
    return $this->sql_query($query);
}

Реализация и способ решения проблемы для нас не столь принципиальны, потому что это просто пример. Если вы действительно захотите делать что-то подобное, то можно придумать решение получше или использовать стандартные решения, например PDO.

Отладка SQL-запросов


В phpBB по какой-то причине отсутствует простой способ выводить SQL-запросы на текущей странице, поэтому добавим небольшой временный патч:

--- a/includes/db/mysqli.php
+++ b/includes/db/mysqli.php
@@ -151,6 +151,8 @@ class dbal_mysqli extends dbal
 		return true;
 	}
 
+	var $queries = array();
+
 	/**
 	* Base query method
 	*
@@ -162,6 +164,7 @@ class dbal_mysqli extends dbal
 	*/
 	function sql_query($query = '', $cache_ttl = 0)
 	{
+		$this->queries[] = $query;
 		if ($query != '')
 		{
 			global $cache;

--- a/includes/functions.php
+++ b/includes/functions.php
@@ -4735,6 +4735,10 @@ function page_footer($run_cron = true)
 			}
 
 			$debug_output .= ' | <a href="' . build_url() . '&explain=1">Explain</a>';
+
+			foreach ($db->queries as $query) {
+				$debug_output .= "<pre style='text-align: left; margin-bottom: 10px;'>" . htmlspecialchars($query) . "</pre>\n<hr/>\n";
+			}
 		}
 	}


Анализ использования sql_query()


Одним из самых важных моментов во время написания скриптов для автоматического рефакторинга является так называемый «метод пристального взгляда». Мы должны посмотреть, как используется тот метод, который мы собираемся поменять, и найти основные паттерны его использования путём внимательного всматривания в код.

Итак, ещё раз воспользуемся grep, но на этот раз просмотрим глазами упоминания sql_query() и попытаемся сформулировать основные найденные паттерны:
$ grep -RF 'sql_query(' * | less

Основные паттерны:

  1. $result = $db->sql_query($sql); // переменная $sql содержит текст запроса;
  2. $db->sql_query(«DELETE|UPDATE|SELECT ...»); // запрос указывается явно.

Также можно заметить, что всегда используется одно и то же имя для объекта базы данных — «$db». Тем не менее, нужно обязательно убедиться, что объект базы данных не используется под другим именем и в других классах нет метода sql_query:
$ grep -RF 'sql_query(' * | grep -vF '$db->sql_query('

Выполнив этот код, мы увидим, что упоминания всё же есть. Их можно разделить на несколько категорий:

  • docs/*.html — пропускаем, поскольку это документация;
  • includes/db/db_tools.php — класс содержит методы sql_table_exists, sql_alter_table и другие DDL;
  • includes/db/dbal.php: используется в методе sql_query_limit, а также в методах insert, которые сами экранируют значения;
  • includes/db/(mysql|mssql,firebird|...).php — вызовы mysql_query, mssql_query и др.;
  • includes/functions_convert.php — функции для конвертации базы (с одной версии phpBB на другую);
  • install/convertors/convert_*.php — скрипты для конвертации базы;
  • install/install_convert.php — также скрипт для конвертации базы.

Из всего этого нам действительно стоит обратить внимание на вызовы sql_query() в наследниках dbal, а также в самом dbal. Таким образом, из нашего дополнительного анализа стало понятно, что нам требуется также отрефакторить все вызовы sql_query_limit.

Скрипты для конвертации базы теоретически тоже могут содержать SQL-инъекции. Тем не менее, эти скрипты составляют малую часть всех SQL-запросов и скорее всего SQL-инъекций не содержат из-за их специфики. Поэтому эти места мы пропустим.

Анализ паттерна $db->sql_query('SELECT/UPDATE/DELETE ...')


Давайте теперь напишем скрипт, который найдет нам все упоминания sql_query, когда SQL-запрос написан напрямую. Таких мест не слишком много по сравнению с sql_query($sql), но для начала выберем более простую задачу.

Алгоритм следующий: разобьём содержимое файлов на токены и найдем там последовательность «$db», «->», и «sql_query». После этого выражение в скобках будет тем самым запросом. Код будем писать на PHP 5.3 с использованием расширения tokenizer.

<?php
// Для начала оставим только файлы с тем паттерном, который мы собираемся заменять:
$files = exec('grep -RF \'$db->sql_query(\' * | awk -F: \'{print $1;}\'', $out, $retval);
if ($retval) exit(1);

// Наш скрипт будет жить в папке replacer/
$excludes = array('includes/db/', 'replacer/');

$num_lines = 0;

foreach (array_unique($out) as $filename) {
	foreach ($excludes as $excl) {
		if (strpos($filename, $excl) === 0) continue(2);
	}

	$contents = file_get_contents($filename);
	if ($contents === false) exit(1);
	
	$lines = explode("\n", $contents);

	// Получаем массив всех токенов в файле
	$tokens = token_get_all($contents);
	$num = count($tokens);
	$line = 1;
	$type = $text = '';

	// Нам нужно найти 5 идущих подряд токенов: '$db', '->', 'sql_query', '(' и начало запроса
	// Сам запрос должен начинаться с токена с типом «строка»
	$accepted_value_types = array(T_CONSTANT_ENCAPSED_STRING, T_ENCAPSED_AND_WHITESPACE, '"');
	
	foreach ($tokens as $cur_idx => $tok) {
		parse_token($tok, $type, $text, $line);

		// Пропускаем токены в самом конце, где точно не могут уместиться наши 5 токенов
		if ($cur_idx >= $num - 5) continue;
		
		$ln = $line;
		$i = $cur_idx + 1;

		// Список токенов доступен в документации (http://php.net/manual/tokens.php)
		if ($type !== T_VARIABLE || $text !== '$db') continue;
		parse_token($tokens[$i++], $type, $text, $ln);
		if ($type !== T_OBJECT_OPERATOR || $text !== '->') continue;
		parse_token($tokens[$i++], $type, $text, $ln);
		if ($type !== T_STRING || $text !== 'sql_query') continue;
		parse_token($tokens[$i++], $type, $text, $ln);
		if ($type !== '(') continue;
		parse_token($tokens[$i++], $type, $text, $ln);
		if (!in_array($type, $accepted_value_types, true)) continue;

		// Теперь получим весь запрос: нам нужно всё, что находится до закрывающей скобки
		$depth = 1;
		$query = '';
		
		for ($i = $i - 1; $i < $num; $i++) {
			parse_token($tokens[$i], $type, $text, $ln);
			if ($type === '(') $depth++;
			else if ($type === ')') $depth--;
			if ($depth == 0) break;

			$query .= $text;
		}

		$query = preg_replace(array("/[\t ]+/s", '/^/m'), array(' ', '      '), $query);
		$results[$filename][] = $query;
		
		$num_lines++;
	}
}

foreach ($results as $filename => $list) {
	echo "$filename\n";
	foreach ($list as $row) echo "$row\n";
	echo "\n";
}

echo "Total lines recognized: $num_lines\n";

// См. формат возвращаемых значений в документации к token_get_all
function parse_token($tok, &$type, &$text, &$line)
{
	if (is_array($tok)) list($type, $text, $line) = $tok;
	else $type = $text = $tok;
}

После запуска увидим, что наш скрипт распознал примерно 130 строк, что составляет около 8% всех текстовых вхождений «sql_query» в проекте. Можно также заметить, что значительная часть найденных строк — это строки вида

includes/acp/acp_attachments.php
      'INSERT INTO ' . EXTENSIONS_TABLE . ' ' . $db->sql_build_array('INSERT', $sql_ary)

includes/acp/acp_bbcodes.php
      'INSERT INTO ' . BBCODES_TABLE . $db->sql_build_array('INSERT', $sql_ary)

То есть используется метод sql_build_array для вставки значений в поля, а значения в этом методе уже экранируются и здесь можно ничего не трогать.

Анализ паттерна $db->sql_query($sql)


Теперь давайте узнаем, как получить текст SQL-запроса в случае, когда его значение берется из переменной $sql. Посмотрим на контекст вызова запроса и попробуем найти способ однозначно получить сам запрос:
$ grep -A 5 -B 5 -RF '$db->sql_query($sql' * | less

Из кода можно увидеть, что большая часть запросов вызывается следующим образом::
$sql = '...'; // текст запроса
$db->sql_query($sql);

Другими словами, определение переменной $sql и присвоение значения происходит непосредственно перед вызовом sql_query(). Поэтому напишем обработку именно такого случая как наиболее распространенного и простого. Прежде всего распечатаем строки с запросами, чтобы было понятно, с чем мы имеем дело:

<?php
// Для начала оставим только файлы с тем паттерном, который мы собираемся заменять
$files = exec('grep -RF \'$db->sql_query($sql\' * | awk -F: \'{print $1;}\'', $out, $retval);
if ($retval) exit(1);

// Наш скрипт будет жить в папке replacer/
$excludes = array('includes/db/', 'replacer/');

$num_lines = 0;

foreach (array_unique($out) as $filename) {
	foreach ($excludes as $excl) {
		if (strpos($filename, $excl) === 0) continue(2);
	}
	
	echo "$filename\n";
	
	$contents = file_get_contents($filename);
	if ($contents === false) exit(1);
	
	$lines = explode("\n", $contents);

	// Получаем массив всех токенов в файле
	$tokens = token_get_all($contents);
	$num = count($tokens);
	$line = 1;
	$type = $text = '';

	// Нам нужно найти 5 идущих подряд токенов: '$db', '->', 'sql_query', '(' и '$sql'
	foreach ($tokens as $cur_idx => $tok) {
		parse_token($tok, $type, $text, $line);

		// Пропускаем токены в самом конце, где точно не могут уместиться наши 5 токенов
		if ($cur_idx >= $num - 5) continue;
		
		$ln = $line;
		$i = $cur_idx + 1;

		// Список токенов доступен в документации (http://php.net/manual/tokens.php)
		if ($type !== T_VARIABLE || $text !== '$db') continue;
		parse_token($tokens[$i++], $type, $text, $ln);
		if ($type !== T_OBJECT_OPERATOR || $text !== '->') continue;
		parse_token($tokens[$i++], $type, $text, $ln);
		if ($type !== T_STRING || $text !== 'sql_query') continue;
		parse_token($tokens[$i++], $type, $text, $ln);
		if ($type !== '(') continue;
		parse_token($tokens[$i++], $type, $text, $ln);
		if ($type !== T_VARIABLE || $text !== '$sql') continue;

		// Для проверки выведем найденные строки. Вывод должен быть аналогичен grep
		printf("%6d    %s\n", $line, ltrim($lines[$line - 1]));
		
		$positions = find_dollar_sql_assignment($tokens, $cur_idx);
		
		if (!is_array($positions)) {
			echo "       $positions\n\n";
			continue;
		}
		
		$query = '';
		for ($i = $positions['begin']; $i <= $positions['end']; $i++) {
			parse_token($tokens[$i], $type, $text, $ln);
			$query .= $text;
		}

		$query = preg_replace(array("/[\t ]+/s", '/^/m'), array(' ', '      '), $query);
		echo "\n" . $query . "\n\n";
		
		$num_lines++;
	}
	
	echo "\n";
}

echo "Total lines recognized: $num_lines\n";

// См. формат возвращаемых значений в документации к token_get_all
function parse_token($tok, &$type, &$text, &$line)
{
	if (is_array($tok)) list($type, $text, $line) = $tok;
	else $type = $text = $tok;
}

// Найти $sql = '...'; перед $db->sql_query($sql
function find_dollar_sql_assignment($tokens, $db_idx)
{
	$depth = 0;
	$line = 1;
	$type = $text = '';

	// Пройдемся по предыдущим токенам и найдем упоминания $sql
	for ($idx = $db_idx - 1; $idx > 0; $idx--) {
		parse_token($tokens[$idx], $type, $text, $line);
		
		// учет вложенности скобок
		if ($type === ')') $depth++;
		else if ($type === '(') $depth--;
		
		if ($depth < 0) {
			return "depth < 0 on line " . __LINE__;
		}
		
		/*
		Мы ничего не можем сделать в случаях следующего вида, поэтому
		их нужно определять и пропускать:
		     
			if (...) {
				$sql = 'SELECT * FROM Table1 WHERE user_id = $user_id';
			} else {
				$sql = 'SELECT * FROM Table2';
			}
		
			$result = $db->sql_query($sql);
		*/
		if ($type === '{' || $type === '}') {
			return "open/close brace found on line " . __LINE__;
		}
		
		if ($type === T_VARIABLE && $text === '$sql') {
			$i = $idx + 1;
			parse_token($tokens[$i++], $type, $text, $line);
			if ($type === T_WHITESPACE) parse_token($tokens[$i++], $type, $text, $line);
			if ($type !== '=') continue;
			// мы нашли "$sql = " !
			
			$begin_pos = $i;
			break;
		}
	}
	
	// Мы уже нашли начало присваивания «$sql = », теперь нужно найти конец
	// Будем считать, что выражение у нас всегда оканчивается на «;»
	
	$depth = 0;
	for ($idx = $begin_pos; $idx < $db_idx; $idx++) {
		parse_token($tokens[$idx], $type, $text, $line);
		
		if ($type === '(') $depth++;
		else if ($type === ')') $depth--;
		
		if ($depth < 0) {
			return "depth < 0 on line " . __LINE__;
		}
		
		if ($depth === 0 && $type === ';') {
			return array('begin' => $begin_pos, 'end' => $idx - 1);
		}
	}

	// Если цикл закончился, значит между найденным нами началом и $db->sql_query нет «;»
	// То есть, наше предположение было неверно
	
	return "Statement does not terminate on line " . __LINE__;
}

Помимо непосредственного вывода запросов мы также выводим в конце количество распознанных строк (около 1150). Это составляет примерно 70% от всех текстовых вхождений sql_query! Давайте теперь приведём примеры вывода нашего скрипта:

cron.php
    59    $db->sql_query($sql);

       'UPDATE ' . CONFIG_TABLE . "
       SET config_value = '" . $db->sql_escape(CRON_ID) . "'
       WHERE config_name = 'cron_lock' AND config_value = '" . $db->sql_escape($config['cron_lock']) . "'"

…

includes/acp/acp_forums.php
   243    $result = $db->sql_query($sql);

       'SELECT *
       FROM ' . FORUMS_TABLE . "
       WHERE forum_id = $forum_id"
...
  1096    $db->sql_query($sql);

       'DELETE FROM ' . FORUMS_TABLE . '
       WHERE ' . $db->sql_in_set('forum_id', $forum_ids)

includes/acp/acp_reasons.php
...
    96    $result = $db->sql_query($sql);

       'SELECT reason_id
       FROM ' . REPORTS_REASONS_TABLE . "
       WHERE reason_title = '" . $db->sql_escape($reason_row['reason_title']) . "'"
...
includes/acp/acp_search.php
   320    $result = $db->sql_query($sql);

       'SELECT post_id, poster_id, forum_id
       FROM ' . POSTS_TABLE . '
       WHERE post_id >= ' . (int) ($post_counter + 1) . '
       AND post_id <= ' . (int) ($post_counter + $this->batch_size)

Обратите внимание на прямое использование методов $db->sql_escape и $db->sql_in_set: вызовы таких методов нам стоит обрабатывать отдельно и учитывать наличие экранирования в непосредственном коде. Также вызовы intval() и приведения типа к int тоже стоит учесть при переписывании этого кода.

Преобразование текста запроса


Теперь, когда мы научились вычленять текст запроса, пришло время написать функцию с возможностью принимать на вход массив токенов, соответствующих запросу, а на выходе — возвращать другой массив токенов, в котором находится шаблон запроса для sql_query_escaped, а также массив аргументов, которые будут передаваться этому методу. Другими словами, мы хотим получить следующее:

$sql = /* начало токенов запроса */
       'SELECT user_id, author_id
       FROM ' . PRIVMSGS_TO_TABLE . '
       WHERE msg_id = ' . $attachment['post_msg_id']
       /* конец токенов */;

$result = $db->sql_query($sql);

// должно быть преобразовано в

$sql = 'SELECT user_id, author_id
       FROM ' . PRIVMSGS_TO_TABLE . '
       WHERE msg_id = ?d';

$result = $db->sql_query_escaped($sql, $attachment['post_msg_id']);

Мы сразу пишем «?d», потому что можем смело предположить, что если это поле, имя которого оканчивается на «_id», то это должно быть число. Также мы не будем трогать имя таблицы, поскольку оно встречается в каждом запросе, и это значение всегда должно вставляться в «сыром» виде.

Что ж, задача сформулирована, давайте приступим к реализации. В этой статье мы напишем лишь простейший код для трансляции, который не будет анализировать значения, вставляемые в запрос. Поэтому будем вставлять только '?r', то есть просто перепишем вызовы sql_query() так, чтобы они использовали sql_query_escaped(), но делали то же самое.

function rewrite_tokens($in_tokens)
{
	static $string_types = array(T_CONSTANT_ENCAPSED_STRING, T_ENCAPSED_AND_WHITESPACE, '"');

	$type = $text = null;
	$line = 1;

	// для начала проверим, что запрос начинается со строки, для обработки, к примеру, такого случая:
	// ($action == 'add') ? 'INSERT INTO ' . EXTENSION_GROUPS_TABLE . ' ' : 'UPDATE ' . EXTENSION_GROUPS_TABLE . ' SET '

	parse_token($in_tokens[0], $type, $text, $line);
	if ($type === T_WHITESPACE) parse_token($in_tokens[1], $type, $text, $line);
	if (!in_array($type, $string_types, true)) return "Expected first token to be string (got $type)";

	$out_params = $out_tokens = array();
	
	//    разобьём токены на группы, считая за разделитель оператор конкатенации:
	//
	// 'SELECT * FROM ' . TABLE_SOMETHING . ' WHERE lang_id = ' . intval($lang_id)
	//
	//    будет превращено в
	//
	// array(array('SELECT * FROM ', TABLE_SOMETHING), array(' WHERE lang_id = ', intval($lang_id)))
	// 
	//    заметим, что первый токен всегда должен быть строкой, а второй может быть любым

	$state = 'begin';
	$left_tokens = $right_tokens = $groups = array();
	$depth = 0; // глубина скобок

	foreach ($in_tokens as $tok) {
		parse_token($tok, $type, $text, $line);

		if ($type === '(') $depth++;
		else if ($type === ')') $depth--;

		if ($depth < 0) return "Brace depth less than zero";

		if ($state == 'begin') {
			// добавляем в массив «левых» токенов, пока не встретим оператор конкатенации
			if ($depth > 0 || $type !== '.') {
				$left_tokens[] = $tok;
			} else {
				$state = 'end';
			}
		} else {
			// всё аналогично предыдущему, но в конце мы добавим нашу группу в массив $groups
			if ($depth > 0 || $type !== '.') {
				$right_tokens[] = $tok;
			} else {
				$state = 'begin';
				$groups[] = array($left_tokens, $right_tokens);
				$left_tokens = $right_tokens = array();
			}
		}
	}

	// если осталось что-то в конце, добавим это в группу
	if (count($left_tokens)) $groups[] = array($left_tokens, $right_tokens);

	foreach ($groups as $grp) {
		list($left_tokens, $right_tokens) = $grp;

		// как мы заметили ранее, первый токен в группе должен быть строкой
		parse_token($left_tokens[0], $type, $text, $line);
		if ($type === T_WHITESPACE) parse_token($left_tokens[1], $type, $text, $line);
		if (!in_array($type, $string_types, true)) return "first token in a group is not string";

		// пересоберём всё обратно, заменив значения на плейсхолдеры
		foreach ($left_tokens as $tok) {
			parse_token($left_tokens[0], $type, $text, $line);
			$out_tokens[] = $tok;
		}
		if (!count($right_tokens)) break;

		$out_tokens[] = '.';

		// если значение - это константа вида *_TABLE, оставим её, как есть
		$param = tokens_to_string($right_tokens);
		if (preg_match('/^\\s*([A-Z0-9_]+)_TABLE\\s*$/s', $param)) {
			foreach ($right_tokens as $tok) $out_tokens[] = $tok;
		} else {
			// вставим просто строку '?r', «сырое» значение
			$out_tokens[] = array(T_CONSTANT_ENCAPSED_STRING, "'?r'", $line);
			$out_params[] = $param;
		}
		$out_tokens[] = '.';
		
	}

	// если мы последним токеном вставили символ конкатенации,
	// уберем его, чтобы не было syntax error
	parse_token(end($out_tokens), $type, $text, $line);
	if ($type === '.') array_pop($out_tokens);

	return array(
		'tokens' => $out_tokens,
		'params' => $out_params,
	);
}

function tokens_to_string($tokens)
{
	$str = '';
	$type = $text = null;
	$line = 1;

	foreach ($tokens as $tok) {
		parse_token($tok, $type, $text, $line);
		$str .= $text;
	}

	$str = preg_replace(array("/[\t ]+/s", '/^/m'), array(' ', '      '), $str);

	return $str;
}

Сделав необходимые изменения в скриптах для замены, получим следующее (пример из cron.php):

$sql = 'UPDATE ' . CONFIG_TABLE . "
	SET config_value = '" .'?r'. "'
	WHERE config_name = 'cron_lock' AND config_value = '" .'?r'. "'";
$db->sql_query_escaped($sql, $db->sql_escape(CRON_ID), $db->sql_escape($config['cron_lock']));

или в виде диффа:

 $sql = 'UPDATE ' . CONFIG_TABLE . "
-       SET config_value = '" . $db->sql_escape(CRON_ID) . "'
-       WHERE config_name = 'cron_lock' AND config_value = '" . $db->sql_escape($config['cron_lock']) . "'";
-$db->sql_query($sql);
+       SET config_value = '" .'?r'. "'
+       WHERE config_name = 'cron_lock' AND config_value = '" .'?r'. "'";
+$db->sql_query_escaped($sql, $db->sql_escape(CRON_ID), $db->sql_escape($config['cron_lock']));

Пример для случая, когда запрос пишется без использования $sql:

- $db->sql_query('UPDATE ' . CONFIG_TABLE . ' SET config_value = ' . $sql_update . " WHERE config_name = '" . $db->sql_escape($config_name) . "'");
+ $db->sql_query_escaped('UPDATE ' . CONFIG_TABLE . ' SET config_value = ' .'?r'. " WHERE config_name = '" .'?r'. "'", $sql_update, $db->sql_escape($config_name));

Полученный результат сложно назвать красивым из-за '?r', которая всегда стоит отдельно. Также наш скрипт не обрабатывает случаи с использованием двойных кавычек (выражения вида «WHERE id = $forum_id»). В рамках данной статьи мы остановимся на этом варианте, «причёсывание» и аккуратная склейка строк остаётся на усмотрение читателя. Стоит также отдельно отметить, что после выполнения всех замен сам форум работает без видимых ошибок.

Посмотрим, сколько ещё осталось:
$ grep -RF 'sql_query(' * | wc -l
     335

Таким образом, двумя скриптами мы покрыли 80% всех случаев. Оставшиеся 20% нужно обработать вручную и, по правилу Парето, они займут у нас 80% всего времени, потраченного на задачу.

Пункты, не вошедшие в этот пример


Мы написали скрипт по выполнению автоматической замены, но только на вариант с «сырыми» данными. После того, как выполнены эти замены, нужно выполнить оставшиеся действия:

  • собрать статистику использования sql_query_escaped и старого метода sql_query;
  • на основе статистики составить более правильные замены вместо "?r";
  • выполнить замены и продолжать собирать статистику;
  • после выполнения работы должно остаться мало мест, где требуется вставлять «сырой» SQL.

Чтобы на основе статистики составить правильные значения плейсхолдеров вместо "?r", можно воспользоваться следующими догадками:

  • из имени поля, используемого в SQL-запросе, можно получить тип требуемого значения;
  • если вставляемые значения — это только числа, то можно использовать «?d»;
  • если в запросе используется конструкция "... '?r' ...", то скорее всего её можно заменить на "... ?s ...";
  • если в значении всегда присутствуют ключевые слова SQL, то это динамическая часть запроса.

Исходный код скриптов для замены


replacer/direct_sql.php — Непосредственно SQL в вызове sql_query()
replacer/sql_assignment.php — Обработка sql_query($sql)
replacer/rewrite.php — Функция по преобразованию SQL запроса
Tags:
Hubs:
+30
Comments30
Badoo
Big Dating
Website
288
Karma
0
Rating
Юрий Насретдинов @youROCK

Программист, в основном на Go

Send message

Articles

Change theme settings

Information

Website
badoo.com
Registered
Founded
2006
Employees
501–1,000 employees
Location
Россия
Representative
Yuliya Telezhkina

Your account

Sections

Information

Services

Support
© 2006–2024, Habr