Мы используем куки, чтобы пользоваться сайтом
было удобно.
Многочисленные опечатки и Copy-Paste код стали темой для дополнительной статьи о проверке кода Haiku анализатором PVS-Studio. Впрочем, будут ошибки, связанные не сколько с опечатками, а скорее с невнимательностью и неудачным рефакторингом. Найденные примеры ошибок демонстрируют, насколько силён человеческий фактор в разработке программного обеспечения.
Haiku - свободная операционная система с открытым исходным кодом для персональных компьютеров. В настоящее время международная группа разработчиков активно трудится над компонентами системы. Из последних значимых событий в разработке были портирование LibreOffice в операционную систему и выпуск первой бета-версии R1 Beta 1.
Команда разработчиков статического анализатора кода PVS-Studio следит за развитием проекта с 2015 года, выпуская обзоры дефектов кода. Это четвертый обзор за всё время. С предыдущими статьями вы можете познакомиться по этим ссылкам:
Особенностью последнего анализа кода является возможность использовать официальную версию PVS-Studio для Linux. В 2015 её не было, как и удобного отчёта для просмотра ошибок. Сейчас мы отправим разработчикам полный отчёт в удобном формате.
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;
}Каждый программист в своей жизни должен перепутать переменные a и b, x и y, i и j... и т.д.
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;
}В условии два раза проверяют один и тот же указатель input, а указатель output остался непроверенным, что может привести к разыменованию нулевого указателя.
Исправленный код:
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);
....
}Тернарный оператор потерял свой смысл, когда программист допустил ошибку и написал два одинаковых возвращаемых значения - 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;
}В функции reset допущена ошибка: опечатка в индексе переменной m_kindex2. Значение этой переменной не обнулится, что, вероятно, повлияет на выполнение других фрагментов кода.
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];
....
}В последних строках тут сразу сравнение одинаковых переменных и присваивание одинаковых переменных. Не могу предположить, что тут задумывалось. Просто отмечу фрагмент как подозрительный.
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;
....
}Переменная wPipeIndex инициализируется собственным значением. Скорее всего, была допущена опечатка.
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;
....
}
....
}Указатель currentInterface изначально инициализируется нулём и в дальнейшем проверяется при входе в ветви оператора switch, но не во всех случаях. Анализатор предупреждает, что при переходе к метке USB_DESCRIPTOR_ENDPOINT_COMPANION возможно разыменование нулевого указателя.
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;
}
....
}Я полагаю, что в условии сравнения указателя directory с нулевым значением допущена ошибка, и условие должно быть противоположным. При текущей реализации, если переменная dryRun будет иметь значение false, то произойдёт разыменование нулевого указателя directory.
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;
}Указатель input инициализируется нулём и с таким значением и останется, т.к. в функции GetInput указатель не меняется. В других методах класса BMediaRecorder пишут иначе, например:
status_t
BMediaRecorder::_Connect(....)
{
....
// Find our Node's free input
media_input ourInput;
fNode->GetInput(&ourInput); // <=
....
}Здесь всё корректно, но в первом фрагменте кода нельзя так написать, иначе функция будет возвращать ссылку на локальный объект, но код исправить как-то нужно.
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;
}
....
}В условном выражении, где проверяются все некорректные входные данные, допустили опечатку при проверке указателя mustFree. Скорее всего, выход из функции должен быть при нулевом значении этого указателя:
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) {
....
}
....
}Вместо указателя missing следовало проверять указатель dir после преобразования типа. Кстати, подобную ошибку часто делают и программисты на языке C#. Это в очередной раз доказывает, что некоторые ошибки не зависят от используемого языка.
Ещё пара похожих мест в коде:
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
}
....
}Массив bdev->nbuffersTx состоит всего из двух элементов, при этом в коде к нему обращаются по константе BT_SCO, которая имеет значение 3. Происходит гарантированный выход за границы массива.
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
....
}Ещё один выход за пределы массива. В этом случае всего на один элемент. Межпроцедурный анализ помог выявить ситуацию, когда к массиву ni->ni_tx_ampdu, состоящему из 16 элементов, обратились по индексу 16. В языках C и C++ индексация массивов выполняется с нуля.
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);
....
}Анализатор обнаружил обращение к массиву sc->intrs по невалидному индексу (выходящему за границы массива). Причина этого - неправильный порядок кода. Здесь выполняется сначала обращение к массиву, а потом проверка, допустимо ли значение индекса.
Кто-то может сказать, что беды не будет. Здесь ведь не извлекается значение элемента массива, а просто берётся адрес ячейки. Нет, так всё равно делать нельзя. Подробнее: "Разыменовывание нулевого указателя приводит к неопределённому поведению".
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;
}Элементу массива с индексом NVME_INTEL_FEAT_MAX_LBA присваивают одно и то же значение. К счастью, в этой функции представлены все возможные константы и код является просто результатом Copy-Paste программирования. Но вероятность допущения ошибок таким образом очень высокая.
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';
}
....
}А тут разработчику не повезло с копированием. К некой строке добавляется символ "точка" и тут же перезаписывается терминальным нулём. Скорее всего, автор кода скопировал строку и забыл сделать инкремент индексу.
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;
}
....
}Копирование кода привело сразу к двум ошибкам. Условные выражения идентичны. Скорее всего, в одном из них должно быть сравнение со строкой "false", вместо "true". Далее, в ветви, которая обрабатывает значение "false", следует изменить значение value с 1 на 0. Алгоритм предполагает, что любые другие значения, отличные от true или false, будут преобразованы в число с помощью функции atoi, но из-за ошибки в функцию будет попадать и текст "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);
}Анализатор обнаружил, что значение переменной error будет всегда B_OK. Скорее всего, в цикле while пропустили модификацию этой переменной.
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;
}Скорее всего, в последнем условном выражении забыли расставить скобочки, как в условиях выше. Вероятно, там тоже оператор отрицания должен быть за скобками:
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;
....
}Это не очевидно, но результат условия не зависит от значения B_OK. Таким образом, его можно упростить:
If (result != B_BUSY)
break;Это легко проверить, построив таблицу истинности для значений переменной result. Если требовалось специально рассмотреть другие значения, отличные от B_OK и B_BUSY, то код следует переписать иначе.
Ещё два похожих условия:
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;
}
....
}Пожалуй, это самый простой пример, который демонстрирует работу диагностики V590. Вывести на экран описание программы необходимо в случае, если нет переданных аргументов, или их не два. Очевидно, что любые значения, отличные от двух, включая ноль, не будут удовлетворять условию. Поэтому условие смело можно упростить до такого:
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);
}
....
}В этом примере сменился логический оператор, но логика осталась прежней. Здесь условие цикла while зависит только от того, равен символ значению SEMI_COLON или нет.
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;
}
....
}В этом примере уже три условия. Его также можно упростить до проверки, выбрал пользователь "yes" или нет:
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(); // <=
}
}Анализатор обнаружил бессмысленный вызов итератора begin(). Не могу предположить, как исправить код. Разработчиком следует обратить внимание на это место.
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); // <=
....
}Функция GetSIMDFormatByteSize действительно возвращает 0 в качестве дефолтного значения, что потенциально может привести к делению на ноль.
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;
}Анализатор прав, что условие оператора while всегда ложно. Из-за этого в цикле выполняется не больше одной итерации. Другими словами, ничего бы не изменилось, если написать while(0). Всё это очень странно и этот код содержит какую-то ошибку в логике работы. Разработчикам следует обратить на это место внимание.
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;
}
}
....
}Переменная path передаётся в функцию FindPath по ссылке. Это означает, что возможна модификация этой переменной в теле функции. Но здесь присутствует одноимённая локальная переменная, которая модифицируется. В этом случае все изменения останутся только в локальной переменной. Возможно, следует переименовать или удалить локальную переменную.
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;
}Пример плохого оформления кода. "Зависшее" ключевое слово else пока не изменяет логику, но это до первого вставленного фрагмента кода перед оператором return.
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();
....
}Нашлось очень много случаев, когда аргументы функции перезаписываются сразу на входе в функцию. Такое поведение вводит в заблуждение других разработчиков, которые вызывают эти самые функции.
Весь список подозрительных мест:
Проект Haiku - источник интересных и редких ошибок. Мы пополнили нашу базу примеров ошибок и исправили несколько проблем в анализаторе, выявленных при анализе кода.
Если вы давно не проверяли свой код какими-нибудь инструментами анализа кода, то что-нибудь из описанного наверняка есть и в вашем коде. Используйте PVS-Studio в своём проекте для контроля качества кода, если он написан на языках C, C++, C# или Java. Скачать анализатор без регистрации и смс можно здесь.
Хотите попробовать Haiku и у вас возникли вопросы? Разработчики Haiku приглашают вас в telegram-канал.
0
0
0
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
0