- PVSM.RU - https://www.pvsm.ru -
Операционные системы являются одними из самых сложных и крупных проектов в мире программного обеспечения, а значит идеально подходят для демонстрации применения методики статического анализа кода. После проверки Linux Kernel, я вдохновился проанализировать и другие открытые операционные системы.
Haiku [1] — свободная операционная система для персональных компьютеров, которая нацелена на двоичную совместимость с операционной системой BeOS. Haiku воплощает в себе основные идеи BeOS. Это модульная система, архитектурно решённая как гибридное ядро: микроядерная архитектура, способная динамически подгружать необходимые модули.
Проект для проверки был предложен пользователем, знакомым с продуктом PVS-Studio [2] и нашей работе по проверке open-source проектов. После сравнительно недавней проверки Linux Kernel [3], я догадывался, с какими проблемами мне придётся столкнуться и описал их в ответном письме. Неожиданно мне предложили содействие в сборке операционной системы и интеграции анализатора. Дополнительно на официальном сайте была доступна очень обширная документация и я решил попробовать.
Через некоторое время я получил долгожданный лог проверки анализатором и после анализа результатов, я решил написать две статьи, описав самые подозрительные на мой взгляд участки кода. Это первая часть.
В первую статью я вынес предупреждения анализатора на условные операторы. Ведь ошибку в условии можно трактовать как ошибку в логике выполнения программы.
V501 [4] There are identical sub-expressions to the left and to the right of the '<' operator: lJack->m_jackType < lJack->m_jackType MediaJack.cpp 783
int __CORTEX_NAMESPACE__ compareTypeAndID(....)
{
int retValue = 0;
....
if (lJack && rJack)
{
if (lJack->m_jackType < lJack->m_jackType) //<==
{
return -1;
}
if (lJack->m_jackType == lJack->m_jackType) //<==
{
if (lJack->m_index < rJack->m_index)
{
return -1;
}
else
{
return 1;
}
}
else if (lJack->m_jackType > rJack->m_jackType)
{
retValue = 1;
}
}
return retValue;
}
На эту функцию анализатор выдал целых два предупреждения. В обоих случаях хорошо заметна опечатка (когда уже анализатор «тыкнул пальцем», конечно) в именах lJack и rjack.
V575 [5] The 'strchr' function processes value '2112800'. Inspect the second argument. CommandActuators.cpp 1517
extern char *strchr(const char *string, int character);
SendMessageCommandActuator::
SendMessageCommandActuator(int32 argc, char** argv)
:
CommandActuator(argc, argv),
fSignature((argc > 1) ? argv[1] : "")
{
....
const char* arg = argv[i];
BString argString(arg);
const char* equals = strchr(arg, ' = '); //<==
....
}
Функция strchr() возвращает указатель на первое вхождение указанного символа в указанной строке. Символ преобразуется в int, в данном случае ' = ' будет представлен как число 2112800. Скорее всего хотели искать одиночный символ '=', а его код будет 61.
Если хотели найти подстроку " = ", то явно используется не та функция и её следует заменить на вызов strstr().
V502 [6] Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. AbstractLayout.cpp 244
bool IsVisible(bool ancestorsVisible) const
{
int16 showLevel = BView::Private(view).ShowLevel();
return (showLevel - (ancestorsVisible) ? 0 : 1) <= 0;
}
К сожалению, в данном случае взятие в скобочки переменной 'ancestorsVisible' никак не влияет на порядок вычислений в этом выражении. Поэтому первой по приоритету будет выполняться операция вычитания (из int16 вычитается bool), только потом выполняется тернарный оператор '?:'.
Правильный код:
bool IsVisible(bool ancestorsVisible) const
{
int16 showLevel = BView::Private(view).ShowLevel();
return (showLevel - (ancestorsVisible ? 0 : 1) ) <= 0;
}
V502 [6] Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '&&' operator. fnmatch.c 58
#define FOLD(c)
((flags & FNM_CASEFOLD) && ISUPPER ((unsigned char) (c))
? tolower ((unsigned char) (c))
: (c))
Также я советую авторам проверить порядок выполнения операций в этом макросе и расставить скобки для наглядности.
V562 [7] It's odd to compare 0 or 1 with a value of 0. cmp.c 300
#ifndef same_file
# define same_file(s, t)
((((s)->st_ino == (t)->st_ino)
&& ((s)->st_dev == (t)->st_dev))
|| same_special_file (s, t))
#endif
int
main (int argc, char **argv)
{
....
if (0 < same_file (&stat_buf[0], &stat_buf[1]) //<==
&& same_file_attributes (&stat_buf[0], &stat_buf[1])
&& file_position (0) == file_position (1))
return EXIT_SUCCESS;
....
}
На первый взгляд обычное условие, но «same_file» является макросом, преобразующимся в логическое выражение, как и 'same_file_attributes', в итоге получили странное сравнение «0 < value_of_boolean_type». В языке Си нет типа 'bool'. Выражение справа будет иметь тип 'int', но по своей сути она всегда «истина» или «лож», поэтому мы и назвали его 'boolean'. И сравнение «0 < boolean_expr» весьма подозрительно.
Аналогичное использование макроса:
V562 [7] It's odd to compare a bool type value with a value of 18: 0x12 == IsProfessionalSpdif(). CEchoGals_mixer.cpp 533
#define ECHOSTATUS_DSP_DEAD 0x12 //<==
virtual BOOL IsProfessionalSpdif() //<==
{
....
return( (....) ? TRUE : FALSE );
}
ECHOSTATUS CEchoGals::ProcessMixerFunction
(
PMIXER_FUNCTION pMixerFunction,
INT32 & iRtnDataSz
)
{
....
case MXF_GET_PROF_SPDIF :
if ( ECHOSTATUS_DSP_DEAD == IsProfessionalSpdif() ) //<==
{
Status = ECHOSTATUS_DSP_DEAD;
}
else
{
pMixerFunction->Data.bProfSpdif = IsProfessionalSpdif();
}
....
}
Ещё одно некорректное сравнение с использованием макросов. Функция IsProfessionalSpdif() возвращает TRUE или FALSE, а мы сравниваем возвращаемый результат с числом 0x12.
V583 [8] The '?:' operator, regardless of its conditional expression, always returns one and the same value. impactv.c 520
void Radeon_CalcImpacTVRegisters(....)
{
....
values->tv_hstart =
internal_encoder ?
values->tv_hdisp + 1 - params->mode888 + 12 :
values->tv_hdisp + 1 - params->mode888 + 12;
....
}
Независимо от значения переменной 'internal_encoder', тернарный оператор возвращает одинаковые значения. Необходимо проверить это место на наличие опечаток.
V523 [9] The 'then' statement is equivalent to the 'else' statement. mkntfs.c 1132
static int insert_positioned_attr_in_mft_record(....)
{
....
if (flags & ATTR_COMPRESSION_MASK) {
hdr_size = 72;
/* FIXME: This compression stuff is all wrong. .... */
/* now. (AIA) */
if (val_len)
mpa_size = 0; /* get_size_for_compressed_....; */
else
mpa_size = 0;
} else {
....
}
....
}
Анализатор напоминает, что необходимо «пофиксить» отложенные места.
Ещё такое место:
V503 [10] This is a nonsensical comparison: pointer <= 0. Header.cpp 900
extern
char *strstr(const char *string, const char *searchString);
void
TTextControl::MessageReceived(BMessage *msg)
{
....
while (node.GetNextAttrName(buffer) == B_OK) {
if (strstr(buffer, "email") <= 0)
continue;
....
}
Функция strstr() возвращает указатель на первое вхождение строки «email» в строке 'buffer', если такого соответствия не найдено, то возвращается NULL. Следовательно, в данном случае необходимо с NULL и сравнивать.
Возможное решение:
void
TTextControl::MessageReceived(BMessage *msg)
{
....
while (node.GetNextAttrName(buffer) == B_OK) {
if (strstr(buffer, "email") == NULL)
continue;
....
}
V512 [11] A call of the 'memcmp' function will lead to underflow of the buffer '«Private-key-format: v»'. dst_api.c 858
dst_s_read_private_key_file(....)
{
....
if (memcmp(in_buff, "Private-key-format: v", 20) != 0)
goto fail;
....
}
Длина сравниваемой строки не совпадает с указанным количеством сравниваемых символов. В строке «Private-key-format: v» 21 символ.
V547 [12] Expression '* r && * r == ' ' && * r == 't'' is always false. Probably the '||' operator should be used here. selection.c 546
static int
selection_rel(....)
{
char *r, *rname;
....
while (*r && *r == ' ' && *r == 't')
r++;
....
}
Скорее всего тут ошибка. В цикле хотели пропустить все пробелы и символы табуляции, но один символ никак не может быть и тем и другим одновременно.
Возможный корректный вариант:
static int
selection_rel(....)
{
char *r, *rname;
....
while (*r == ' ' || *r == 't')
r++;
....
}
V590 [13] Consider inspecting the 'path[i] == '/' && path[i] != ''' expression. The expression is excessive or contains a misprint. storage_support.cpp 309
status_t
parse_first_path_component(const char *path, int32& length,
int32& nextComponent)
{
....
for (; path[i] == '/' && path[i] != ''; i++); //<==
if (path[i] == '') // this covers "" as well
nextComponent = 0;
else
nextComponent = i;
....
}
Здесь всё хорошо, но одна проверка является лишней и её стоит удалить. Для сохранения логики работы, достаточно оставить: «for (; path[i] == '/'; i++);».
Похожие места:
V547 [12] Expression is always true. Probably the '&&' operator should be used here. StatusView.cpp 1397
void
TDragRegion::Draw(BRect)
{
....
if (fDragLocation != kDontDrawDragRegion ||
fDragLocation != kNoDragRegion)
DrawDragRegion();
}
В этой функции что-то всегда отрисовывается. Если построить таблицу истинности логического выражения в условии, можно убедиться, что оно всегда истинно. Возможно, тут должен быть оператор '&&'.
V547 [12] Expression 'reservedBase < 0' is always false. Unsigned type value is never < 0. agp_gart.cpp 1172
/* address types */
typedef unsigned long int __haiku_addr_t; //<==
typedef __haiku_addr_t addr_t; //<==
static status_t
bind_aperture(...., addr_t reservedBase, ....)
{
....
if (status < B_OK) {
if (reservedBase < 0) //<==
aperture->DeleteMemory(memory);
return status;
}
....
}
При таком сравнении с беззнаковым типом, условие всегда ложно и где-то не выполняется очистка памяти. К сожалению, подобных проверок с участием беззнаковых типов в коде около двух сотен. Описывать в статье все такие сравнения, или хотя-бы часть нет никакого смысла. Они все однотипны и неинтересны. Однако, мы предоставим разработчикам полный отчёт и они смогут проанализировать все такие подозрительные места.
V713 [14] The pointer lp was utilized in the logical expression before it was verified against nullptr in the same logical expression. util.c 311
char *
bittok2str(register const struct tok *lp, ....)
{
....
while (lp->s != NULL && lp != NULL) {
....
}
....
}
В условии цикла перепутана последовательность проверки указателя. В начале он разыменовывается, а уже потом проверяется на равенство нулю. Правильный вариант:
while (lp != NULL && lp->s != NULL) {
V593 [15] Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. VideoProducer.cpp 766
int32
VideoProducer::_FrameGeneratorThread()
{
....
err = B_OK;
// Send the buffer on down to the consumer
if (wasCached || (err = SendBuffer(buffer, fOutput.source,
fOutput.destination) != B_OK)) {
....
}
....
}
Анализатор обнаружил потенциальную ошибку в выражении, которое, скорее всего, работает не так, как задумывал программист. Планировалось выполнить присваивание «err = SendBuffer()», а результат сравнить с константой 'B_OK', но приоритет оператора '!=' выше, чем у '=', поэтому в переменную 'err' записывается результат логической операции.
Похожие места:
V547 [12] Expression 'nogscale >= 0' is always true. Unsigned type value is always >= 0. tvp3026.c 212
status_t mil2_dac_init (void)
{
uint32 rfhcnt, nogscale, memconfig;
....
for (nogscale = 1; nogscale >= 0; nogscale--) { //<==
int max = -1 + 33.2 * mclk / (nogscale? 1: 4);
for (rfhcnt = 15; rfhcnt > 0; rfhcnt--) {
int value = (rfhcnt & 0x0e) * 256 + (rfhcnt & 0x01) * 64;
LOG(2,("mil2_dac_init factor %d, rfhcnt %2d: %d ?<= %dn",
nogscale, rfhcnt, value, max));
if (value <= max) goto rfhcnt_found;
}
}
....
}
Скорее всего из-за оператора перехода 'goto' никогда не замечали, что один из циклов «вечный», т.к. при проверке «nogscale >= 0» декремент беззнаковой переменной можно делать бесконечно.
V621 [16] Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. if_ae.c 1670
#define AE_IDLE_TIMEOUT 100
static void
ae_stop_rxmac(ae_softc_t *sc)
{
....
/*
* Wait for IDLE state.
*/
for (i = 0; i < AE_IDLE_TIMEOUT; i--) {
val = AE_READ_4(sc, AE_IDLE_REG);
if ((val & (AE_IDLE_RXMAC | AE_IDLE_DMAWRITE)) == 0)
break;
DELAY(100);
}
....
}
Почему-то значение счётчика в этом цикле изменяется не в ту сторону, логичнее делать инкремент переменной 'i', чтобы ожидание длилось максимум 100 итераций, а не в миллионы раз больше.
Похожее место:
V646 [17] Consider inspecting the application's logic. It's possible that 'else' keyword is missing. Filter.cpp 760
uchar
Scaler::Limit(intType value)
{
if (value < 0) {
value = 0;
} if (value > 255) {
value = 255;
}
return value;
}
В этой функции нет серьёзной ошибки, но код плохо оформлен. Необходимо добавить ключевое слово 'else', либо разместить условия на одном уровне.
V640 [18] The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. strftime.c 1263
#define DO_NUMBER(d, v)
digits = width == -1 ? d : width;
number_value = v; goto do_number
size_t
my_strftime (s, maxsize, format, tp extra_args)
{
....
if (modifier == L_('O'))
goto bad_format;
else
DO_NUMBER (1, tp->tm_year + TM_YEAR_BASE);
....
}
Макросы и так доставляют неудобства при отладке, так ещё и являются источником вот таких ошибок: макрос 'DO_NUMBER' раскрывается в несколько строк, но только первая их них будет частью условного оператора, последующие же операторы будут выполняться независимо от условия.
Аналогично неправильно макрос используется здесь:
Благодаря помощи заинтересованных людей в настройке сборки операционной системы и интеграции анализатора, удалось в короткие сроки подготовить всё для проверки. По сути это идеальный сценарий проверки open-source проектов, потому что часто приходится сталкиваться с абсолютно незнакомыми проектами и, зачастую, имеющими собственные сборочные системы.
В следующей статье будут представлены оставшиеся предупреждения анализатора, о которых я хотел бы рассказать. Они будут различных типов и разделены на несколько групп.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analysis of Haiku Operating System (BeOS Family) by PVS-Studio. Part 1 [19].
Автор: SvyatoslavMC
Источник [21]
Сайт-источник PVSM.RU: https://www.pvsm.ru
Путь до страницы источника: https://www.pvsm.ru/pvs-studio/89514
Ссылки в тексте:
[1] Haiku: http://www.viva64.com/go.php?url=1530
[2] PVS-Studio: http://www.viva64.com/ru/pvs-studio/
[3] проверки Linux Kernel: http://www.viva64.com/ru/b/0299/
[4] V501: http://www.viva64.com/ru/d/0090/
[5] V575: http://www.viva64.com/ru/d/0175/
[6] V502: http://www.viva64.com/ru/d/0091/
[7] V562: http://www.viva64.com/ru/d/0155/
[8] V583: http://www.viva64.com/ru/d/0186/
[9] V523: http://www.viva64.com/ru/d/0112/
[10] V503: http://www.viva64.com/ru/d/0092/
[11] V512: http://www.viva64.com/ru/d/0101/
[12] V547: http://www.viva64.com/ru/d/0137/
[13] V590: http://www.viva64.com/ru/d/0194/
[14] V713: http://www.viva64.com/ru/d/0354/
[15] V593: http://www.viva64.com/ru/d/0197/
[16] V621: http://www.viva64.com/ru/d/0238/
[17] V646: http://www.viva64.com/ru/d/0265/
[18] V640: http://www.viva64.com/ru/d/0258/
[19] Analysis of Haiku Operating System (BeOS Family) by PVS-Studio. Part 1: http://www.viva64.com/en/b/0317/
[20] Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014: http://www.viva64.com/ru/a/0085/
[21] Источник: http://habrahabr.ru/post/256347/
Нажмите здесь для печати.