Анализируя исходные коды различных программ, невольно возникает ассоциация подверженности их той или иной болезни. Часто сами собой выделяются паттерны ошибочного кода, встречающиеся в различных файлах проекта. В одних программах это ошибки Copy-Paste, в других, проверки вида unsigned_integer < 0. У каждого проекта своя болезнь. Для очередного проверенного проекта MAME, таким больным местом оказалась функция memset().
MAME - эмулятор, разработанный для воссоздания электронного оборудования аркадных автоматов в виде программного обеспечения, с целью сохранения истории игр и предотвращения исчезновения старых игр [1]. Хотя почти все файлы в проекте имеют расширение ".c", на самом деле NAME является проектом на языке Си++. Объем исходного кода достаточно велик и составляет 110 мегабайт.
Проверка MAME с помощью PVS-Studio ранее была невозможна, так как под Windows он собирается с помощью MinGW. MinGW представляет собой нативный программный порт GNU Compiler Collection (GCC) под Microsoft Windows [2]. Это значит, что PVS-Studio должен корректно поддерживать особенности синтаксиса GCC и специальные ключевые слова.
Поддержка MinGW появилась в PVS-Studio, начиная с версии 4.70. Поддержка пока не полная, но достаточная для проверки большинства проектов. Одним из первых проверенных проектов и стал MAME.
Примечание. В процессе анализа, встречается множество однотипных ложных срабатываний. Подозрительный код находится в нескольких макросах, широко используемых в различных частях проекта. В первый момент, кажется, что ничего кроме ложных срабатываний нет. В них тонут отдельные полезные сообщения. Однако ситуацию легко исправить, вписав всего несколько комментариев, для подавления предупреждений в макросах. Как это сделать, можно узнать в разделе документации "Подавление ложных предупреждений".
Теперь рассмотрим найденные ошибки.
Как уже говорилось, в проекте MAME можно встретить большое количество мест, где некорректно используется функция memset. Типичной ошибкой является заполнение только части массива. Рассмотрим простой пример:
UINT32 m_pstars_regs[16];
static DRIVER_INIT( pstar )
{
...
memset(state->m_pstars_regs, 0, 16);
...
}PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_pstars_regs'. pgm.c 4458
Число 16 означает количество элементов в массиве "m_pstars_regs". Однако в функцию memset следует передавать количество заполняемых в буфере байт. В результате, только часть массива будет заполнена нулями.
Корректный вариант кода:
memset(state->m_pstars_regs, 0, 16 * sizeof(UINT32));Ошибка тривиальна. Программисты часто думают, что тривиальных ошибок в программах мало (см. второй миф [3]). Это не так. Как раз большинство ошибок в программах являются простыми и глупыми.
Как вы считаете, приведенная выше ошибка единична? Нет. Вот как минимум ещё 8 мест, где можно встретить ошибки идентичного типа:
В рассмотренном выше примере, количество элементов задавалось конкретным числом. Это плохо. Лучше не использовать константы, а вычислять размер массива. К сожалению, это не помогает от рассматриваемого типа ошибки.
UINT16 m_control_0[8];
#define ARRAY_LENGTH(x) (sizeof(x) / sizeof(x[0]))
static MACHINE_RESET( tumbleb )
{
...
memset(state->m_control_0, 0,
ARRAY_LENGTH(state->m_control_0));
}PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_control_0'. tumbleb.c 2065
Количество элементов в массиве вычисляется с помощью макроса ARRAY_LENGTH. Это опять неправильно. Здесь нужно было вычислить размер массива, а не количество элементов в нем.
Возможно два способа исправления.
Первый:
memset(state->m_control_0, 0, sizeof(state->m_control_0));Второй:
memset(state->m_control_0, 0,
ARRAY_LENGTH(state->m_control_0) * sizeof(UINT16));Вот ещё несколько мест, где подобным образом неудачно заполняются массивы:
На этом злоключения с функцией memset() заканчиваются. Хотя возможно, я приспустил некоторые ошибки. Но ещё есть не менее страшная функция memcpy().
Рассмотрим способ, как выйти за границы массива:
#define CHD_SHA1_BYTES 20
#define CHD_V4_HEADER_SIZE 108
#define CHD_MAX_HEADER_SIZE CHD_V4_HEADER_SIZE
static chd_error header_read(...., chd_header *header)
{
UINT8 rawheader[CHD_MAX_HEADER_SIZE];
...
memcpy(header->parentsha1, &rawheader[100], CHD_SHA1_BYTES);
...
}PVS-Studio: V512 A call of the 'memcpy' function will lead to the '& rawheader[100]' buffer becoming out of range. chd.c 1870
Массив 'rawheader' состоит из 108 байт. Мы хотим скопировать его содержимое, начиная с байта под номером 100. Беда в том, что при этом мы выйдем за границы массива. Можно скопировать только 8 байт. А копируется 20 байт. К сожалению, я не знаю, как исправить этот код, так как не знаком с логикой программы.
При использовании функции memset() часто заполняется только часть массива. Соответственно, при использовании функции memcpy() нередко можно наблюдать ошибку, из-за которой копируется только часть массива. Рассмотрим пример:
UINT16 m_spriteram16[0x1000];
UINT16 m_spriteram16_buffered[0x1000];
static WRITE32_HANDLER( deco32_buffer_spriteram_w )
{
deco32_state *state =
space->machine().driver_data<deco32_state>();
memcpy(state->m_spriteram16_buffered,
state->m_spriteram16, 0x1000);
}PVS-Studio: V512 A call of the 'memcpy' function will lead to underflow of the buffer 'state->m_spriteram16_buffered'. deco32.c 706
Маленькая функция. Однако в ней есть ошибка. Я думаю, вы уже догадались, что здесь забыто умножение на sizeof(UINT16).
Корректный вариант:
memcpy(state->m_spriteram16_buffered,
state->m_spriteram16,
0x1000 * sizeof(UINT16));Аналогичная ошибка:
V512 A call of the 'memcpy' function will lead to underflow of the buffer 'state->m_spriteram16_2_buffered'. deco32.c 726
В любом проекте можно встретить опечатки и ошибки, возникшие из-за использования технологии Copy-Paste. Где-то больше таких ошибок, где-то меньше. В MAME их нашлось не много, но всё-таки они есть. Рассмотрим некоторые примеры.
static WRITE8_HANDLER( tms70x0_pf_w )
{
...
if( ((cpustate->pf[0x03] & 0x80) == 0) &&
((data & 0x80) == 0x80 ) )
{
...
}
else if( ((data & 0x80) == 0x80 ) &&
((cpustate->pf[0x03] & 0x80) == 0) )
{
...
}
...
}PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 577, 584. tms7000.c 577
Если присмотреться, то можно заметить, что первое и второе условия идентичны. В них изменён порядок сравнений, но это никак не сказывается на результате.
Рассмотрим другой пример.
class device_debug
{
device_disasm_interface *m_disasm;
...
int min_opcode_bytes() const
{
return (m_disasm != NULL) ?
m_disasm->max_opcode_bytes() : 1;
}
int max_opcode_bytes() const
{
return (m_disasm != NULL) ?
m_disasm->max_opcode_bytes() : 1;
}
}PVS-Studio: V524 It is odd that the body of 'max_opcode_bytes' function is fully equivalent to the body of 'min_opcode_bytes' function (debugcpu.h, line 150). debugcpu.h 151
Функция max_opcode_bytes() идентична функции min_opcode_bytes(). Скорее всего это неправильно. Я думаю, функция min_opcode_bytes()должна была выглядеть так:
int min_opcode_bytes() const
{
return (m_disasm != NULL) ?
m_disasm->min_opcode_bytes() : 1;
}Некоторые другие фрагменты кода, скорее всего являющиеся опечатками:
Достаточно много предупреждений, выданных PVS-Studio, относится операциям сдвигов. Эти операции приводят к undefined behavior. Конечно, при использовании конкретных компиляторов, программа может вести себя правильно многие годы. Поэтому ошибки можно назвать потенциальными. Они могут проявить себя при смене платформы, компиляторов, ключей оптимизации. Подробнее рассуждения на эту тему приводятся в статье: "Не зная брода, не лезь в воду. Часть третья." [4].
Рассмотрим пару примеров, приводящих к неопределенному поведению. Пример первый:
#define ATARIRLE_PRIORITY_SHIFT 12
#define ATARIRLE_PRIORITY_MASK \
((~0 << ATARIRLE_PRIORITY_SHIFT) & 0xffff)PVS-Studio: V610 Undefined behavior. Check the shift operator '<<. The left operand '~0' is negative. atarig42.c 220
Любой фрагмент кода, использующий макрос ATARIRLE_PRIORITY_MASK приводит к неопределенному поведению. Нельзя сдвигать отрицательные числа. Этот макрос лучше переписать так:
#define ATARIRLE_PRIORITY_MASK \
((~(0u) << ATARIRLE_PRIORITY_SHIFT) & 0xffff)А вот другой, более длинный пример:
UINT32 m_color1_mask;
#define ARRAY_LENGTH(x) (sizeof(x) / sizeof(x[0]))
PALETTE_INIT( montecar )
{
static const UINT8 colortable_source[] =
{
0x00, 0x00, 0x00, 0x01, 0x00, 0x02, 0x00, 0x03,
0x03, 0x03, 0x03, 0x02, 0x03, 0x01, 0x03, 0x00,
0x00, 0x00, 0x02, 0x00, 0x02, 0x01, 0x02, 0x02,
0x00, 0x10, 0x20, 0x30, 0x00, 0x04, 0x08, 0x0c,
0x00, 0x44, 0x48, 0x4c, 0x00, 0x84, 0x88, 0x8c,
0x00, 0xc4, 0xc8, 0xcc
};
...
for (i = 0; i < ARRAY_LENGTH(colortable_source); i++)
{
UINT8 color = colortable_source[i];
if (color == 1)
state->m_color1_mask |= 1 << i;
...
}
...
}PVS-Studio: V610 Undefined behavior. Check the shift operator '<<. The right operand ('i' = [0..43]) is greater than or equal to the length in bits of the promoted left operand. firetrk.c 111
В массиве 'colortable_source' находится 44 элемента. Соответственно, счетчик цикла 'i' принимает значения от 0 до 43. Число '1' имеет тип int. Его нельзя сдвигать более чем на 31 бит. Сдвиг на большее количество, согласно стандарту, приводит к Undefined behavior.
Так как предупреждения, связанных со сдвигами крайне много, нет смысла приводить их в статье. Список этих сообщений, можно посмотреть в текстовом файле: mame-shift-ub.txt.
Помимо memset() и memcpy(), я чуть не забыл про memcmp(). Эта функция из той же банды. К счастью в проекте MAME я обнаружил только одну ошибку, связанную с её использованием.
static const char *apr_magic = "ACT Apricot disk image\x1a\x04";
FLOPPY_IDENTIFY( apridisk_identify )
{
UINT8 header[APR_HEADER_SIZE];
floppy_image_read(floppy, &header, 0, sizeof(header));
if (memcmp(header, apr_magic, sizeof(apr_magic)) == 0)
...
}PVS-Studio: V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. apridisk.c 128
Оператор sizeof() вычисляет не количество байт в строке, а размер указателя. В результате, будет сравниваться только несколько первых байт. Ситуацию можно исправить, объявив переменную 'apr_magic' как массив:
static const char apr_magic[] = "ACT Apricot disk image\x1a\x04";Пример выражения, которое всегда истинно:
int m_led_extender;
#define CARD_A 1
#define NO_EXTENDER 0
static WRITE8_DEVICE_HANDLER( pia_ic5_porta_w )
{
...
else if ((state->m_led_extender != CARD_A)||
(state->m_led_extender != NO_EXTENDER))
...
}PVS-Studio: V547 Expression is always true. Probably the '&&' operator should be used here. mpu4.c 934
Условие "X != 1 || X != 0" всегда истинно. Скорее всего, вместо оператора '||' здесь следует написать '&&'.
Использование указателя до его проверки. Приведу только один пример. Я видел и другие сообщения V595, но не стал их выписывать. Часто код вполне работоспособен, так как указатель в этом коде никогда не равен нулю. Пример подозрительного кода:
static void stv_vdp2_drawgfxzoom(...,
const gfx_element *gfx, ...)
{
...
if (gfx->pen_usage &&
transparency == STV_TRANSPARENCY_PEN)
{
...
}
if( gfx )
{
...
}
...
}PVS-Studio: V595 The 'gfx' pointer was utilized before it was verified against nullptr. Check lines: 2457, 2483. stvvdp2.c 2457
Попадается странный код, по который я не могу с уверенностью сказать, есть в нём ошибка или нет. Возможно, в нём имеется ошибка Copy-Paste. А возможно, всё верно и две ветви кода действительно должны совпадать. Пример:
static DEVICE_START( deco16ic )
{
...
if (intf->split)
deco16ic->pf2_tilemap_16x16 =
tilemap_create_device(device, get_pf2_tile_info,
deco16_scan_rows, 16, 16, fullwidth ?
64 : 32, fullheight ? 64 : 32);
else
deco16ic->pf2_tilemap_16x16 =
tilemap_create_device(device, get_pf2_tile_info,
deco16_scan_rows, 16, 16, fullwidth ?
64 : 32, fullheight ? 64 : 32);
...
}PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. deco16ic.c 943
В независимости от условия выполняется одно и то же действие. А вот ещё один аналогичный пример:
int compute_res_net(int inputs, int channel, const res_net_info *di)
{
...
if (OpenCol)
{
rTotal += 1.0 / di->rgb[channel].R[i];
v += vOL / di->rgb[channel].R[i];
}
else
{
rTotal += 1.0 / di->rgb[channel].R[i];
v += vOL / di->rgb[channel].R[i];
}
...
}PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. resnet.c 628
Как всегда отмечу, что в статье, скорее всего, описаны далеко не все ошибки, которые можно найти в MAME с помощью PVS-Studio. Задача статьи – показать, что PVS-Studio учится проверять кроссплатформенные проекты. Как именно интегрироваться в make-файл, можно узнать из документации. Также вы можете обратиться к нам, если у вас возникнут затруднения с проверкой проектов, собираемых с помощью MinGW.
P.S. Просмотр результатов анализа сейчас подразумевает наличие среды Visual Studio, где можно открыть и изучить отчёт. Анализировать отчёт вручную затруднительно. Возможно, в дальнейшем появится специальный инструмент, позволяющий удобно просматривать отчёт и осуществлять навигацию по коду, не имея установленного Visual Studio.