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

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

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

21.10.2015 13:25

SvyatoslavMC 31 37065 Источник

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

Введение

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

Рисунок 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.

Рисунок 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 стал значительно чище с точки зрения нашего анализатора, теперь разработчики могут ещё улучшить свой код.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analyzing Wine: One Year Later.

Прочитали статью и есть вопрос?

Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.

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

Mrrl
21.10.2015 16:51
#8622407
Type casting operation is utilized 2 times in succession.

А как он отнесётся к такому коду?
```
int x;
...
y=(long long)(unsigned int)x;
```
1. Andrey2008
  21.10.2015 16:54
  #8622415
  +1
  Никак. Здесь нет повода для паники. Понятно, что потерян знак. Но раз так написано, видимо программист знал, что делал. Иначе к чему бы эти явные приведения типа.

khim
21.10.2015 18:05
#8622539
Кстати раз уж вы об этом заговорили… может в одной из статей приведёте хотя бы примерную статистику: сколько из «ошибок» отловленных PVS-Studio являются реальными ошибками, а сколько — просто «придирками»?

Люди часто не очень любят пользоваться статическими анализаторами не потому, что они ничего не отлавливают, а потому, что они производят много «мусора», который потом нужно отфильтровывать.

Вы много говорите о том, что PVS-Studio старается всякие «странности», которые ошибками не являются, пропускать, но, тем не менее: сколько в сухом остатке остаётся-то?
1. SvyatoslavMC
  21.10.2015 18:26
  #8622563
  +8
  По этому критерию нельзя дать статистики (по крайне мне и скорее всего коллегам тоже). Как вы поняли из статьи, автор проекта сам написал нам о проделанной работе и даже составил табличку, в остальных случаях мы такой отдачи не получаем от проверки проекта, следовательно некому просто сделать вывод или составить статистику.
  
  Нашей примерной статистикой являются примеры в статьях. Те примеры, в которых с некой вероятностью мы находим возможные ошибки в чужом коде.
  
  На примере этого проекта вы можете наблюдать, что я выписал мало примеров, а команда Wine делает сотни исправлений.
1. CodeRush
  21.10.2015 19:07
  #8622601
  +11
  Приведу свою статистику, которую набрал, проверяя разные реализации UEFI, до которых есть или был доступ: на 1000 предупреждений около 50 — самые настоящие ошибки, причем некоторые классы предупреждений таковы, что практически сколько их — столько и ошибок (оператор запятая в условии, к примеру).
  Остальное либо хитрые отладочные макросы, которые раскрываются в (BOOLEAN)(1 == 0) в релизном билде, либо преобразование типов у левого выражения вместо правого (наркомания, но работает), либо преобразования с потерей знака, либо сдвиг отрицательных чисел (UB, но работает), и тому подобное.
  Да, нужно фильтровать вывод, но после того, как он отфильтрован, жить становится значительно легче, особенно если гонять анализатор не раз в год, а на каждом релизном билде (но на такое счастье мне пока денег не дают).
  1. khim
    22.10.2015 00:13
    #8622825
    +1
    Спасибо. Опасения в целом, в общем, подтвердились. 95% «мусора» — это очень много, это означает, что внедрение PVS-Studio — это очень большой проект сам по себе, а главное — он требует внимания со стороны более-менее всех разработчиков, очень сложно отделаться отдельной «командой», которая будет просматривать логи и «тормошить» остальных.
    
    То есть понятно, что для статического анализатора это нормально (предупреждения от GCC часто тоже «пальцем в небо» попадают, если код никогда под GCC не собирался), но в сравнении со всякими фаззерами, где статистика, в целом, обратная (любое сообщение об ошибке — с вероятностью 95% это ошибка… а с вероятностью 5% — тоже ошибка, просто не там, где она задиагностирвована)… в общем понятно почему не все на это подписываются…
    
    Andrey2008
    22.10.2015 06:12
    #8623067
    +6
    Не всё так плохо. Помимо идеального пути внедрения (когда садимся и правим все предупреждения), есть обходной манёвр. PVS-Studio позволяет пометить в специальной базе все имеющиеся на данный момент предупреждения как ложные. И тогда для начала можно работать только с предупреждениями, относящимися к новому коду. Подробности: "Как внедрить статический анализ в проект, в котором более 10 мегабайт исходного кода?".
    
    CodeRush
    22.10.2015 09:19
    #8623129
    Добавлю еще, что даже если сесть и исправить все у себя, завтра от IBV снова придет обновление с неисправленным, поэтому исправить у себя мало — нужно сообщить об ошибке «наверх», а это порой занимает больше времени, чем собственно разгребание предупреждений и исправление ошибок. К счастью, опыт показывает, что на сообщения об ошибках, найденных статическим анализатором, IBV реагируют быстро и правильно.

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

Эта функция меняет состояние программы — потому что делает InterlockedIncrement счетчику ссылок:
```
static ULONG WINAPI DSCF_AddRef(IClassFactory *iface)
{
    IClassFactoryImpl *This = impl_from_IClassFactory(iface);
    return InterlockedIncrement(&This->ref);
}
```
Возвращаемое же из нее значение — исключительно отладочное. Так принято в COM.

UPD: блин, не заметил верхних строчек примера кода…
1. mayorovp
  21.10.2015 21:00
  #8622711
  +2
  Разобрался. Там фабрика классов никогда не выгружается — а потому счетчик ссылок для нее отключен. Операция AddRef ничего не делает, но ее вызов ошибкой не является, поскольку находится на том самом месте, где и должен быть согласно спецификации COM.

vladon
22.10.2015 11:57
#8623329
Тут попробовал триал, он ругается на подобное (код условный):
```
class C 
{
public:
     void setField(SomeClass value) { field_ = value; }
private:
     SomeClass field_;
}
```
Не помню точно код ошибки, ругается на тип параметра value, что-то вроде «Better to use constant reference».

При copy-elision (все три основных компилятора делают это) вовсе не better, готов даже поспорить. И это не только моё мнение (про pass-by-value, если мы не просто читаем параметр).
1. mayorovp
  22.10.2015 12:12
  #8623379
  Как минимум, передача по ссылке будет не медленнее копирования, даже при успешном copy-elision.
  1. vladon
    22.10.2015 12:42
    #8623447
    При передаче по const SomeClass & вы не сможете вызвать неконстантные методы переданного объекта, например:
    
    void SetInternalString(const std::string & s) { internal_string_ = s.append(" addition"); // oops }
    
    il--ya
    27.10.2015 16:58
    #8629907
    В вашем примере нет вызовов неконстантных методов, просто присвоение, вот он и выдаёт рекомендацию — применить более эффективный способ передачи и более restrictive qualifier. Если вам в будущем понадобится использовать неконстантные методы, всегда можно поменять на неконстантный тип без нарушения совместимости кода.
    
    vladon
    27.10.2015 17:00
    #8629909
    Согласен, пример такой, но при присвоении лучше передавать. Например, в мультипоточном приложении референс может стать dangling.
    
    Или чтобы принципиально исключить передачу в метод ссылки на локальную переменную или приватный член другого класса.
    
    Есть такое видео от STL — Don't help the compiler, там всё объясняется.
1. EvgeniyRyzhkov
  22.10.2015 12:18
  #8623403
  +1
  Надо заметить, что эта диагностика относится к группе микрооптимизаций, поэтому не является супер-рекомендуемой.
  1. vladon
    22.10.2015 17:08
    #8623897
    Кстати, тогда можно было бы добавить такие оптимизации как:
    
    — Если не меняет ничего в объекте и не вызывает неконстантных функций -> «Этот метод можно объявить константным»
    — Если кроме этого вообще не пытается получить доступ к членам класса -> «Этот метод можно сделать статичным»
    
    EvgeniyRyzhkov
    22.10.2015 17:12
    #8623911
    Диагностики микрооптимизаций странная вещь. С одной стороны вроде и не ошибка, чтобы прям бросаться править в коде. С другой — людям нравится похоже.
    
    vladon
    22.10.2015 17:13
    #8623917
    А вы позиционируйте (прежде всего перед собой) свой статический анализатор не только как «инструмент для поиска ошибок», но и как «инструмент для оптимизации кода», новые просторы, новые покупашки, почему нет.

leggiermente
22.10.2015 13:28
#8623525
Скажите пожалуйста, заинтересована ли команда PVS в анализе кода издательских систем на основе LaTeX? Используете ли сами LaTeX?
1. EvgeniyRyzhkov
  22.10.2015 13:30
  #8623531
  Не используем (хотя конечно знаем что это крутая штука). Проверить что-то может и стоило бы. Подкинете конкретный проект? Желательно, чтобы под Windows собирался.
  1. leggiermente
    22.10.2015 13:50
    #8623573
    Прошу прощения, пошёл смотреть проекты, и только сейчас понял, что LaTeX написан Лэмпортом на TeX, раньше был свято уверен, что на C. Такие дела.
    
    EvgeniyRyzhkov
    22.10.2015 14:03
    #8623605
    Ого… Сам думал, что на C.
    
    Mrrl
    22.10.2015 14:10
    #8623615
    Значит, надо анализировать исходники TeX'а. Интересно, открыты ли они.
    
    leggiermente
    22.10.2015 14:16
    #8623627
    Кнут писал TeX на WEB'е: www.ctan.org/tex-archive/systems/knuth/dist/tex
    
    khim
    22.10.2015 18:15
    #8624023
    +1
    Ну его тоже можно в C перегнать CWEBом и проверить. Всё-таки речь идёт о продукте, каждая ошибка в котором денег стоит (когда-то Кнут выписывал реальные чеки, которые можно было реально обменять на деньги в банке, но это мало кто делал: на аукционе за них можно больше получить, чем на них написано).

il--ya
27.10.2015 17:46
#8629999
Какие-то директивы для анализатора можно оставлять в коде, чтобы заблокировать определённые сообщения на уровне файла, блока, строки?
1. Andrey2008
  27.10.2015 17:55
  #8630023
  Вот хорошая статья о работе с ложными срабатываниями: habrahabr.ru/company/pvs-studio/blog/263695 Там нет кажется про подавление предупреждения в файле, но это можно легко сделать из контекстного меню (правый клик мышки на сообщении).
  1. il--ya
    27.10.2015 18:07
    #8630039
    Уже сам нашёл про подавление предупреждений: http://www.viva64.com/ru/d/0021/
    Там описано как можно это сделать в файле (под заголовком «Полное отключение предупреждений»).
  1. Andrey2008
    27.10.2015 19:02
    #8630125
    P.S. Про подавления предупреждений в файле (через меню) я написал неверно. Так отключаются все предупреждения для файла/папки. Если следует подавить конкретное предупреждение в файле, то как уже отметили, нужно использовать специальный комментарий.
1. Andrey2008
  27.10.2015 17:58
  #8630029
  Плюс документация:
  www.viva64.com/ru/d/0021
  www.viva64.com/ru/d/0345