PHP — скриптовый язык программирования общего назначения, интенсивно применяемый для разработки веб-приложений. В настоящее время поддерживается подавляющим большинством хостинг-провайдеров и является одним из лидеров среди языков программирования, применяющихся для создания динамических веб-сайтов.
В случае с компиляторами и интерпретаторами к исходному коду и тестированию, как правило, предъявляются повышенные требования качества и надёжности. Тем не менее, в исходном коде интерпретатора PHP нашлись подозрительные места.
В данной статье будут рассмотрены результаты проверки интерпретатора PHPhttp://www.asterisk.org/, полученные с помощью PVS-Studio 5.18.
V501 There are identical sub-expressions '!memcmp("auto", charset_hint, 4)' to the left and to the right of the '||' operator. html.c 396
static enum
entity_charset determine_charset(char *charset_hint TSRMLS_DC)
{
....
if ((len == 4) /* sizeof (none|auto|pass) */ && // <=
(!memcmp("pass", charset_hint, 4) ||
!memcmp("auto", charset_hint, 4) || // <=
!memcmp("auto", charset_hint, 4))) // <=
{
charset_hint = NULL;
len = 0;
}
....
}Условное выражение содержит вызов функций 'memcmp' с одинаковыми параметрами. Комментарий /* sizeof (none|auto|pass) */ подсказывает нам, что в одну из функций необходимо передать значение "none".
V605 Consider verifying the expression: shell_wrote > - 1. An unsigned value is compared to the number -1. php_cli.c 266
PHP_CLI_API size_t sapi_cli_single_write(....)
{
....
size_t shell_wrote;
shell_wrote = cli_shell_callbacks.cli_shell_write(....);
if (shell_wrote > -1) { // <=
return shell_wrote;
}
....
}Данное сравнение является явной ошибкой. '-1' преобразуется в максимальное значение типа 'size_t', поэтому условие всегда будет ложным, и вся проверка сошла на нет. Возможно, переменная 'shell_wrote' раньше имела знаковый тип, но после рефакторинга не учли особенности операций с беззнаковыми типами.
V547 Expression 'tmp_len >= 0' is always true. Unsigned type value is always >= 0. ftp_fopen_wrapper.c 639
static size_t php_ftp_dirstream_read(....)
{
size_t tmp_len;
....
/* Trim off trailing whitespace characters */
tmp_len--;
while (tmp_len >= 0 && // <=
(ent->d_name[tmp_len] == '\n' ||
ent->d_name[tmp_len] == '\r' ||
ent->d_name[tmp_len] == '\t' ||
ent->d_name[tmp_len] == ' ')) {
ent->d_name[tmp_len--] = '\0';
}
....
}Тип 'size_t' , являясь беззнаковым, позволяет индексировать максимальное количество элементов массива для текущей разрядности приложения. Проверка (tmp_len >= 0) является некорректной. В худшем случае декремент может привести к переполнению индекса и обращению к памяти за границей массива. Скорее всего, код выполняется верно благодаря дополнительным условиям и корректным исходным данным, но опасность "зацикливания" или выхода за пределы массива тут присутствует.
V555 The expression 'out_buf_size - ocnt > 0' will work as 'out_buf_size != ocnt'. filters.c 1702
static int strfilter_convert_append_bucket(
{
size_t out_buf_size;
....
size_t ocnt, icnt, tcnt;
....
if (out_buf_size - ocnt > 0) { // <=
....
php_stream_bucket_append(buckets_out, new_bucket TSRMLS_CC);
} else {
pefree(out_buf, persistent);
}
....
}Возможно, ветвь 'else' будет выполнять реже, чем следовало бы, т.к. разность беззнаковых чисел почти всегда будет больше нуля. Исключением будет являться равенство операндов: в этом случае условие лучше переписать на более информативный вариант.
V595 The 'function_name' pointer was utilized before it was verified against nullptr. Check lines: 4859, 4860. basic_functions.c 4859
static int user_shutdown_function_call(zval *zv TSRMLS_DC)
{
....
php_error(E_WARNING, "....", function_name->val); // <=
if (function_name) { // <=
STR_RELEASE(function_name);
}
....
}Проверка указателя после разыменования всегда выглядит подозрительно. В случае реальной ошибки может привести к краху программы.
Аналогичное место:
V597 The compiler could delete the 'memset' function call, which is used to flush 'final' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. php_crypt_r.c 421
/*
* MD5 password encryption.
*/
char* php_md5_crypt_r(const char *pw,const char *salt, char *out)
{
static char passwd[MD5_HASH_MAX_LEN], *p;
unsigned char final[16];
....
/* Don't leave anything around in vm they could use. */
memset(final, 0, sizeof(final)); // <=
return (passwd);
}Массив 'final' может содержать приватную информацию о пароле, которая затем обнуляется, но вызов функции 'memset' будет удалён компилятором. Подробности, почему так может произойти и чем это опасно, можно узнать из статьи "Перезаписывать память - зачем?" и описания диагностики V597.
Аналогичные места:
Стоит отметить, что сторонние библиотеки вносят большой вклад в развитие проекта, позволяя переиспользовать уже реализованные алгоритмы, но за их качеством тоже необходимо следить, как и за основным проектом. Я приведу всего несколько примеров из некоторых библиотек, дабы следовать тематике статьи и просто поднять вопрос о доверии к сторонним библиотекам.
В интерпретаторе PHP используется много библиотек, некоторые из них немного переписаны "под себя".
V579 The sqlite3_result_blob function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. sqlite3.c 82631
static void statInit(....)
{
Stat4Accum *p;
....
sqlite3_result_blob(context, p, sizeof(p), stat4Destructor);
....
}Скорее всего, здесь хотели получить размер объекта, а не указателя. Следовало написать sizeof(*p).
V501 There are identical sub-expressions '(1 << ucp_gbL)' to the left and to the right of the '|' operator. pcre_tables.c 161
const pcre_uint32 PRIV(ucp_gbtable[]) = {
(1<<ucp_gbLF),
0,
0,
....
(1<<ucp_gbExtend)|(1<<ucp_gbSpacingMark)|(1<<ucp_gbL)| // <=
(1<<ucp_gbL)|(1<<ucp_gbV)|(1<<ucp_gbLV)|(1<<ucp_gbLVT), // <=
(1<<ucp_gbExtend)|(1<<ucp_gbSpacingMark)|(1<<ucp_gbV)|
(1<<ucp_gbT),
....
};В выражении для вычисления одного элемента массива есть повторяющееся (1<<ucp_gbL). Судя по коду далее, одна из переменных ucp_gbL вполне могла бы называться ucp_gbT, либо просто лишняя.
V595 The 'dbh' pointer was utilized before it was verified against nullptr. Check lines: 103, 110. pdo_dbh.c 103
PDO_API void pdo_handle_error(pdo_dbh_t *dbh, ....)
{
pdo_error_type *pdo_err = &dbh->error_code; // <=
....
if (dbh == NULL || dbh->error_mode == PDO_ERRMODE_SILENT) {
return;
}
....
}Здесь в самом начале функции выполняется разыменование пришедшего указателя, а далее он проверяется на валидность.
V519 The '* code' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 100, 101. encoding.c 101
protected int file_encoding(...., const char **code, ....)
{
if (looks_ascii(buf, nbytes, *ubuf, ulen)) {
....
} else if (looks_utf8_with_BOM(buf, nbytes, *ubuf, ulen) > 0) {
DPRINTF(("utf8/bom %" SIZE_T_FORMAT "u\n", *ulen));
*code = "UTF-8 Unicode (with BOM)";
*code_mime = "utf-8";
} else if (file_looks_utf8(buf, nbytes, *ubuf, ulen) > 1) {
DPRINTF(("utf8 %" SIZE_T_FORMAT "u\n", *ulen));
*code = "UTF-8 Unicode (with BOM)"; // <=
*code = "UTF-8 Unicode"; // <=
*code_mime = "utf-8";
} else if (....) {
....
}
}Два раза задали кодировку в переменную, одна строчка лишняя и, возможно, приводит к неправильному поведению программы далее.
Несмотря на то, что PHP существует давно и популярен, в его коде и используемых библиотеках всё же нашлись подозрительные места, хотя подобного рода проект наверняка проверяется различными анализаторами.
Используя статический анализ регулярно, можно сэкономить массу времени на решение более полезных задач.