Numerous typos and Copy-Paste code became the main topic of the additional article about checking the Haiku code by the PVS-Studio analyzer. Yet this article mostly tells about errors related to thoughtlessness and failed refactoring, rather than to typos. The errors found demonstrate how strong the human factor is in software development.
Haiku is a free open source operating system for personal computers. An international development team is currently working on the components of the system. Porting LibreOffice in the OS and the first R1 Beta 1 release stand out among the recent significant development improvements.
Team of developers from PVS-Studio follows this project development since 2015 and posts reviews of code defects. This is the fourth review of all time. You can read the previous articles by these links:
The feature of the last code analysis is the ability to use the official version of PVS-Studio for Linux. Neither PVS-Studio for Linux, nor a convenient report to view errors was available in 2015. This time we'll send the full report in a convenient format to Haiku developers.
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 developer has to mix up variables a and b, x and y, i and j ... at least once in his life.
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;
}The same input pointer is checked in the condition twice. Whereas the output pointer remained unchecked, which can result in the null pointer dereference.
Fixed 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 became pointless, when the code author 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;
}There is an error in the reset function: a typo in the m_kindex2 variable index. This variable won't be reset, which will probably 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 are two issues at once: comparison and assignment of equal variables. I even cannot suggest what the author's brainchild was. I'll just note this snippet 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 by its own value. Most likely, a typo was made.
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 by null and then checked when entering in the branches of the switch operator, but not in all cases. The analyzer warns that when jumping to the USB_DESCRIPTOR_ENDPOINT_COMPANION case label, null pointer dereference might occur.
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 think, there is an error in the comparison condition of the directory pointer with the null value; the condition has to be the opposite. With the current implementation, if the dryRun variable is false, 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 by null and stays with such value, as the pointer isn't changing in the GetInput function. In other methods of the BMediaRecorder class, the implementation is different, for instance:
status_t
BMediaRecorder::_Connect(....)
{
....
// Find our Node's free input
media_input ourInput;
fNode->GetInput(&ourInput); // <=
....
}It's all correct here, but the first fragment has to be rewritten, otherwise the function will return a reference to a local object.
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 incorrect data is checked, the author made a typo when checking the mustFree pointer. Most likely, the function should exit when having the null 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) {
....
}
....
}The developer should have checked the dir pointer instead of missing after type conversion. By the way, C# developers also often make a similar mistake. This proves once again that some errors don't depend on the language used.
A couple more similar places in the code:
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 only of 2 elements, but it's addressed by the BT_SCO constant, which is 3. Here comes the surefire array index out of bounds.
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 array index out of bounds. This time, just by one element. Interprocedural analysis helped to reveal the case when the ni->ni_tx_ampdu array, consisting of 16 elements was addressed by the index 16. In C and C++ arrays are indexed from zero.
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 has detected that an element of the sc->intrs array was addressed by an invalid index, which was out of bounds. The reason is the incorrect order of operations in the code. First, the element is addressed and then comes the check if the index value is valid.
Some might say there won't be any trouble. It doesn't remove the value of the array element, it just takes the address of the cell. But no, that's not the way to do things. Read more: "Null Pointer Dereferencing Causes 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 NVME_INTEL_FEAT_MAX_LBA index is assigned the same value. The good news is this function presents all possible constants which makes this code just the result of the Copy-Paste programming. But chances are errors will sneak in here.
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';
}
....
}Well, here the programmer got bad luck with copying. The symbol "dot" is added to a line and gets rewritten with a terminal null. It is highly likely, the author just copied the line and forgot to increment the index.
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 led to two errors at once. The conditional expressions are identical. Most likely, a comparison with the "false" string instead of "true" has to be in one of them. Further in the branch that handles the "false" value, the value that should be changed from 1 to 0. The algorithm requires that any other values, different from true or false be converted in a number using the atoi function. But due to an error, the text "false" will get into the function.
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 error variable value will always be B_OK. Most definitely, this variable modification 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;
}It's most likely that in the last conditional expression one forgot to place brackets, as in the conditions above. The complementary operator is likely to 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 doesn't depend on the value of B_OK value. So it can be simplified:
If (result != B_BUSY)
break;You can easily check it by drawing a truth table for the values of the result variable. If one wanted to specifically consider other values, different from B_OK and B_BUSY, the code should be rewritten in another way.
Two more similar fragments:
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;
}
....
}This is perhaps the simplest example that demonstrates the work of the V590 diagnostic. You need to display the program description in case if there are no passed arguments, 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 safely simplified to this:
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 was changed, but the logic is still the same. Here the condition of the while loop depends only on whether the character is equal to 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 before checking whether the user has chosen "yes" or not:
if (choice != "yes") {
cerr << "MBR was NOT written" << endl;
fs.close();
return B_ERROR;
}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 found a pointless call of the iterator begin(). I can't imagine how to fix the code. Developers should pay attention to this code.
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 function GetSIMDFormatByteSize truly returns 0 as a default value, which might 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 operator is always false. Due to this, the loop doesn't run more than one iteration. In other words, nothing would change if you wrote while(0). All this is weird and this code contains a logic error. Developers should carefully consider this snippet.
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. Which means, this variable can be modified in the body of the function. But there is a local variable with the same name, which is modified. In this case, all changes will remain only in the local variable. The code author might want to rename or remove 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;
}The example of poor code formatting. The "hanging" keyword else doesn't change the logic yet, but once a code fragment is inserted before the return operator, the logic won't be the same.
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();
....
}I found many cases when function arguments are rewritten when entering the function. This behavior misleads other developers who call these very functions.
The entire list of suspicious places:
The Haiku project is a source of interesting and rare errors. We've added to our database some error examples and fixed a few analyzer issues that showed up when analyzing the code.
If you haven't checked your code with some code analysis tools for a long time, then some of the issues I described are probably hiding in your code. Use PVS-Studio in your project (if written in C, C++, C# or Java) to control code quality. Download the analyzer here without registration or sms.
Do you want to try Haiku and you have questions? Haiku developers invite you to the telegram-channel.