PVS-Studio vs Hacker: who's a better reviewer?

Jul 21 2022

Author: Andrey Karpov

Sometimes we publish articles about "a static analyzer that surpassed a C++ developer". And we carry on the tradition, but today we replace "developer" with "hacker".

A short review article about our static analyzer was published on the Hacker blog. Here it is — "PVS-Studio. Testing a static code analyzer on a real project [RU]". The following code fragment caught my eye:

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);

The author of the article thought that the analyzer issued a false positive here. Take a look at the quote from the article:

The if (bNewDesktopSet) line triggers the analyzer. And PVS-Studio issues the following warning: V547 Expression 'bNewDesktopSet' is always true. Dlgcode.c 14113.

Let's find out what we have here: bNewDesktopSet is declared and initialized as FALSE. Then we enter the loop where the bNewDesktopSet can be set to TRUE only if the WinAPI SwitchDesktop executes. The analyzer is technically right, but is it right in essence? First, we can't be sure if the SwitchDesktop(pParam->hDesk) event will be raised, because we are not responsible for WinAPI behavior. Second, look at the code structure: the if body's execution depends on what the WinAPI SwitchDesktop function returns. If it executes successfully, we enter the body of the if statement. Otherwise, the infinite loop does not terminate. My opinion is that the "Expression ... is always true" error should not be issued in this case.

The author rushed to consider the analyzer warning as a false positive and didn't take a good look at the code. Let's examine the infinite loop again:

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

if (bNewDesktopSet)  // <= V547

The code below the loop can execute only if the break statement executes. Notice that the call to the break statement is always complemented by assigning TRUE to the bNewDesktopSet variable.

Therefore, if the loop is terminated, the bNewDesktopSet variable will definitely be TRUE. The analyzer uses data flow analysis to detect this (see "PVS-Studio: static code analysis technology").

In the article published on the Hacker blog, the author discussed whether or not the SwitchDesktop(pParam->hDesk) condition will execute. But this discussion is not so important. If the condition is not true, the loop does not terminate. If the condition is true, the bNewDesktopSet = TRUE assignment executes. Therefore, when the analyzer issues a warning, it is absolutely right.

Did the analyzer detect a real error or just redundant code?

Let's look at the source code. The article does not mention the analyzed project, but after a little googling we can easily understand that it is VeraCrypt. Here is the function that contains the code fragment we discussed:

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;
}

The bNewDesktopSet variable is used in two conditions. Since I do not know the project, it's difficult to say whether we have detected a real error or not. But one thing is certain: the code is very suspicious.

Perhaps the loop should be non-infinite and stopped after a certain time. Then we can see the incorrect and incomplete code. In other words, the developer had an idea but did not implement it.

It may also be possible that the code went through some changes with time. It finally became redundant — but nobody noticed this. In this case, we can simplify the function and delete some pointless checks:

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;
}

Probably the diff will help you see the changes more clearly:

So, we simplified the code by 12 lines. By the way, the "How warnings simplify your code" article discusses a similar idea. It's better to make the function shorter and simpler. Unless it is not a mistake and there actually should be more lines :).

Thank you for attention! Why don't you read similar articles?

#Cpp