Анализ PHP7

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

Проект для анализа

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

Релиз новой версии — PHP v.7.0.0. был анонсирован 3 декабря 2015 года. Он основан на экспериментальной ветви 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 Указатель на локальную переменную tmp хранится вне области видимости этой переменной. Такой указатель станет недействительным. 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 всегда ложно. Значение беззнакового типа никогда не равно 0. spl_directory.c 2886

Логика кода проста — сначала сравниваются два значения, затем сравнивается наименьшее из них с нулем, а затем в переменную 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 Попробуйте проверить выражение: ub_wrote › — 1. Беззнаковое значение сравнивается с числом -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 Рассмотрим проверку выражения: shell_wrote › — 1. Беззнаковое значение сравнивается с числом -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 Периодическая проверка. Условие if (!sapi_module.phpinfo_as_text) уже проверено в строке 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 Периодическая проверка. Условие if (!sapi_module.phpinfo_as_text) уже проверено в строке 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 Рассмотрите возможность проверки ‘* walk == 0 || * ходьба != выражение ‘}’’. Выражение является избыточным или содержит опечатку. php_pcre.c 1033

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

if (a == 0 || a != 125)

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

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

Причиной некоторых проблем был Zend Engine:

static zend_mm_heap *zend_mm_init(void)
{
  ....
  heap->limit = (Z_L(-1) >> Z_L(1));
  ....
}

Предупреждение PVS-Studio:V610 Неопределенное поведение. Проверьте оператора смены ››. Левый операнд ‘(- 1)’ отрицательный. zend_alloc.c 1865

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

Еще одна интересная ошибка была обнаружена в библиотеке 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 Слева и справа от оператора | одинаковые подвыражения ‘(1 ‹‹ ucp_gbL)’. 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 Переменной firstchar дважды подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 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Можно упростить лишнюю проверку. Оператор || окружен противоположными выражениями !path и path. plain_wrapper.c 1487

Это выражение избыточно: во втором подвыражении мы можем убрать проверку указателя path на соответствие nullptr. Тогда упрощенное выражение будет таким:

if (!path || !*path)) {

Не стоит недооценивать такие ошибки. Вероятно, вместо переменной path должно было быть что-то другое, и тогда такое выражение было бы ошибочным, а не лишним. Кстати, это не единственный фрагмент. Было еще несколько:

V728 Излишняя проверка может быть упрощена. Оператор «||» окружен противоположными выражениями «!path» и «path». fopen_wrappers.c 643
V728 Излишняя проверка может быть упрощена. Оператор «||» окружен противоположными выражениями «!headers_lc» и «headers_lc». sendmail.c 728

Сторонние библиотеки

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

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

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

Вывод

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

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

P.S. С нами связались разработчики Zend Engine и сообщили, что проблемы, описанные в статье, уже устранены. Молодец!

Анализ PHP7

Проект для анализа

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

Сторонние библиотеки

Вывод

Вопросы по теме