Geekly Articles each Day
📜 ⬆️ ⬇️

Checking the Haiku operating system (BeOS family) with PVS-Studio. Part 2


This is the final article on validating the Haiku operating system. In the first article were collected possible errors of various types of diagnostics, but one way or another connected with the verification of conditions. This article will present the remaining analyzer warnings, which I would like to talk about. The collected examples are divided into several groups.

Haiku is a free operating system for personal computers that is aimed at binary compatibility with the BeOS operating system. Haiku embodies the basic ideas of BeOS. This is a modular system, architecturally designed as a hybrid core: a microkernel architecture that can dynamically load the necessary modules.

The project was tested at the request of the Haiku user community with the help of PVS-Studio 5.24 .
')


Working with strings


V527 It is odd that the '\ 0' value is assigned to the '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; }

Most likely, after working with a string, we wanted to write a terminal zero to the end of the string, rather than nullifying the pointer. The correct option is: "* scratchPtr = '\ 0';".

V692 An inappropriate attempt to make a string. To determine the correct length of the string st. 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 << ") "; } .... }

To write a terminal zero to the end of the line, the strlen () functions were used here, but the result of such an action is unpredictable, because for the operation of the strlen () function, the line should already end with a terminal zero. Zero will be written just in the cell where it was found 0. In this case, the function strlen () can go far beyond the buffer, which will lead to undefined program behavior. To fix the code, you need to calculate the length of the string in some other way.

Bad cycles


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

Something is wrong with this function: a loop without a body due to a semicolon at the end; formatting the code indicates that the condition must be in the body of the loop. On the other hand, the variable 'tmp' will not be used anywhere else.

Maybe they wanted to do this:
 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; }

But changing the for (;;) loop counter in the body is not a very good style.

V535 The variable 'k' is being used for the 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); } .... }

The code is so badly formatted that if there is an error, then it was definitely made because of this. Bad style to use one counter in nested loops for (;;).

Similar place:
V634 The operation of the operation is higher than that of the operation. It's possible that parentheses should not be used in the expression. RAW.cpp 1141
 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; } .... }

The multiplication operation takes precedence over shear. It is not known how it was planned to do here, so it is necessary to check and determine the order of operations using round brackets for clarity in the future.

Similar place:
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); .... }

The 'continue' operator in the “do {...} while (...)” loop goes to the calculation of the condition for stopping the loop, and it is always false — in fact, this is an unconditional exit from the loop and the “try again” comment, it turns out, is just misleading .

V706 Suspicious division: sizeof (kBaudrates) / sizeof (char *). Size of each element in the 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); } .... }

To get the number of elements in the array 'kBaudrates', for some reason, divide its size by the size of the pointer, in the end it turns out that in the 32-bit system the whole array will be indexed, and in the 64-bit system - only half.

Arrays


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

The array 'cameraXYZ', declared as “double cameraXYZ [4] [3]”, is converted to the type “double **”. This type conversion most likely does not make sense and may lead to errors.

The types “type [a] [b]” and “type **” are different data structures. Type [a] [b] is a single piece of memory with which you can work as a two-dimensional array. Type ** is an array of pointers to some areas of memory.

V554 Incorrect use of auto_ptr. The memory is allocated with 'new []' will be cleaned using 'delete'. DefaultCatalog.cpp 208
 status_t DefaultCatalog::ReadFromFile(const char *path) { .... auto_ptr<char> buf(new(std::nothrow) char [sz]); .... }

The analyzer detected a situation where the use of a smart pointer could lead to undefined behavior. The 'auto_ptr' class is not designed to work with arrays, it uses the 'delete' operator to free memory, and if you specify 'delete []', the code will not compile.

Correct code example:
 status_t DefaultCatalog::ReadFromFile(const char *path) { .... unique_ptr<char[]> buf(new(std::nothrow) char[sz]); .... }

Another place:
V557 Array overrun is possible. The '8' index is pointing beyond array bound. floppy_ctrl.c 637
V557 Array overrun is possible. The '9' index is pointing beyond array bound. floppy_ctrl.c 638
 typedef struct floppy { .... uint8 result[8]; /* status of the last finished command */ .... }; 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] & 0x01, (result[8] & 0x40) >> 6, (result[8] & 0x20) >> 5, (result[8] & 0x10) >> 4, result[8] & 0x0f, result[9]); .... }

Two analyzer warnings indicate an out of bounds array. Judging by the commented code, earlier the 'result []' array consisted of 10 elements, and after editing it became 8 elements long. In this case, the code still accesses ten elements of the array with indices from 0 to 9.

Variable names


V672 There is probably no need to create the new 'path' variable here. 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; } } .... }

The coincidence of the name of the local variable 'path' with the function parameter (especially with the link, as in this case) can lead to the loss of local changes in this variable and other logical errors.

It is a variable in 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; }

In the loop body, a declaration was found for the variable 'it', which coincides with the variable used to control the loop. Such a code may contain logical errors, up to the receipt of an eternal loop.

Work with memory


V597 The compiler couldn’t delete the memset function call, which is used to flush the password password. 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; }

Unfortunately, this code may leave the password uncleaned. Note that the 'password' array is cleared at the end and is no longer used. Therefore, when building the Release version of a program, the compiler will most likely remove the memset () function call. The compiler has every right to do this. The analyzer suggests using the function for Windows, but for this operating system you should find another way to avoid compiler optimization.

More dangerous places:
V630 The 'malloc' function is to allocate memory. 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(); .... }

When allocating memory with the help of malloc for an array of objects of some class, neither the object's constructor is called at creation, nor the destructor is destroyed. Such code can lead to working with uninitialized variables and other errors.

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

The size of the memory being reset is not equal to the size of the structure, but to the size of the pointer.

More places like this:

Other


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

Most likely, when calling the set_var () function, the return value is not used at all. But if they ever use it, they will get an undefined value.

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&>())); }

Incomprehensible use of the swap () function: the same arguments.

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; //<== .... }

Assigning different values ​​of one variable in a row. Perhaps a typo.

V568 It's odd that the 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)); .... }

The argument of the sizeof () operator is also sizeof (). This operator calculates the type of expression and returns the size of this type, but the expression itself is not calculated, i.e. the size of the structure does not affect anything here.

There are many such places:

Conclusion


Haiku is a big and unusual project. It was interesting to check it out and make a small contribution to the development. Despite my experience with open-source projects, here I met many rare warnings of the analyzer. The articles included the most suspicious, in my opinion, source code examples. Anything else that was not included in the article or was missed by me, the authors of the project will be able to find in the full verification log.

This article is in English.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Analysis of Haiku Operating System (BeOS Family) by PVS-Studio. Part 2 .

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio and CppCat, version 2015 . Please review the list.

Source: https://habr.com/ru/post/256525/

More articles:


All Articles