Перевірка PHP 7


Повторна перевірка проектів нерідко буває вельми цікавою. Вона дозволяє дізнатися, які нові помилки були допущені в ході розробки програми, а які помилки були виправлені. Раніше мій колега вже писав про перевірку 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("Центр");
}
....
}

Попередження 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, "Центр"); 
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:
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 цілком могло матися на увазі щось ще, тоді вираз буде не надмірною, а помилковим. До речі, це не єдине місце. Зустрілися ще кілька:
  • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!path' and 'path'. fopen_wrappers.c 643
  • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!headers_lc' and 'headers_lc'. sendmail.c 728
Про сторонніх бібліотеках
Про це я писав вище, але хочу повторитися ще раз. PHP використовує деякі сторонні бібліотеки, які, на жаль, не ідеальні і містять помилки. Багато попередження видавалися як раз на код цих бібліотек. Можна було б проаналізувати їх усі, але тоді, повірте, стаття вийшла б дуже великий.

Визначити, чи є помилка у коді інтерпретатора PHP або сторонньої бібліотеки досить просто – на початку всіх вихідних файлів знаходиться коментар, описує ліцензію, проект, авторів та ін. Відштовхуючись від цих коментарів легко визначити, у файлі якого проекту причаїлася помилка.

З іншого боку — деякі цікаві місця все ж варто розглянути. У будь-якому випадку, якщо ви використовуєте якісь сторонні бібліотеки, відповідальність перед користувачами за помилки, допущені в цих бібліотеках, лягає також і на ваші плечі, так як помилка може проявити себе в ході використання саме вашого продукту. Тому варто уважніше ставитися до того, які залежно ви тягнете в своєму проекті.

Висновок


Результати перевірки вийшли цікавими. Насправді знайшлося багато помилок, у статті розглядалася лише мала частина загальних попереджень високого і середнього рівнів достовірності. Багато з цих помилок знаходяться у бібліотеках, які використовуються в PHP, а значить, неявно, кочують у його код. В самому коді PHP теж виявилися цікаві помилки, деякі з яких і були виписані вище.

Підводячи підсумок, хотілося б сказати, що необхідно використовувати різні засоби, що дозволяють підвищити продуктивність праці і якості коду. Не варто обмежуватися тестами і code review. Статичний аналізатор — один з тих інструментів, які можуть допомогти програмісту писати більш якісний код дозволяючи корисніше використовувати той час, який було б витрачено на пошук помилки, допущеної з-за якої-небудь помилки. Але не варто забувати, що статичний аналізатор — інструмент регулярного застосування. І якщо ви ще не використовуєте його — рекомендую завантажити PVS-Studio, перевірити свій проект і подивитися, що ж цікавого вдасться знайти.



Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Sergey Vasiliev. Analysis of PHP7.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.


Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.