PVS-Studio покопався в нутрощах Linux (3.18.1)

Linux and PVS-Studio
Співавтор: Святослав Размыслов SvyatoslavMC.

У рекламних цілях ми вирішили спробувати перевірити ядро Linux за допомогою нашого статичного аналізатора коду. Ця задача цікава своєю складністю. Вихідні коди Linux чим тільки не перевірялися і перевіряються. Тому знайти хоч щось нове, вельми складне завдання. Але якщо вийде, то це буде хороша рекламна замітка про можливості аналізатора PVS-Studio.

ми перевіряли
Ядро Linux було взято з сайту The Linux Kernel Archives. Перевірялася Latest Stable Kernel 3.18.1.

До того моменту, як я пишу цю статтю, вже з'явилося ядро 3.19-rc1. На жаль, перевірка проекту та написання статті займають чимало часу. Тому будемо задовольнятися перевіркою не самої останньої версії.

Тим, хто скаже, що ми повинні були перевірити саме останню версію, я хочу відповісти наступне.
  1. Ми регулярно перевіряємо велику кількість проектів. Крім безкоштовної перевірки проектів, у нас багато інших завдань. І тому немає ніякої можливості починати все спочатку, тільки тому, що з'явилася нова версія. Так ми можемо ніколи нічого не опублікувати :).
  2. 99% знайдених помилок залишилося на своєму місці. Так що цю статтю сміливо можна використовувати щоб трохи поліпшити код ядра Linux.
  3. Мета статті — реклама PVS-Studio. Якщо ми можемо знайти помилки в проекті версії X, то ми зможемо знайти що-то і у версії Y. Наші перевірки досить поверхні (ми не знайомі з проектом) і їх метою є написання таких статей. Справжню користь проекту приносить придбання ліцензії на PVS-Studio та його регулярне використання.

Як ми перевіряли
Для перевірки ядра використовувався статичний аналізатор коду PVS-Studio версії 5.21.

Для перевірки Linux Kernel був використаний дистрибутив Ubuntu-14.04, для якого було багато докладних інструкцій по конфігурації і складання ядра. Аналізатор перевіряє препроцессированные файли, які краще отримувати успішно компилируемых файлів, тому складання проекту один з найбільш важливих етапів перевірки.

Далі була написана невелика утиліта на C++, яка для кожного запущеного процесу компілятора зберігала командний рядок, поточну директорію і змінні оточення. Читачам, знайомим з продуктами PVS-Studio, повинна відразу згадається утиліта PVS-Studio Standalone, яка дозволяє перевірити будь-який проект під Windows. Там для звернення до процесів використовується WinAPI, тому для Linux даний механізм моніторингу був переписаний, решті ж-код, пов'язаний з запуском препроцессирования і перевірки, був повністю перенесено і перевірка ядра Linux стала лише питанням часу.

На початку безпеки
Якось так склалося, що аналізатор PVS-Studio сприймають виключно як інструмент для пошуку помилок. Але ніхто не звертає увагу, що PVS-Studio вміє виявляти і ряд вразливостей. Ми, звичайно, винні в цьому самі. Треба трохи виправити ситуацію.

Можна по-різному сприймати ті повідомлення, які видає PVS-Studio. Наприклад, з одного боку, це помилка, а з іншого може бути вразливістю. Все залежить від точки зору.

Хочу запропонувати розглянути кілька попереджень, які PVS-Studio видала при аналізі Linux. Я не кажу, що аналізатор знайшов уразливість в Linux. Але розглянуті попередження цілком могли б це зробити.

Небезпечне використання функції memcmp()
static unsigned char eprom_try_esi(
struct atm_dev *dev, unsigned short cmd,
int offset, int swap)
{
unsigned char buf[ZEPROM_SIZE];
struct zatm_dev *zatm_dev;
int i;

zatm_dev = ZATM_DEV(dev);
for (i = 0; i < ZEPROM_SIZE; i += 2) {
eprom_set(zatm_dev,ZEPROM_CS,cmd); /* select EPROM */
eprom_put_bits(zatm_dev,ZEPROM_CMD_READ,ZEPROM_CMD_LEN,cmd);
eprom_put_bits(zatm_dev,i >> 1,ZEPROM_ADDR_LEN,cmd);
eprom_get_byte(zatm_dev,buf+i+swap,cmd);
eprom_get_byte(zatm_dev,buf+i+1-swap,cmd);
eprom_set(zatm_dev,0,cmd); /* deselect EPROM */
}
memcpy(dev->esi,buf+offset,ESI_LEN);
return memcmp(dev->esi,"\0\0\0\0\0",ESI_LEN);
}

Попередження PVS-Studio: V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. zatm.c 1168

Зверніть увагу на оператор 'return' в самому кінці тіла функції.

Функція 'memcmp' повертає значення типу 'int':
  • < 0 — buf1 less than buf2;
  • 0 — buf1 identical to buf2;
  • > 0 — buf1 greater than buf2;
Зверніть увагу:
  • "> 0", означає будь-які числа, а зовсім не 1;
  • "< 0", це не обов'язково -1.
Возвращаемыми значеннями можуть бути: -100, 2, 3, 100, 256, 1024, 5555 і так далі. Це означає, що цей результат не можна перетворювати в тип 'unsigned char' (це тип, який повертає функція).

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

Небезпека такого роду помилок полягає в тому, що обчислене значення може залежати від архітектури і реалізації конкретної функції на даній архітектурі. Наприклад, програма буде коректно працювати в 32-бітному варіанті і некоректно в 64-бітному.

Ну і що? Подумаєш, неправильно буде перевірено щось, пов'язане з EPROM. Помилка, звичайно, але при чому тут вразливість?

А те, що діагностика V642 може виявити і вразливість! Не вірите? Будь ласка, ось ідентичний код з MySQL/MariaDB.
typedef char my_bool;
...
my_bool check(...) {
return memcmp(...);
}

Не PVS-Studio знайшов цю помилку. Але міг би.

Ця помилка стала причиною серйозної уразливості в MySQL/MariaDB до версій 5.1.61, 5.2.11, 5.3.5, 5.5.22. Суть в тому, що при підключенні користувача MySQL /MariaDB обчислюється токен (SHA від пароля і хеша), який порівнюється з очікуваним значенням функцією 'memcmp'. На деяких платформах обчислене значення може випадати з діапазону [-128..127]. У підсумку, в 1 випадку з 256 процедура порівняння хеша з очікуваним значенням завжди повертає значення 'true', незалежно від хеша. В результаті, проста команда на bash дає зловмиснику рутовий доступ до уразливого сервера MySQL, навіть якщо він не знає пароль. Причиною цього став код у файлі 'sql/password.c', показаний вище. Більш докладний опис цієї проблеми можна прочитати тут: Security vulnerability in MySQL/MariaDB.

Повернемося тепер до Linux. Ось ще один небезпечний фрагмент коду:
void sci_controller_power_control_queue_insert (....)
{
....
for (i = 0; i < SCI_MAX_PHYS; i++) {
u8 other;
current_phy = &ihost->phys[i];

other = memcmp(current_phy->frame_rcvd.iaf.sas_addr,
iphy->frame_rcvd.iaf.sas_addr,
sizeof(current_phy->frame_rcvd.iaf.sas_addr));

if (current_phy->sm.current_state_id == SCI_PHY_READY &&
current_phy->protocol == SAS_PROTOCOL_SSP &&
other == 0) {
sci_phy_consume_power_handler(iphy);
break;
}
}
....
}

Попередження PVS-Studio: V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. host.c 1846

Результат роботи функції memcmp() поміщається в змінну other, що має тип unsigned char. Не думаю, що ця якась уразливість, але робота SCSI контролер в небезпеці.

Ще парочку таких місць можна знайти тут:
  • V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. zatm.c 1168
  • V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. host.c 1789

Небезпечне використання функції memset()
Продовжимо шукати небезпечні місця. Звернемо свій погляд на функції, які «підчищають» за собою приватні дані. Зазвичай це різні функції шифрування. На жаль, не завжди обнулення пам'яті написано правильно. І тоді можна отримати вкрай неприємний результат. Детальніше про ці неприємні моменти можна дізнатися з цієї статті: "Перезаписувати пам'ять — навіщо?".

Розглянемо приклад неправильного коду:
static int crypt_iv_tcw_whitening (....)
{
....
u8 buf[TCW_WHITENING_SIZE];
....
out:
memset(buf, 0, sizeof(buf));
return r;
}

Попередження PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'buf' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. dm-crypt.c 708

На перший погляд все добре. Функція crypt_iv_tcw_whitening() виділила на стеку тимчасовий буфер, щось зашифрувала, а потім обнулила буфер з приватними даними за допомогою виклику функції memset(). Але, насправді, виклик функції memset() буде видалений компілятором при оптимізації. З точки зору мови C/C++ після обнулення буфера він ніяк не використовується. А значить буфер можна не обнуляти.

При цьому, помітити таку помилку вкрай складно. Написати юніт-тест складно. Під відладчиком така помилка теж не буде видно (Debug-версії буде присутній виклик функції memset).

При цьому хочу наголосити. Це не «теоретично-можливе поведінка компілятора», а дуже навіть практичне. Компілятори дійсно видаляють виклик функції memset(). Детальніше про це можна прочитати в описі діагностики V597.

PVS-Studio недоречно рекомендує використовувати функцію RtlSecureZeroMemory(), оскільки орієнтований на Windows. В Linux такої функції, звичайно, немає. Але тут головне-попередити, а підібрати потрібну функцію вже не складно.

Наступний аналогічний приклад:
static int sha384_ssse3_final(struct shash_desc *desc, u8 *hash)
{
u8 D[SHA512_DIGEST_SIZE];

sha512_ssse3_final(desc, D);

memcpy(hash, D, SHA384_DIGEST_SIZE);
memset(D, 0, SHA512_DIGEST_SIZE);

return 0;
}

Попередження PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'D' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha512_ssse3_glue.c 222

А нижче приклад, де можуть бути не обнулені відразу 4 буфера: keydvt_out, keydvt_in, ccm_n, mic. Код взято з файлу security.c (рядка 525 — 528).
int wusb_dev_4way_handshake (....)
{
....
struct aes_ccm_nonce ccm_n;
u8 mic[8];
struct wusb_keydvt_in keydvt_in;
struct wusb_keydvt_out keydvt_out;
....
error_dev_update_address:
error_wusbhc_set_gtk:
error_wusbhc_set_ptk:
error_hs3:
error_hs2:
error_hs1:
memset(hs, 0, 3*sizeof(hs[0]));
memset(&keydvt_out, 0, sizeof(keydvt_out));
memset(&keydvt_in, 0, sizeof(keydvt_in));
memset(&ccm_n, 0, sizeof(ccm_n));
memset(mic, 0, sizeof(mic));
if (result < 0)
wusb_dev_set_encryption(usb_dev, 0);
error_dev_set_encryption:
kfree(hs);
error_kzalloc:
return result;
....
}

І останній приклад, де в пам'яті залишається «бовтатися» якийсь пароль:
int
E_md4hash(const unsigned char *passwd, unsigned char *p16,
const struct nls_table *codepage)
{
int rc;
int len;
__le16 wpwd[129];

/* Password cannot be longer than 128 characters */
if (passwd) /* Password must be converted to NT unicode */
len = cifs_strtoUTF16(wpwd, passwd, 128, codepage);
else {
len = 0;
*wpwd = 0; /* Ensure string is null-terminated */
}

rc = mdfour(p16, (unsigned char *) wpwd, len * sizeof(__le16));
memset(wpwd, 0, 129 * sizeof(__le16));

return rc;
}

Попередження PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'wpwd' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. smbencrypt.c 224

На цьому зупинюся. Ще 3 невдалих memset() можна знайти тут:
  • sha256_ssse3_glue.c 214
  • dev-sysfs.c 104
  • qp.c 143

Небезпечні перевірки
У аналізатора PVS-Studio є діагностика V595, яка виявляє ситуації, коли вказівник на початку разыменовывается, а потім перевіряється на рівність NULL. Іноді з цією діагностикою все просто і зрозуміло. Розглянемо такий простий випадок:
static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n)
{
struct net *net = sock_net(skb->sk);
struct nlattr *tca[TCA_ACT_MAX + 1];
u32 portid = skb ? NETLINK_CB(skb).portid : 0;
....
}

Попередження PVS-Studio: V595 The 'skb' pointer was utilized before it was verified against nullptr. Check lines: 949, 951. act_api.c 949

Тут все просто. Якщо покажчик 'skb' буде дорівнює нулю, то бути біді. Покажчик разыменовывается в першому рядку.

Слід зазначити, що аналізатор лається не тому, що побачив розіменування неперевіреного покажчика. Так було б надто багато помилкових спрацьовувань. Адже не завжди аргумент функції може бути дорівнює 0. Можливо, перевірка здійснювалася десь ще.

Логіка роботи інша. PVS-Studio вважає код небезпечним, якщо вказівник разыменовывается, а потім нижче перевіряється. Якщо покажчик перевіряють, то припускають, що він може бути дорівнює 0. Отже це привід видати попередження.

З цим простим прикладом розібралися. Але нам цікавий зовсім не він.

Тепер перейдемо до більш складного випадку, пов'язаного з оптимизациями, які виконують компілятори.
static int podhd_try_init(struct usb_interface *interface,
struct usb_line6_podhd *podhd)
{
int err;
struct usb_line6 *line6 = &podhd->line6;

if ((interface == NULL) || (podhd == NULL))
return-ENODEV;
....
}

Попередження PVS-Studio: V595 The 'podhd' pointer was utilized before it was verified against nullptr. Check lines: 96, 98. podhd.c 96

Це приклад коду, побачивши який, люди починають сперечатися і говорити, що все добре. Вони міркують так.

Нехай покажчик podhd дорівнює NULL. Негарно виглядає вираз &podhd->line6. Але помилки немає. Тут немає доступу до пам'яті. Просто обчислюється адресу одного з членів класу. Так, значення покажчика 'line6' некоректно. Воно вказує «в нікуди». Але адже цей покажчик не використовується. Вирахували некоректну адресу, ну і що? Нижче в коді розташована перевірка і якщо 'podhd', то функція завершить свою роботу. Покажчик 'line6' ніде не використовується, а значить на практиці ніякої помилки не буде.

Панове, ви не праві! Так робити все одно не можна. Не лінуйтеся і виправляйте такий код.

При оптимізації компілятор міркує так. Ось тут покажчик разыменовывается: podhd->line6. Ага, програміст знає що робить. Значить покажчик тут точно не дорівнює нулю. Відмінно, запам'ятаємо це.

Далі компілятор зустрічає перевірку:
if ((interface == NULL) || (podhd == NULL))
return-ENODEV;

І оптимізує її. Він вважає, що покажчик 'podhd' не дорівнює нулю. Тому він скоротить перевірку до:
if ((interface == NULL))
return-ENODEV;

Як і у випадку з memset() додаткова складність у тому, що ми не побачимо відсутність перевірки в Debug() версії. Таку помилку дуже складно шукати.

В результаті, якщо передати у функцію нульовий покажчик, то функція поверне статус (-ENODEV), а продовжить свою роботу. Наслідки можуть бути непередбачувані.

Важливо наступне. Компілятор може видалити важливу перевірку покажчика з погано написаного коду. Тобто існують функції, які тільки здається, що перевіряють покажчики. Насправді, вони будуть працювати з нульовим покажчиком. Не знаю, чи можна це якось використовувати. Але, на мій погляд, таку ситуацію цілком можна розглядати як потенційну вразливість.

Ще один подібний приклад:
int wpa_set_keys(struct vnt_private *pDevice, void *ctx,
bool fcpfkernel) __must_hold(&pDevice->lock)
{
....
if (is_broadcast_ether_addr(&param->addr[0]) ||
(param->addr == NULL) {
....
}

Попередження PVS-Studio: V713 The pointer param->addr was utilized in the logical expression before it was verified against nullptr in the same logical expression. wpactl.c 333

Компілятор при оптимізації може скоротити перевірку до:
if (is_broadcast_ether_addr(&param->addr[0]))

Ядро Linux велике. Так що попереджень V595 було видано більше 200 штук. На свій сором, я полінувався їх переглядати і вибрав тільки один приклад для статті. Підозрілі місця я пропоную вивчити комусь із розробників. Я наводжу їх списком: Linux-V595.txt.

Так, далеко не всі ці попередження вказують на реальну помилку. Часто покажчик точно не може бути дорівнює нулю. Тим не менш, цей список варто вивчити. Майже напевно можна знайти пару десятків справжніх помилок.

Знайдені підозрілі місця
Можливо, не всі фрагменти коду, описані в статті, містять реальну помилку. Однак, вони вкрай підозрілі і вимагають пильної уваги для вивчення.

Некоректні логічні умови
void b43legacy_phy_set_antenna_diversity (....)
{
....
if (phy->rev >= 2) {
b43legacy_phy_write(
dev, 0x0461, b43legacy_phy_read(dev, 0x0461) | 0x0010);
....
} else if (phy->rev >= 6)
b43legacy_phy_write(dev, 0x049B, 0x00DC);
....
}

Попередження PVS-Studio: V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 2147, 2162. phy.c 2162

Друга умова ніколи не виконується. Для наочності спростимо код:
if ( A >= 2)
X();
else if ( A >= 6)
Y();

Як бачите, не існує такого значення змінної 'A', при якому буде викликана функція Y().

Розглянемо інші схожі випадки. У поясненні вони не потребують.
static int __init scsi_debug_init(void)
{
....
if (scsi_debug_dev_size_mb >= 16)
sdebug_heads = 32;
else if (scsi_debug_dev_size_mb >= 256)
sdebug_heads = 64;
....
}

Попередження PVS-Studio: V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 3858, 3860. scsi_debug.c 3860

static ssize_t ad5933_store (....)
{
....
/* 2x, 4x handling, see datasheet */
if (val > 511)
val = (val >> 1) | (1 << 9);
else if (val > 1022)
val = (val >> 2) | (3 << 9);
....
}

Попередження PVS-Studio: V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 439, 441. ad5933.c 441

Є ще парочка схожих ситуацій, які не буду наводити, щоб не робити статтю надмірно довгої:
  • V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 1417, 1422. bnx2i_hwi.c 1422
  • V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 4815, 4831. stv090x.c 4831
Тепер розглянемо інший різновид підозрілих умов.
static int dgap_parsefile(char **in)
{
....
int module_type = 0;
....
module_type = dgap_gettok(in);
if (module_type == 0 || module_type != PORTS ||
module_type != MODEM) {
pr_err("failed to set a type of module");
return -1;
}
....
}

Попередження PVS-Studio: V590 Consider inspecting the 'module_type == 0 || module_type != 68' expression. The expression is excessive or contains a misprint. dgap.c 6733

Я не знайомий з кодом і у мене немає ідей, як повинна виглядати ця перевірка. Тому утримаюся від коментарів. Ось ще одне схоже місце:
  • V590 Consider inspecting the 'conc_type == 0 || conc_type != 65' expression. The expression is excessive or contains a misprint. dgap.c 6692

«Вирви око»
В ході вивчення повідомлень аналізатора я зустрів функцію name_msi_vectors(). Вона хоч і коротка, але читати її зовсім не хочеться. Мабуть, саме тому вона містить дуже підозрілу рядок.
static void name_msi_vectors(struct ipr_ioa_cfg *ioa_cfg)
{
int vec_idx, n = sizeof(ioa_cfg->vectors_info[0].desc) - 1;

for (vec_idx = 0; vec_idx < ioa_cfg->nvectors; vec_idx++) {
snprintf(ioa_cfg->vectors_info[vec_idx].desc, n,
"host%d-%d", ioa_cfg->host->host_no, vec_idx);
ioa_cfg->vectors_info[vec_idx].
desc[strlen(ioa_cfg->vectors_info[vec_idx].desc)] = 0;
}
}

Попередження: V692 An inappropriate attempt to append a null character to a string. To determine the length of a string by 'strlen' function correctly, a string ending with a null terminator should be used in the first place. ipr.c 9409

Підозрілою є останній рядок:
ioa_cfg->vectors_info[vec_idx].
desc[strlen(ioa_cfg->vectors_info[vec_idx].desc)] = 0;

Зараз я її спрощу і відразу стане зрозуміло, що тут щось не так:
S[strlen(S)] = 0;

У такому кроці немає ніякого сенсу. Нуль буде записаний туди, де він знаходиться. Є підозра, що хотіли зробити щось інше.

Нескінченне очікування
static int ql_wait_for_drvr_lock(struct ql3_adapter *qdev)
{
int i = 0;

while (i < 10) {
if (i)
ssleep(1);

if (ql_sem_lock(qdev,
QL_DRVR_SEM_MASK,
(QL_RESOURCE_BITS_BASE_CODE | (qdev->mac_index)
* 2) << 1)) {
netdev_printk(KERN_DEBUG, qdev->ndev,
"driver lock acquired\n");
return 1;
}
}

netdev_err(qdev->ndev,
"Timed out waiting for driver lock...\n");
return 0;
}

Попередження PVS-Studio: V654 The condition 'i < 10' of loop is always true. qla3xxx.c 149

Функція намагається залочить драйвер. Якщо це не виходить, вона чекає 1 секунду і повторює спробу. Все повинно бути зроблено 10 спроб.

Але, насправді, буде нескінченна кількість спроб. Причина в тому, що змінна 'i' ніде не збільшується.

Неправильна інформація про помилку
static int find_boot_record(struct NFTLrecord *nftl)
{
....
if ((ret = nftl_read_oob(mtd, block * nftl->EraseSize +
SECTORSIZE + 8, 8, &retlen,
(char *)&h1) < 0) ) {
printk(KERN_WARNING "ANAND header found at 0x%x in mtd%d "
"but OOB data read failed (err %d)\n",
block * nftl->EraseSize, nftl->mbd.mtd->index, ret);
continue;
....
}

Попередження PVS-Studio: V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. nftlmount.c 92

Якщо виникне помилка, функція повинна роздрукувати інформацію про неї. В тому числі має бути роздрукований код помилки. Але, насправді, буде роздруковано (err 0) або (err 1), а не справжній код помилки.

Причина — програміст заплутався у пріоритетах операції. На початку він хотів помістити результат роботи функції nftl_read_oob() в змінну 'ret'. Потім він хоче порівняти цю змінну з 0. Якщо (ret < 0), то потрібно роздрукувати повідомлення про помилку.

Насправді, все працює не так. На початку результат роботи функції nftl_read_oob() порівнюється з 0. Результатом порівняння є значення 0 або 1. Цей значення буде поміщено в змінну 'ret'.

Таким чином, якщо функція nftl_read_oob() повернула від'ємне значення, то ret == 1. Буде виведено повідомлення, але неправильне.

В умови видно, що використовуються додаткові квадратні дужки. Невідомо, чи були вони написані щоб придушити попередження компілятора про присвоювання всередині 'if' або щоб явно вказати послідовність операцій. Якщо друге, то ми маємо справу з помилкою — дужка закривається стоїть не на своєму місці. Правильно буде так:
if ((ret = nftl_read_oob(mtd, block * nftl->EraseSize +
SECTORSIZE + 8, 8, &retlen,
(char *)&h1)) < 0 ) {

Підозра на опечатку
int wl12xx_acx_config_hangover(struct wl1271 *wl)
{
....
acx->recover_time = cpu_to_le32(conf->recover_time);
acx->hangover_period = conf->hangover_period;
acx->dynamic_mode = conf->dynamic_mode;
acx->early_termination_mode = conf->early_termination_mode;
acx->max_period = conf->max_period;
acx->min_period = conf->min_period;
acx->increase_delta = conf->increase_delta;
acx->decrease_delta = conf->decrease_delta;
acx->quiet_time = conf->quiet_time;
acx->increase_time = conf->increase_time;
acx->window_size = acx->window_size; <<<---
....
}

Попередження PVS-Studio: V570 The 'acx->window_size' variable is assigned to itself. acx.c 1728

Скрізь поля однієї структури копіюються в поля іншої структури. І тільки в одному місці раптом написано:
acx->window_size = acx->window_size;

Це помилка? Чи так і задумано? Я не можу дати відповідь.

Підозріле вісімкове число
static const struct XGI330_LCDDataDesStruct2 XGI_LVDSNoScalingDesData[] = {
{0, 648, 448, 405, 96, 2}, /* 00 (320x200,320x400,
640x200,640x400) */
{0, 648, 448, 355, 96, 2}, /* 01 (320x350,640x350) */
{0, 648, 448, 405, 96, 2}, /* 02 (360x400,720x400) */
{0, 648, 448, 355, 96, 2}, /* 03 (720x350) */
{0, 648, 1, 483, 96, 2}, /* 04 (640x480x60Hz) */
{0, 840, 627, 600, 128, 4}, /* 05 (800x600x60Hz) */
{0, 1048, 805, 770, 136, 6}, /* 06 (1024x768x60Hz) */
{0, 1328, 0, 1025, 112, 3}, /* 07 (1280x1024x60Hz) */
{0, 1438, 0, 1051, 112, 3}, /* 08 (1400x1050x60Hz)*/
{0, 1664, 0, 1201, 192, 3}, /* 09 (1600x1200x60Hz) */
{0, 1328, 0, 0771, 112, 6} /* 0A (1280x768x60Hz) */
^^^^
^^^^
};

Попередження PVS-Studio: V536 Be advised that the utilized constant value is represented by an octal form. Oct: 0771, Dec: 505. vb_table.h 1379

Всі числа в цій структурі задані в десятковому форматі. І раптом зустрічається одне вісімкове число: 0771. Аналізатор це насторожило. І мене теж.

Є підозра, що цей нуль написаний для того, щоб красиво вирівняти стовпчик. Але тоді це явно некоректне значення.

Підозріла рядок
static void sig_ind(PLCI *plci)
{
....
byte SS_Ind[] =
"\x05\x02\x00\x02\x00\x00"; /* Hold_Ind struct*/
byte CF_Ind[] =
"\x09\x02\x00\x06\x00\x00\x00\x00\x00\x00";
byte Interr_Err_Ind[] =
"\x0a\x02\x00\x07\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00";
byte CONF_Ind[] =
"\x09\x16\x00\x06\x00\x00\0x00\0x00\0x00\0x00";
^^^^^^^^^^^^^^^^^^^
....
}

Попередження PVS-Studio: V638 A terminal null is present inside a string. The '\0x00' characters were encountered. Probably meant: '\x00'. message.c 4883

В масивах містяться якісь магічні значення. Підозру викликає вміст масиву CONF_Ind[]. У ньому чергуються нульові символи з текстом «x00». Мені здається, це помилка і, насправді, має бути написано так:
byte CONF_Ind[] =
"\x09\x16\x00\x06\x00\x00\x00\x00\x00\x00";

Тобто '0' перед 'x' зайвий і написаний випадково. В результаті «x00» інтерпретується як текст, а не як код символу.

Підозріле форматування коду
static int grip_xt_read_packet (....)
{
....
if ((u ^ v) & 1) {
buf = (buf << 1) | (u >> 1);
t = strobe;
i++;
} else

if((((u ^ v) & (v ^ w)) >> 1) & ~(u | v | w) & 1) {
....
}

Попередження PVS-Studio: V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. grip.c 152

Я не думаю, що тут є помилка. Але код відформатований дуже погано. Тому і наводжу його в статті. Краще зайвий раз перевірити.

Невизначений поведінку в операціях зсуву
static s32 snto32(__u32 value, unsigned n)
{
switch (n) {
case 8: return ((__s8)value);
case 16: return ((__s16)value);
case 32: return ((__s32)value);
}
return value & (1 << (n - 1)) ? value | (-1 << n) : value;
}

Попередження PVS-Studio: V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. hid-core.c 1016

Зсув від'ємних значень призводить до невизначеної поведінки. Про це я багато разів писав і не буду зупинятися на цьому докладно. Тим, хто не знає, в чому тут справа, пропоную для ознайомлення статтю "Не знаючи броду, не лізь у воду. Частина третя (про зрушення)".

Передбачаю заперечення в дусі: «але ж працює!». Можливо і працює. Але, я думаю, ядро Linux це не те місце, де можна покладатися на такий підхід. Код краще переписати.

Таких зрушень дуже багато, тому я зібрав їх в окремий файл: Linux-V610.txt.

Плутанина з enum
Є ось такий два enum:
enum iscsi_param {
....
ISCSI_PARAM_CONN_PORT,
ISCSI_PARAM_CONN_ADDRESS, <<<<----
....
};

enum iscsi_host_param {
ISCSI_HOST_PARAM_HWADDRESS,
ISCSI_HOST_PARAM_INITIATOR_NAME,
ISCSI_HOST_PARAM_NETDEV_NAME,
ISCSI_HOST_PARAM_IPADDRESS, <<<<----
ISCSI_HOST_PARAM_PORT_STATE,
ISCSI_HOST_PARAM_PORT_SPEED,
ISCSI_HOST_PARAM_MAX,
};

Зверніть увагу на константу ISCSI_PARAM_CONN_ADDRESS і ISCSI_HOST_PARAM_IPADDRESS. Їх назви схожі. Мабуть, це і стало причиною плутанини.

Розглянемо фрагмент коду:
int iscsi_conn_get_addr_param(
struct sockaddr_storage *addr,
enum iscsi_param param, char *buf)
{
....
switch (param) {
case ISCSI_PARAM_CONN_ADDRESS:
case ISCSI_HOST_PARAM_IPADDRESS: <<<<----
....
case ISCSI_PARAM_CONN_PORT:
case ISCSI_PARAM_LOCAL_PORT:
....
default:
return-EINVAL;
}

return len;
}

Попередження PVS-Studio: V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. libiscsi.c 3501

Константа ISCSI_HOST_PARAM_IPADDRESS не відноситься до enum iscsi_param. Швидше за все це помилка, і повинна використовуватися константа ISCSI_PARAM_CONN_ADDRESS.

Аналогічні попередження PVS-Studio:
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. svm.c 1360
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. vmx.c 2690
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. request.c 2842
  • V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. request.c 2868

Дивний цикл
Тут я не зможу показати фрагмент коду. Фрагмент великий, і я не знаю, як його скоротити і красиво відформатувати. Тому я напишу псевдокод.
void pvr2_encoder_cmd ()
{
do {
....
if (A) break;
....
if (B) break;
....
if © continue;
....
if (E) break;
....
} while(0);
}

Цикл виконується 1 раз. Є підозра, що така конструкція створена для того, щоб обійтися без оператора goto. Якщо щось пішло не так, викликається оператор 'break', і починають виконуватися оператори, розташовані після циклу.

Мене бентежить, що в одному місці написаний оператор 'continue', а не 'break'. При цьому працює оператор 'continue', як 'break'. Поясню.

Ось що говорить стандарт:

§6.6.2 in the standard: «continue The statement (...) causes control to pass to the loop-continuation portion of the smallest enclosing iteration-statement, that is, to the end of the loop.» (Not to the beginning.)

Таким чином, після виклику оператора 'continue' буде перевірено умова (0), і цикл завершиться так як умова хибна.

Можливо 2 варіанти.
  1. Код коректний. Оператор 'continue' повинен переривати цикл. Тоді я рекомендую замінити його для однаковості на 'break', щоб він плутав розробників, які будуть працювати з цим кодом надалі.
  2. Оператор 'continue' повинен за задумом програміста відновлювати цикл. Тоді код некоректний і його слід переписати.

Copy-Paste помилка
void dm_change_dynamic_initgain_thresh(
struct net_device *dev, u32 dm_type, u32 dm_value)
{
....
if (dm_type == DIG_TYPE_THRESH_HIGH)
{
dm_digtable.rssi_high_thresh = dm_value;
}
else if (dm_type == DIG_TYPE_THRESH_LOW)
{
dm_digtable.rssi_low_thresh = dm_value;
}
else if (dm_type == DIG_TYPE_THRESH_HIGHPWR_HIGH) <<--
{ <<--
dm_digtable.rssi_high_power_highthresh = dm_value; <<--
} <<--
else if (dm_type == DIG_TYPE_THRESH_HIGHPWR_HIGH) <<--
{ <<--
dm_digtable.rssi_high_power_highthresh = dm_value; <<--
} <<--
....
}

Попередження PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1755, 1759. r8192U_dm.c 1755

Код писався з допомогою Copy-Paste і в одному блоці тексту забули замінити:
  • DIG_TYPE_THRESH_HIGHPWR_HIGH на DIG_TYPE_THRESH_HIGHPWR_LOW
  • rssi_high_power_highthresh на rssi_high_power_lowthresh
Плюс прошу розробників подивитися сюди:
  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1670, 1672. rtl_dm.c 1670
  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 530, 533. ioctl.c 530

Повторна ініціалізація
Є дивні місця, де змінної два рази поспіль присвоюють різні значення. Думаю, варто перевірити ці ділянки коду.
static int saa7164_vbi_fmt(struct file *file, void *priv,
struct v4l2_format *f)
{
/* ntsc */
f->fmt.vbi.samples_per_line = 1600; <<<<----
f->fmt.vbi.samples_per_line = 1440; <<<<----
f->fmt.vbi.sampling_rate = 27000000;
f->fmt.vbi.sample_format = V4L2_PIX_FMT_GREY;
f->fmt.vbi.offset = 0;
f->fmt.vbi.flags = 0;
f->fmt.vbi.start[0] = 10;
f->fmt.vbi.count[0] = 18;
f->fmt.vbi.start[1] = 263 + 10 + 1;
f->fmt.vbi.count[1] = 18;
return 0;
}

Попередження PVS-Studio: V519 The 'f->fmt.vbi.samples_per_line' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1001, 1002. saa7164-vbi.c 1002
static int saa7164_vbi_buffers_alloc(struct saa7164_port *port)
{
....
/* Init and establish defaults */
params->samplesperline = 1440;
params->numberoflines = 12; <<<<----
params->numberoflines = 18; <<<<----
params->pitch = 1600; <<<<----
params->pitch = 1440; <<<<----
params->numpagetables = 2 +
((params->numberoflines * params->pitch) / PAGE_SIZE);
params->bitspersample = 8;
....
}

Попередження:
  • V519 The 'params->numberoflines' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 118, 119. saa7164-vbi.c 119
  • V519 The 'params->pitch' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 120, 121. saa7164-vbi.c 121


Висновок
В будь-якому великому проекті можна знайти якісь помилки. Ядро Linux не стало винятком. Однак, разові перевірки коду статичним аналізатором є неправильним способом його застосування. Так, вони дозволяють написати ось таку рекламну статтю, але приносять мало користі проекту.

Використовуйте статичний аналіз регулярно, і ви заощадите масу часу, виявляючи ряд помилок на самому ранньому етапі їх виникнення. Захистити свій проект від багів з допомогою статичного аналізатора!

Linux and PVS-Studio

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

Ця стаття англійською
Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Andrey Karpov. PVS-Studio dives into Linux insides (3.18.1).

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


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

0 коментарів

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