29.08.2025 09:27
AleksandraUvarova 1 806 Источник

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

О проекте

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

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

В этой статье мы будем говорить про языки C и C++, где неопределённое поведение поджидает на каждом углу. Недавно мы даже выпустили об этом целую книгу "Путеводитель C++ программиста по неопределённому поведению".

Но вернёмся же к нашим слонам. Мы проверили проект на коммите c998c36 с помощью статического анализатора PVS-Studio и плагина для Visual Studio Code.

Дюжина багов, мешающая правильно мыслить

Фрагмент N1

int fcgi_listen(const char *path, int backlog)
{
  char *s;
  int   tcp = 0;
  ....
  if (!tcp) {
    chmod(path, 0777);
  }
  ....
}

Предупреждение PVS-Studio: V1118 Excessive file permissions can lead to vulnerabilities. Consider restricting file permissions. fastcgi.c 761

Небольшой фрагмент кода, что тут может быть не так? Разберёмся. Функция chmod предоставляет права на файл пользователям системы. В нашем случае маска 0777 позволяет любому произвольному пользователю читать, изменять или исполнять этот файл. Такая уязвимость может привести к плачевным последствиям, подвергнуться атаке злоумышленников и нарушить работу всего программного обеспечения. Чтобы не допустить такого, лучше задать более щадящий режим и дополнительные условия для предоставления прав.

Кстати, эта первая ошибка, которую мы нашли с помощью этого диагностического правила. Оно появилось недавно, но уже приносит свои плоды. Попробуйте бесплатно PVS-Studio на своём проекте и проверьте безопасность вашего кода.

Фрагмент N2

Этот фрагмент содержит больше тысячи символов на одной строке. Найти проблемы в таком коде просто невозможно, и поэтому для статьи он был отформатирован. Кстати, понять, в чём проблема кода-колбасы, можно в небольшой статье.

static inline zend_string* phar_get_stub(....)
{
  ....
  static const char newstub1_1[] = 
  "Extract_Phar::$temp)) 
    {
    \nheader('HTTP/1.0 404 Not Found');
    \necho 
    \"<html>
      \\n <head>
        \\n  <title>File Not Found<title>       // <=
      \\n </head>
      \\n <body>
        \\n  <h1>404 - File Not Found</h1>
      \\n </body>
    \\n</html>\"; 
    // part of the string is hidden
    ";
  ....
}

Предупреждение PVS-Studio: V735 Possibly an incorrect HTML. The "\</head\>" closing tag was encountered, while the "\</title\>" tag was expected. stub.h 23

"Слона-то я и не приметил". Где, если не в проекте PHP, найти предупреждение на неправильно закрытый тег. Такая маленькая ошибка, а сломала всю вёрстку: браузер не сможет отобразить эту страницу. Для корректной работы следует заменить открывающий тег на закрывающий:

\\n  <title>File Not Found</title>

Фрагмент N3

static void zend_compile_class_decl(....)
{
  ....
  if (....) {
    if (toplevel) {
      if (extends_ast) {
        zend_class_entry *parent_ce = zend_lookup_class_ex(
          ce->parent_name, NULL, ZEND_FETCH_CLASS_NO_AUTOLOAD);  // <=
        ....  
      }
    }
  }
  ....
  if (ce->parent_name) {                                         // <=
    /* Lowercased parent name */
    zend_string *lc_parent_name = zend_string_tolower(ce->parent_name);
    opline->op2_type = IS_CONST;
    LITERAL_STR(opline->op2, lc_parent_name);
  }
}

Здесь можно увидеть следующий паттерн: указатель ce->parent_name передаётся как аргумент в функцию zend_lookup_class_ex, а затем проверяется на валидность. Если указатель проверяют на NULL, значит, он может быть нулевым, а его использование допустимо только после проверки.

Но вот в функцию zend_lookup_class_ex его передали без какой-либо проверки. Если внутри функции есть дополнительное условие, то ничего страшного в этом нет. Но давайте заглянем внутрь функции. Анализатор уже проверил её и указал на строчку, где происходит разыменование:

ZEND_API zend_class_entry *zend_lookup_class_ex(zend_string *name, 
                                                zend_string *key, 
                                                uint32_t flags)
{
  zend_class_entry *ce = NULL;
  zval *zv;
  zend_string *lc_name;
  zend_string *autoload_name;
  uint32_t ce_cache = 0;

  if ((   zval_gc_flags((name)->gc.u.type_info) & (1<<5))         // <=
       && __builtin_expect(
   !!((zend_gc_refcount(&(name)->gc)-1)/sizeof(void *) 
       <(compiler_globals.map_ptr_last)), 1)) {
    ce_cache = GC_REFCOUNT(name);
    ce = GET_CE_CACHE(ce_cache);
    if (EXPECTED(ce)) {
      return ce;
    }
  }
  ....
}

Вот и бум! Параметр name, в который передавался указатель ce->parent_name, используется без проверки. А как мы знаем, при разыменовании нулевого указателя поведение не определено. И может быть всё что угодно.

Предупреждение PVS-Studio: V595 The 'ce->parent_name' pointer was utilized before it was verified against nullptr. Check lines: 'zend_execute_API.c:1169', 'zend_compile.c:9135', 'zend_compile.c:9165'. zend_compile.c 9135

Подобные срабатывания

  • V595 The 'ce->parent_name' pointer was utilized before it was verified against nullptr. Check lines: 'zend_execute_API.c:1169', 'zend_compile.c:9135', 'zend_compile.c:9165'. zend_compile.c 9135

  • V595 The 'lcname' pointer was utilized before it was verified against nullptr. Check lines: 'zend_API.c:2764', 'zend_compile.c:8374', 'zend_compile.c:8394'. zend_compile.c 8374

  • V595 The 'fcc->function_handler' pointer was utilized before it was verified against nullptr. Check lines: 4078, 4097. zend_API.c 4078

Фрагмент N4

ZEND_API zend_result zend_register_functions(...) 
{
  while (ptr->fname) {
      reg_function = malloc(sizeof(zend_internal_function));
      memcpy(reg_function, &function, sizeof(zend_internal_function));
  }
}

Предупреждение PVS-Studio: V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 3069, 3068. zend_API.c 3069

Анализатор выдаёт предупреждение, что при попадании нулевого указателя в функцию memcpy поведение не определено. "Так, а где же тут нулевой указатель? Строчкой выше явное выделение памяти", — может возмутиться внимательный читатель. Но спешу вас огорчить, если невозможно выделить необходимый участок памяти, функция malloc может вернуть нулевой указатель. Поэтому после выделения памяти следует добавить проверку на NULL.

Подобные срабатывания

  • V575 The potential null pointer is passed into 'strncat' function. Inspect the first argument. Check lines: 1300, 1299. zend.c 1300

  • V575 The potential null pointer is passed into 'strncat' function. Inspect the second argument. Check lines: 1300, 1295. zend.c 1300

  • V575 The 'memmove' function processes '0' elements. Inspect the third argument. iconv.c 2435

  • V575 The 'memmove' function processes '0' elements. Inspect the third argument. filters.c 1411

Фрагмент N5

ZEND_API void zend_collect_module_handlers(void)
{
  modules_dl_loaded = realloc(modules_dl_loaded, 
                              sizeof(zend_module_entry*) 
                              * (dl_loaded_count + 1));
  modules_dl_loaded[dl_loaded_count] = NULL;
}

Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer 'modules_dl_loaded'. Check lines: 2527, 2526. zend_API.c 2527

Работа с памятью — неотъемлемая часть языков С и С++, и именно поэтому тут так просто выстрелить себе в ногу. Функция realloc так же, как и функция malloc, может вернуть нулевой указатель.

Подобные срабатывания

  • V522 There might be dereferencing of a potential null pointer 'module_request_startup_handlers'. Check lines: 2520, 2514. zend_API.c 2520

  • V522 There might be dereferencing of a potential null pointer 'class_cleanup_handlers'. Check lines: 2557, 2553. zend_API.c 2557

  • V522 There might be dereferencing of a potential null pointer 'list'. Check lines: 3162, 3161. zend_API.c 3162

Фрагмент N6

ZEND_API void zend_collect_module_handlers(void)
{
  modules_dl_loaded = realloc(modules_dl_loaded, 
                              sizeof(zend_module_entry*) 
                              * (dl_loaded_count + 1));
  modules_dl_loaded[dl_loaded_count] = NULL;
}

Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'modules_dl_loaded' is lost. Consider assigning realloc() to a temporary pointer. zend_API.c 2526

Насколько вы были внимательны? Это тот же фрагмент кода. Здесь ещё проблема в том, что в функцию realloc мы передаём тот же указатель, который сохраняет возвращаемый результат. Если функция вернёт нулевой указатель, то оригинальный указатель будет потерян, что приведёт к утечке памяти.

Подобные срабатывания

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'zend_version_info' is lost. Consider assigning realloc() to a temporary pointer. zend.c 1299

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'module_request_startup_handlers' is lost. Consider assigning realloc() to a temporary pointer. zend_API.c 2514

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'modules_dl_loaded' is lost. Consider assigning realloc() to a temporary pointer. zend_API.c 2526

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'class_cleanup_handlers' is lost. Consider assigning realloc() to a temporary pointer. zend_API.c 2553

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'zend_flf_handlers' is lost. Consider assigning realloc() to a temporary pointer. zend_API.c 3086

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'zend_flf_functions' is lost. Consider assigning realloc() to a temporary pointer. zend_API.c 3087

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'p' is lost. Consider assigning realloc() to a temporary pointer. zend_alloc.c 3299

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'ce->interfaces' is lost. Consider assigning realloc() to a temporary pointer. zend_inheritance.c 1576

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'ce->interfaces' is lost. Consider assigning realloc() to a temporary pointer. zend_inheritance.c 2194

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'q->set' is lost. Consider assigning realloc() to a temporary pointer. ir_private.h 593

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'strtab->buf' is lost. Consider assigning realloc() to a temporary pointer. ir_strtab.c 81

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'blacklist->entries' is lost. Consider assigning realloc() to a temporary pointer. zend_accelerator_blacklist.c 236

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer '* tmphstbuf' is lost. Consider assigning realloc() to a temporary pointer. network.c 1296

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'php_ini_scanned_files' is lost. Consider assigning realloc() to a temporary pointer. php_ini.c 702

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'b->value' is lost. Consider assigning realloc() to a temporary pointer. php_ini_builder.h 65

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'zend_extensions' is lost. Consider assigning realloc() to a temporary pointer. phpdbg.c 1216

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'state->code' is lost. Consider assigning realloc() to a temporary pointer. phpdbg_prompt.c 245

Фрагмент N7

typedef union _znode_op {
  uint32_t      num;
  ....
}
struct _zend_op {
  _znode_op op2;
  ....
}

struct _zend_op_array {
  uint32_t num_dynamic_func_defs;
  ....
}

typedef struct _zend_op_array zend_op_array;

static void preload_remove_declares(zend_op_array *op_array)
{
  zend_op *opline = op_array->opcodes;
  ....
  while (opline != end) {
    switch (opline->opcode) {
      ....
      case ZEND_DECLARE_FUNCTION:
        ....
        if (func && func == op_array->dynamic_func_defs[opline->op2.num]) {
          ....
          if (op_array->num_dynamic_func_defs == 0) {
            dynamic_func_defs = NULL;
          } else {
            ....
            if (op_array->num_dynamic_func_defs - opline->op2.num > 0) {  // <=
              memcpy(
                dynamic_func_defs + opline->op2.num,
                op_array->dynamic_func_defs + (opline->op2.num + 1),
                sizeof(zend_op_array*) 
                * (op_array->num_dynamic_func_defs - opline->op2.num));
            }
  // the rest of the code
}

Пришло время подключать математическую логику. Переменные op_array->num_dynamic_func_defs и opline->op2.num имеют беззнаковый тип, а значит, и само выражение будет также иметь беззнаковый тип и никогда не сможет быть меньше нуля.

Что мы видим тут? Вычитание двух беззнаковых чисел. Если уменьшаемое число будет меньше вычитаемого, то результатом будет число меньше нуля. Но, так как тут беззнаковые числа, происходит оборачивание по модулю. То есть 0u - 1 == UINT_MAX. Из-за этого условие может быть ложным только тогда, когда переменные не равны друг другу.

Это выражение следует заменить на следующее:

if (op_array->num_dynamic_func_defs > opline->op2.num)

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

Предупреждение PVS-Studio: V555 The expression of the 'A - B > 0' kind will work as 'A != B'. ZendAccelerator.c 3915

Фрагмент N8

uint64_t flags
....
PHPDBG_API void phpdbg_delete_breakpoint(zend_ulong num)
{
  ....     
  if ((brake = phpdbg_find_breakbase_ex(num, &table, &numkey, &strkey))) {
      int type = brake->type;
      char *name = NULL;
      size_t name_len = 0L;

      switch (type) {
        ....
        default: {
          if (zend_hash_num_elements(table) == 1) {
            PHPDBG_G(flags) &= ~(1<<(brake->type+1));              // <=
          }
        }
      }
    ....
  }
}

Предупреждения PVS-Studio:

V629 Consider inspecting the '1 << (brake->type + 1)' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. phpdbg_bp.c 1209

V784 The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. phpdbg_bp.c 1209

Математики, не расслабляемся. Переменная flags имеет тип unsigned long int, в то время как brake->type имеет тип int. Код призван снять определённый бит из flags. А теперь следим за руками:

  1. Константу 1 типа int сдвигают влево на какое-то число бит. Тип int чаще всего 32-битный. Очень надеемся, что сдвиг не производится на 32 и более битов, иначе поведение не определено.

  2. Результат сдвига побитово инвертируется. Результат инверсии по-прежнему имеет тип int.

  3. Результат инверсии расширяется до 64-битного беззнакового типа из-за левого операнда. Поскольку исходный тип был знаковым, то произойдёт преобразование с расширением знака. Это значит, что для положительных чисел в 32 старших бита попадут нулевые биты, а для отрицательных — единичные.

  4. Результат преобразования накладывается побитовым "И" на flags. Потеря старших значимых битов во flags будет происходить, когда правый операнд положительный. А им он будет только когда происходит сдвиг на 31 бит влево, т.е. когда нужно сбросить 31-й бит во flags. Пруф.

Заметили, сколько всего нужно держать в голове для такого безобидного выражения? А проблема кроется в разных размерах операндов и различной знаковости некоторых подвыражений. А надо всего лишь поменять у константы 1 тип с int до unsigned long long, и всё будет работать, как и задумывалось:

PHPDBG_G(flags) &= ~( 1uLL <<(brake->type+1));

Фрагмент N9

typedef signed long long timelib_sll;
....
timelib_sll timelib_get_current_offset(timelib_time *t);
....
static void php_do_date_sunrise_sunset(INTERNAL_FUNCTION_PARAMETERS, 
                                       bool calc_sunset)
{
  double latitude, longitude, zenith, gmt_offset, altitude;
  ....
  if (gmt_offset_is_null) {
      gmt_offset = timelib_get_current_offset(t) / 3600;  // <=
    }
  ....
}

Предупреждение PVS-Studio: V636 The 'timelib_get_current_offset(t) / 3600' expression was implicitly cast from 'long long' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. php_date.c 5557

Результат деления двух целочисленных типов — целочисленный тип. А для чего же тогда разработчик записывал результат в переменную с плавающей запятой?

Чтобы не терять точность вычисления, необходимо один из операндов привести к типу double:

gmt_offset = timelib_get_current_offset(t) / 3600.0;

Фрагмент N10

static zend_always_inline zend_result _zend_update_type_info(....)
{
  ....
  if (ssa_op->result_def >= 0) {
    tmp =   MAY_BE_ANY | MAY_BE_ARRAY_KEY_ANY 
          | MAY_BE_ARRAY_OF_ANY | MAY_BE_ARRAY_OF_REF;
    if (opline->result_type == IS_TMP_VAR) {
      if (   opline->opcode == ZEND_FETCH_R 
          || opline->opcode == ZEND_FETCH_IS) {
        /* Variable reference counter may be decremented before use */
        /* See: ext/opcache/tests/jit/fetch_r_001.phpt */
        tmp |= MAY_BE_RC1 | MAY_BE_RCN;
      } else {
        tmp |= MAY_BE_RC1 | MAY_BE_RCN;
      }
    }
  }
  ....
}

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. zend_inference.c 4078

Перед нами классический случай ошибки copy-paste. Независимо от того, как вычисляется условие if, код делает абсолютно одно и то же: добавляет флаги MAY_BE_RC1 | MAY_BE_RCN к переменной tmp.

Если здесь действительно должна быть разная логика — теперь её нет, и код работает неверно. Если разница не нужна, зачем тогда if?

Фрагмент N11

#define zend_error_noreturn_impl(type, format) do { \
    zend_string *filename; \
    uint32_t lineno; \
    va_list args; \
    get_filename_lineno(type, &filename, &lineno); \
    va_start(args, format); \
    zend_error_va_list(type, filename, lineno, format, args); \
    va_end(args); \
    /* Should never reach this. */ \
    abort(); \                                           // <=
  } while (0)
....
ZEND_API ZEND_COLD ZEND_NORETURN 
  void zend_error_noreturn(int type, const char *format, ...)
{
  zend_error_noreturn_impl(type, format);
}
....
ZEND_API zend_result zend_startup_module_ex(zend_module_entry *module)
{
  .... 
  if (module->module_startup_func) {
    EG(current_module) = module;
    if (module->module_startup_func(module->type, 
                                    module->module_number)==FAILURE) {
      zend_error_noreturn(E_CORE_ERROR,
                          "Unable to start %s module", 
                          module->name);
      EG(current_module) = NULL;                         // <=
      return FAILURE;
    }
    EG(current_module) = NULL;
  }
  return SUCCESS;
}

Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. zend_API.c 2437

Недостижимый код после вызова функции, не возвращающей управление. Такой код не только бесполезен, но и вводит в заблуждение.

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

Фрагмент N12

static void check_conflict(LexState*ls,struct LHS_assign*lh,expdesc*v){
  FuncState*fs = ls->fs;
  int extra = fs->freereg;
  int conflict = 0;
  for(; lh; lh = lh->prev){
    if(lh->v.k == VINDEXED){
      if(lh->v.u.s.info == v->u.s.info){ // <=
        conflict = 1;
        lh->v.u.s.info = extra;
      }
      if(lh->v.u.s.aux == v->u.s.info){  // <=
        conflict = 1;
        lh->v.u.s.aux = extra;
      }
    }
  }
  if(conflict){
    luaK_codeABC(fs, OP_MOVE, fs->freereg, v->u.s.info, 0);
    luaK_reserveregs(fs, 1);
  }
}

Предупреждение PVS-Studio: V778 Two similar code fragments were found. Perhaps, this is a typo and 'aux' variable should be used instead of 'info'. minilua.c 4331

Два схожих фрагмента кода. А ведь и правда, зачем писать одно и то же несколько раз, если можно скопировать и поменять в нужных местах? Чтобы не допускать таких ошибок.

В условии не все фрагменты info заменили на aux, из-за чего возможна некорректная работа программы, и она может выдавать неверные результаты.

Заключение

Вот такое получилось путешествие по диким джунглям PHP-кода. Давайте вместе делать этот мир безопаснее. Для этого у PVS-Studio есть варианты бесплатного лицензирования как для open source проектов, так и для студентов и преподавателей. Остальным же предлагаю получить пробную версию анализатора и попробовать его в деле :)

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Aleksandra Uvarova. What if your elephant thinks it is bug?.

Комментарии (1)


  1. datacompboy
    29.08.2025 12:17
    #28771726

    Такая маленькая ошибка, а сломала всю вёрстку: браузер не сможет отобразить эту страницу.

    Вы это, слона не натягивайте! Браузер спокойно такое пережовывает.