# Конкурс внимательности: PVS\-Studio vs Хакер

Время от времени мы пишем статьи в духе "статический анализатор внимательнее C\+\+ программиста"\. Сегодня мы продолжим эту традицию, разве что заменив "программист" на "хакер"\.

![0970_hacker_comment_ru/image1.png](https://import.viva64.com/docx/blog/0970_hacker_comment_ru/image1.png)

Про наш статический анализатор написали небольшую обзорную статью в журнале Хакер: "[PVS\-Studio\. Тестируем статический анализатор кода на реальном проекте](https://xakep.ru/2022/07/13/pvs-studio-test/)"\. Моё внимание привлёк разбор следующего фрагмента кода:

```cpp
BOOL bNewDesktopSet = FALSE;

// wait for SwitchDesktop to succeed before using it for current thread
while (true)
{
  if (SwitchDesktop (pParam->hDesk))
  {
    bNewDesktopSet = TRUE;
    break;
  }
  Sleep (SECUREDESKTOP_MONOTIR_PERIOD);
}

if (bNewDesktopSet)
{
  SetThreadDesktop (pParam->hDesk);
```

Автор статьи посчитал, что анализатор ошибся, выдав здесь предупреждение\. Процитирую соответствующие два абзаца из статьи:

> Анализатор ругается на строку _if \(bNewDesktopSet\)_ со следующим вердиктом: [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'bNewDesktopSet' is always true\. Dlgcode\.c 14113\.
>
> Давай разбираться: _bNewDesktopSet_ инициализируется как _FALSE_ при объявлении, далее входим в цикл, в котором переключение _bNewDesktopSet_ на _TRUE_ возможно только в том случае, если сработает WinAPI _SwitchDesktop_\. Де\-юре анализатор прав, но прав ли он по сути? Во\-первых, мы не можем быть уверены, произойдёт ли событие _SwitchDesktop\(pParam\-\>hDesk\)_, потому что за поведение WinAPI мы не отвечаем\. Во\-вторых, взгляни на архитектуру кода: выполнение тела _if_ отдано на откуп поведению функции WinAPI _SwitchDesktop_, которая или сработает \(будет переход\), или образует вечный цикл, потому как _while \(true\)_\. На мой взгляд, ошибки "Expression \.\.\. is always true" в таком случае быть не должно\.

Автор публикации поспешил классифицировать сообщение анализатора как [ложноположительное срабатывание](https://pvs-studio.ru/ru/blog/terms/6461/), не разобравшись в коде\. Давайте ещё раз внимательно взглянем на вечный цикл:

```cpp
while (true)
{
  if (SwitchDesktop (pParam->hDesk))
  {
    bNewDesktopSet = TRUE;
    break;
  }
  Sleep (SECUREDESKTOP_MONOTIR_PERIOD);
}

if (bNewDesktopSet)  // <= V547
```

Код ниже цикла может начать выполняться в одном единственном случае – сработает оператор _break_\. Обратите внимание, что вызов оператора _break_ всегда сопровождается присваиванием переменной _bNewDesktopSet_ значения _TRUE_\.

Поэтому если цикл прекратил своё выполнение, то переменная _bNewDesktopSet_ однозначно будет равна _TRUE_\. Анализатор это понимает, основываясь на анализе потока данных \(см\. "[Технологии статического анализа кода PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/0908/)"\)\.

Автор рассуждает о том, сработает или нет условие _SwitchDesktop\(pParam\-\>hDesk\)_\. Но эти рассуждения не имеют значения\. Если не сработает – цикл не закончится\. Если сработает – то выполнится присваивание _bNewDesktopSet \= TRUE_\. Поэтому анализатор абсолютно прав, выдавая предупреждение\.

Анализатором найдена настоящая ошибка или просто избыточный код?

Для этого обратимся к первоисточнику\. В статье не говорится, какой проект анализировался, но, немного погуглив, легко понять, что это [VeraCrypt](https://github.com/veracrypt/VeraCrypt)\. Вот [функция](https://github.com/veracrypt/VeraCrypt/blob/08716954031eb0329fbd1ad2cd3fa593d3a046ab/src/Common/Dlgcode.c#L14093), содержащая рассмотренный нами фрагмент кода:

```cpp
static DWORD WINAPI SecureDesktopThread(LPVOID lpThreadParameter)
{
  volatile BOOL bStopMonitoring = FALSE;
  HANDLE hMonitoringThread = NULL;
  unsigned int monitoringThreadID = 0;
  SecureDesktopThreadParam* pParam =
    (SecureDesktopThreadParam*) lpThreadParameter;
  SecureDesktopMonitoringThreadParam monitorParam;
  HDESK hOriginalDesk = GetThreadDesktop (GetCurrentThreadId ());
  BOOL bNewDesktopSet = FALSE;

  // wait for SwitchDesktop to succeed before using it for current thread
  while (true)
  {
    if (SwitchDesktop (pParam->hDesk))
    {
      bNewDesktopSet = TRUE;
      break;
    }
    Sleep (SECUREDESKTOP_MONOTIR_PERIOD);
  }

  if (bNewDesktopSet)
  {
    SetThreadDesktop (pParam->hDesk);

    // create the thread that will ensure that VeraCrypt secure desktop
    // has always user input
    monitorParam.szVCDesktopName = pParam->szDesktopName;
    monitorParam.hVcDesktop = pParam->hDesk;
    monitorParam.pbStopMonitoring = &bStopMonitoring;
    hMonitoringThread =
      (HANDLE) _beginthreadex (NULL, 0, SecureDesktopMonitoringThread,
                               (LPVOID) &monitorParam, 0, &monitoringThreadID);
  }

  pParam->retValue = DialogBoxParamW (pParam->hInstance, pParam->lpTemplateName,
            NULL, pParam->lpDialogFunc, pParam->dwInitParam);

  if (hMonitoringThread)
  {
    bStopMonitoring = TRUE;

    WaitForSingleObject (hMonitoringThread, INFINITE);
    CloseHandle (hMonitoringThread);
  }

  if (bNewDesktopSet)
  {
    SetThreadDesktop (hOriginalDesk);
    SwitchDesktop (hOriginalDesk);
  }

  return 0;
}
```

Переменная _bNewDesktopSet_ используется в двух условиях\. Поскольку я не знаком с проектом, мне сложно сказать, нашли мы настоящую ошибку или нет\. Но одно могу сказать точно: код очень подозрителен\.

Возможно, цикл должен быть невечным и прерываться по истечению какого\-то времени\. Тогда перед нами недописанный некорректный код\. Т\.е\. перед нами задумка, которую не довели до конца\.

Другой вариант – код подвергался постепенным правкам и в какой\-то момент стал избыточным, но этого не заметили\. В этом случае функцию можно упростить, удалив часть бессмысленных проверок:

```cpp
static DWORD WINAPI SecureDesktopThread(LPVOID lpThreadParameter)
{
  volatile BOOL bStopMonitoring = FALSE;
  HANDLE hMonitoringThread = NULL;
  unsigned int monitoringThreadID = 0;
  SecureDesktopThreadParam* pParam =
    (SecureDesktopThreadParam*) lpThreadParameter;
  SecureDesktopMonitoringThreadParam monitorParam;
  HDESK hOriginalDesk = GetThreadDesktop (GetCurrentThreadId ());

  // wait for SwitchDesktop to succeed before using it for current thread
  while (!SwitchDesktop (pParam->hDesk))
  {
    Sleep (SECUREDESKTOP_MONOTIR_PERIOD);
  }

  SetThreadDesktop (pParam->hDesk);

  // create the thread that will ensure that VeraCrypt secure desktop
  // has always user input
  monitorParam.szVCDesktopName = pParam->szDesktopName;
  monitorParam.hVcDesktop = pParam->hDesk;
  monitorParam.pbStopMonitoring = &bStopMonitoring;
  hMonitoringThread =
    (HANDLE) _beginthreadex (NULL, 0, SecureDesktopMonitoringThread,
                             (LPVOID) &monitorParam, 0, &monitoringThreadID);

  pParam->retValue = DialogBoxParamW (pParam->hInstance, pParam->lpTemplateName,
            NULL, pParam->lpDialogFunc, pParam->dwInitParam);

  if (hMonitoringThread)
  {
    bStopMonitoring = TRUE;

    WaitForSingleObject (hMonitoringThread, INFINITE);
    CloseHandle (hMonitoringThread);
  }

  SetThreadDesktop (hOriginalDesk);
  SwitchDesktop (hOriginalDesk);

  return 0;
}
```

Возможно, чуть нагляднее изменения будут видны на diff\-е:

![0970_hacker_comment_ru/image3.png](https://import.viva64.com/docx/blog/0970_hacker_comment_ru/image3.png)

Минус 12 строк\. Это, кстати, пересекается со статьёй "[Предупреждения помогают писать лаконичный код](https://pvs-studio.ru/ru/blog/posts/cpp/0968/)"\. Неплохое сокращение и упрощение функции, если, конечно, это не ошибка, и строк наоборот должно быть больше :\)\.

Спасибо за внимание и приглашаю познакомиться с аналогичными заметками:

1. [Любите статический анализ кода\!](https://pvs-studio.ru/ru/blog/posts/cpp/0535/)
1. [В очередной раз анализатор PVS\-Studio оказался внимательнее человека](https://pvs-studio.ru/ru/blog/posts/cpp/0582/)\.
1. [Как PVS\-Studio оказался внимательнее, чем три с половиной программиста](https://pvs-studio.ru/ru/blog/posts/cpp/0587/)\.
1. [Один день из жизни разработчика PVS\-Studio, или отладка диагностики, которая оказалась внимательнее трёх программистов](https://pvs-studio.ru/ru/blog/posts/cpp/0842/)\.