Проверка операционной системы Haiku (семейство BeOS) c помощью PVS-Studio. Часть 2
Это заключительная статья о проверке операционной системы Haiku. В первой статье были собраны возможные ошибки различных типов диагностик, но так или иначе связанных с проверкой условий. В этой статье будут представлены оставшиеся предупреждения анализатора, о которых я хотел бы рассказать. Собранные примеры разделены на несколько групп.Haiku — свободная операционная система для персональных компьютеров, которая нацелена на двоичную совместимость с операционной системой BeOS. Haiku воплощает в себе основные идеи BeOS. Это модульная система, архитектурно решённая как гибридное ядро: микроядерная архитектура, способная динамически подгружать необходимые модули.
Проект проверялся по просьбе сообщества пользователей Haiku с помощью PVS-Studio 5.24.
Работа со строкамиV527 It is odd that the '\0' value is assigned to 'char' type pointer. Probably meant: *scratchPtr = '\0'. TextGapBuffer.cpp 228 const char* TextGapBuffer: Text () { const char* realText = RealText ();
if (fPasswordMode) { …
char* scratchPtr = fScratchBuffer; for (uint32 i = 0; i < numChars; i++) { memcpy(scratchPtr, B_UTF8_BULLET, bulletCharLen); scratchPtr += bulletCharLen; } scratchPtr = '\0'; //<==
return fScratchBuffer; }
return realText; } Скорее всего, после работы со строкой хотели записать терминальный ноль в конец строки, а не обнулить указатель. Правильный вариант:»*scratchPtr = '\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. PoorManWindow.cpp 254
void PoorManWindow: MessageReceived (BMessage* message) { … if (inet_ntop (AF_INET, &sin_addr, addr, sizeof (addr)) != NULL){ addr[strlen (addr)] = '\0'; //<== line << '(' << addr << ") "; } .... } Чтобы записать терминальный ноль в конец строки, тут воспользовались функций strlen(), но результат такого действия непредсказуем, ведь для работы функции strlen() строка уже должна заканчиваться терминальным нулём. Ноль будет записан как раз в ту ячейку, где был найден 0. При этом, функция strlen() может выйти далеко за пределы буфера, что приведет к неопределенному поведению программы. Чтобы исправить код, надо вычислить длину строки каким-то другим способом.Плохие циклы V529 Odd semicolon ';' after 'for' operator. ringqueue.cpp 39 int compute_order(unsigned long size) { int order; unsigned long tmp; for (order = 0, tmp = size; tmp >>= 1; ++order); //<== if (size & ~(1 << order)) ++order; return order; } Что-то не так с этой функцией: цикл без тела из-за точки с запятой в конце; форматирование кода показывает, что условие должно быть в теле цикла. С другой стороны, переменная 'tmp' всё равно нигде использоваться не будет.Может хотели так сделать:
int compute_order (unsigned long size) { int order; unsigned long tmp; for (order = 0, tmp = size; tmp >>= 1; ++order) if (tmp & ~(1 << order)) ++order; return order; } Но изменять счётчик цикла for(;;) в теле не очень хороший стиль.V535 The variable 'k' is being used for this loop and for the outer loop. Check lines: 3598, 3610. rules.c 3610
void solver_get_unneeded (Solver *solv, Queue *unneededq, int filtered) { … if (dep_possible (solv, *dp, &installedm)) { Queue iq; Id iqbuf[16]; queue_init_buffer (&iq, iqbuf, sizeof (iqbuf)/sizeof (*iqbuf)); dep_pkgcheck (solv, *dp, 0, &iq); for (k = 0; k < iq.count; k++) //<== { Id p = iq.elements[k]; Solvable *sp = pool->solvables + p; if (…) continue; for (j = 0; j < count; j++) if (p == unneededq->elements[j]) break; /* now add edge from j + 1 to i + 1 */ queue_insert (…); /* addapt following edge pointers */ for (k = j + 2; k < count + 2; k++) //<== edges.elements[k]++; } queue_free(&iq); } .... } Код так плохо отформатирован, что если тут есть ошибка, то она точно была допущена из-за этого. Плохой стиль использовать один счётчик во вложенных циклах for(;;).Аналогичное место:
V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 2319, 2349. solver.c 2349 V634 The priority of the '*' operation is higher than that of the ' void DCRaw::_WaveletDenoise () { … for (i = 0; i < (1 << dim * 2); i++) { //<== if (fimg[i] < -fThreshold) fimg[i] += fThreshold; else if (fimg[i] > fThreshold) fimg[i] -= fThreshold; else fimg[i] = 0; } … } Операция умножения имеет приоритет выше, чем у сдвига. Неизвестно, как здесь планировалось сделать, поэтому необходимо проверить и определить порядок операций с помощью круглых скобок для наглядности в бедующем.Похожее место:
V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. RAW.cpp 1099 V696 The 'continue' operator will terminate 'do {… } while (FALSE)' loop because the condition is always false. Check lines: 1939, 1945. Roster.cpp 1939 status_t BRoster::_LaunchApp(....) const { .... do { // find the app .... if (appType.InitCheck() == B_OK && appType.GetAppHint(&hintRef) == B_OK && appRef == hintRef) { appType.SetAppHint(NULL); // try again continue; } ... } while (false); .... } Оператор 'continue' в цикле «do {… } while(… )» выполняет переход к вычислению условия остановки цикла, а так оно всегда ложно — по сути это безусловный выход из цикла и комментарий «try again», получается, только вводит в заблуждение.V706 Suspicious division: sizeof (kBaudrates) / sizeof (char *). Size of every element in 'kBaudrates' array does not equal to divisor. SerialWindow.cpp 162
const int SerialWindow: kBaudrates[] = { 50, 75, 110, … };
SerialWindow: SerialWindow () : … { … for (int i = sizeof (kBaudrates) / sizeof (char*); --i >= 0;)//<== { message = new BMessage(kMsgSettings); message->AddInt32(«baudrate», kBaudrateConstants[i]);
char buffer[7]; sprintf (buffer,»%d», kBaudrates[i]); //<== BMenuItem* item = new BMenuItem(buffer, message);
fBaudrateMenu→AddItem (item); } … } Чтобы получить количество элементов в массиве 'kBaudrates', почему-то делят его размер на размер указателя, в итоге получится, что в 32-х битной системе будет индексироваться весь массив, а в 64-х битной системе — только половина.Массивы V548 Consider reviewing type casting. TYPE X[][] in not equivalent to TYPE **X. RAW.cpp 1668 void DCRaw::_AdobeCoefficients (const char *make, const char *model) { static const struct { const char *prefix; short black, trans[12]; } table[] = { { «Canon EOS D2000», 0, { 24542,-10860,-3401,-1490,11370,-297,2858,-605,3225 }}, { «Canon EOS D6000», 0, { 20482,-7172,-3125,-1033,10410,-285,2542,226,3136 }}, … }; double cameraXYZ[4][3];
for (uint32 i = 0; i < sizeof table / sizeof *table; i++) { if (!strncasecmp(model, table[i].prefix, strlen(....))) { if (table[i].black) fMeta.black = table[i].black; for (uint32 j = 0; j < 12; j++) { ((double**)cameraXYZ)[0][j] = table[i].trans[j] /10000.0; } _CameraXYZCoefficients(cameraXYZ); break; } } } Массив 'cameraXYZ', объявленный как «double cameraXYZ[4][3]», приводится к типу «double **». Это приведение типа, скорее всего, не имеет смысла и может повлечь ошибки.Типы «type[a][b]» и «type **» представляют собой разные структуры данных. Type[a][b] это единый участок памяти с которым можно работать как с двумерным массивом. Type ** — это массив указателей на какие-то участки памяти.
V554 Incorrect use of auto_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. DefaultCatalog.cpp 208
status_t
DefaultCatalog: ReadFromFile (const char *path)
{
…
auto_ptr
status_t
DefaultCatalog: ReadFromFile (const char *path)
{
…
unique_ptr
void floppy_dump_reg (floppy_t *flp) { … //uint8 result[10]; //<== This was correct! uint8 *result = flp->result; //<== Bad fix! :) .... dprintf(FLO "gap=%d wg=%d eis=%d fifo=%d poll=%d thresh=%d pretrk=%d\n", (result[7] & 0x02) >> 1, result[7] & 0×01, (result[8] & 0×40) >> 6, (result[8] & 0×20) >> 5, (result[8] & 0×10) >> 4, result[8] & 0×0f, result[9]); … } Два предупреждения анализатора указывают на выход за границу массива. Если судить по закомментированному коду, раньше массив 'result[]' состоял из 10 элементов, а после правки стал длиной 8 элементов. При этом в коде по-прежнему осуществляется доступ к десяти элементам массива с индексами от 0 до 9.Имена переменных V672 There is probably no need in creating the new 'path' variable here. One of the function’s arguments possesses the same name and this argument is a reference. Check lines: 348, 429. translate.cpp 429 status_t Translator: FindPath (const translation_format *format, BPositionIO &stream, TypeList &typesSeen, TypeList &path, …) { … TypeList path; double quality; if (FindPath (…) == B_OK) { if (bestQuality < quality * formatQuality) { bestQuality = quality * formatQuality; bestPath.SetTo(path); bestPath.Add(formats[j].type); status = B_OK; } } .... } Совпадение имени локальной переменной 'path' с параметром функции (тем более с ссылкой, как в данном случае) может приводить к потере локальных изменений в этой переменной и другим логических ошибкам.V711 It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop. ipv4.cpp 514
static int dump_ipv4_multicast (int argc, char** argv) { MulticastState: Iterator it = sMulticastState→GetIterator ();
while (it.HasNext ()) { … int count = 0; IPv4GroupInterface: AddressSet: Iterator it = state→Sources ().GetIterator (); while (it.HasNext ()) { … }
kprintf (»}> sock %p\n», state→Parent ()→Socket ()); }
return 0; } В теле цикла обнаружено объявление переменной 'it', совпадающей с переменной, используемой для контроля цикла. Такой код может содержать логические ошибки, вплоть до получения вечного цикла.Работа с памятью V597 The compiler could delete the 'memset' function call, which is used to flush 'password' buffer. The RtlSecureZeroMemory () function should be used to erase the private data. login.cpp 126 static status_t login (const char* user, struct passwd** _passwd) { … bool ok = verify_password (passwd, spwd, password); memset (password, 0, sizeof (password)); if (! ok) return B_PERMISSION_DENIED;
*_passwd = passwd; return B_OK; } К сожалению, приведенный код может оставить пароль неочищенным. Обратите внимание, что массив 'password' очищается в конце и более не используется. Поэтому при сборке Release версии программы, компилятор, скорее всего, удалит вызов функции memset (). На это компилятор имеет полное право. Анализатор предлагает использовать функцию для Windows, но для этой операционной системы следует найти другой способ избежать оптимизации компилятора.Ещё опасные места:
V597 The compiler could delete the 'memset' function call, which is used to flush 'finalcount' buffer. The RtlSecureZeroMemory () function should be used to erase the private data. sha1.c 228 V597 The compiler could delete the 'memset' function call, which is used to flush 'encoded_block' buffer. The RtlSecureZeroMemory () function should be used to erase the private data. dst_api.c 446 V597 The compiler could delete the 'memset' function call, which is used to flush 'in_buff' buffer. The RtlSecureZeroMemory () function should be used to erase the private data. dst_api.c 916 V597 The compiler could delete the 'memset' function call, which is used to flush 'repeatedPassword' buffer. The RtlSecureZeroMemory () function should be used to erase the private data. passwd.cpp 171 V630 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors. PDFWriter.cpp 117 status_t PDFWriter: PrintPage (int32 pageNumber, int32 pageCount) { … pictures = (BPicture **)malloc (pictureCount * sizeof (BPicture *)); picRects = (BRect *)malloc (pictureCount * sizeof (BRect)); //<== picPoints = (BPoint *)malloc(pictureCount * sizeof(BPoint)); //<== picRegion = new BRegion(); .... } При выделении памяти с помощью malloc под массив объектов какого-нибудь класса, не вызывается ни конструктор объекта при создании, ни деструктор при уничтожении. Подобный код может привести к работе с неинициализированными переменными и другим ошибкам.V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 623
#define MEMSET_BZERO (p, l) memset ((p), 0, (l))
void solv_SHA256_Final (sha2_byte digest[], SHA256_CTX* context) { … /* Clean up state data: */ MEMSET_BZERO (context, sizeof (context)); usedspace = 0; } Размер обнуляемой памяти равен не размеру структуры, а размеру указателя.Ещё такие места:
V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 644 V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 953 V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 973 V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 1028 V512 A call of the 'memset' function will lead to underflow of the buffer 'context'. sha2.c 1048 Прочее V591 Non-void function should return a value. pc.c 1031 ULONG set_var (char *name, ULONG val) { variable *v;
v = lookup_var (name); if (v!= NULL) v→value = val; else add_var (name, val); } Скорее всего, при вызове функции set_var (), возвращаемое значение никак не используется. Но если когда-нибудь им воспользуются, то получат неопределённое значение.V671 It is possible that the 'swap' function interchanges the 'std: declval < _Alloc & > ()' variable with itself. alloc_traits.h 191
static constexpr bool _S_nothrow_swap () { using std: swap; return!_S_propagate_on_swap () || noexcept ( swap (std: declval<_Alloc&>(), std: declval<_Alloc&>())); } Непонятное использование функции swap (): одинаковые аргументы.V519 The 'data→error' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 222, 223. repo_solv.c 223
static unsigned char * data_read_idarray (… , Repodata *data) { … data→error = pool_error (//<== data->repo→pool, SOLV_ERROR_ID_RANGE, «data_read_idarray: id too large (%u/%u)», x, max); data→error = SOLV_ERROR_ID_RANGE; //<== .... } Присваивание разных значений одной переменной подряд. Возможно, опечатка.V568 It's odd that the argument of sizeof() operator is the 'sizeof (struct tlv_header_t)' expression. print-slow.c 255
void slow_print (register const u_char *pptr, register u_int len) { … if (vflag > 1) print_unknown_data (tptr+sizeof (sizeof (struct tlv_header_t)), »\n\t », tlv_len-sizeof (struct tlv_header_t)); … } Аргументом оператора sizeof () тоже является sizeof (). Этот оператор вычисляет тип выражения и возвращает размер этого типа, но само выражение не вычисляется, т.е. размер структуры здесь ни на что не влияет.Таких мест много:
V568 It’s odd that the argument of sizeof () operator is the 'sizeof (struct lmp_object_header)' expression. print-lmp.c 872 V568 It’s odd that the argument of sizeof () operator is the 'sizeof (struct tlv_header_t)' expression. print-slow.c 182 V568 It’s odd that the argument of sizeof () operator is the 'sizeof (struct eigrp_tlv_header)' expression. print-eigrp.c 283 V568 It’s odd that the argument of sizeof () operator is the 'sizeof (struct eigrp_tlv_header)' expression. print-eigrp.c 471 Заключение Haiku — большой и необычный проект. Было интересно его проверить и сделать маленький вклад в развитие. Несмотря на мой опыт работы с open-source проектами, тут я встретил много редких предупреждений анализатора. В статьи вошли самые подозрительные, на мой взгляд, примеры исходного кода. Всё остальное, что не вошло в статью или было мною пропущено, авторы проекта смогут найти в полном логе проверки.Эта статья на английском Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analysis of Haiku Operating System (BeOS Family) by PVS-Studio. Part 2.
