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

21 Июл 2022

Автор: Андрей Карпов

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

Про наш статический анализатор написали небольшую обзорную статью в журнале Хакер: "PVS-Studio. Тестируем статический анализатор кода на реальном проекте". Моё внимание привлёк разбор следующего фрагмента кода:

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 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" в таком случае быть не должно.

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

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

if (bNewDesktopSet)  // <= V547

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

Поэтому если цикл прекратил своё выполнение, то переменная bNewDesktopSet однозначно будет равна TRUE. Анализатор это понимает, основываясь на анализе потока данных (см. "Технологии статического анализа кода PVS-Studio").

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

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

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

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

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

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

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-е:

Минус 12 строк. Это, кстати, пересекается со статьёй "Предупреждения помогают писать лаконичный код". Неплохое сокращение и упрощение функции, если, конечно, это не ошибка, и строк наоборот должно быть больше :).

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

#Cpp