metrica
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо
to the top
close form

Заполните форму в два простых шага ниже:

Ваши контактные данные:

Шаг 1
Поздравляем! У вас есть промокод!

Тип желаемой лицензии:

Шаг 2
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности
close form
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS‑Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

Вебинар: Трудности при интеграции SAST, как с ними справляться - 04.04

>
>
>
Болезни программ: memset

Болезни программ: memset

23 Июл 2012

Анализируя исходные коды различных программ, невольно возникает ассоциация подверженности их той или иной болезни. Часто сами собой выделяются паттерны ошибочного кода, встречающиеся в различных файлах проекта. В одних программах это ошибки 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 мест, где можно встретить ошибки идентичного типа:

  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_kb_regs'. pgm.c 4975
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_kb_regs'. pgm.c 4996
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_kb_regs'. pgm.c 5056
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_oldsplus_ram'. pgm.c 5780
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_oldsplus_regs'. pgm.c 5781
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_sysreg'. rungun.c 399
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_ttl_vram'. rungun.c 400
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_playfield_code'. malzak.c 392

В рассмотренном выше примере, количество элементов задавалось конкретным числом. Это плохо. Лучше не использовать константы, а вычислять размер массива. К сожалению, это не помогает от рассматриваемого типа ошибки.

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));

Вот ещё несколько мест, где подобным образом неудачно заполняются массивы:

  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_pmac_read'. megadriv.c 7156
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_pmac_write'. megadriv.c 7157
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_cart_is_genesis'. megatech.c 426
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_vol_ctrl'. nycaptor.c 841
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_rotate_ctrl'. wgp.c 949
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'state->m_vreg'. othldrby.c 237

На этом злоключения с функцией memset() заканчиваются. Хотя возможно, я приспустил некоторые ошибки. Но ещё есть не менее страшная функция memcpy().

Неудачное использование функции 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

В любом проекте можно встретить опечатки и ошибки, возникшие из-за использования технологии 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;
}

Некоторые другие фрагменты кода, скорее всего являющиеся опечатками:

  • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: ",(%d,". 9900dasm.c 670
  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 549, 579. cdrom.c 549
  • V501 There are identical sub-expressions 'offset != (0x370 >> 1)' to the left and to the right of the '&&' operator. decoprot.c 118
  • V501 There are identical sub-expressions 'offset != (0x3c0 >> 1)' to the left and to the right of the '&&' operator. decoprot.c 118
  • V501 There are identical sub-expressions 'offset != 0x2c / 2' to the left and to the right of the '&&' operator. decoprot.c 240
  • V501 There are identical sub-expressions 'offset != 0xe' to the left and to the right of the '&&' operator. decoprot.c 447

Undefined behavior

Достаточно много предупреждений, выданных 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.

Дополнительные ресурсы

Популярные статьи по теме


Комментарии (0)

Следующие комментарии next comments
close comment form