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

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"\.

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

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\]](https://xakep.ru/2022/07/13/pvs-studio-test/)"\. The following code fragment caught my eye:

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

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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/terms/6461/) and didn't take a good look at the code\. Let's examine the infinite loop again:

```cpp
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](https://pvs-studio.com/en/blog/posts/0908/)"\)\.

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](https://github.com/veracrypt/VeraCrypt)\. Here is the [function](https://github.com/veracrypt/VeraCrypt/blob/08716954031eb0329fbd1ad2cd3fa593d3a046ab/src/Common/Dlgcode.c#L14093) that contains the code fragment we discussed:

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

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:

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

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

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

So, we simplified the code by 12 lines\. By the way, the "[How warnings simplify your code](https://pvs-studio.com/en/blog/posts/cpp/0968/)" 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?

1. [Appreciate Static Code Analysis\!](https://pvs-studio.com/en/blog/posts/cpp/0535/)
1. [Once again the PVS\-Studio analyzer has proved to be more attentive than a person](https://pvs-studio.com/en/blog/posts/cpp/0582/)\.
1. [How PVS\-Studio Proved to Be More Attentive Than Three and a Half Programmers](https://pvs-studio.com/en/blog/posts/cpp/0587/)\.
1. [One day in the life of PVS\-Studio developer, or how I debugged diagnostic that surpassed three programmers](https://pvs-studio.com/en/blog/posts/cpp/0842/)\.