Для получения триального ключа
заполните форму ниже
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 строк. Это, кстати, пересекается со статьёй "Предупреждения помогают писать лаконичный код". Неплохое сокращение и упрощение функции, если, конечно, это не ошибка, и строк наоборот должно быть больше :).

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

Последние статьи:

Опрос:

Популярные статьи по теме
Holy C++

Дата: 23 Ноя 2022

Автор: Гость

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

Дата: 01 Ноя 2022

Автор: Гость

Прочитав эту статью, вы узнаете следующее: способы, которыми можно продлить время жизни временного объекта в С++; рекомендации и подводные камни этого механизма, с которыми может столкнуться С++ прог…
Как мы баг в PVS-Studio искали или 278 Гигабайтов логов

Дата: 28 Окт 2022

Автор: Григорий Семенчев, Сергей Ларин, Филипп Хандельянц

Предлагаем вашему вниманию интересную историю о поиске бага внутри анализатора PVS-Studio. Да, мы тоже допускаем ошибки, но мы готовы засучить рукава и залезть в самую глубину "кроличьей норы".
0, 1, 2, Фредди забрал Blender

Дата: 26 Окт 2022

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

Эта статья могла бы получить название "Как PVS-Studio защищает от поспешных правок кода, пример N7". Однако так именовать статьи становится скучновато. Поэтому сейчас вы узнаете, причём здесь Фредди …
Примеры ошибок, которые может обнаружить PVS-Studio в коде LLVM 15.0

Дата: 25 Окт 2022

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

Компиляторы развиваются и выдают всё больше предупреждений. Остаются ли преимущества от использования статических анализаторов кода, таких как PVS-Studio? Да, так как анализаторы тоже развиваются. Пе…

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

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