[C, C++, Open source, Программирование] Проверка QEMU с помощью PVS-Studio
Автор
Сообщение
news_bot ®
Стаж: 6 лет 9 месяцев
Сообщений: 27286
QEMU – достаточно известное приложение для эмуляции. Статический анализ может помочь разработчикам таких сложных проектов, как QEMU, отлавливать ошибки на раннем этапе и в целом повысить его качество и надёжность. В этой статье будет проверен исходный код приложения QEMU на потенциальные уязвимости и ошибки с помощью инструмента статического анализа PVS-Studio.
QEMU является свободным ПО, предназначенным для эмуляции аппаратного обеспечения различных платформ. Оно позволяет запускать приложения и операционные системы на отличающихся от целевой аппаратных платформах, например, приложение, написанное для MIPS запустить для архитектуры x86. QEMU также поддерживает эмуляцию разнообразных периферийных устройств, например, видеокарты, usb и т.д. Проект достаточно сложный и достойный внимания, именно такие проекты представляют интерес для статического анализа, поэтому было решено проверить его код с помощью PVS-Studio.
Об анализе
Исходный код проекта можно получить из зеркала на github. Проект достаточно объемный и может компилироваться для различных платформ. Для более легкой проверки кода воспользуемся системой мониторинга компиляции PVS-Studio. Эта система предназначена для очень простой интеграции статического анализа практически в любую сборочную платформу. Система основана на отслеживании вызовов компилятора во время сборки и позволяет собрать всю информацию для последующего анализа файлов. Другими словами, просто запускаем сборку, PVS-Studio собирает необходимую информацию, а после запускаем анализ — все просто. Подробности можно почитать по ссылке выше.
После проверки анализатор обнаружил множество потенциальных проблем. Для диагностик общего назначения (General Analysis) было получено: 1940 уровня High, 1996 уровня Medium, 9596 уровня Low. После просмотра всех предупреждений было решено остановиться на диагностиках для первого уровня достоверности (High). Таких предупреждений нашлось достаточно много (1940), но большая часть предупреждений либо однотипна, либо связана с многократным использованием подозрительного макроса. Для примера рассмотрим макрос g_new.
#define g_new(struct_type, n_structs)
_G_NEW (struct_type, n_structs, malloc)
#define _G_NEW(struct_type, n_structs, func) \
(struct_type *) (G_GNUC_EXTENSION ({ \
gsize __n = (gsize) (n_structs); \
gsize __s = sizeof (struct_type); \
gpointer __p; \
if (__s == 1) \
__p = g_##func (__n); \
else if (__builtin_constant_p (__n) && \
(__s == 0 || __n <= G_MAXSIZE / __s)) \
__p = g_##func (__n * __s); \
else \
__p = g_##func##_n (__n, __s); \
__p; \
}))
На каждое использование этого макроса анализатор выдает предупреждение V773 (Visibility scope of the '__p' pointer was exited without releasing the memory. A memory leak is possible). Макрос g_new определен в библиотеке glib, он использует макрос _G_NEW, а этот макрос в свою очередь использует другой макрос G_GNUC_EXTENSION, говорящий компилятору GCC пропускать предупреждения о нестандартном коде. Именно этот нестандартный код и вызывает предупреждение анализатора, обратите внимание на предпоследнюю строку. В целом же макрос является рабочим. Предупреждений этого типа нашлось 848 штук, то есть почти половина срабатываний приходится всего лишь на одно единственное место в коде.
Все эти лишние предупреждения легко убрать, используя настройки анализатора. Впрочем, конкретно данный случай, который встретился при написании статьи, является поводом нашей команды немного доработать логику анализатора для таких ситуаций.
Таким образом, большое количество предупреждений не всегда говорит о плохом качестве кода. Тем не менее есть некоторые по-настоящему подозрительные места. Что же, давайте приступим к разбору предупреждений.
Предупреждение N1
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2395, 2397. megasas.c 2395
#define MEGASAS_MAX_SGE 128 /* Firmware limit */
....
static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
{
....
if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
....
} else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
....
}
....
}
Любые использования "магических" чисел в коде всегда вызывают подозрение. Здесь два условия, и на первый взгляд они кажутся разными, но, если посмотреть значение макроса MEGASAS_MAX_SGE, то окажется, что условия дублируют друг друга. Скорее всего, здесь опечатка и вместо 128 должно стоять другое число. Конечно, это проблема всех "магических" чисел, достаточно просто опечататься при их использовании. Применение макросов и констант сильно помогает разработчику в этом случае.
Предупреждение N2
V523 The 'then' statement is equivalent to the 'else' statement. cp0_helper.c 383
target_ulong helper_mftc0_cause(CPUMIPSState *env)
{
....
CPUMIPSState *other = mips_cpu_map_tc(env, &other_tc);
if (other_tc == other->current_tc) {
tccause = other->CP0_Cause;
} else {
tccause = other->CP0_Cause;
}
....
}
В рассматриваемом коде тела then и else условного оператора идентичны. Здесь, скорее всего, copy-paste. Просто скопировали тело then ветвления, а исправить забыли. Можно предположить, что вместо объекта other необходимо было использовать env. Исправление этого подозрительного места могло бы выглядеть следующим образом:
if (other_tc == other->current_tc) {
tccause = other->CP0_Cause;
} else {
tccause = env->CP0_Cause;
}
Однозначно сказать, как должно быть на самом деле могут только разработчики этого кода. Еще похожее место:
- V523 The 'then' statement is equivalent to the 'else' statement. translate.c 641
Предупреждение N3
V547 Expression 'ret < 0' is always false. qcow2-cluster.c 1557
static int handle_dependencies(....)
{
....
if (end <= old_start || start >= old_end) {
....
} else {
if (bytes == 0 && *m) {
....
return 0; // <= 3
}
if (bytes == 0) {
....
return -EAGAIN; // <= 4
}
....
}
return 0; // <= 5
}
int qcow2_alloc_cluster_offset(BlockDriverState *bs, ....)
{
....
ret = handle_dependencies(bs, start, &cur_bytes, m);
if (ret == -EAGAIN) { // <= 2
....
} else if (ret < 0) { // <= 1
....
}
}
Здесь анализатор обнаружил, что условие (комментарий 1) никогда не выполнится. Значение переменной ret инициализируется результатом выполнения функции handle_dependencies, эта функция возвращает только 0 или -EAGAIN (комментарии 3, 4, 5). Чуть выше, в первом условии, мы проверили значение ret на -EAGAIN (комментарий 2), поэтому результат выполнения выражения ret < 0 будет всегда ложным. Возможно, раньше функция handle_dependencies и возвращала другие значения, но потом в результате, например, рефакторинга поведение поменялось. Здесь надо просто завершить рефакторинг. Похожие срабатывания:
- V547 Expression is always false. qcow2.c 1070
- V547 Expression 's->state != MIGRATION_STATUS_COLO' is always false. colo.c 595
- V547 Expression 's->metadata_entries.present & 0x20' is always false. vhdx.c 769
Предупреждение N4
V557 Array overrun is possible. The 'dwc2_glbreg_read' function processes value '[0..63]'. Inspect the third argument. Check lines: 667, 1040. hcd-dwc2.c 667
#define HSOTG_REG(x) (x) // <= 5
....
struct DWC2State {
....
#define DWC2_GLBREG_SIZE 0x70
uint32_t glbreg[DWC2_GLBREG_SIZE / sizeof(uint32_t)]; // <= 1
....
}
....
static uint64_t dwc2_glbreg_read(void *ptr, hwaddr addr, int index,
unsigned size)
{
....
val = s->glbreg[index]; // <= 2
....
}
static uint64_t dwc2_hsotg_read(void *ptr, hwaddr addr, unsigned size)
{
....
switch (addr) {
case HSOTG_REG(0x000) ... HSOTG_REG(0x0fc): // <= 4
val = dwc2_glbreg_read(ptr, addr,
(addr - HSOTG_REG(0x000)) >> 2, size); // <= 3
....
}
....
}
В этом коде есть потенциальная проблема с выходом за границу массива. В структуре DWC2State определен массив glbreg,состоящий из 28 элементов (комментарий 1). В функции dwc2_glbreg_read по индексу обращаются к нашему массиву (комментарий 2). Теперь обратите внимание, что в функцию dwc2_glbreg_read в качестве индекса передают выражение (addr — HSOTG_REG(0x000)) >> 2 (комментарий 3), которое может принимать значение в диапазоне [0..63]. Для того чтобы в этом убедиться, обратите внимание на комментарии 4 и 5. Возможно, тут надо скорректировать диапазон значений из комментария 4.
Еще похожие срабатывания:
- V557 Array overrun is possible. The 'dwc2_hreg0_read' function processes value '[0..63]'. Inspect the third argument. Check lines: 814, 1050. hcd-dwc2.c 814
- V557 Array overrun is possible. The 'dwc2_hreg1_read' function processes value '[0..191]'. Inspect the third argument. Check lines: 927, 1053. hcd-dwc2.c 927
- V557 Array overrun is possible. The 'dwc2_pcgreg_read' function processes value '[0..127]'. Inspect the third argument. Check lines: 1012, 1060. hcd-dwc2.c 1012
Предупреждение N5
V575 The 'strerror_s' function processes '0' elements. Inspect the second argument. commands-win32.c 1642
void qmp_guest_set_time(bool has_time, int64_t time_ns,
Error **errp)
{
....
if (GetLastError() != 0) {
strerror_s((LPTSTR) & msg_buffer, 0, errno);
....
}
}
Функция strerror_s возвращает текстовое описание кода системной ошибки. Её сигнатура выглядит так:
errno_t strerror_s( char *buf, rsize_t bufsz, errno_t errnum );
Первый параметр — это указатель на буфер, куда будет скопировано текстовое описание, второй параметр — размер буфера, третий — код ошибки. В коде в качестве размера буфера передается 0, это явно ошибочное значение. Кстати, есть возможность узнать заранее сколько байт надо выделять: надо просто вызвать strerrorlen_s, которая возвращает длину текстового описания ошибки. Это значение можно использовать для выделения буфера достаточного размера.
Предупреждение N6
V595 The 'blen2p' pointer was utilized before it was verified against nullptr. Check lines: 103, 106. dsound_template.h 103
static int glue (
....
DWORD *blen1p,
DWORD *blen2p,
int entire,
dsound *s
)
{
....
dolog("DirectSound returned misaligned buffer %ld %ld\n",
*blen1p, *blen2p); // <= 1
glue(.... p2p ? *p2p : NULL, *blen1p,
blen2p ? *blen2p : 0); // <= 2
....
}
В этом коде значение аргумента blen2p сначала используется (комментарий 1), а потом проверяется на nullptr (комментарий 2). Это крайне подозрительное место выглядит так, как будто просто забыли вставить проверку перед первым использованием (комментарий 1). Как вариант исправления — просто добавить проверку:
dolog("DirectSound returned misaligned buffer %ld %ld\n",
*blen1p, blen2p ? *blen2p : 0);
Тут еще возникает вопрос по поводу аргумента blen1p. Вероятно, он тоже может быть нулевым указателем, и тут тоже надо будет добавить проверку. Еще несколько подобных срабатываний:
- V595 The 'ref' pointer was utilized before it was verified against nullptr. Check lines: 2191, 2193. uri.c 2191
- V595 The 'cmdline' pointer was utilized before it was verified against nullptr. Check lines: 420, 425. qemu-io.c 420
- V595 The 'dp' pointer was utilized before it was verified against nullptr. Check lines: 288, 294. onenand.c 288
- V595 The 'omap_lcd' pointer was utilized before it was verified against nullptr. Check lines: 81, 87. omap_lcdc.c 81
Предупреждение N7
V597 The compiler could delete the 'memset' function call, which is used to flush 'op_info' object. The RtlSecureZeroMemory() function should be used to erase the private data. virtio-crypto.c 354
static void virtio_crypto_free_request(VirtIOCryptoReq *req)
{
if (req) {
if (req->flags == CRYPTODEV_BACKEND_ALG_SYM) {
....
/* Zeroize and free request data structure */
memset(op_info, 0, sizeof(*op_info) + max_len); // <= 1
g_free(op_info);
}
g_free(req);
}
}
В этом фрагменте кода вызывается функция memset для объекта op_info (комментарий 1), после этого op_info сразу удаляется, то есть, другими словами, после очистки этот объект нигде больше не модифицируется. Это как раз тот самый случай, когда в процессе оптимизации компилятор может удалить вызов memset. Чтобы исключить подобное потенциальное поведение, можно воспользоваться специальными функциями, которые компилятор никогда не удаляет. См. также статью "Безопасная очистка приватных данных".
Предупреждение N8
V610 Unspecified behavior. Check the shift operator '>>'. The left operand is negative ('number' = [-32768..2147483647]). cris.c 2111
static void
print_with_operands (const struct cris_opcode *opcodep,
unsigned int insn,
unsigned char *buffer,
bfd_vma addr,
disassemble_info *info,
const struct cris_opcode *prefix_opcodep,
unsigned int prefix_insn,
unsigned char *prefix_buffer,
bfd_boolean with_reg_prefix)
{
....
int32_t number;
....
if (signedp && number > 127)
number -= 256; // <= 1
....
if (signedp && number > 32767)
number -= 65536; // <= 2
....
unsigned int highbyte = (number >> 24) & 0xff;
....
}
Так как переменная number может иметь отрицательное значение, побитовый сдвиг вправо является неуточнённым поведением (unspecified behavior). Чтобы убедиться в том, что рассматриваемая переменная может принять отрицательное значение, обратите внимание на комментарии 1 и 2. Для устранения различий поведения вашего кода на различных платформах, таких случаев нужно не допускать.
Еще предупреждения:
- V610 Undefined behavior. Check the shift operator '<<'. The left operand is negative ('(hclk_div — 1)' = [-1..15]). aspeed_smc.c 1041
- V610 Undefined behavior. Check the shift operator '<<'. The left operand '(target_long) — 1' is negative. exec-vary.c 99
- V610 Undefined behavior. Check the shift operator '<<'. The left operand is negative ('hex2nib(words[3][i * 2 + 2])' = [-1..15]). qtest.c 561
Также есть несколько предупреждений такого же типа, только в качестве левого операнда выступает -1.
V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. hppa.c 2702
int print_insn_hppa (bfd_vma memaddr, disassemble_info *info)
{
....
disp = (-1 << 10) | imm10;
....
}
Другие подобные предупреждения:
- V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. hppa.c 2718
- V610 Undefined behavior. Check the shift operator '<<'. The left operand '-0x8000' is negative. fmopl.c 1022
- V610 Undefined behavior. Check the shift operator '<<'. The left operand '(intptr_t) — 1' is negative. sve_helper.c 889
Предупреждение N9
V616 The 'TIMER_NONE' named constant with the value of 0 is used in the bitwise operation. sys_helper.c 179
#define HELPER(name) ....
enum {
TIMER_NONE = (0 << 30), // <= 1
....
}
void HELPER(mtspr)(CPUOpenRISCState *env, ....)
{
....
if (env->ttmr & TIMER_NONE) { // <= 2
....
}
}
Можно легко убедиться, что значение макроса TIMER_NONE равно нулю (комментарий 1). Далее этот макрос используется в побитовой операции, результат которой всегда будет 0. Как итог, тело условного оператора if (env->ttmr & TIMER_NONE) никогда не выполнится.
Предупреждение N10
V629 Consider inspecting the 'n << 9' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. qemu-img.c 1839
#define BDRV_SECTOR_BITS 9
static int coroutine_fn convert_co_read(ImgConvertState *s,
int64_t sector_num, int nb_sectors, uint8_t *buf)
{
uint64_t single_read_until = 0;
int n;
....
while (nb_sectors > 0) {
....
uint64_t offset;
....
single_read_until = offset + (n << BDRV_SECTOR_BITS);
....
}
....
}
В этом фрагменте кода над переменной n, имеющей 32-битный знаковый тип, выполняется операция сдвига, потом этот 32-битный знаковый результат расширяется до 64-битного знакового типа, и далее, как беззнаковый тип, складывается с беззнаковой 64-битной переменной offset. Предположим, что на момент выполнения выражения переменная n имеет некоторые значимые старшие 9 бит. Мы выполняем операцию сдвига на 9 разрядов (BDRV_SECTOR_BITS), а это, в свою очередь, является неопределенным поведением, тогда в качестве результата мы можем получить выставленный бит в старшем разряде. Напомним, что этот бит в знаковом типе отвечает за знак, то есть результат может стать отрицательным. Так как переменная n знакового типа, то при расширении будет учтен знак. Далее результат складывается с переменной offset. Из этих рассуждений нетрудно увидеть, что результат выполнения выражения может отличаться от предполагаемого. Одним из возможных вариантов решения является замена типа переменной n на 64-битный беззнаковый тип, то есть на uint64_t.
Вот еще похожие срабатывания:
- V629 Consider inspecting the '1 << refcount_order' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. qcow2.c 3204
- V629 Consider inspecting the 's->cluster_size << 3' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. qcow2-bitmap.c 283
- V629 Consider inspecting the 'i << s->cluster_bits' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. qcow2-cluster.c 983
- V629 Consider inspecting the expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. vhdx.c 1145
- V629 Consider inspecting the 'delta << 2' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. mips.c 4341
Предупреждение N11
V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. nand.c 310
static void nand_command(NANDFlashState *s)
{
....
s->addr &= (1ull << s->addrlen * 8) - 1;
....
}
Тут просто подозрительное место. Непонятно, что программист хотел сделать вначале: сдвиг или умножение. Даже если здесь нет ошибки, все равно надо еще раз взглянуть на код и правильно расставить скобочки. Тут как раз одно из таких мест, которые должны смотреть разработчики, чтобы убедиться в корректности своего алгоритма. Другие такие места:
- V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. exynos4210_mct.c 449
- V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. exynos4210_mct.c 1235
- V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. exynos4210_mct.c 1264
Предупреждение N12
V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. pl181.c 400
static void pl181_write(void *opaque, hwaddr offset,
uint64_t value, unsigned size)
{
....
if (s->cmd & PL181_CMD_ENABLE) {
if (s->cmd & PL181_CMD_INTERRUPT) {
....
} if (s->cmd & PL181_CMD_PENDING) { // <= else if
....
} else {
....
}
....
}
....
}
В этом коде, судя по форматированию, прямо напрашивается использование else if вместо if. Возможно, здесь забыли дописать else. Тогда вариант исправления может быть такой:
} else if (s->cmd & PL181_CMD_PENDING) { // <= else if
Однако есть вероятность, что с этим кодом все в порядке, и тут некорректное форматирование текста программы, которое сбивает с толку. Тогда код мог бы выглядеть так:
if (s->cmd & PL181_CMD_INTERRUPT) {
....
}
if (s->cmd & PL181_CMD_PENDING) { // <= if
....
} else {
....
}
Предупреждение N13
V773 The function was exited without releasing the 'rule' pointer. A memory leak is possible. blkdebug.c 218
static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
{
....
struct BlkdebugRule *rule;
....
rule = g_malloc0(sizeof(*rule)); // <= 1
....
if (local_error) {
error_propagate(errp, local_error);
return -1; // <= 2
}
....
/* Add the rule */
QLIST_INSERT_HEAD(&s->rules[event], rule, next); // <= 3
....
}
В данном коде выделяется объект rule (комментарий 1) и добавляется в список для последующего использования (комментарий 3), но в случае ошибки происходит возврат из функции без удаления ранее созданного объекта rule (комментарий 2). Здесь надо просто правильно обработать ошибку: удалить ранее созданный объект, иначе будет утечка памяти.
Предупреждение N14
V781 The value of the 'ix' index is checked after it was used. Perhaps there is a mistake in program logic. uri.c 2110
char *uri_resolve_relative(const char *uri, const char *base)
{
....
ix = pos;
if ((ref->path[ix] == '/') && (ix > 0)) {
....
}
Здесь анализатор обнаружил потенциальный выход за границу массива. Сначала читается элемент массива ref->path по индексу ix, а потом ix проверяется на корректность (ix > 0). Правильным решением тут будет поменять эти действия местами:
if ((ix > 0) && (ref->path[ix] == '/')) {
Таких мест нашлось несколько:
- V781 The value of the 'ix' index is checked after it was used. Perhaps there is a mistake in program logic. uri.c 2112
- V781 The value of the 'offset' index is checked after it was used. Perhaps there is a mistake in program logic. keymaps.c 125
- V781 The value of the 'quality' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 326, 335. vnc-enc-tight.c 326
- V781 The value of the 'i' index is checked after it was used. Perhaps there is a mistake in program logic. mem_helper.c 1929
Предупреждение N15
V784 The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. cadence_gem.c 1486
typedef struct CadenceGEMState {
....
uint32_t regs_ro[CADENCE_GEM_MAXREG];
}
....
static void gem_write(void *opaque, hwaddr offset, uint64_t val,
unsigned size)
{
....
val &= ~(s->regs_ro[offset]);
....
}
В этом коде выполняется побитовая операция с объектами разных типов. Левый операнд — это аргумент val, имеющий 64-битный беззнаковый тип. В качестве правого операнда выступает полученное значение элемента массива s->regs_ro по индексу offset, имеющий 32-битный беззнаковый тип. Результат операции в правой части (~(s->regs_ro[offset])) является 32-битным беззнаковым типом, и перед побитовым умножением он расширится до 64-битного типа нулями, то есть после вычисления всего выражения обнулятся все старшие биты переменной val. Такие места всегда выглядят подозрительными. Тут можно только порекомендовать разработчикам еще раз пересмотреть этот код. Еще похожее:
- V784 The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. xlnx-zynq-devcfg.c 199
- V784 The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. soc_dma.c 214
- V784 The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. fpu_helper.c 418
Предупреждение N16
V1046 Unsafe usage of the 'bool' and 'unsigned int' types together in the operation '&='. helper.c 10821
static inline uint32_t extract32(uint32_t value, int start, int length);
....
static ARMVAParameters aa32_va_parameters(CPUARMState *env, uint32_t va,
ARMMMUIdx mmu_idx)
{
....
bool epd, hpd;
....
hpd &= extract32(tcr, 6, 1);
}
В этом фрагменте кода происходит операция побитового И над переменной hpd, имеющей тип bool, и результатом выполнения функции extract32, имеющим тип uint32_t. Так как битовое значение булевой переменной может быть только 0 или 1, то результат выражения будет всегда false, если младший бит, возвращаемый функцийе extract32, равен нулю. Давайте рассмотрим это на примере. Предположим, что значение hpd равно true, а функция вернула значение 2, то есть в двоичном представлении операция будет выглядеть так 01 & 10 = 0, а результат выражения будет равен false. Скорее всего, программист хотел выставлять значение true, если функция возвращает что-то отличное от нуля. По всей видимости, надо исправить код так, чтобы результат выполнения функции приводился к типу bool, например, так:
hpd = hpd && (bool)extract32(tcr, 6, 1);
Заключение
Как видно, анализатор нашел много подозрительных мест. Возможно, найденные потенциальные проблемы пока никак себя не проявляют, но их наличие не может не тревожить, так как они способны выстрелить в самый неожиданный момент. Просмотреть все подозрительные места заранее и исправить — лучше, чем чинить бесконечный поток багов. Очевидно, что для сложных проектов, подобных этому, статический анализ может принести ощутимую пользу, особенно если организовать регулярную проверку проекта. Если Вы хотите попробовать PVS-Studio для своего проекта, вы можете скачать анализатор и получить бесплатный триальный ключ на этой странице.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Evgeniy Ovsannikov. Checking QEMU using PVS-Studio.
===========
Источник:
habr.com
===========
Похожие новости:
- [Астрономия, Космонавтика, Научно-популярное, Физика] Космический субботник: уборка пыли на Луне
- [C, C++, Open source, Программирование] Checking QEMU using PVS-Studio
- [Python, Data Mining, Открытые данные] Black [O]lives Matter: раса, криминал и огонь на поражение в США. Часть 2
- Компания Oracle выпустила ядро Unbreakable Enterprise Kernel R5U4
- [Информационная безопасность, IT-компании] Минуту внимания, мы засекали
- [Python, Data Mining, Открытые данные] Black [O]lives Matter: раса, криминал и огонь на поражение в США. Часть 1
- [JavaScript, Программирование] Как я искал работу в Берлине
- [Python, Высокая производительность] Немного Сythonа
- [IT-компании, Законодательство в IT] Ситуация с PandaDoc в Беларуси: четверо сотрудников задержано, на топ-менеджеров компании завели уголовные дела
- [Карьера в IT-индустрии, IT-эмиграция] [Личный опыт] Профессиональная эмиграция: чем отличается Амстердам от Риги и Booking.com от молодого стартапа
Теги для поиска: #_c, #_c++, #_open_source, #_programmirovanie (Программирование), #_s++ (с++), #_open_source, #_programming, #_qemu, #_virtual_machine, #_blog_kompanii_pvsstudio (
Блог компании PVS-Studio
), #_c, #_c++, #_open_source, #_programmirovanie (
Программирование
)
Вы не можете начинать темы
Вы не можете отвечать на сообщения
Вы не можете редактировать свои сообщения
Вы не можете удалять свои сообщения
Вы не можете голосовать в опросах
Вы не можете прикреплять файлы к сообщениям
Вы не можете скачивать файлы
Текущее время: 22-Ноя 07:59
Часовой пояс: UTC + 5
| Автор | Сообщение |
|---|---|
|
news_bot ®
Стаж: 6 лет 9 месяцев |
|
 QEMU – достаточно известное приложение для эмуляции. Статический анализ может помочь разработчикам таких сложных проектов, как QEMU, отлавливать ошибки на раннем этапе и в целом повысить его качество и надёжность. В этой статье будет проверен исходный код приложения QEMU на потенциальные уязвимости и ошибки с помощью инструмента статического анализа PVS-Studio. QEMU является свободным ПО, предназначенным для эмуляции аппаратного обеспечения различных платформ. Оно позволяет запускать приложения и операционные системы на отличающихся от целевой аппаратных платформах, например, приложение, написанное для MIPS запустить для архитектуры x86. QEMU также поддерживает эмуляцию разнообразных периферийных устройств, например, видеокарты, usb и т.д. Проект достаточно сложный и достойный внимания, именно такие проекты представляют интерес для статического анализа, поэтому было решено проверить его код с помощью PVS-Studio. Об анализе Исходный код проекта можно получить из зеркала на github. Проект достаточно объемный и может компилироваться для различных платформ. Для более легкой проверки кода воспользуемся системой мониторинга компиляции PVS-Studio. Эта система предназначена для очень простой интеграции статического анализа практически в любую сборочную платформу. Система основана на отслеживании вызовов компилятора во время сборки и позволяет собрать всю информацию для последующего анализа файлов. Другими словами, просто запускаем сборку, PVS-Studio собирает необходимую информацию, а после запускаем анализ — все просто. Подробности можно почитать по ссылке выше. После проверки анализатор обнаружил множество потенциальных проблем. Для диагностик общего назначения (General Analysis) было получено: 1940 уровня High, 1996 уровня Medium, 9596 уровня Low. После просмотра всех предупреждений было решено остановиться на диагностиках для первого уровня достоверности (High). Таких предупреждений нашлось достаточно много (1940), но большая часть предупреждений либо однотипна, либо связана с многократным использованием подозрительного макроса. Для примера рассмотрим макрос g_new. #define g_new(struct_type, n_structs)
_G_NEW (struct_type, n_structs, malloc) #define _G_NEW(struct_type, n_structs, func) \ (struct_type *) (G_GNUC_EXTENSION ({ \ gsize __n = (gsize) (n_structs); \ gsize __s = sizeof (struct_type); \ gpointer __p; \ if (__s == 1) \ __p = g_##func (__n); \ else if (__builtin_constant_p (__n) && \ (__s == 0 || __n <= G_MAXSIZE / __s)) \ __p = g_##func (__n * __s); \ else \ __p = g_##func##_n (__n, __s); \ __p; \ })) На каждое использование этого макроса анализатор выдает предупреждение V773 (Visibility scope of the '__p' pointer was exited without releasing the memory. A memory leak is possible). Макрос g_new определен в библиотеке glib, он использует макрос _G_NEW, а этот макрос в свою очередь использует другой макрос G_GNUC_EXTENSION, говорящий компилятору GCC пропускать предупреждения о нестандартном коде. Именно этот нестандартный код и вызывает предупреждение анализатора, обратите внимание на предпоследнюю строку. В целом же макрос является рабочим. Предупреждений этого типа нашлось 848 штук, то есть почти половина срабатываний приходится всего лишь на одно единственное место в коде. Все эти лишние предупреждения легко убрать, используя настройки анализатора. Впрочем, конкретно данный случай, который встретился при написании статьи, является поводом нашей команды немного доработать логику анализатора для таких ситуаций. Таким образом, большое количество предупреждений не всегда говорит о плохом качестве кода. Тем не менее есть некоторые по-настоящему подозрительные места. Что же, давайте приступим к разбору предупреждений. Предупреждение N1 V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2395, 2397. megasas.c 2395 #define MEGASAS_MAX_SGE 128 /* Firmware limit */
.... static void megasas_scsi_realize(PCIDevice *dev, Error **errp) { .... if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) { .... } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) { .... } .... } Любые использования "магических" чисел в коде всегда вызывают подозрение. Здесь два условия, и на первый взгляд они кажутся разными, но, если посмотреть значение макроса MEGASAS_MAX_SGE, то окажется, что условия дублируют друг друга. Скорее всего, здесь опечатка и вместо 128 должно стоять другое число. Конечно, это проблема всех "магических" чисел, достаточно просто опечататься при их использовании. Применение макросов и констант сильно помогает разработчику в этом случае. Предупреждение N2 V523 The 'then' statement is equivalent to the 'else' statement. cp0_helper.c 383 target_ulong helper_mftc0_cause(CPUMIPSState *env)
{ .... CPUMIPSState *other = mips_cpu_map_tc(env, &other_tc); if (other_tc == other->current_tc) { tccause = other->CP0_Cause; } else { tccause = other->CP0_Cause; } .... } В рассматриваемом коде тела then и else условного оператора идентичны. Здесь, скорее всего, copy-paste. Просто скопировали тело then ветвления, а исправить забыли. Можно предположить, что вместо объекта other необходимо было использовать env. Исправление этого подозрительного места могло бы выглядеть следующим образом: if (other_tc == other->current_tc) {
tccause = other->CP0_Cause; } else { tccause = env->CP0_Cause; } Однозначно сказать, как должно быть на самом деле могут только разработчики этого кода. Еще похожее место:
Предупреждение N3 V547 Expression 'ret < 0' is always false. qcow2-cluster.c 1557 static int handle_dependencies(....)
{ .... if (end <= old_start || start >= old_end) { .... } else { if (bytes == 0 && *m) { .... return 0; // <= 3 } if (bytes == 0) { .... return -EAGAIN; // <= 4 } .... } return 0; // <= 5 } int qcow2_alloc_cluster_offset(BlockDriverState *bs, ....) { .... ret = handle_dependencies(bs, start, &cur_bytes, m); if (ret == -EAGAIN) { // <= 2 .... } else if (ret < 0) { // <= 1 .... } } Здесь анализатор обнаружил, что условие (комментарий 1) никогда не выполнится. Значение переменной ret инициализируется результатом выполнения функции handle_dependencies, эта функция возвращает только 0 или -EAGAIN (комментарии 3, 4, 5). Чуть выше, в первом условии, мы проверили значение ret на -EAGAIN (комментарий 2), поэтому результат выполнения выражения ret < 0 будет всегда ложным. Возможно, раньше функция handle_dependencies и возвращала другие значения, но потом в результате, например, рефакторинга поведение поменялось. Здесь надо просто завершить рефакторинг. Похожие срабатывания:
Предупреждение N4 V557 Array overrun is possible. The 'dwc2_glbreg_read' function processes value '[0..63]'. Inspect the third argument. Check lines: 667, 1040. hcd-dwc2.c 667 #define HSOTG_REG(x) (x) // <= 5
.... struct DWC2State { .... #define DWC2_GLBREG_SIZE 0x70 uint32_t glbreg[DWC2_GLBREG_SIZE / sizeof(uint32_t)]; // <= 1 .... } .... static uint64_t dwc2_glbreg_read(void *ptr, hwaddr addr, int index, unsigned size) { .... val = s->glbreg[index]; // <= 2 .... } static uint64_t dwc2_hsotg_read(void *ptr, hwaddr addr, unsigned size) { .... switch (addr) { case HSOTG_REG(0x000) ... HSOTG_REG(0x0fc): // <= 4 val = dwc2_glbreg_read(ptr, addr, (addr - HSOTG_REG(0x000)) >> 2, size); // <= 3 .... } .... } В этом коде есть потенциальная проблема с выходом за границу массива. В структуре DWC2State определен массив glbreg,состоящий из 28 элементов (комментарий 1). В функции dwc2_glbreg_read по индексу обращаются к нашему массиву (комментарий 2). Теперь обратите внимание, что в функцию dwc2_glbreg_read в качестве индекса передают выражение (addr — HSOTG_REG(0x000)) >> 2 (комментарий 3), которое может принимать значение в диапазоне [0..63]. Для того чтобы в этом убедиться, обратите внимание на комментарии 4 и 5. Возможно, тут надо скорректировать диапазон значений из комментария 4. Еще похожие срабатывания:
Предупреждение N5 V575 The 'strerror_s' function processes '0' elements. Inspect the second argument. commands-win32.c 1642 void qmp_guest_set_time(bool has_time, int64_t time_ns,
Error **errp) { .... if (GetLastError() != 0) { strerror_s((LPTSTR) & msg_buffer, 0, errno); .... } } Функция strerror_s возвращает текстовое описание кода системной ошибки. Её сигнатура выглядит так: errno_t strerror_s( char *buf, rsize_t bufsz, errno_t errnum );
Первый параметр — это указатель на буфер, куда будет скопировано текстовое описание, второй параметр — размер буфера, третий — код ошибки. В коде в качестве размера буфера передается 0, это явно ошибочное значение. Кстати, есть возможность узнать заранее сколько байт надо выделять: надо просто вызвать strerrorlen_s, которая возвращает длину текстового описания ошибки. Это значение можно использовать для выделения буфера достаточного размера. Предупреждение N6 V595 The 'blen2p' pointer was utilized before it was verified against nullptr. Check lines: 103, 106. dsound_template.h 103 static int glue (
.... DWORD *blen1p, DWORD *blen2p, int entire, dsound *s ) { .... dolog("DirectSound returned misaligned buffer %ld %ld\n", *blen1p, *blen2p); // <= 1 glue(.... p2p ? *p2p : NULL, *blen1p, blen2p ? *blen2p : 0); // <= 2 .... } В этом коде значение аргумента blen2p сначала используется (комментарий 1), а потом проверяется на nullptr (комментарий 2). Это крайне подозрительное место выглядит так, как будто просто забыли вставить проверку перед первым использованием (комментарий 1). Как вариант исправления — просто добавить проверку: dolog("DirectSound returned misaligned buffer %ld %ld\n",
*blen1p, blen2p ? *blen2p : 0); Тут еще возникает вопрос по поводу аргумента blen1p. Вероятно, он тоже может быть нулевым указателем, и тут тоже надо будет добавить проверку. Еще несколько подобных срабатываний:
Предупреждение N7 V597 The compiler could delete the 'memset' function call, which is used to flush 'op_info' object. The RtlSecureZeroMemory() function should be used to erase the private data. virtio-crypto.c 354 static void virtio_crypto_free_request(VirtIOCryptoReq *req)
{ if (req) { if (req->flags == CRYPTODEV_BACKEND_ALG_SYM) { .... /* Zeroize and free request data structure */ memset(op_info, 0, sizeof(*op_info) + max_len); // <= 1 g_free(op_info); } g_free(req); } } В этом фрагменте кода вызывается функция memset для объекта op_info (комментарий 1), после этого op_info сразу удаляется, то есть, другими словами, после очистки этот объект нигде больше не модифицируется. Это как раз тот самый случай, когда в процессе оптимизации компилятор может удалить вызов memset. Чтобы исключить подобное потенциальное поведение, можно воспользоваться специальными функциями, которые компилятор никогда не удаляет. См. также статью "Безопасная очистка приватных данных". Предупреждение N8 V610 Unspecified behavior. Check the shift operator '>>'. The left operand is negative ('number' = [-32768..2147483647]). cris.c 2111 static void
print_with_operands (const struct cris_opcode *opcodep, unsigned int insn, unsigned char *buffer, bfd_vma addr, disassemble_info *info, const struct cris_opcode *prefix_opcodep, unsigned int prefix_insn, unsigned char *prefix_buffer, bfd_boolean with_reg_prefix) { .... int32_t number; .... if (signedp && number > 127) number -= 256; // <= 1 .... if (signedp && number > 32767) number -= 65536; // <= 2 .... unsigned int highbyte = (number >> 24) & 0xff; .... } Так как переменная number может иметь отрицательное значение, побитовый сдвиг вправо является неуточнённым поведением (unspecified behavior). Чтобы убедиться в том, что рассматриваемая переменная может принять отрицательное значение, обратите внимание на комментарии 1 и 2. Для устранения различий поведения вашего кода на различных платформах, таких случаев нужно не допускать. Еще предупреждения:
Также есть несколько предупреждений такого же типа, только в качестве левого операнда выступает -1. V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. hppa.c 2702 int print_insn_hppa (bfd_vma memaddr, disassemble_info *info)
{ .... disp = (-1 << 10) | imm10; .... } Другие подобные предупреждения:
Предупреждение N9 V616 The 'TIMER_NONE' named constant with the value of 0 is used in the bitwise operation. sys_helper.c 179 #define HELPER(name) ....
enum { TIMER_NONE = (0 << 30), // <= 1 .... } void HELPER(mtspr)(CPUOpenRISCState *env, ....) { .... if (env->ttmr & TIMER_NONE) { // <= 2 .... } } Можно легко убедиться, что значение макроса TIMER_NONE равно нулю (комментарий 1). Далее этот макрос используется в побитовой операции, результат которой всегда будет 0. Как итог, тело условного оператора if (env->ttmr & TIMER_NONE) никогда не выполнится. Предупреждение N10 V629 Consider inspecting the 'n << 9' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. qemu-img.c 1839 #define BDRV_SECTOR_BITS 9
static int coroutine_fn convert_co_read(ImgConvertState *s, int64_t sector_num, int nb_sectors, uint8_t *buf) { uint64_t single_read_until = 0; int n; .... while (nb_sectors > 0) { .... uint64_t offset; .... single_read_until = offset + (n << BDRV_SECTOR_BITS); .... } .... } В этом фрагменте кода над переменной n, имеющей 32-битный знаковый тип, выполняется операция сдвига, потом этот 32-битный знаковый результат расширяется до 64-битного знакового типа, и далее, как беззнаковый тип, складывается с беззнаковой 64-битной переменной offset. Предположим, что на момент выполнения выражения переменная n имеет некоторые значимые старшие 9 бит. Мы выполняем операцию сдвига на 9 разрядов (BDRV_SECTOR_BITS), а это, в свою очередь, является неопределенным поведением, тогда в качестве результата мы можем получить выставленный бит в старшем разряде. Напомним, что этот бит в знаковом типе отвечает за знак, то есть результат может стать отрицательным. Так как переменная n знакового типа, то при расширении будет учтен знак. Далее результат складывается с переменной offset. Из этих рассуждений нетрудно увидеть, что результат выполнения выражения может отличаться от предполагаемого. Одним из возможных вариантов решения является замена типа переменной n на 64-битный беззнаковый тип, то есть на uint64_t. Вот еще похожие срабатывания:
Предупреждение N11 V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. nand.c 310 static void nand_command(NANDFlashState *s)
{ .... s->addr &= (1ull << s->addrlen * 8) - 1; .... } Тут просто подозрительное место. Непонятно, что программист хотел сделать вначале: сдвиг или умножение. Даже если здесь нет ошибки, все равно надо еще раз взглянуть на код и правильно расставить скобочки. Тут как раз одно из таких мест, которые должны смотреть разработчики, чтобы убедиться в корректности своего алгоритма. Другие такие места:
Предупреждение N12 V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. pl181.c 400 static void pl181_write(void *opaque, hwaddr offset,
uint64_t value, unsigned size) { .... if (s->cmd & PL181_CMD_ENABLE) { if (s->cmd & PL181_CMD_INTERRUPT) { .... } if (s->cmd & PL181_CMD_PENDING) { // <= else if .... } else { .... } .... } .... } В этом коде, судя по форматированию, прямо напрашивается использование else if вместо if. Возможно, здесь забыли дописать else. Тогда вариант исправления может быть такой: } else if (s->cmd & PL181_CMD_PENDING) { // <= else if
Однако есть вероятность, что с этим кодом все в порядке, и тут некорректное форматирование текста программы, которое сбивает с толку. Тогда код мог бы выглядеть так: if (s->cmd & PL181_CMD_INTERRUPT) {
.... } if (s->cmd & PL181_CMD_PENDING) { // <= if .... } else { .... } Предупреждение N13 V773 The function was exited without releasing the 'rule' pointer. A memory leak is possible. blkdebug.c 218 static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
{ .... struct BlkdebugRule *rule; .... rule = g_malloc0(sizeof(*rule)); // <= 1 .... if (local_error) { error_propagate(errp, local_error); return -1; // <= 2 } .... /* Add the rule */ QLIST_INSERT_HEAD(&s->rules[event], rule, next); // <= 3 .... } В данном коде выделяется объект rule (комментарий 1) и добавляется в список для последующего использования (комментарий 3), но в случае ошибки происходит возврат из функции без удаления ранее созданного объекта rule (комментарий 2). Здесь надо просто правильно обработать ошибку: удалить ранее созданный объект, иначе будет утечка памяти. Предупреждение N14 V781 The value of the 'ix' index is checked after it was used. Perhaps there is a mistake in program logic. uri.c 2110 char *uri_resolve_relative(const char *uri, const char *base)
{ .... ix = pos; if ((ref->path[ix] == '/') && (ix > 0)) { .... } Здесь анализатор обнаружил потенциальный выход за границу массива. Сначала читается элемент массива ref->path по индексу ix, а потом ix проверяется на корректность (ix > 0). Правильным решением тут будет поменять эти действия местами: if ((ix > 0) && (ref->path[ix] == '/')) {
Таких мест нашлось несколько:
Предупреждение N15 V784 The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. cadence_gem.c 1486 typedef struct CadenceGEMState {
.... uint32_t regs_ro[CADENCE_GEM_MAXREG]; } .... static void gem_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) { .... val &= ~(s->regs_ro[offset]); .... } В этом коде выполняется побитовая операция с объектами разных типов. Левый операнд — это аргумент val, имеющий 64-битный беззнаковый тип. В качестве правого операнда выступает полученное значение элемента массива s->regs_ro по индексу offset, имеющий 32-битный беззнаковый тип. Результат операции в правой части (~(s->regs_ro[offset])) является 32-битным беззнаковым типом, и перед побитовым умножением он расширится до 64-битного типа нулями, то есть после вычисления всего выражения обнулятся все старшие биты переменной val. Такие места всегда выглядят подозрительными. Тут можно только порекомендовать разработчикам еще раз пересмотреть этот код. Еще похожее:
Предупреждение N16 V1046 Unsafe usage of the 'bool' and 'unsigned int' types together in the operation '&='. helper.c 10821 static inline uint32_t extract32(uint32_t value, int start, int length);
.... static ARMVAParameters aa32_va_parameters(CPUARMState *env, uint32_t va, ARMMMUIdx mmu_idx) { .... bool epd, hpd; .... hpd &= extract32(tcr, 6, 1); } В этом фрагменте кода происходит операция побитового И над переменной hpd, имеющей тип bool, и результатом выполнения функции extract32, имеющим тип uint32_t. Так как битовое значение булевой переменной может быть только 0 или 1, то результат выражения будет всегда false, если младший бит, возвращаемый функцийе extract32, равен нулю. Давайте рассмотрим это на примере. Предположим, что значение hpd равно true, а функция вернула значение 2, то есть в двоичном представлении операция будет выглядеть так 01 & 10 = 0, а результат выражения будет равен false. Скорее всего, программист хотел выставлять значение true, если функция возвращает что-то отличное от нуля. По всей видимости, надо исправить код так, чтобы результат выполнения функции приводился к типу bool, например, так: hpd = hpd && (bool)extract32(tcr, 6, 1);
Заключение Как видно, анализатор нашел много подозрительных мест. Возможно, найденные потенциальные проблемы пока никак себя не проявляют, но их наличие не может не тревожить, так как они способны выстрелить в самый неожиданный момент. Просмотреть все подозрительные места заранее и исправить — лучше, чем чинить бесконечный поток багов. Очевидно, что для сложных проектов, подобных этому, статический анализ может принести ощутимую пользу, особенно если организовать регулярную проверку проекта. Если Вы хотите попробовать PVS-Studio для своего проекта, вы можете скачать анализатор и получить бесплатный триальный ключ на этой странице.  Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Evgeniy Ovsannikov. Checking QEMU using PVS-Studio. =========== Источник: habr.com =========== Похожие новости:
Блог компании PVS-Studio ), #_c, #_c++, #_open_source, #_programmirovanie ( Программирование ) |
|
Вы не можете начинать темы
Вы не можете отвечать на сообщения
Вы не можете редактировать свои сообщения
Вы не можете удалять свои сообщения
Вы не можете голосовать в опросах
Вы не можете прикреплять файлы к сообщениям
Вы не можете скачивать файлы
Вы не можете отвечать на сообщения
Вы не можете редактировать свои сообщения
Вы не можете удалять свои сообщения
Вы не можете голосовать в опросах
Вы не можете прикреплять файлы к сообщениям
Вы не можете скачивать файлы
Текущее время: 22-Ноя 07:59
Часовой пояс: UTC + 5