to the top
close form
Для получения триального ключа
заполните форму ниже
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

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

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

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

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

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

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


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

>
>
>
Проверка Wine: Год спустя

Проверка Wine: Год спустя

21 Окт 2015

Чуть больше года назад для написания статьи о проверки проекта с помощью PVS-Studio был взят проект Wine. Статья написана, авторы были уведомлены и даже попросили полный отчёт проверки анализатором, на что получили положительный ответ. Недавно нам написал один из разработчиков проекта. В статье будет рассказано о нашем общении, проделанной работе команды разработчиков проекта Wine по улучшению кода и о том, что ещё предстоит сделать.

0352_Wine_ru/image1.png

Введение

Wine (Wine Is Not Emulator -- Wine - не эмулятор) - это набор программ, позволяющий пользователям Linux, Mac, FreeBSD, и Solaris запускать Windows-приложения без необходимости установки на компьютер самой Microsoft Windows. Wine является активно развивающимся кросс-платформенным свободным ПО, распространяемым под лицензией GNU Lesser General Public License.

В августе 2014 года была опубликована статья: Проверяем Wine с помощью PVS-Studio и Clang Static Analyzer. Недавно мы получили письмо от одного из разработчиков Wine - Michael Stefaniuc. В письме он поблагодарил команду PVS-Studio за использование статического анализатора и предоставление отчёта.

Также он привёл небольшую статистку по исправлению предупреждений анализатора за год. По этой ссылке можно найти около 180 коммитов, содержащих исправления исходного кода с пометкой "PVS-Studio".

На рисунке 1 представлена статистика исправления 20 самых полезных, с точки зрения авторов, типов предупреждений анализатора для проекта.

0352_Wine_ru/image2.png

Рисунок 1 - The top 20 successful error codes for Wine

Michael пояснил, что совмещать текущий исходный код со старым отчётом анализатора уже затруднительно и попросил проверить проект ещё раз. Проект Wine активно развивается, статический анализатор PVS-Studio тоже активно развивается, поэтому я снова решил проверить этот проект. Результатом стала эта небольшая заметка, где я опишу 10 самых подозрительных участков кода. Естественно разработчики получили полный отчет и смогут изучить и прочие потенциально опасные места.

Top 10 предупреждений

Предупреждение V650

V650 Type casting operation is utilized 2 times in succession. Next, the '+' operation is executed. Probably meant: (T1)((T2)a + b). descriptor.c 967

WINE_HIDP_PREPARSED_DATA* build_PreparseData(....)
{
  ....
  wine_report =
    (WINE_HID_REPORT*)((BYTE*)wine_report)+wine_report->dwSize;
  ....
}

Анализатор обнаружил операцию сложения с переменной, с которой выполняется двойное приведение типа. Скорее всего здесь забыли заключить первое приведение типа и операцию сложения в скобки. Выше по коду есть точно такой же фрагмент, только со скобками:

wine_report =
  (WINE_HID_REPORT*)(((BYTE*)wine_report)+wine_report->dwSize);

Предупреждение V590

V590 Consider inspecting the 'lret == 0 || lret != 234' expression. The expression is excessive or contains a misprint. winemenubuilder.c 3430

static void cleanup_menus(void)
{
  ...
  while (1)
  {
    ....
    lret = RegEnumValueW(....);
    if (lret == ERROR_SUCCESS || lret != ERROR_MORE_DATA)
      break;
  ....
}

В коде имеется избыточное сравнение " lret == ERROR_SUCCESS". Видимо имеет место логическая ошибка. Условие истинно для всех значений переменной 'lret', неравных 'ERROR_MORE_DATA'. Для наглядности можно посмотреть на таблицу истинности на рисунке 2.

0352_Wine_ru/image3.png

Рисунок 2 - Таблица истинности условного выражения

Красным выделены два столбца, где результаты логических операций полностью совпадают.

Ещё одно такое место:

  • V590 Consider inspecting the 'last_error == 183 || last_error != 3' expression. The expression is excessive or contains a misprint. schedsvc.c 90

Предупреждение V576

V576 Incorrect format. Consider checking the fourth actual argument of the 'printf' function. To print the value of pointer the '%p' should be used. msvcirt.c 828

DEFINE_THISCALL_WRAPPER(streambuf_dbp, 4)
void __thiscall streambuf_dbp(streambuf *this)
{
  ....
  printf(" base()=%p, ebuf()=%p,  blen()=%d\n",
         this->base, this->ebuf, streambuf_blen(this));
  printf("pbase()=%p, pptr()=%p, epptr()=%d\n",
         this->pbase, this->pptr, this->epptr);
  printf("eback()=%p, gptr()=%p, egptr()=%d\n",
         this->eback, this->gptr, this->egptr);
  ....
}

Анализатор обнаружил подозрительное место, в котором значение указателя пытаются распечатать с помощью спецификатора '%d. Написание этого фрагмента кода с большой вероятностью было выполнено методом copy-paste. Можно предположить, что сначала был написал первый вызов функции printf(), последний аргумент в которой правильно соответствует используемому спецификатору '%d'. Но потом эту строчку скопировали ещё два раза и в качестве последнего аргумента стали передавать указатель, а формат строки поменять забыли.

Предупреждение V557

V557 Array overrun is possible. The '16' index is pointing beyond array bound. winaspi32.c 232

/* SCSI Miscellaneous Stuff */
#define SENSE_LEN      14

typedef struct tagSRB32_ExecSCSICmd {
  ....
  BYTE        SenseArea[SENSE_LEN+2];
} SRB_ExecSCSICmd, *PSRB_ExecSCSICmd;

static void
ASPI_PrintSenseArea(SRB_ExecSCSICmd *prb)
{
  BYTE  *rqbuf = prb->SenseArea;
  ....
  if (rqbuf[15]&0x8) {
    TRACE("Pointer at %d, bit %d\n",
          rqbuf[16]*256+rqbuf[17],rqbuf[15]&0x7);      // <=
  }
  ....
}

Анализатор обнаружил обращение к памяти за пределы массива 'rgbuf' к элементам с индексами 16 и 17. Сам массив содержит только 16 элементов. Возможно, условие "rqbuf[15]&0x8" редко является истинным и такую ошибку не заметили.

Предупреждение V711

V711 It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop. dplobby.c 765

static HRESULT WINAPI
IDirectPlayLobby3AImpl_EnumAddressTypes(....)
{
  ....
  FILETIME filetime;
  ....
  /* Traverse all the service providers we have available */
  for( dwIndex=0; RegEnumKeyExA( hkResult, dwIndex, subKeyName,
       &sizeOfSubKeyName,
       NULL, NULL, NULL, &filetime ) != ERROR_NO_MORE_ITEMS;
       ++dwIndex, sizeOfSubKeyName=50 )
  {
    ....
    FILETIME filetime;
    ....
    /* Traverse all the address type we have available */
      for( dwAtIndex=0; RegEnumKeyExA( hkServiceProviderAt,
           dwAtIndex, atSubKey, &sizeOfSubKeyName,
           NULL, NULL, NULL, &filetime ) != ERROR_NO_MORE_ITEMS;
        ++dwAtIndex, sizeOfSubKeyName=50 )
      {
        ....
      }
    ....
  }
  ....
}

В теле цикла обнаружено объявление переменной "filetime", совпадающей с переменной, используемой для контроля цикла. Это будет приводить к потере локальных изменений в "filename" при выходе из внутреннего цикла. Глядя на весь код функции можно предположить, что большой фрагмент кода был скопирован в тело цикла с незначительными изменениями. Это может и не нести серьёзной опасности, всё равно это является нехорошим стилем программирования.

Предупреждение V530

V530 The return value of function 'DSCF_AddRef' is required to be utilized. dsound_main.c 760

static ULONG WINAPI DSCF_AddRef(LPCLASSFACTORY iface)
{
    return 2;
}

HRESULT WINAPI DllGetClassObject(....)
{
  ....
  while (NULL != DSOUND_CF[i].rclsid) {
    if (IsEqualGUID(rclsid, DSOUND_CF[i].rclsid)) {
      DSCF_AddRef(&DSOUND_CF[i].IClassFactory_iface);  // <=
      *ppv = &DSOUND_CF[i];
      return S_OK;
    }
    i++;
  }
  ....
}

В коде найдена функция DSCF_AddRef(), возвращаемое значение которой не используется. Более того, эта функция не меняет какие-то состояния в программе. Это очень подозрительное место, которое необходимо проверить разработчикам.

Предупреждение V593

V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. user.c 3247

DWORD WINAPI FormatMessage16(....)
{
  ....
  int     ret;
  int     sz;
  LPSTR   b = HeapAlloc(..., sz = 100);

  argliststart=args+insertnr-1;

  /* CMF - This makes a BIG assumption about va_list */
  while ((ret = vsnprintf(....) < 0) || (ret >= sz)) {
      sz = (ret == -1 ? sz + 100 : ret + 1);
      b = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, b, sz);
  }
  ....
}

Приоритет логических операций выше приоритета операции присваивания. Таким образом, в этом выражении первым вычисляется выражение "vsnprintf(....) < 0", следовательно в переменную 'ret' будет сохранено не количество записанных символов, а значение 0 или 1. Выражение "ret >= sz" будет всегда ложным, поэтому цикл выполнится только если в 'ret' запишется единица. Такой сценарий будет возможен, если функция vsnprintf() выполнится с ошибкой и вернёт отрицательное значение.

Предупреждение V716

V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. ordinal.c 5198

#define E_INVALIDARG _HRESULT_TYPEDEF_(0x80070057)

BOOL WINAPI SHPropertyBag_ReadLONG(....)
{
    VARIANT var;
    HRESULT hr;
    TRACE("%p %s %p\n", ppb,debugstr_w(pszPropName),pValue);
    if (!pszPropName || !ppb || !pValue)
        return E_INVALIDARG;
    V_VT(&var) = VT_I4;
    hr = IPropertyBag_Read(ppb, pszPropName, &var, NULL);
    if (SUCCEEDED(hr))
    {
        if (V_VT(&var) == VT_I4)
            *pValue = V_I4(&var);
        else
            hr = DISP_E_BADVARTYPE;
    }
    return hr;
}

В проекте Wine много мест, где тип HRESULT преобразуют в BOOL или просто работают с переменной это типа как с булевым значением. Опасность заключается в том, что тип HRESULT устроен достаточно сложно и должен сигнализировать о том, прошла ли операция успешно, какой результат был возвращён после выполнения операции, в случае ошибки - где произошла ошибка, обстоятельства этой ошибки и так далее.

К счастью, разработчики активно исправляют такие места и в баг-трекере можно найти много соответствующих коммитов.

Предупреждение V523

V523 The 'then' statement is equivalent to the 'else' statement. resource.c 661

WORD WINAPI GetDialog32Size16( LPCVOID dialog32 )
{
  ....
  p = (const DWORD *)p + 1; /* x */
  p = (const DWORD *)p + 1; /* y */
  p = (const DWORD *)p + 1; /* cx */
  p = (const DWORD *)p + 1; /* cy */

  if (dialogEx)
      p = (const DWORD *)p + 1; /* ID */
  else
      p = (const DWORD *)p + 1; /* ID */
  ....
}

Анализатор обнаружил условие с одинаковыми блоками кода. Возможно, фрагмент кода просто скопировали и забыли изменить.

Предупреждение V519

V519 The 'res' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 5905, 5907. action.c 5907

static void test_publish_components(void)
{
  ....
  res = RegCreateKeyExA(....);
  res = RegSetValueExA(....);
  ok(res == ERROR_SUCCESS, "RegSetValueEx failed %d\n", res);
  RegCloseKey(key);
....
}

Тестирование должно обеспечивать надёжность приложения, а если ошибки допущены в тестах, то беда. В этом фрагменте кода забыли проверить результат одной функции и сразу перешли к получению и проверке значения другой функции.

Заключение

В ответ на просьбу о повторной проверке проекта, мы отправили свежий отчёт анализатора PVS-Studio и временный ключ продукта для удобного просмотра отчёта средствами плагина для Visual Studio или утилиты Standalone. За год код проекта Wine стал значительно чище с точки зрения нашего анализатора, теперь разработчики могут ещё улучшить свой код.

Популярные статьи по теме
Под капотом SAST: как инструменты анализа кода ищут дефекты безопасности

Дата: 26 Янв 2023

Автор: Сергей Васильев

Сегодня речь о том, как SAST-решения ищут дефекты безопасности. Расскажу, как разные подходы к поиску потенциальных уязвимостей дополняют друг друга, зачем нужен каждый из них и как теория ложится на…
Ложные представления программистов о неопределённом поведении

Дата: 17 Янв 2023

Автор: Гость

Неопределённое поведение (UB) – непростая концепция в языках программирования и компиляторах. Я слышал много заблуждений в том, что гарантирует компилятор при наличии UB. Это печально, но неудивитель…
Топ-10 ошибок в C++ проектах за 2022 год

Дата: 29 Дек 2022

Автор: Владислав Столяров

Дело идёт к Новому году, а значит, самое время традиционно вспомнить десять самых интересных срабатываний, которые нашёл PVS-Studio в 2022 году.
PVS-Studio и RPCS3: лучшие предупреждения в один клик

Дата: 12 Дек 2022

Автор: Александр Куренев

Best Warnings — режим анализатора, оставляющий в окне вывода 10 лучших предупреждений. Мы предлагаем вам ознакомиться с обновлённым режимом Best Warnings на примере проверки проекта RPCS3.
Holy C++

Дата: 23 Ноя 2022

Автор: Гость

В этой статье постараюсь затронуть все вещи, которые можно без зазрения совести выкинуть из С++, не потеряв ничего (кроме боли), уменьшить стандарт, нагрузку на создателей компиляторов, студентов, из…

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

Следующие комментарии next comments
close comment form
Unicorn with delicious cookie
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо