Анализ 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 истинно, указателю смещения может быть присвоен адрес переменной tmp. Срок службы переменной tmp ограничен ее областью действия, то есть телом оператора if. Далее в коде мы видим вызов функции, которая принимает указатель смещение в качестве одного из параметров, который ссылается на переменную, которая уже была уничтожена; это может привести к ошибке при работе с этим указателем.

Еще один странный фрагмент кода:

#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: Попробуйте проверить выражение: 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-битной - 0xFFFFFFFFFFFFFF. Таким образом, переменная 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: повторяющаяся проверка. Условие «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 || * walk! = ‘}’ ’выражение. Выражение является чрезмерным или содержит опечатку. php_pcre.c 1033

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

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

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

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

Некоторые проблемы были вызваны фреймворком Zend:

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 мы обнаружили несколько занимательных ошибок, о которых рассказали в этой статье.

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