Повторная проверка проектов нередко бывает весьма интересной. Она позволяет узнать, какие новые ошибки были допущены в ходе разработке приложения, а какие ошибки уже были исправлены. Раньше мой коллега уже писал о проверке PHP. С выходом новой версии (PHP7), я решил ещё раз проверить исходный код интерпретатора и нашёл кое-что интересное.


Проверяемый проект


PHP — скриптовый язык общего назначения, интенсивно применяемый для разработки веб-приложений. Язык и его интерпретатор разрабатываются в рамках проекта с открытым кодом.



3 декабря 2015 года было объявлено о выходе PHP версии 7.0.0. Новая версия основывается на экспериментальной ветке PHP, которая изначально называлась phpng (Следующее поколение PHP), и разрабатывалась с упором на увеличение производительности и уменьшение потребления памяти.

Объектом проверки стал интерпретатор PHP, исходный код которого доступен в репозитории на GitHub. Проверяемая ветвь — master.

В качестве инструмента анализа использовался статический анализатор кода PVS-Studio. Для проверки использовалась система мониторинга компиляции, позволяющая осуществлять анализ проекта независимо от того, какая система используется для сборки этого проекта. Пробная версия анализатора доступна для загрузки по ссылке.

С предыдущей проверкой проекта можно познакомиться в статье Святослава Размыслова "Заметка про проверку PHP".

Найденные ошибки


Стоит отметить, что многие ошибки, найденные анализатором, находятся в библиотеках, используемых в PHP. Если все их рассматривать в этой статье, её объем бы порядочно вырос. Но с другой стороны ошибки, допущенные в библиотеках, используемых в проекте, проявят себя и при использовании проекта. Поэтому некоторые из этих ошибок всё же будут выписаны в этой статье.

Отдельно хотелось бы отметить, что во время анализа сложилось впечатление, что код целиком написан на макросах. Они просто повсюду. Это сильно усложняет задачу анализа, про отладку подобного кода я вообще молчу. Их повсеместное использование, к слову, вышло же боком – ошибки из макросов растаскивались по коду. Но об этом ниже.
static void spl_fixedarray_object_write_dimension(zval *object, 
                                                  zval *offset, 
                                                  zval *value) 
{
  ....
  if (intern->fptr_offset_set) {
    zval tmp;
    if (!offset) {
      ZVAL_NULL(&tmp);
      offset = &tmp;
    } else {
      SEPARATE_ARG_IF_REF(offset);
  }
  ....
  spl_fixedarray_object_write_dimension_helper(intern, offset, value)
}

Предупреждение PVS-Studio: V506 Pointer to local variable 'tmp' is stored outside the scope of this variable. Such a pointer will become invalid. spl_fixedarray.c 420

При истинности условного выражения оператора if, приведённого выше, указателю offset может быть присвоен адрес переменной tmp. При этом время жизни переменной tmp ограничено её областью видимости, т.е. телом оператора if. Но ниже по коду вызывается функция, принимающая в качестве одного из параметров указатель offset, который ссылается на уже уничтоженную переменную, что может привести к ошибке при работе с данным указателем.

Другой странный код:
#define MIN(a, b)  (((a)<(b))?(a):(b))
#define MAX(a, b)  (((a)>(b))?(a):(b))
SPL_METHOD(SplFileObject, fwrite)
{
  ....
  size_t str_len;
  zend_long length = 0;
  ....
  str_len = MAX(0, MIN((size_t)length, str_len));
  ....
} 

Предупреждение PVS-Studio: V547 Expression is always false. Unsigned type value is never < 0. spl_directory.c 2886

Логика кода проста — сначала сравнивают 2 величины и берут из них меньшую, после чего полученный результат сравнивают с нулём и записывают в переменную str_len большее из этих значений. Проблема кроется в том, что size_t — беззнаковый тип, следовательно, его значение всегда неотрицательно. В итоге использование второго макроса MAX попросту не имеет смысла. Что это — просто лишняя операция, или какая-то более серьёзная ошибка — судить разработчику, писавшему код.

Это не единственное странное сравнение, встретились и другие.
static size_t sapi_cli_ub_write(const char *str, size_t str_length)
{
  ....
  size_t ub_wrote;
  ub_wrote = cli_shell_callbacks.cli_shell_ub_write(str, str_length);
  if (ub_wrote > -1) {
    return ub_wrote;
  }
}

Предупреждение PVS-Studio: V605 Consider verifying the expression: ub_wrote > — 1. An unsigned value is compared to the number -1. php_cli.c 307

Переменная ub_wrote имеет тип size_t, являющийся беззнаковым. Однако ниже выполняется проверка вида ub_wrote > -1. На первый взгляд может показаться, что это выражение всегда будет истинным, так как ub_wrote может хранить в себе только неотрицательные значения. На самом деле всё обстоит интереснее.

Тип литерала -1 (int) будет преобразован к типу переменной ub_wrote (size_t), то есть в сравнении ub_wrote с переменной будет участвовать преобразованное значение. В 32-битной программе это будет беззнаковое значение 0xFFFFFFFF, а в 64-битной – 0xFFFFFFFFFFFFFFFF. Таким образом, переменная ub_wrote будет сравниваться с максимальным значением типа unsigned long. В итоге результатом этого сравнения всегда будет значение false, и оператор return никогда не выполнится.

Схожий код встретился ещё раз. Соответствующее сообщение: V605 Consider verifying the expression: shell_wrote > — 1. An unsigned value is compared to the number -1. php_cli.c 272

Следующий код, на который анализатор выдал предупреждение, также связан с макросом.
PHPAPI void php_print_info(int flag)
{
  ....
  if (!sapi_module.phpinfo_as_text) {
    php_info_print("<h1>Configuration</h1>\n");
  } else {
    SECTION("Configuration");
  }
  ....
}

Предупреждение PVS-Studio: V571 Recurring check. The 'if (!sapi_module.phpinfo_as_text)' condition was already verified in line 975. info.c 978

На первый взгляд может показаться, что всё нормально, и ошибки нет. Но давайте взглянем на то, что из себя представляет макрос SECTION.
#define SECTION(name) if (!sapi_module.phpinfo_as_text) {                         php_info_print("<h2>" name "</h2>\n");                       } else {                         php_info_print_table_start();                         php_info_print_table_header(1, name);                         php_info_print_table_end();                       } \

В итоге, после препроцессирования в *.i-файле будет содержаться код следующего вида:
PHPAPI void php_print_info(int flag)
{
  ....
  if (!sapi_module.phpinfo_as_text) {
    php_info_print("<h1>Configuration</h1>\n");
  } else {
    if (!sapi_module.phpinfo_as_text) { 
      php_info_print("<h2>Configuration</h2>\n"); 
    } else { 
      php_info_print_table_start(); 
      php_info_print_table_header(1, "Configuration"); 
      php_info_print_table_end(); 
    } 
  }
  ....
}

Сейчас проблему заметить стало куда проще. Проверяется некоторое условие (!sapi_module.phpinfo_as_text) и, если оно не выполняется, опять проверяется это условие (которое, естественно, никогда не выполнится). Согласитесь, выглядит, как минимум, странно.

Схожая ситуация, связанная с использованием этого макроса, встретилась ещё раз в этой же функции:
PHPAPI void php_print_info(int flag)
{
  ....
  if (!sapi_module.phpinfo_as_text) {
    SECTION("PHP License");
    ....
  }
  ....
}

Предупреждение PVS-Studio: V571 Recurring check. The 'if (!sapi_module.phpinfo_as_text)' condition was already verified in line 1058. info.c 1059

Аналогичная ситуация. То же условие, тот же макрос. Раскрываем, получаем код следующего вида:
PHPAPI void php_print_info(int flag)
{
  ....
  if (!sapi_module.phpinfo_as_text) {
    if (!sapi_module.phpinfo_as_text) { 
      php_info_print("<h2>PHP License</h2>\n"); 
    } else { 
      php_info_print_table_start(); 
      php_info_print_table_header(1, "PHP License"); 
      php_info_print_table_end(); 
    }
    ....
  }
  ....
}

Опять дважды проверяется одно и то же условие. Второе будет проверяться только в случае, если истинно первое. Тогда, если истинно первое условие (!sapi_module.phpinfo_as_text), то всегда будет истинным и второе условие. В таком случае код, находящийся в ветви else второго оператора if, не будет выполнен вообще никогда.

Идём дальше.
static int preg_get_backref(char **str, int *backref)
{
  ....
  register char *walk = *str;
  ....
  if (*walk == 0 || *walk != '}')
  ....
}

Предупреждение PVS-Studio: V590 Consider inspecting the '* walk == 0 || * walk != '}'' expression. The expression is excessive or contains a misprint. php_pcre.c 1033

В данном коде указатель разыменовывается, и его значение сравнивается с некоторыми литералами. Данный код избыточен. Для наглядности перепишем, упростив, выражение:
if (a == 0 || a != 125)

Как видите, условие можно упростить до a != 125.

Это может свидетельствовать как об избыточности кода, так и о том, что программист допустил более серьёзную ошибку.



Источником некоторых проблемных мест стал фреймворк Zend:
static zend_mm_heap *zend_mm_init(void)
{
  ....
  heap->limit = (Z_L(-1) >> Z_L(1));
  ....
}

Предупреждение PVS-Studio: V610 Unspecified behavior. Check the shift operator '>>'. The left operand '(- 1)' is negative. zend_alloc.c 1865

В данном коде используется операция правостороннего битового сдвига отрицательного значения. Это случай неуточнённого поведения (unspecified behavior). Хоть с точки зрения языка такой случай и не является ошибочным, в отличии от неопределённого поведения, лучше избегать подобных случаев, так как поведение подобного кода может различаться в зависимости от платформы и компилятора.

Другая интересная ошибка содержится в библиотеке PCRE:
const pcre_uint32 PRIV(ucp_gbtable[]) = {
  ....
  (1<<ucp_gbExtend)|(1<<ucp_gbSpacingMark)|(1<<ucp_gbL)|   /*  6 L */
  (1<<ucp_gbL)|(1<<ucp_gbV)|(1<<ucp_gbLV)|(1<<ucp_gbLVT),
  ....
};

Предупреждение PVS-Studio: V501 There are identical sub-expressions '(1 << ucp_gbL)' to the left and to the right of the '|' operator. pcre_tables.c 161

Ошибки подобного рода можно назвать классическими. Они находились и находятся в C++ проектах, не избавлены от них проекты, написанные на C#, и, наверняка, на других языках тоже. Программист допустил опечатку и в выражении продублировал подвыражение (1<<ucp_gbL). Скорее всего (если судить по остальной части исходного кода), подразумевалось подвыражение (1<<ucp_gbT). Такие ошибки не бросаются в глаза даже в отдельно выписанном фрагменте кода, а уж на фоне остального — становятся вообще крайне трудно обнаруживаемыми.

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

Другое место из той же библиотеки:
....
firstchar = mcbuffer[0] | req_caseopt;
firstchar = mcbuffer[0];
firstcharflags = req_caseopt;
....

Предупреждение PVS-Studio: V519 The 'firstchar' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 8163, 8164. pcre_compile.c 8164

Согласитесь, код выглядит странно. Записываем результат операции '|' в переменную firstchar, и тут же перезаписываем её значение, игнорируя результат предыдущей операции. Возможно, во втором случае вместо firstchar подразумевалась другая переменная, но сказать наверняка сложно.

Встретились и избыточные условия. Например:
PHPAPI php_stream *_php_stream_fopen_with_path(.... const char *path, 
                                               ....)
{
  ....
  if (!path || (path && !*path)) {
  ....
}

Предупреждение PVS-Studio: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!path' and 'path'. plain_wrapper.c 1487

Данное выражение является избыточным: во втором подвыражении можно убрать проверку указателя path на неравенство nullptr. Тогда упрощённое выражение примет следующий вид:
if (!path || !*path)) {

Не стоит недооценивать подобные ошибки. Вместо переменной path вполне могло подразумеваться что-то ещё, тогда выражение будет не избыточным, а ошибочным. Кстати, это не единственное место. Встретились ещё несколько:
  • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!path' and 'path'. fopen_wrappers.c 643
  • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!headers_lc' and 'headers_lc'. sendmail.c 728

О сторонних библиотеках


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

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

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

Заключение




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

Подводя итог, хотелось бы сказать, что необходимо использовать различные средства, позволяющие повысить продуктивность работы и качества кода. Не стоит ограничиваться тестами и code review. Статический анализатор — один из тех инструментов, которые могут помочь программисту писать более качественный код позволяя полезнее использовать то время, которое было бы потрачено на поиск ошибки, допущенной из-за какой-нибудь опечатки. Но не стоит забывать, что статический анализатор — инструмент регулярного применения. И если вы ещё не используете его — рекомендую загрузить PVS-Studio, проверить свой проект и посмотреть, что же интересного удастся найти.



Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. Analysis of PHP7.

Прочитали статью и есть вопрос?
Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.

Комментарии (26)


  1. romy4
    28.04.2016 14:15
    +1

    Часть классических ошибок можно избежать просто не используя копипасту, а переписывая код. Ну потратишь ты на две минуты больше, за то избежишь ошибок «V501 There are identical sub-expressions»


    1. EvgeniyRyzhkov
      28.04.2016 14:26
      +67

      Это верно.

      Но Вы же скопировали «V501 There are identical sub-expressions», а не набрали заново? :-).


      1. romy4
        28.04.2016 16:33
        -10

        Это хабр, а не редактор/ide для проектного кода.


      1. splav_asv
        28.04.2016 22:01
        +1

        Довольно странно при цитировании не использовать копирование. Задача как раз повторить.
        С кодом да, лень берет своё, тут вы правы.


    1. zenn
      28.04.2016 14:38
      +15

      Это сомнительное утверждение. Нет, я не защищаю копипасту, но вероятность того, что при переписывании вы допустите «свою» опечатку или ошибку скорей всего НЕ ниже, чем вероятность ошибки в уже написанном коде.


      1. romy4
        28.04.2016 16:33
        -5

        Подскажите, как вы вычислили такую вероятность?


        1. zenn
          28.04.2016 16:47
          +1

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


          1. romy4
            28.04.2016 16:55
            -6

            Моё утверждение абсолютно — нет копипасты, нет ошибок, которые тянут за собой недосмотры по копипасте. То, что при этом могут быть ошибки программиста в его логике, которую он пишет — тоже возможно. А могут и не быть.
            А высказывания — то такое. Все вокруг высказываются. Но я программист, а не статист, и не занимаюсь вычислением вероятности без каких либо данных.


            1. Fesor
              02.05.2016 01:07

              нет копипасты, нет ошибок, которые тянут за собой недосмотры по копипасте


              вы заменяете один класс ошибок (ошибки при копипасте) другими (опечатки). По итогу человеческий фактор всеравно берет свое.


  1. SamDark
    28.04.2016 15:37
    +6

    Источником некоторых проблемных мест стал фреймворк Zend

    Это вроде не фреймворк. Это Zend Engine.


    1. foto_shooter
      28.04.2016 16:56
      +1

      Точно, спасибо.
      Ошибкам, правда, везде уютно живётся — и в коде фреймворков, и в коде виртуальных машин. :)


  1. SergeAx
    28.04.2016 16:43
    +5

    В spl_fixedarray_object_write_dimension ошибки нет — в конце тела условия if (intern->fptr_offset_set) стоит return, так что tmp объявлена в правильном scope, в противном случае могла быть утечка памяти.

    UPD: чёрт, я был уверен, что markdown в комментах поддерживается.


    1. bolk
      29.04.2016 07:18

      UPD: чёрт, я был уверен, что markdown в комментах поддерживается.
      Поддерживается, если установить галочку под комментарием.


      1. SergeAx
        01.05.2016 19:28

        Прошу прощения, где именно «под комментарием»?

        https://yadi.sk/i/yesu-7MCrRWBa


        1. bolk
          01.05.2016 19:30

          Странно, у меня есть.


          1. SergeAx
            01.05.2016 19:45

            Понятно, отрицательная карма. Спасибо!


  1. khrisanfov
    28.04.2016 17:06
    -3

    Упрощение конечно хорошо, но вот в этом примере, где вы предлагаете заменить if (!path || (path && !*path)) на if (!path || !*path)) человеку будет проще понять первое условие, по крайней мере мне.


    1. romy4
      28.04.2016 17:12
      -6

      а не вызовет ли тут чтение по указателю 0 ошибку доступа к странице в упрощённом случае?


      1. khrisanfov
        28.04.2016 17:15
        -5

        Да скорее всего так и есть.


        1. romy4
          28.04.2016 17:26
          -4

          «Вредные советы» от PVS-Studio? :)


          1. Andrey2008
            28.04.2016 17:31
            +6

            Нет. Просто надо знать, как работает оператор ||. :)
            Подсказка: Short circuit


            1. romy4
              28.04.2016 17:35

              Точно


            1. khrisanfov
              28.04.2016 17:43

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


              1. Temtaime
                28.04.2016 18:06
                +5

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


          1. khrisanfov
            28.04.2016 17:31
            -3

            Ждем официальных объяснений ;) С первого взгляда упрощение кажется ошибочным, да и не нужно тут ничего упрощать, компилятору пофиг на все эти упрощения, а человеку во многих случаях понять сложнее будет в итоге.


  1. fondue
    29.04.2016 18:16
    -2

    Ох уж этот php…