Повторная проверка проектов нередко бывает весьма интересной. Она позволяет узнать, какие новые ошибки были допущены в ходе разработке приложения, а какие ошибки уже были исправлены. Раньше мой коллега уже писал о проверке 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 Engine:
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 вполне могло подразумеваться что-то ещё, тогда выражение будет не избыточным, а ошибочным. Кстати, это не единственное место. Встретились ещё несколько:
Об этом я писал выше, но хочу повториться ещё раз. PHP использует некоторые сторонние библиотеки, которые, увы, не идеальны и содержат ошибки. Многие предупреждения выдавались как раз на код этих библиотек. Можно было бы проанализировать их все, но тогда, поверьте, статья вышла бы очень большой.
Определить, находится ли ошибка в коде интерпретатора PHP или сторонней библиотеки достаточно просто – в начале всех исходных файлов находится комментарий, описывающий лицензию, проект, авторов и пр. Отталкиваясь от этих комментариев легко определить, в файле какого проекта притаилась ошибка.
С другой стороны - некоторые интересные места всё же рассмотреть стоило. В любом случае, если вы используете какие-то сторонние библиотеки, ответственность перед пользователями за ошибки, допущенные в этих библиотеках, ложится также и на ваши плечи, так как ошибка может проявить себя в ходе использования именно вашего продукта. Поэтому стоит внимательнее относиться к тому, какие зависимости вы тянете в своём проекте.
Результаты проверки вышли интересными. На самом деле нашлось много ошибок, в статье рассматривалась только малая часть общих предупреждений высокого и среднего уровней достоверности. Многие из этих ошибок находятся в библиотеках, которые используются в PHP, а значит, неявно, кочуют в его код. В самом коде PHP тоже обнаружились интересные ошибки, некоторые из которых и были выписаны выше.
Подводя итог, хотелось бы сказать, что необходимо использовать различные средства, позволяющие повысить продуктивность работы и качества кода. Не стоит ограничиваться тестами и code review. Статический анализатор - один из тех инструментов, которые могут помочь программисту писать более качественный код позволяя полезнее использовать то время, которое было бы потрачено на поиск ошибки, допущенной из-за какой-нибудь опечатки. Но не стоит забывать, что статический анализатор - инструмент регулярного применения. И если вы ещё не используете его - рекомендую загрузить PVS-Studio, проверить свой проект и посмотреть, что же интересного удастся найти.
P.S. Разработчики, занимающиеся разработкой Zend Engine, связались с нами и сообщили, что ошибки, которые были описаны в статье, уже исправлены. Так держать!