Debugging bugs in the x64dbg debugger. Without a debugger

No program development process goes without debugging. Modern IDEs provide the programmer with a built-in debugger. However, there are situations when using an IDE for debugging is redundant or impossible, and then standalone debuggers come to the rescue, one of which is x64dbg.

Introduction

x64dbg — is an open source debugger for 32-bit and 64-bit Windows that offers an “intuitive and familiar, albeit updated” user interface. It looks like OllyDbgbut with a tweaked design and expanded functionality.

This debugger has no stable releases. Instead, there are approximately once a month Snapshot assemblies are published. Development is underway in parallel cross-platform version.

In what situation might you need a standalone debugger? For example, Visual Studio 2022 builds executables with Windows Vista support by default, but you won't be able to install it on Windows Vista and use remote debugging because unsupported operating system — minimum Windows 7 required.

The main scenario for using a standalone debugger is still studying executable files, to the source code and debugging symbols of which there is no access. Antivirus labs determine the algorithm of malware operation in order to describe it in their knowledge base, and then add its definition to antivirus databases. Various communities for the revival of old network games or programs do not take their hands off the debugger to recreate their server side. And not only old ones – even for relatively recent network games there were server emulators. But how well is the reverse engineer's Swiss Army knife made? It's time to launch the PVS-Studio analyzer.

Installing PVS-Studio

You can download PVS-Studio by going to this pageTo analyze the project, you will need a license, a trial version of which can be obtained Here. The PVS-Studio installation procedure will not cause any difficulties: each step is accompanied by an easy-to-understand explanation, and if difficulties arise, the quick start help will always help under Windows. Two additional IDE integrations are required: for Qt Creator (the installer will unpack it into the PVS-Studio folder) and Visual Studio. The analyzer version currently in effect at the time of writing is 7.31.

Assembly and Analysis Configuration

The debugger consists of two components: the core and the graphical interface. The first component is built in Visual Studio, the second in Qt Creator. The project wiki has assembly instructionsand the author strongly recommends using specific versions of dependencies. In this part of the article, we will look at the debugger core. The check is performed based on the commit f518e50 branches development.

Analysis of the errors found

Open the solution file x64dbg.sln and first of all we remove third-party libraries from the check, so as not to drown in the noise of unrelated x64dbg code triggerings. These libraries are listed in the filter Third Party project x64dbg_dbg in the header files section:

\dbghelp\
\DeviceNameResolver\
\jansson\
\LLVMDemangle\
\lz4\
\msdia\
\ntdll\
\TitanEngine\
\WinInet-Downloader\
\XEDParse\

You can exclude folders from analysis using the Visual Studio plugin settings: Extensions > PVS-Studio > Options > Don't Check Files. Alternative ways to manage the list of excluded paths are described in our documentation.

Get your insecticides ready, let's start etching bugs from the debugger! Without a debugger, as promised.

No time to explain, rewrite!

V570 The 'mLastChar' variable is assigned to itself. lexer.cpp 149

class Lexer
{
....
private:
  ....
  int mLastChar=" ";
  ....
....
}

Lexer::Token Lexer::getToken()
{
  ....
  //character literal
  if(mLastChar == '\'')
  {
    std::string charLit;
    while(true)
    {
      ....
      if(mLastChar == '\\') //escape sequence
      {
        nextChar();
        if(mLastChar == EOF)
          return reportError("unexpected end of file in character literal (2)");
        if(mLastChar == '\r' || mLastChar == '\n')
          return reportError("unexpected newline in character literal (2)");
        if(   mLastChar == '\'' || mLastChar == '\"'
           || mLastChar == '?' || mLastChar == '\\')
          mLastChar = mLastChar;                                   // <=
        else if(mLastChar == 'a')
          mLastChar="\a";
        ....
      }
      ....
    }
    ....
  }
}

This function is used to escape certain characters: line feeds, backslashes, quotes, and other service characters. Variable mLastChar is a member of the class Lexer and contains the last literal read. Function nextChar reads the next character and writes it to mLastChar. If the value mLastChar updated by its call, then why assign it the same thing again? Similar triggering:

But what if?

V547 Expression '!expr' is always true. parser.cpp 118

uptr<Expr> Parser::ParseExpr()
{
  return nullptr;
}

uptr<Return> Parser::ParseReturn()
{
  if(CurToken.Token == Lexer::tok_return)
  {
    NextToken();
    auto expr = ParseExpr();
    if(!expr)                              // <=
    {
      ReportError("failed to parse Return (ParseExpr failed)");
      return nullptr;
    }
    return make_uptr<Return>(move(expr));
  }
  return nullptr;
}

What are the chances that something will appear out of nothing? Is the medieval superstition true that mice spontaneously appear from dirty laundry? It has been scientifically proven that this is impossible. As well as obtaining other data from nullptr.

Just in case, I will also provide the definition of the class here uptrwhich is actually ordinary std::unique_ptr:

template<class T>
using uptr = unique_ptr<T>;

What if not?

V560 A part of conditional expression is always false: !haveCurrValue. watch.cpp 61

....
currValue = val;
haveCurrValue = true;
if(getType() != WATCHVARTYPE::TYPE_INVALID)
{
  switch(getWatchdogMode())
  {
  ....
  case WATCHDOGMODE::MODE_CHANGED:
    if(currValue != origVal || !haveCurrValue)         // <=
    {
      duint cip = GetContextDataEx(hActiveThread, UE_CIP);
      dprintf(....);
      watchdogTriggered = 1;
    }
    break;
  case WATCHDOGMODE::MODE_UNCHANGED:
    if(currValue == origVal || !haveCurrValue)         // <=
    {
      duint cip = GetContextDataEx(hActiveThread, UE_CIP);
      dprintf(....);
      watchdogTriggered = 1;
    }
    break;
  }
}
return val;
....

At a minimum, we have a case of copy-paste-oriented programming, at a maximum, the creation of a variable that is never modified haveCurrValue before the end of the switch-case block.

Similar operation:

Big ambitions for a small lambda

V783 Dereferencing of the invalid iterator might take place. LinearPass.cpp 130

void LinearPass::AnalyseOverlaps()
{
  ....
  // Erase blocks marked for deletion
  m_MainBlocks.erase(std::remove_if(
    m_MainBlocks.begin(), m_MainBlocks.end(), [](BasicBlock & Elem)
    {
      return Elem.GetFlag(BASIC_BLOCK_FLAG_DELETE);
    }));
  ....
}

To remove data from an array, developers often use erase-remove idiom. Its essence is as follows: the elements of the array selected by the function std::removewill be moved to the end, and the function itself will return the iterator from which the function std::erase will start erasing them. The author intended this call to delete all elements that have a deletion flag, as evidenced by the comment above the call m_MainBlocks.erase.

This code has two problems:

  1. If the array does not contain any element with the flag set, then the function std::remove will return an iterator to end()which cannot be passed to a function std::erase! This is due to the fact that the iterator end() although it is valid, it cannot be dereferenced.

  2. Overload std::erase with one parameter not only does it not accept end() iterator, and it also deletes only one value! And this directly contradicts the comment above. If x64dbg was written using the C++20 specifications, it would be possible to use std::erase_ifto avoid such an incident. But at the time of Visual C++ 2013, the C++11 specification was still new.

Let's try to fix both problems at once. To do this, we'll use the second overload of the function std::erasewhich takes as input the position from which to start erasing elements, and the final element after which the erasing will be completed.

m_MainBlocks.erase(std::remove_if(
  m_MainBlocks.begin(), m_MainBlocks.end(), [](BasicBlock & Elem)
  {
      return Elem.GetFlag(BASIC_BLOCK_FLAG_DELETE);
  }), m_MainBlocks.end());

Thus, we fixed the deletion of only one element, and at the same time protected ourselves from an empty list to delete. The documentation clearly states that in this case nothing will be executed.

A minute of entertaining mathematics

V560 A part of conditional expression is always true: addr < _base + _size. cmd-undocumented.cpp 382

bool cbInstrVisualize(int argc, char* argv[])
{
  if(IsArgumentsLessThan(argc, 3))
    return false;
  duint start;
  duint maxaddr;
  ....
  {
    ....
    //initialize
    Zydis zydis;
    duint _base = start;
    duint _size = maxaddr - start;
    Memory<unsigned char*> _data(_size);
    MemRead(_base, _data(), _size);
    for(duint addr = start, fardest = 0; addr < maxaddr;)
    {
      ....
      //continue algorithm
      const unsigned char* curData =
        (addr >= _base && addr < _base + _size)         // <=
          ? _data() + (addr - _base)
          : nullptr;
      if(zydis.Disassemble(addr, curData, MAX_DISASM_BUFFER))
      {
        if(addr + zydis.Size() > maxaddr)
          break; //we went past the maximum allowed address
        ....
      }
      ....
    }
    ....
  }
  ....
}

Variable _base a value was assigned to a variable start. This means that the check for the maximum address is already in the loop condition (addr < maxaddr) and does not make sense in the ternary operator when initializing a variable curData. It is clear that nothing is clear. Watch your hands:

  • Cycle condition: addr < maxaddr

  • Ternary operator condition: addr < _base + _size

  • Variable value _size: maxaddr-start

  • Variable _base assigned a value to a variable start.

To obtain maxaddryou need to start add _sizeUsing simple mathematics, we obtain an expression of the following type:

maxaddr = start + _size

Thus, it becomes clear that the loop condition addr < maxaddr identical to the condition addr < _base + _size inside the cycle. It turns out to be the same, but in a more “complete” notation.

“Copied correctly”

V1053 Calling the 'AddRef' virtual function in the constructor may lead to unexpected result at runtime. pdbdiafile.cpp 23

//Taken from: https://msdn.microsoft.com/en-us/library/ms752876(v=vs.85).aspx
class FileStream : public IStream
{
  FileStream(HANDLE hFile)
  {
    AddRef();                         // <=
    _hFile = hFile;
  }

....
public:
  virtual ULONG STDMETHODCALLTYPE AddRef(void)
  {
    return (ULONG)InterlockedIncrement(&_refcount);
  }
....
}

I see the phrase “taken from”. I look at the code sample from Microsoft Learn. I see instead of calling the function AddRef simple use of variable _refcount:

class FileStream : public IStream  
{
  FileStream(HANDLE hFile)   
  {
    _refcount = 1;  
    _hFile = hFile;  
  }
....
}

What are the risks of using virtual functions in constructors or destructors? violation of the logic of their call in the inherited class. In general, yes, “copied correctly”, but it was possible to call the function right away InterlockedIncrement with reference to _refcount? It's even systemic!

Code Bloat: A Beginner's Guide

V547 Expression '!bRedirectSupported' is always true. x64dbg_launcher.cpp 76

static BOOL isWowRedirectionSupported()
{
  BOOL bRedirectSupported = FALSE;

  _Wow64DisableRedirection = (LPFN_Wow64DisableWow64FsRedirection)
    GetProcAddress(GetModuleHandle(TEXT("kernel32")),
                   "Wow64DisableWow64FsRedirection");
  _Wow64RevertRedirection = (LPFN_Wow64RevertWow64FsRedirection)
    GetProcAddress(GetModuleHandle(TEXT("kernel32")),
                   "Wow64RevertWow64FsRedirection");

  if(!_Wow64DisableRedirection || !_Wow64RevertRedirection)
    return bRedirectSupported;
  else
    return !bRedirectSupported;     // <=
}

At first glance, the diagnostics seem completely illogical. I stared at these lines for a long time, I even had to go get eye drops. If I had seen before memory abuse string variables, then now we have to see the dentist's victim with a slightly unconventional approach to removing a bad tooth. But, putting aside the frightening comparative phrases, this is a great reason for refactoring! Static analyzer will serve you well in this case.

Function GetProcAddress returns the address of an exported function by its name or ordinal number from the module, or NULLif the requested function was not found. There is absolutely no need for another variable to return the result of the support check file system redirections for the subsystem WoW64. If we are missing one of the functions, then the default is returned. FALSE. Therefore, the entire function can be reduced to literally three operations:

static BOOL isWowRedirectionSupported()
{
  _Wow64DisableRedirection = (LPFN_Wow64DisableWow64FsRedirection)
    GetProcAddress(GetModuleHandle(TEXT("kernel32")),
                  "Wow64DisableWow64FsRedirection");
  _Wow64RevertRedirection = (LPFN_Wow64RevertWow64FsRedirection)
    GetProcAddress(GetModuleHandle(TEXT("kernel32")),
                  "Wow64RevertWow64FsRedirection");

  return !_Wow64DisableRedirection || !_Wow64RevertRedirection;
}

But then my eyes go up to the Microsoft Learn documentation for a description of the feature Wow64RevertWow64FsRedirection:

This function should not be called without a previous call to the Wow64DisableWow64FsRedirection function.

Any data allocation on behalf of the Wow64DisableWow64FsRedirection function is cleaned up by this function.

It turns out that one function cannot exist without the other, so the redirection support check is fundamentally wrong! Let's improve the revision and fix the return value in it:

return _Wow64DisableRedirection && _Wow64RevertRedirection;

Now everything is correct: if at least one function is missing, then redirection is not supported.

Macro with sliding surprise

V1003 The macro 'TITANGETDRX' is a dangerous expression. The parameter 'titantype' must be surrounded by parentheses. breakpoint.h 8

#define TITANGETDRX(titantype) UE_DR0 + ((titantype >> 8) & 0xF)

I didn't find any cases in the code where the parameter titantype represented any expression. Only variants with the transfer of one variable. However, if someone stumbles and forgets that it is necessary to do exactly this way, and passes a conditional mathematical expression to the macro, then the debugger may suddenly come to Dr. Watson or WER.

It's solved quickly: the parameter is simply wrapped in one pair of parentheses.

#define TITANGETDRX(titantype) UE_DR0 + (((titantype) >> 8) & 0xF)

Similar triggers:

  • V1003 The macro 'TITANGETTYPE' is a dangerous expression. The parameter 'titantype' must be surrounded by parentheses. breakpoint.h 11

  • V1003 The macro 'TITANGETSIZE' is a dangerous expression. The parameter 'titantype' must be surrounded by parentheses. breakpoint.h 13

A small incident of a big lambda

V595 The 'Info.headers' pointer was utilized before it was verified against nullptr. Check lines: 66, 483, 491. module.cpp 483

#define IMAGE_FIRST_SECTION( ntheader ) ((PIMAGE_SECTION_HEADER)        \
  ((ULONG_PTR)(ntheader) +                                            \
   FIELD_OFFSET( IMAGE_NT_HEADERS, OptionalHeader ) +                 \
   ((ntheader))->FileHeader.SizeOfOptionalHeader   \
  ))

ULONG64 ModRvaToOffset(ULONG64 base, PIMAGE_NT_HEADERS ntHeaders, ULONG64 rva)
{
  PIMAGE_SECTION_HEADER section = IMAGE_FIRST_SECTION(ntHeaders);
  ....
}

static void ReadDebugDirectory(MODINFO & Info, ULONG_PTR FileMapVA)
{
  ...
  const auto supported = [&Info, FileMapVA](PIMAGE_DEBUG_DIRECTORY entry)
  {
    // Check for valid RVA
    ULONG_PTR offset = 0;
    if(entry->AddressOfRawData)
      offset = Info.isVirtual
        ? entry->AddressOfRawData
        : (ULONG_PTR)ModRvaToOffset(0,
                                    Info.headers,                     // <=
                                    entry->AddressOfRawData);
    ....
  
    // Check size is sane and end of data lies within the image
    if(entry->SizeOfData < sizeof(CV_INFO_PDB20) /*smallest supported type*/
      || entry->AddressOfRawData
       + entry->SizeOfData > HEADER_FIELD(Info.headers, SizeOfImage)) 
      return false;  
    ....
    return false;
  };
  ....
}

Multiple triggering on Info.headers due to the lack of a null pointer check. First of all, the macro IMAGE_FIRST_SECTIONwhich is involved in the function ModRvaToOffsetwill encounter a null pointer dereference. An old friend — Undefined Behavior — with us again. The macro does not contain a check for a null pointer, the optional header of the PE image section is immediately obtained from the NT header. Never neglect data checking!

Read datasheets less

V560 A part of conditional expression is always true: * memorySize <= 512. The value range of unsigned char type: [0, 255]. TraceRecord.cpp 239

//See https://www.felixcloutier.com/x86/FXSAVE.html, max 512 bytes
#define memoryContentSize 512

static void HandleZydisOperand(
                 const Zydis & zydis, int opindex,
                 DISASM_ARGTYPE* argType, duint* value,
                 unsigned char memoryContent[memoryContentSize],
                 unsigned char* memorySize)
{
  ....
  case ZYDIS_OPERAND_TYPE_MEMORY:
  {
    *argType = arg_memory;
    const auto & mem = op.mem;
    if(mem.segment == ArchValue(ZYDIS_REGISTER_FS, ZYDIS_REGISTER_GS))
    {
      *value += ThreadGetLocalBase(ThreadGetId(hActiveThread));
    }
    *memorySize = op.size / 8;
    if(*memorySize <= memoryContentSize && DbgMemIsValidReadPtr(*value))  // <=
    {
      MemRead(*value, memoryContent, max(op.size / 8, sizeof(duint)));
    }
  }
  break;
  ....
}

Again, code where the debugger developer left a comment in the form of a link to documentation. This time, to the instructions FXSAVE x86 architecture. This instruction saves the state of the math coprocessor, as well as the MMX, XMM, and MXCSR registers, to a 512-byte memory area. A colleague questioned the documentation from the Internet, considering it unreliable, which led to a loud argument that lasted for several minutes. In the process, I remembered that I had seen this table somewhere before…

With a sly grin on my face, I walk up to the bookshelf and slowly pull out the second volume of the “IA-32 Architecture Software Developer's Guide”, published by Intel back in 2002! The book describes every supported instruction of the then still fresh and hot Pentium 4 processor. The heavy and thick book lands on my desk with a loud bang, and I open it to the page where I had previously left a bookmark. In front of my opponent, in all its glory, appears the same table as in electronic form. This is the end of the argument. I can only suggest removing the redundant size check. unsigned char cannot take a value greater than 255 – it is too short to reach all the data in the instruction FXSAVE.

....
*memorySize = op.size / 8;
if(DbgMemIsValidReadPtr(*value))
{
  MemRead(*value, memoryContent, max(op.size / 8, sizeof(duint)));
}
....

Size matters

V1048 The 'titsize' variable was assigned the same value. cmd-breakpoint-control.cpp 427

bool cbDebugSetHardwareBreakpoint(int argc, char* argv[])
{
  ....
  DWORD titsize = UE_HARDWARE_SIZE_1;
  if(argc > 3)
  {
    duint size;
    if(!valfromstring(argv[3], &size))
      return false;
    switch(size)
    {
    case 1:
      titsize = UE_HARDWARE_SIZE_1;         // <=
      break;
    case 2:
      titsize = UE_HARDWARE_SIZE_2;
      break;
    case 4:
      titsize = UE_HARDWARE_SIZE_4;
      break;
#ifdef _WIN64
    case 8:
      titsize = UE_HARDWARE_SIZE_8;
      break;
#endif // _WIN64
    default:
      titsize = UE_HARDWARE_SIZE_1;        // <=
      dputs(QT_TRANSLATE_NOOP("DBG", "Invalid size, using 1"));
      break;
    }
    ....
  }
  ....
}

Usually, numeric variables are initialized with a zero value. No one forbids doing this with a specific value. In addition, no one forbids overwriting a variable with the same value during program execution, but why… For persuasiveness or insurance?

Still a safety net?

V1037 Two or more case-branches perform the same actions. Check lines: 42, 45 commandparser.cpp 42

Command::Command(const String & command)
{
  ParseState state = Default;
  int len = (int)command.length();
  for(int i = 0; i < len; i++)
  {
    char ch = command[i];
    switch(state)
    {
    ....
    case Escaped:
      switch(ch)
      {
      case '\t':
      case ' ':
        dataAppend(' ');
        break;
      case ',':
        dataAppend(ch);         // <=
        break;
      case '\"':
        dataAppend(ch);         // <=
        break;
      default:
        dataAppend('\\');
        dataAppend(ch);
        break;
      }
      state = Default;
      break;
    ....
    }
  }
}

I got the impression that x64dbg was written either on a “just in case” basis or “it'll do”. If we start from the first version, it is quite logical that the entire project is filled with identical operations and repeated assignments. If from the second, then for a tool used in a high-responsibility environment this is an unacceptable absent-mindedness.

This block of code contains both fallthrough and duplicate parts. For tabs and spaces, fallthroughwhich means the author is clearly confident that both symbols will easily add a space to the data. What's wrong with the comma or double quote, why couldn't it be done fallthrough and for this pair of symbols? The same action is performed: the value of the variable is added chand not any other symbol. If we apply it here fallthroughas for space and tab, there will be no disaster:

switch(ch)
{
case '\t':
case ' ':
  dataAppend(' ');
  break;
case ',':
case '\"':
  dataAppend(ch);       // <=
  break;
default:
  dataAppend('\\');
  dataAppend(ch);
  break;
}

Looks like a savings matches bytes, but in fact, in front of the eyes of the person reading the code and the processor time of his brain to understand what is happening.

Beware of (non)modernism!

V1109 The 'InitCommonControls' function is deprecated. Consider switching to an equivalent newer function. x64dbg_launcher.cpp 426

int APIENTRY WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance,
                     LPSTR lpCmdLine, int nShowCmd)
{
  InitCommonControls();
  ....
}

This function is deprecated for a reason. First, it clutters the import table. Second, there are application manifestssupport for which was introduced in Windows XP! At first, they could be used to enable support Windows themes in the application windows and select specific versions of libraries, and with the release of Windows Vista, install privilege level applications. Just a second, Visual Studio 2013 is used for assembly, where you can even embed them! This is not such a new feature that you can neglect it – the functionality of integrating the manifest directly into the project appeared in Visual Studio 2005. I had to blow the dust off this IDE to check if my memory had failed me.

Including a manifest in Windows applications or libraries has long been a standard development activity that is done once and never returned to. Without a manifest, by the way, some features will not work properly.

V1109 The 'PathRemoveFileSpecW' function is deprecated. Consider switching to an equivalent newer function. x64dbg_launcher.cpp 114

static HRESULT AddDesktopShortcut(TCHAR* szPathOfFile,
                                  const TCHAR* szNameOfLink)
{
  HRESULT hRes = NULL;

  //Get the working directory
  TCHAR pathFile[MAX_PATH + 1];
  _tcscpy_s(pathFile, szPathOfFile);
  PathRemoveFileSpec(pathFile);
  ....
}

There are still a few legacy calls, such as PathRemoveFileSpecW via macro. This is a function to remove the trailing backslash from the path. It is recommended to use PathCchRemoveFileSpecbut this feature is only available with Windows 8. Since the application is deliberately written with Windows XP support, such a trigger can be suppressif you need to work with legacy functions:

PathRemoveFileSpec(pathFile); //-V1109 //-VH"2078475722"

Note that we have used a new mechanism – hash suppression. It allows you to automatically remove all False Alarm marks if the code in the line has changed. Thanks to this, you can provide protection against replacing one obsolete function with another. That is, manually expanding the macro to one of the versions of the function (PathRemoveFileSpecW for Unicode, PathRemoveFileSpecA for ANSI) or replacing it with another function will break the hash-to-string correspondence, and the analyzer will again report a diagnostic problem V1109 in this line if it occurs.

Similar triggers:

Results

There is an opinion that to fix one soldering iron, you need a second soldering iron. Technically, the same statement applies to a debugger. How to debug a debugger? Perhaps you should try to look for bugs in it using a static analyzer. For example, using PVS-Studio? x64dbg also monitors you, records the time spent on it, so appreciate this limited resource!

The story doesn't end there: we still have to make a “step with an exit” to the GUI. What surprises does the Qt binding have in store for us? This is in the second part. Stay with us, don't switch the processor context!

If you would like to share this article with an English-speaking audience, please use the translation link: Taras Shevchenko. Debugging bugs in x64dbg debugger. No debugger.

Similar Posts

Leave a Reply

Your email address will not be published. Required fields are marked *