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



    PHP — скриптовый язык программирования общего назначения, интенсивно применяемый для разработки веб-приложений. В настоящее время поддерживается подавляющим большинством хостинг-провайдеров и является одним из лидеров среди языков программирования, применяющихся для создания динамических веб-сайтов.

    В случае с компиляторами и интерпретаторами к исходному коду и тестированию, как правило, предъявляются повышенные требования качества и надёжности. Тем не менее, в исходном коде интерпретатора PHP нашлись подозрительные места.

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

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

    Эта статья на английском


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. A Post About Analyzing PHP.

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    780,00
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Похожие публикации

    Комментарии 35

      –5
      Причины некоторых багов стали мне вдруг ясны.
        +41
        Поделитесь, с чем именно вы сталкивались?
          0
          Такое, неверное, только по рецепту выпускают.
          +5
          Полезная статья, даже не задумывался, что можно сам PHP на баги проверить.
          –25
            +16
            Уже прочитали спойлер «Прочитали статью и есть вопрос?» в конце статьи? ;)
              –8
              Да. Я не нашёл ссылки на статью в трекере, а авторы по каждому пункту статьи разные. Не уверен, что отправили всем.
              +2
              Конечно же нет! Они никогда не отправляют багрепорты, в каждой статье об этом пишут, а люди все равно спрашивают.
              +1
              Интересно количество багов в сравнении с конкурентами.
              –21
              Не ожидал что так мало полезных сообщений будет, учитывая сколько багав в багтрекере.
              Было бы интересно сравнить например с HipHop PHP, как в Фейсбуке дила с качаством.
                +10
                Очень часто баги, это не «очепятки» в коде, когда два раза пишется одно и то же условие и т.п., а абсолютно верная с точки зрения синтаксиса языка и логики конструкция, но в итоге выполняющая не то, что надо. И анализатор тут не поможет совершенно, ведь все же правильно и логично. Простейший пример, программа должна находить разность, а в коде сумма. Но анализатору-то неизвестно, что тут должно быть, с его точки зрения все будет совершенно правильным. Ну разве что переменные беззнаковые, где-то дальше проверка на неотрицательность результата и он скажет, что в ней нет смысла. Но это еще постараться надо.
                  –4
                  тоесть вы говорите что написание тестов гарантировано лучше статического анализа, так как кроме багов еще и опичатки отлавливает?
                    0
                    Я попрошу вас не додумывать за меня мои мысли, я такого не говорил ни разу и это никоим образом не следует из моего комментария. Я лишь указал на то, что нет прямой связи между кол-вом заявок в багтрекере и ошибок в коде, найденных анализатором, и попытался объяснить причину этого. Каждый метод поиска ошибок для своего. Для того, чтобы найти где «очепятка» проще постоянно использовать статический анализатор, а для проверки правильности тесты и живое использование с багтрекером. К слову, тест может и не отловить опечатку, если он не покрывает то место, в котором она расположена, так что вы дважды неправы, тесты не лучше анализатора (как я уже говорил, каждому свое) да еще и делаете какие-то свои, непонятные выводы из чужих слов, додумывая их «скрытый смысл» без участия автора.
                      –3
                      Cпасибо за развернутый ответ. К слову анализатор не найдет опичатку если нет соответсвующего правила ;)
                +3
                Удивительно мало багов, причём в extensions (хоть и включенных в поставку), а не в ядре.
                  +1
                  Я приведу всего несколько примеров из некоторых библиотек, дабы следовать тематике статьи и просто поднять вопрос о доверии к сторонним библиотекам.

                  Много подозрительных мест есть в libiconv, zend и других, особенно много в pcrelib. Разработчикам, знакомым с кодом, явно будет что посмотреть в полном отчёте проверки.
                    0
                    То что вы привели выше этого раздела тоже не является ядром PHP.
                    SAPI / ftp / crypt это встроенные extensions.
                      0
                      Немного запоздало, но все же, раз уж речь пошла о расширениях php, можно ли попросить вас проверить PHP-CPP?
                    0
                    В случае PDO указатель не разыменовывается. Просто прибавляется смещение.
                    0
                    А проверьте, пожалуйста, cppcms!
                      +6
                      Интересно проверять популярные проекты.
                      +1
                      Может я невнимателен, но я не понял какую версию PHP проверяли.
                      0
                      Спасибо за труд. Надеюсь, это пойдёт на пользу любимому языку, который в последнее время быстро развивается и радует своими новшествами.
                        +7
                        По моему, разрабы PHP имели лицензию на PVS-Studio, даже в их репе видел ветки где фиксили 64битные замечания. Так что не удивительно что баги в основном в сторонних либах.

                        P.S. Мне ваш коммент напомнил одну старую шутку про PHP:
                        У нас было два Оптерона, 16 гигабайт оперативки, 2 гига под мемкеш, полвинчестера свободно и целое множество плагинов и модулей всех сортов и расцветок, а также Апач, MySQL и гигабайт чистого свопа. Не то чтобы это был необходимый набор для Друпала, но если начал писать на PHP, становится трудно остановиться. Единственное, что вызывало у меня опасение — это своп. Нет ничего более беспомощного, безответственного и испорченного, чем свопящийся сервер. Я знал, что рано или поздно мы перейдем и на эту дрянь.
                          0
                          О! Я недавно искал эту цитату!
                        +5
                        Как насчет того чтобы стабильную или HEAD ветку ruby проверить?
                          +6
                          Уже по sqlite-dev рассылке письмецо пришло только что:
                          Greeting.

                          On
                          > www.viva64.com/en/b/0277/
                          there is a static analysis of the PHP interpreter. So far, so good.
                          What makes this of potential interest for the SQLite developers is the
                          following excerpt:

                          «Third-party libraries do make a large contribution to project
                          development allowing one to reuse already implemented algorithms, but
                          their quality should be checked as carefully as that of the basic
                          project code. I will cite just a few examples from third-party
                          libraries to meet the article's topic and simply muse over the
                          question of our trust in third-party libraries.

                          The PHP interpreter employs plenty of libraries, some of them slightly
                          customized by the authors for their needs.

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

                          }

                          I guess the programmer wanted to get the size of the object, not the
                          pointer. So it should have been sizeof(*p).»

                          Is this a
                          a) a bug in SQLite that
                          a1) has been fixed for a long time,
                          a2) is still open,
                          b) not a bug in SQLite (thus a fallacy by the author of the article)?

                          Проверил свежий сегодняшний снапшот, все так же — sizeof(p).

                          Пробежавшись поиском по sqlite.c, вроде, нашел еще одно место:
                          static int fts3ColumnMethod(
                            sqlite3_vtab_cursor *pCursor,   /* Cursor to retrieve value from */
                            sqlite3_context *pCtx,          /* Context for sqlite3_result_xxx() calls */
                            int iCol                        /* Index of column to read value from */
                          ){
                            int rc = SQLITE_OK;             /* Return Code */
                            Fts3Cursor *pCsr = (Fts3Cursor *) pCursor;
                            Fts3Table *p = (Fts3Table *)pCursor->pVtab;
                          ...
                              /* The extra column whose name is the same as the table.
                              ** Return a blob which is a pointer to the cursor.  */
                              sqlite3_result_blob(pCtx, &pCsr, sizeof(pCsr), SQLITE_TRANSIENT);
                          ...
                          }
                          


                          В общем, дальше по коду внутри функции(sqlite3_result_blob) вызывается:
                           SQLITE_PRIVATE int sqlite3VdbeMemSetStr(
                            Mem *pMem,          /* Memory cell to set to string value */
                            const char *z,      /* String pointer */
                            int n,              /* Bytes in string, or negative */
                            u8 enc,             /* Encoding of z.  0 for BLOBs */
                            void (*xDel)(void*) /* Destructor function */
                          ){ }
                          

                          Похоже на баг, жду ответов в подписке.
                            +3
                            А вот и ответ. Как оказывается, все немного сложнее:
                            I doubt this is an error at all. While it is true that the sqlite3_result_blob function uses a pointer to storage and the length of that storage are to determine the size of the blob, this information is only really necessary if the entire blob needs to be copied somewhere, as for example, if the destructor were SQLITE_TRANSIENT or if the blob were to be copied directly to a record.

                            When the blob value is accessed (with sqlite3_value_blob), the pointer returned will be the pointer to the original memory structure. Since that pointer is then cast back to the pointer type of the original structure (Stat4Accum*) there is no need for the length (size of the blob) to be accurate — the size and extent information is encoded right in the structure itself.

                            This may simply optimize internal processing within the various value/result routines. Since the value returned has its own destructor, sqlite knows when the pointer is no longer valid, and the destructor will clear and release the whole allocated memory.
                            –1
                            Очень интересно. Думаю, что не менее интересно будет про SpiderMonkey и v8.

                            Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                            Самое читаемое