Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

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

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

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

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

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

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


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

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

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

21 Июл 2022

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

0970_hacker_comment_ru/image1.png

Про наш статический анализатор написали небольшую обзорную статью в журнале Хакер: "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-е:

0970_hacker_comment_ru/image2.png

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

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

Популярные статьи по теме
Почему в С++ массивы нужно удалять через delete[]

Дата: 27 Июл 2022

Автор: Михаил Гельвих

Заметка рассчитана на начинающих C++ программистов, которым стало интересно, почему везде твердят, что нужно использовать delete[] для массивов, но вместо внятного объяснения – просто прикрываются ма…
Предупреждения помогают писать лаконичный код

Дата: 19 Июл 2022

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

Некоторые предупреждения анализатора или компилятора сложно однозначно классифицировать как ложное срабатывание или указание на настоящую ошибку. Бывает, что формально анализатор/компилятор прав, но …
Заводим трактор: QMake -> CMake

Дата: 18 Июл 2022

Автор: Гость

Заводим трактор: переезжаем с QMake на CMake. По дороге заглянем на улицу "Кросс компиляторщиков", в сквер "Систем сборки" и посидим в баре "Управления зависимостями". Заодно увидим тех, кто использу…
Межмодульный анализ C и C++ проектов в деталях. Часть 2

Дата: 14 Июл 2022

Автор: Олег Лысый

В первой части статьи мы рассматривали основы теории компиляции C и C++ проектов, в частности особое внимание уделили алгоритмам компоновки и оптимизациям. Во второй части мы погрузимся глубже и пока…
Статический анализатор кода vs разработчики. Шо, опять?

Дата: 11 Июл 2022

Автор: Алексей Саркисов

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

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

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