Geekly Articles each Day
📜 ⬆️ ⬇️

Best Copy-Paste Algorithms for C and C ++. Haiku OS Recipe Collection

Numerous typos and Copy-Paste code became the topic for an additional article on Haiku code verification by the PVS-Studio analyzer. However, there will be errors associated not so much with typos, but rather with carelessness and unsuccessful refactoring. The examples of errors found demonstrate how strong the human factor is in software development.

Picture 1

Introduction


Haiku is a free, open source operating system for personal computers. Currently, an international group of developers is actively working on the components of the system. Of the last significant developments in the development were porting LibreOffice to the operating system and the release of the first beta version of R1 Beta 1.

The team of developers of the PVS-Studio static code analyzer monitors the development of the project since 2015, releasing reviews of code defects. This is the fourth review of all time. You can get acquainted with previous articles at these links:
')
  1. Checking the Haiku operating system (BeOS family). Part 1 ;
  2. Checking the Haiku operating system (BeOS family). Part 2 ;
  3. How to shoot yourself in the foot in C and C ++. Haiku OS Recipe Collection

A feature of the latest code analysis is the ability to use the official version of PVS-Studio for Linux. In 2015, it was not there, as well as a convenient report for viewing errors. Now we will send the developers a full report in a convenient format.

Classics of the genre


V501 There are identical sub-expressions to the left and to the right of the '-' operator: (addr_t) b - (addr_t) b BitmapManager.cpp 51

int compare_app_pointer(const ServerApp* a, const ServerApp* b) { return (addr_t)b - (addr_t)b; }

Every programmer in his life must mix up the variables a and b , x and y , i and j ... etc.

V501 There are identical sub-expressions to the left and to the right of the '||' operator: input == __null || input == __null MediaClient.cpp 182

 status_t BMediaClient::Unbind(BMediaInput* input, BMediaOutput* output) { CALLED(); if (input == NULL || input == NULL) return B_ERROR; if (input->fOwner != this || output->fOwner != this) return B_ERROR; input->fBind = NULL; output->fBind = NULL; return B_OK; }

In the condition, the same input pointer is checked twice, and the output pointer remains unchecked, which can lead to dereference of the null pointer.

Corrected Code:

 if (input == NULL || output == NULL) return B_ERROR;

V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: 500000. usb_modeswitch.cpp 361

 static status_t my_transfer_data(....) { .... do { bigtime_t timeout = directionIn ? 500000 : 500000; result = acquire_sem_etc(device->notify, 1, B_RELATIVE_TIMEOUT, timeout); .... } while (result == B_INTERRUPTED); .... }

The ternary operator lost its meaning when the programmer made a mistake and wrote two identical return values ​​- 500000 .

V519 The 'm_kindex1' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 40, 41. agg_trans_double_path.cpp 41

 trans_double_path::trans_double_path() : m_kindex1(0.0), m_kindex2(0.0), m_base_length(0.0), m_base_height(1.0), m_status1(initial), m_status2(initial), m_preserve_x_scale(true) { } void trans_double_path::reset() { m_src_vertices1.remove_all(); m_src_vertices2.remove_all(); m_kindex1 = 0.0; m_kindex1 = 0.0; m_status1 = initial; m_status2 = initial; }

An error was made in the reset function: a typo in the index of the m_kindex2 variable. The value of this variable will not be reset, which is likely to affect the execution of other code fragments.

V501 There are identical sub-expressions to the left and to the right of the '>' operator: fg [order_type :: R]> fg [order_type :: R] agg_span_image_filter_rgba.h 898

 typedef Source source_type; typedef typename source_type::color_type color_type; typedef typename source_type::order_type order_type; void generate(color_type* span, int x, int y, unsigned len) { .... if(fg[0] < 0) fg[0] = 0; if(fg[1] < 0) fg[1] = 0; if(fg[2] < 0) fg[2] = 0; if(fg[3] < 0) fg[3] = 0; if(fg[order_type::A] > base_mask) fg[order_type::A] = base_mask; if(fg[order_type::R] > fg[order_type::R])fg[order_type::R] = fg[order_type::R]; if(fg[order_type::G] > fg[order_type::G])fg[order_type::G] = fg[order_type::G]; if(fg[order_type::B] > fg[order_type::B])fg[order_type::B] = fg[order_type::B]; .... }

In the last lines there is immediately a comparison of the same variables and assignment of the same variables. I can’t assume what was thought of here. Just mark the fragment as suspicious.

V570 The 'wPipeIndex' variable is assigned to itself. CEchoGals_transport.cpp 244

 ECHOSTATUS CEchoGals::CloseAudio (....) { .... wPipeIndex = wPipeIndex; m_ProcessId[ wPipeIndex ] = NULL; m_Pipes[ wPipeIndex ].wInterleave = 0; .... }

The wPipeIndex variable is initialized to its own value. Most likely, a typo was made.

Errors with pointers


V522 Dereferencing of the null pointer 'currentInterface' might take place. Device.cpp 258

 Device::Device(....) : .... { .... usb_interface_info* currentInterface = NULL; // <= uint32 descriptorStart = sizeof(usb_configuration_descriptor); while (descriptorStart < actualLength) { switch (configData[descriptorStart + 1]) { .... case USB_DESCRIPTOR_ENDPOINT: { .... if (currentInterface == NULL) // <= break; currentInterface->endpoint_count++; .... } .... case USB_DESCRIPTOR_ENDPOINT_COMPANION: { usb_endpoint_descriptor* desc = currentInterface // <= ->endpoint[currentInterface->endpoint_count - 1].descr; .... } .... }

The currentInterface pointer is initially initialized to zero and then checked when entering the switch statement branch, but not in all cases. The analyzer warns that when moving to the USB_DESCRIPTOR_ENDPOINT_COMPANION label, it is possible to dereference a null pointer.

V522 Dereferencing of the null pointer 'directory' might take place. PathMonitor.cpp 1465

 bool PathHandler::_EntryCreated(....) { .... Directory* directory = directoryNode->ToDirectory(); if (directory == NULL) { // We're out of sync with reality. if (!dryRun) { if (Entry* nodeEntry = directory->FirstNodeEntry()) { .... } } return false; } .... }

I believe that there was a mistake in comparing the directory pointer with a null value and the condition should be the opposite. In the current implementation, if the dryRun variable is false , then the directory null pointer will be dereferenced.

V522 Dereferencing of the null pointer 'input' might take place. MediaRecorder.cpp 343

 void GetInput(media_input* input); const media_input& BMediaRecorder::MediaInput() const { CALLED(); media_input* input = NULL; fNode->GetInput(input); return *input; }

The input pointer is initialized to zero and remains at that value, because in the GetInput function, the pointer does not change. Other methods of the BMediaRecorder class write differently, for example:

 status_t BMediaRecorder::_Connect(....) { .... // Find our Node's free input media_input ourInput; fNode->GetInput(&ourInput); // <= .... }

Everything is correct here, but it is impossible to write like this in the first code fragment, otherwise the function will return a link to the local object, but somehow the code needs to be fixed.

V522 Dereferencing of the null pointer 'mustFree' might take place. RequestUnflattener.cpp 35

 status_t Reader::Read(int32 size, void** buffer, bool* mustFree) { if (size < 0 || !buffer || mustFree) // <= return B_BAD_VALUE; if (size == 0) { *buffer = NULL; *mustFree = false; // <= return B_OK; } .... }

In the conditional expression, where all invalid input data is checked, a typo was made when checking the mustFree pointer. Most likely, the exit from the function should be at zero value of this pointer:

 if (size < 0 || !buffer || !mustFree) // <= return B_BAD_VALUE;

V757 It is possible that an incorrect variable is compared with nullptr after type conversion using 'dynamic_cast'. Check lines: 474, 476. recover.cpp 474

 void checkStructure(Disk &disk) { .... Inode* missing = gMissing.Get(run); dir = dynamic_cast<Directory *>(missing); if (missing == NULL) { .... } .... }

Instead of the missing pointer, you should check the dir pointer after type conversion. By the way, C # programmers often make a similar mistake. This once again proves that some errors are independent of the language used.

A couple more similar places in the code:


Index Errors


V557 Array overrun is possible. The 'BT_SCO' index is pointing beyond array bound. h2upper.cpp 75

 struct bt_usb_dev { .... struct list nbuffersTx[(1 + 1 + 0 + 0)]; // <= [0..1] .... } typedef enum { BT_COMMAND = 0, BT_EVENT, BT_ACL, BT_SCO, // <= 3 BT_ESCO, HCI_NUM_PACKET_TYPES } bt_packet_t; void sched_tx_processing(bt_usb_dev* bdev) { .... if (!list_is_empty(&bdev->nbuffersTx[BT_SCO])) { // <= fail // TODO to be implemented } .... }

The bdev-> nbuffersTx array consists of only two elements, and in the code it is accessed by the constant BT_SCO, which has a value of 3. A guaranteed exit beyond the boundaries of the array occurs.

V557 Array overrun is possible. The 'ieee80211_send_setup' function processes value '16'. Inspect the fourth argument. Check lines: 842, 911. ieee80211_output.c 842

 struct ieee80211_node { .... struct ieee80211_tx_ampdu ni_tx_ampdu[16]; // <= [0..15] .... }; #define IEEE80211_NONQOS_TID 16 int ieee80211_mgmt_output(....) { .... ieee80211_send_setup(ni, m, IEEE80211_FC0_TYPE_MGT | type, IEEE80211_NONQOS_TID, // <= 16 vap->iv_myaddr, ni->ni_macaddr, ni->ni_bssid); .... } void ieee80211_send_setup( struct ieee80211_node *ni, struct mbuf *m, int type, int tid, // <= 16 ....) { .... tap = &ni->ni_tx_ampdu[tid]; // <= 16 .... }

Another way out of the array. In this case, just one item. Interprocedural analysis helped to identify a situation where the array ni-> ni_tx_ampdu , consisting of 16 elements, was accessed at index 16 . In C and C ++, array indexing is done from scratch.

V781 The value of the 'vector' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 802, 805. oce_if.c 802

 #define OCE_MAX_EQ 32 typedef struct oce_softc { .... OCE_INTR_INFO intrs[OCE_MAX_EQ]; .... } OCE_SOFTC, *POCE_SOFTC; static int oce_alloc_intr(POCE_SOFTC sc, int vector, void (*isr) (void *arg, int pending)) { POCE_INTR_INFO ii = &sc->intrs[vector]; int rc = 0, rr; if (vector >= OCE_MAX_EQ) return (EINVAL); .... }

The analyzer detected access to the sc-> intrs array by an invalid index (going beyond the bounds of the array). The reason for this is the wrong code order. Here, the array is first accessed, and then a check is made to see if the index value is valid.

Someone may say that there will be no trouble. Here, after all, the value of the array element is not retrieved, but the cell address is simply taken. No, you can’t do it anyway Read more: " Dereferencing a null pointer leads to undefined behavior ."

V519 The variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 199, 200. nvme_ctrlr.c 200

 static void nvme_ctrlr_set_intel_supported_features(struct nvme_ctrlr *ctrlr) { bool *supported_feature = ctrlr->feature_supported; supported_feature[NVME_INTEL_FEAT_MAX_LBA] = true; supported_feature[NVME_INTEL_FEAT_MAX_LBA] = true; supported_feature[NVME_INTEL_FEAT_NATIVE_MAX_LBA] = true; supported_feature[NVME_INTEL_FEAT_POWER_GOVERNOR_SETTING] = true; supported_feature[NVME_INTEL_FEAT_SMBUS_ADDRESS] = true; supported_feature[NVME_INTEL_FEAT_LED_PATTERN] = true; supported_feature[NVME_INTEL_FEAT_RESET_TIMED_WORKLOAD_COUNTERS] = true; supported_feature[NVME_INTEL_FEAT_LATENCY_TRACKING] = true; }

The array element with the index NVME_INTEL_FEAT_MAX_LBA is assigned the same value. Fortunately, all possible constants are represented in this function and the code is simply the result of Copy-Paste programming. But the probability of making mistakes in this way is very high.

V519 The 'copiedPath [len]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 92, 93. kernel_emu.cpp 93

 int UserlandFS::KernelEmu::new_path(const char *path, char **copy) { .... // append a dot, if desired if (appendDot) { copiedPath[len] = '.'; copiedPath[len] = '\0'; } .... }

And here the developer was not lucky with copying. The symbol "dot" is added to a certain line and is immediately overwritten by a terminal zero. Most likely, the author of the code copied the string and forgot to increment the index.

Strange conditions


V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1407, 1410. FindPanel.cpp 1407

 void FindPanel::BuildAttrQuery(BQuery* query, bool &dynamicDate) const { .... case B_BOOL_TYPE: { uint32 value; if (strcasecmp(textControl->Text(), "true") == 0) { value = 1; } else if (strcasecmp(textControl->Text(), "true") == 0) { value = 1; } else value = (uint32)atoi(textControl->Text()); value %= 2; query->PushUInt32(value); break; } .... }

Copying the code immediately led to two errors. Conditional expressions are identical. Most likely, in one of them there should be a comparison with the string “false”, instead of “true”. Next, in the branch that processes the value false, you should change the value value from 1 to 0 . The algorithm assumes that any other values ​​other than true or false will be converted to a number using the atoi function, but due to an error, the text will also get the text “false”.

V547 Expression 'error == ((int) 0)' is always true. Directory.cpp 688

 int32 BDirectory::CountEntries() { status_t error = Rewind(); if (error != B_OK) return error; int32 count = 0; BPrivate::Storage::LongDirEntry entry; while (error == B_OK) { if (GetNextDirents(&entry, sizeof(entry), 1) != 1) break; if (strcmp(entry.d_name, ".") != 0 && strcmp(entry.d_name, "..") != 0) count++; } Rewind(); return (error == B_OK ? count : error); }

The analyzer detected that the value of the error variable will always be B_OK . Most likely, a modification of this variable was missed in the while loop .

V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. strtod.c 545

 static int lo0bits(ULong *y) { int k; ULong x = *y; .... if (!(x & 1)) { k++; x >>= 1; if (!x & 1) // <= return 32; } *y = x; return k; }

Most likely, in the last conditional expression they forgot to place brackets, as in the conditions above. Probably, there too, the negation operator should be outside the brackets:

 if (!(x & 1)) // <= return 32;

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. PoseView.cpp 5851

 bool BPoseView::AttributeChanged(const BMessage* message) { .... result = poseModel->OpenNode(); if (result == B_OK || result != B_BUSY) break; .... }

This is not obvious, but the result of the condition does not depend on the value of B_OK. Thus, it can be simplified:

 If (result != B_BUSY) break;

This is easily verified by building a truth table for the values ​​of the result variable. If you need to specifically consider other values ​​other than B_OK and B_BUSY , then the code should be rewritten differently.

Two more similar conditions:


V590 Consider inspecting the 'argc == 0 || argc! = 2 'expression. The expression is excessive or contains a misprint. cmds.c 2667

 void unsetoption(int argc, char *argv[]) { .... if (argc == 0 || argc != 2) { fprintf(ttyout, "usage: %s option\n", argv[0]); return; } .... }

Perhaps this is the simplest example that demonstrates the operation of the V590 diagnostic. It is necessary to display the description of the program if there are no arguments passed, or there are not two of them. Obviously, any values ​​other than two, including zero, will not satisfy the condition. Therefore, the condition can be easily simplified to the following:

 if (argc != 2) { fprintf(ttyout, "usage: %s option\n", argv[0]); return; }

V590 Consider inspecting the '* ptr =='; ' && * ptr! = '\ 0' 'expression. The expression is excessive or contains a misprint. pc.c 316

 ULONG parse_expression(char *str) { .... ptr = skipwhite(ptr); while (*ptr == SEMI_COLON && *ptr != '\0') { ptr++; if (*ptr == '\0') continue; val = assignment_expr(&ptr); } .... }

In this example, the logical operator has changed, but the logic has remained the same. Here the condition of the while loop depends only on whether the character is equal to the value of SEMI_COLON or not.

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. writembr.cpp 99

 int main(int argc, char** argv) { .... string choice; getline(cin, choice, '\n'); if (choice == "no" || choice == "" || choice != "yes") { cerr << "MBR was NOT written" << endl; fs.close(); return B_ERROR; } .... }

There are already three conditions in this example. It can also be simplified to check if the user has selected “yes” or not:

 if (choice != "yes") { cerr << "MBR was NOT written" << endl; fs.close(); return B_ERROR; }

Miscellaneous errors


V530 The return value of function 'begin' is required to be utilized. IMAPFolder.cpp 414

 void IMAPFolder::RegisterPendingBodies(...., const BMessenger* replyTo) { .... IMAP::MessageUIDList::const_iterator iterator = uids.begin(); for (; iterator != uids.end(); iterator++) { if (replyTo != NULL) fPendingBodies[*iterator].push_back(*replyTo); else fPendingBodies[*iterator].begin(); // <= } }

The analyzer detected a meaningless call to the begin () iterator . I can’t guess how to fix the code. The developer should pay attention to this place.

V609 Divide by zero. Denominator range [0..64]. UiUtils.cpp 544

 static int32 GetSIMDFormatByteSize(uint32 format) { switch (format) { case SIMD_RENDER_FORMAT_INT8: return sizeof(char); case SIMD_RENDER_FORMAT_INT16: return sizeof(int16); case SIMD_RENDER_FORMAT_INT32: return sizeof(int32); case SIMD_RENDER_FORMAT_INT64: return sizeof(int64); case SIMD_RENDER_FORMAT_FLOAT: return sizeof(float); case SIMD_RENDER_FORMAT_DOUBLE: return sizeof(double); } return 0; } const BString& UiUtils::FormatSIMDValue(const BVariant& value, uint32 bitSize, uint32 format, BString& _output) { _output.SetTo("{"); char* data = (char*)value.ToPointer(); uint32 count = bitSize / (GetSIMDFormatByteSize(format) * 8); // <= .... }

The GetSIMDFormatByteSize function actually returns 0 as the default value, which could potentially lead to division by zero.

V654 The condition 'specificSequence! = Sequence' of loop is always false. pthread_key.cpp 55

 static void* get_key_value(pthread_thread* thread, uint32 key, int32 sequence) { pthread_key_data& keyData = thread->specific[key]; int32 specificSequence; void* value; do { specificSequence = keyData.sequence; if (specificSequence != sequence) return NULL; value = keyData.value; } while (specificSequence != sequence); keyData.value = NULL; return value; }

The analyzer is right that the condition of the while statement is always false. Because of this, no more than one iteration is performed in a loop. In other words, nothing would change if you write while (0) . All this is very strange and this code contains some kind of error in the logic of work. Developers should pay attention to this place.

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(...., TypeList &path, double &pathQuality) { .... TypeList path; double quality; if (FindPath(&formats[j], stream, typesSeen, path, quality) == B_OK) { if (bestQuality < quality * formatQuality) { bestQuality = quality * formatQuality; bestPath.SetTo(path); bestPath.Add(formats[j].type); status = B_OK; } } .... }

The path variable is passed to the FindPath function by reference. This means that it is possible to modify this variable in the body of the function. But here there is a local variable of the same name that is being modified. In this case, all changes will remain only in the local variable. Perhaps you should rename or delete the local variable.

V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. HostnameView.cpp 109

 status_t HostnameView::_LoadHostname() { BString fHostnameString; char hostname[MAXHOSTNAMELEN]; if (gethostname(hostname, MAXHOSTNAMELEN) == 0) { fHostnameString.SetTo(hostname, MAXHOSTNAMELEN); fHostname->SetText(fHostnameString); return B_OK; } else return B_ERROR; }

An example of poor code design. The “frozen” else keyword does not change the logic yet, but this is until the first piece of code inserted before the return statement .

V763 Parameter 'menu' is always rewritten in function body before being used. video.cpp 648

 bool video_mode_hook(Menu *menu, MenuItem *item) { video_mode *mode = NULL; menu = item->Submenu(); item = menu->FindMarked(); .... }

There were many cases where function arguments are overwritten immediately at the entrance to the function. This behavior is misleading to other developers who invoke these same features.

The whole list of suspicious places:


Conclusion


The Haiku project is a source of interesting and rare bugs. We replenished our database of error examples and fixed several problems in the analyzer identified during code analysis.

If you haven’t checked your code with any code analysis tools for a long time, then something of the described is probably in your code. Use PVS-Studio in your project to control the quality of the code if it is written in C, C ++, C # or Java. You can download the analyzer without registration and SMS here .

Want to try Haiku and have any questions? Haiku developers invite you to the telegram channel .



If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Best Copy-Paste Algorithms for C and C ++. Haiku OS Cookbook

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

More articles:


All Articles