Святослав Размыслов

01 Сен 2014

Теги:

#Cpp

Заметка про проверку PHP

01 Сен 2014

Автор: Святослав Размыслов

Одинаковые условные выражения
Всегда ложное условие
Некорректное условие
Разность беззнаковых чисел
Разыменование указателя
Коварная оптимизация
Можем ли мы доверять используемым библиотекам?
Заключение

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);
  }
  ....
}

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

Аналогичное место:

V595 The 'callback_name' pointer was utilized before it was verified against nullptr. Check lines: 5007, 5021. basic_functions.c 5007

Коварная оптимизация

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.

Аналогичные места:

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
V597 The compiler could delete the 'memset' function call, which is used to flush 'output' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. crypt.c 214
V597 The compiler could delete the 'memset' function call, which is used to flush 'temp_result' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. crypt_sha512.c 622
V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The RtlSecureZeroMemory() function should be used to erase the private data. crypt_sha512.c 625
V597 The compiler could delete the 'memset' function call, which is used to flush 'alt_ctx' object. The RtlSecureZeroMemory() function should be used to erase the private data. crypt_sha512.c 626
V597 The compiler could delete the 'memset' function call, which is used to flush 'temp_result' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. crypt_sha256.c 574
V597 The compiler could delete the 'memset' function call, which is used to flush 'ctx' object. The RtlSecureZeroMemory() function should be used to erase the private data. crypt_sha256.c 577
V597 The compiler could delete the 'memset' function call, which is used to flush 'alt_ctx' object. The RtlSecureZeroMemory() function should be used to erase the private data. crypt_sha256.c 578

Можем ли мы доверять используемым библиотекам?

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

В интерпретаторе PHP используется много библиотек, некоторые из них немного переписаны "под себя".

libsqlite

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).

pcrelib

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, либо просто лишняя.

PDO

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;
  }
  ....
}

Здесь в самом начале функции выполняется разыменование пришедшего указателя, а далее он проверяется на валидность.

libmagic

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 существует давно и популярен, в его коде и используемых библиотеках всё же нашлись подозрительные места, хотя подобного рода проект наверняка проверяется различными анализаторами.

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

#Cpp