Модуль Quote или почему нужно проводить аудит сторонних модулей.

Главные вкладки

Аватар пользователя Demimurych Demimurych 14 сентября 2009 в 20:10

Ко мне обратился один знакомый, который обратил внимание на странное поведение некоторых страниц на его друпал сайте.
При запросе этих страниц, браузер показывал белую страницу.

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

после 5 минут работы в x_debug стало ясно, что виновником всему модуль quote, а точнее функция preg_replace_callback

function _quote_filter_process($text) {
if (stristr($text, '[quotе')) {
   
    $text = preg_replace_callback('#\[(quotе.*?)]((?>\[(?!/?quote[^[]*?])|[^[]|(?R))*)\[/quote]#is', '_quote_filter_process_callback', $text);
  }

  return $text;
}

Для людей умеющих навскидку читать регулярные выражения сразу становится понятно, что этот regexp занимается тем, что выделяет куски вида [ quote] ..... [/quote] сколь угодно любой вложенности.

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

Экспериментальным путем было установлено, что достаточно заключить в [ quote] данные обьемом в 13 килобайт чтобы php завершался аварийно.

Увеличение обьема оперативной памяти, выделяемой для работы скрипта, не решало ровным счетом ничего.

Исправить положение можно заменив строку с регулярным выражением автора модуля на вот это

 $regex = '#\[(quote[^\]]*?)\]((?>[^\[]*)(?>(?!\[/?quote[^\]]*?\])\][^\[]*)*)\[/quote\]#is';
       
        while (preg_match($regex, $text))
       
        $text = preg_replace_callback($regex, '_quote_filter_process_callback', $text);

Спасибо -OC-drupal.org за гениальную подсказку.

Несколько необходимых замечаний.

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

Эффективность использование таких фильтров как quote или bbcode, построенных на регулярных выражениях, обратно пропорционально количеству псевдо-кодов которые они обслуживают.

В настоящий момент рекомендуется использовать алгоритмы построенные на принципах конечных автоматов

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

Комментарии

Аватар пользователя axel axel 15 сентября 2009 в 0:03

Круто! Wink Очень неочевидная ошибка и интересное исследование проблемы. Пофиксю теперь на drupal.ru по приведённому рецепту. Шли патч майнтейнеру модуля на drupal.org!

Аватар пользователя Demimurych Demimurych 15 сентября 2009 в 0:31

"axel" wrote:
Круто! Wink Очень неочевидная ошибка и интересное исследование проблемы. Пофиксю теперь на drupal.ru по приведённому рецепту. Шли патч майнтейнеру модуля на drupal.org!

Да отправлено.

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

Формально у автора все верно.

И я не уверен в том что мое решение оптимально. Времени подумать особенно не было. Решение скорее в лоб.

Аватар пользователя Nikit Nikit 15 сентября 2009 в 1:42

а почему бы просто не пробежаться по строке? тут помоему ничего сложного нету, чем использовать тормозной регэксп.

Аватар пользователя Demimurych Demimurych 15 сентября 2009 в 11:26

"Nikit" wrote:
а почему бы просто не пробежаться по строке? тут помоему ничего сложного нету, чем использовать тормозной регэксп.

сколь угодно "любой" степени вложенности.

кроме того, Ты будешь удивлен, НО regexp зачастую бывает быстрее чем например str_replace

Аватар пользователя Demimurych Demimurych 15 сентября 2009 в 12:23

"Nikit" wrote:
ну почему реплейс, просто пробежаться по строке подсчитываю и

Не исключено что ты прав. приведи пример кода, сравним производительность.

Аватар пользователя Nikit Nikit 15 сентября 2009 в 13:30

делать нечего мне больше Smile ну раз вызвался, то тут набросал, может не то, готов поправить:

<?php

$str = '<quote>dwdwd <quote>dwdw d<quote>dw dwd<quote>dwdwd</quote></quote></quote></quote>';

$l = strlen($str);
$i = 0;

$quot = array();

$quotnum = 0;

while ($i<$l) {
  if ($str[$i] == '<') {
    if (substr($str, $i+1, 6) == 'quote>') {
      $i = $i + 7;
      $quotnum++;
    }
    elseif (substr($str, $i+1, 7) == '/quote>') {
      $quotnum--;
      $i = $i + 8;
    }
  }

  $quot[$quotnum] = $quot[$quotnum] . $str[$i];
  $i++;
}

print_r($quot);
?>

это расчленит по квотам, нету проверки на неправильные <quote><quote></quote>, но так на "идеальном варианте" вроде срабатывает Smile

больше всего тормозить наверное будет в конкатенации $quot[$quotnum] = $quot[$quotnum] . $str[$i], незнаю как пхп с этим справляется, я дельфист Biggrin

Аватар пользователя Demimurych Demimurych 15 сентября 2009 в 14:42

"Nikit" wrote:
ну раз вызвался, то тут набросал, может не то, готов поправить:

Формально нужен код который из

aaaaaaaaa <quote> bbbbbbbbbb <quote> ccccccccccc </quote> dddddddd </quote> <quote> eeeeeeeeeee </quote> fffffffffffff

выдаст что то подобное

aaaaaaaaa

Quote:
bbbbbbbbbb
Quote:
ccccccccccc
dddddddd
Quote:
eeeeeeeeeee
fffffffffffff

обрати внимание на вложенность цитаты, и то что первый уровень(bbbbbbbb) имеет продолжение поcле окончания второго уровня (ddddddddd) в рамках первой же цитаты.

Аватар пользователя Sinn Sinn 15 сентября 2009 в 21:38

столкнулся с таким багом, но только сейчас понял из-за чего, а так пришлось в базе данных вручную убирать quote

Аватар пользователя Nikit Nikit 16 сентября 2009 в 9:40
$str = 'aaaaaaaaa <quote> bbbbbbbbbb <quote> ccccccccccc </quote> dddddddd </quote> <quote> eeeeeeeeeee </quote> fffffffffffff';
                                             
$l = strlen($str);
$i = 0;

$op = 0;
$opp = array();

$quotes = array();

while ($i<$l) {
  if (substr($str, $i, 7) == '<quote>') {
    $opp[$op] = $i;
    $i = $i + 6;
    $op++;

  }
  elseif (substr($str, $i, 8) == '</quote>') {
    $op--;
    $quotes[] = substr($str, $opp[$op], $i - $opp[$op]+9);

    $i = $i + 7;
  }

  $i++;
}

print_r($quotes);

в массиве обернутые квоты, то что не обернуто не записываю.

Аватар пользователя Demimurych Demimurych 16 сентября 2009 в 11:25

"Nikit" wrote:
в массиве обернутые квоты, то что не обернуто не записываю.

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

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

А следовательно, проводить сравнение производительности его, с тем что было написано выше, пока нельзя.

Аватар пользователя Nikit Nikit 16 сентября 2009 в 12:13

Demimurych я просто показываю что данные можно разделить квоты. Cогласно

"Demimurych" wrote:
Для людей умеющих навскидку читать регулярные выражения сразу становится понятно, что этот regexp занимается тем, что выделяет куски вида [ quote] .....
сколь угодно любой вложенности.[/quote]
А дальше с ними можно делать что угодно. Заметьте цикл однопроходник, не думаю что регэксп так делает, напрягает только substr.

Аватар пользователя Demimurych Demimurych 16 сентября 2009 в 12:24

"Nikit" wrote:
А дальше с ними можно делать что угодно. Заметьте цикл однопроходник, не думаю что регэксп так делает, напрягает только substr.

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

Твой алгоритм показывает, что это возможно иным способом. Безусловно. Но в нем не хватает той составляющей, которая бы приравняла его к решению с регулярным выражением. И оценивая, безусловно на глаз, реализацию полноценного механизма, я прихожу к выводу что накладные расходы на его реализацию могут перекрыть все преимущества (если они конечно есть).

"Nikit" wrote:
А дальше с ними можно делать что угодно. Заметьте цикл однопроходник, не думаю что регэксп так делает, напрягает только substr.

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

Ну если уж совсем в падлу. Могу сравнить то что есть, хотя и сравнение будет не вполне корректным.

Аватар пользователя Nikit Nikit 16 сентября 2009 в 13:30

ну да согласен, но тут дело вступает частота использования. Если я делаю сайт, то конечно не буду терять время и решу общими методами. А вот если что-то будет использоваться часто, в данном случае выдирание квот, то не лучше ли его оптимизировать, чем использовать обобщенные вещи? "Накладные расходы" будут существенно выше при разработке этого модуля, но намного меньше после, если его будут использовать многие.

Аватар пользователя Demimurych Demimurych 16 сентября 2009 в 13:34

"Nikit" wrote:
А вот если что-то будет использоваться часто, в данном случае выдирание квот, то не лучше ли его оптимизировать

Конечно лучше. Но у меня серьезные основания полагать, что твой алгоритм будет работать медленнее. Что бы это перестало быть пердежом в лужу с моей стороны, нужна "полная" реализация твоего алгоритма.
Чтобы провести замеры на время выполнения и потребления ресурсов.

Аватар пользователя -OC-@drupal.org -OC-@drupal.org 16 сентября 2009 в 15:07

вставлю и свои 5 копеек Smile
проблема собственно заключается в не совсем эффективном регулярном выражении. Автор модуля позаимствовал его в PHP Manual, об чем в злополучной функции и написано.
Вариант решения

<?php
        $regex 
'#\[(quote[^\]]*?)\]((?>[^\[]*)(?>(?!\[/?quote[^\]]*?\])\][^\[]*)*)\[/quote\]#is';
        
        while (
preg_match($regex$text))
        
        
$text preg_replace_callback($regex'_quote_filter_process_callback'$text);
        

?>

вместо

<?php
    $text 
preg_replace_callback('#\[(quote.*?)]((?>\[(?!/?quote[^[]*?])|[^[]|(?R))*)\[/quote]#is''_quote_filter_process_callback'$text);
?>

в функции _quote_filter_process

Детально работоспособность не проверял, но на тексте порядка 60К работает без проблем в отличии от "оригинала". Цитат было штук 6 (4 из них по 10-15 К текста) различных уровней вложенности. Если есть у кого желание потестить более серьезно - welcome!!!

Аватар пользователя Karsonito Karsonito 16 сентября 2009 в 16:56

Плохое название. Аудит нужен PHP. Хотя мало чем поможет. Детская болезнь созрела до патологии. А родители все еще ходят по гадалкам: http://bugs.php.net/bug.php?id=48153
<?php
$contents = 'sud' . str_repeat('a', 15000) . 'bccess';
$contents = preg_replace('/d(a)+b/', '', $contents);
echo $contents;
?>
PHP 5.3.0 ничего не вылечил. И вылечат ли в перспективе?

Добавка: обратите внимание на статус бага - Bogus. Что еще тут говорить...

Аватар пользователя Demimurych Demimurych 16 сентября 2009 в 17:57

"-OC-[user=drupal wrote:
drupal[/user].org"]в функции _quote_filter_process

Детально работоспособность не проверял, но на тексте порядка 60К работает без проблем в отличии от "оригинала". Цитат было штук 6 (4 из них по 10-15 К текста) различных уровней вложенности. Если есть у кого желание потестить более серьезно - welcome!!!

Твой алгоритм быстрее на порядок(то есть почти в 10 раз) моего. И потребляет в ДВА раза меньше памяти(и эта цифра будет расти по мере увеличения обрабатываемого текста).
Тестировалось на 52 килобайтов-ом файле с 4 степенью вложенности тега.
поверено x_debug

НО

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

Пример когда

[quotе] текст [  текст [/quotе]
который регексп не приведет к должному виду.

Аватар пользователя Demimurych Demimurych 16 сентября 2009 в 17:53

"Karsonito" wrote:
PHP 5.3.0 ничего не вылечил. И вылечат ли в перспективе?

Добавка: обратите внимание на статус бага - Bogus. Что еще тут говорить...

Качество php это тема отдельного разговора. Используя пхп ты принимаешь его правила, которые, к сожалению, часто ставят тебя раком. Это очевидно сколько нибудь серьезному php разработчику.

Потому и выбрал именно такое название

А не например, пхп гавно или ...

Аватар пользователя -OC-@drupal.org -OC-@drupal.org 16 сентября 2009 в 18:46

"Demimurych" wrote:
НО

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

Пример когда

[quotе] текст [ текст [/quotе]
который регексп не приведет к должному виду.


упссссссс..ошибочка однако Smile
вот правильный вариант

<?php
$regex 
'#\[(quote[^\]]*?)\]((?>[^\[]*)(?>(?!\[/?quote[^\]]*?\])\[[^\[]*)*)\[/quote\]#is';
?>

тяжко когда неудобочитаемое выражение в одну строку Smile

что касаемо цены.. не соглашусь.. задача поиска текста в парных ограничителях встречается довольно часто. Посему решение мной просто взято из личной копилки и модифицировано для данного случая(правда с ошибочкой, звиняйте - недостаток- неудобочитаемость, а разбивать на строки не хотелось)
Взято из "классики" ИМХО - Фридл, Дж. Регулярные выражения.2-е издание

Аватар пользователя Demimurych Demimurych 17 сентября 2009 в 11:48

"-OC-[user=drupal wrote:
drupal[/user].org"]упссссссс..ошибочка однако Smile
вот правильный вариант

ВО шикарно.

Мой алгоритм выбросить. Это поставить !!!

Магия.

Аватар пользователя andypost@drupal.org andypost@drupal.org 20 сентября 2009 в 20:27

<a href="mailto:Mr.Alinaki@drupal.org">Mr.Alinaki@drupal.org</a> wrote:
Предлагаю другое решение проблемы: http://drupal.ru/node/34261[/quote]
Это скорее для модуля bbcode
Можно также попробовать предложить, как альтернативу текущей реализации... хотя сторонние библиотеки не очень любят

Аватар пользователя andypost@drupal.org andypost@drupal.org 20 сентября 2009 в 19:49

Что-то я так и не нашел issue в очереди Модуль пока в бэта-стадии, так что, имхо, стоит заменить в нем сие безобразия. Насколько я понимаю, найдено эффективное решение!

Аватар пользователя Demimurych Demimurych 20 сентября 2009 в 20:00

"<a href="mailto:andypost@drupal.org">andypost@drupal.org</a>" wrote:
Что-то я так и не нашел issue в очереди Модуль пока в бэта-стадии, так что, имхо, стоит заменить в нем сие безобразия. Насколько я понимаю, найдено эффективное решение!

писалось на security@drupal.org

ответ пришел, мол заявка дуп, над ней работают.

Аватар пользователя andypost@drupal.org andypost@drupal.org 20 февраля 2010 в 15:49

"Demimurych" wrote:

"<a href="mailto:andypost@drupal.org">andypost@drupal.org</a>" wrote:

Что-то я так и не нашел issue в очереди Модуль пока в бэта-стадии, так что, имхо, стоит заменить в нем сие безобразия. Насколько я понимаю, найдено эффективное решение!

писалось на security@drupal.org

ответ пришел, мол заявка дуп, над ней работают.

И как же обстоят дела с вложеным квотированем сейчас?