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

Восстанавливаем контекст

В первой части статьи про автономный отладчик x64dbg мы рассматривали его ядро, держа в одной руке документацию на процессоры Intel Pentium 4, в другой — на Windows API, а в третьей учебник математики. Получился неплохой суп из математического сопроцессора, поданный в лёгкой посуде из титана, но без второго блюда из графического интерфейса обед был бы неполным.

С момента публикации прошлой части прошло несколько месяцев, и x64dbg продолжали улучшать функционально, а также появилась задача на актуализацию средств разработки. Здесь же мы продолжим анализ на основе кода коммита f518e50 и, где возможно, проведём сравнение с актуальным на момент написания статьи коммитом 9785d1a.

Для сборки нам понадобится Qt 5.6.3, о чём уже было сказано в инструкции от разработчиков отладчика. Эту версию Qt можно без проблем использовать в актуальной версии Qt Creator 14.0.2, в которую можно установить плагин PVS-Studio и провести статический анализ кода.

Шагаем по интерфейсу

Испачкались в пробелке

Начнём.

Предупреждение PVS-Studio:

V781 The value of the 'post' index is checked after it was used. Perhaps there is a mistake in program logic. Utf8Ini.h 243

static inline std::string trim(const std::string & str)
{
    auto len = str.length();
    if(!len)
        return "";
    size_t pre = 0;
    while(str[pre] == ' ')
        pre++;
    size_t post = 0;
    while(str[len - post - 1] == ' ' && post < len)   // <=
        post++;
    auto sublen = len - post - pre;
    return sublen > 0 ? str.substr(pre, len - post - pre) : "";
}

Что будет, если вся строка состоит только из пробелов? Обрезка пробелов обрежет не только строку, но и программу под хоровое пение. Достаточно поменять местами проверки, и EXCEPTION_ACCESS_VIOLATION обойдёт вас стороной.

while(post < len && str[len - post - 1] == ' ')
    post++;

Идёт в комплекте с:

• V781 The value of the 'len' index is checked after it was used. Perhaps there is a mistake in program logic. Utf8Ini.h 243

Два по цене одного

Предупреждения анализатора:

V1048 The 'accept' variable was assigned the same value. HexDump.cpp 405

V547 Expression 'accept' is always true. HexDump.cpp 417

void HexDump::mouseMoveEvent(QMouseEvent* event)
{
    bool accept = true;

    int x = event->x();
    int y = event->y();

    if(mGuiState == HexDump::MultiRowsSelectionState)
    {
        //qDebug() << "State = MultiRowsSelectionState";

        if((transY(y) >= 0) && y <= this->height())
        {
            ....
            accept = true;               // <=
        }
        ....
    }

    if(accept)                           // <=
        AbstractTableView::mouseMoveEvent(event);
}

Здесь наблюдается интересная ситуация: работа с событиями перемещения мыши выполняется безусловно, поскольку переменная accept инициализирована со значением true, но не false. Предполагаю, что это опечатка, и разработчик изначально хотел инициализировать её со значением false.

Дополнительно:

• V1048 The 'mUpdateCountLabel' variable was assigned the same value. ReferenceView.cpp 166

Надеемся на понимание

Предупреждение PVS-Studio:

V614 Potentially uninitialized variable 'funcType' used. Consider checking the fourth actual argument of the 'paintFunctionGraphic' function. Disassembly.cpp 548

QString Disassembly::paintContent(QPainter* painter, duint row, duint col,
                                  int x, int y, int w, int h)
{
  ....
  switch(col)
  {
  ....
  case ColDisassembly: //draw disassembly (with colours needed)
  {
    int loopsize = 0;
    int depth = 0;

    while(1) //paint all loop depths
    {
      LOOPTYPE loopFirst = DbgGetLoopTypeAt(va, depth);
      LOOPTYPE loopLast =
        DbgGetLoopTypeAt(va + mInstBuffer.at(rowOffset).length - 1, depth);
      HANDLE_RANGE_TYPE(LOOP, loopFirst, loopLast);
      if(loopFirst == LOOP_NONE)
        break;
      Function_t funcType;              //  <=
      switch(loopFirst)
      {
      case LOOP_SINGLE:
        funcType = Function_single;
        break;
      case LOOP_BEGIN:
        funcType = Function_start;
        break;
      case LOOP_ENTRY:
        funcType = Function_loop_entry;
        break;
      case LOOP_MIDDLE:
        funcType = Function_middle;
        break;
      case LOOP_END:
        funcType = Function_end;
        break;
      default:
        break;
      }
      loopsize += paintFunctionGraphic(painter,
                                       x + loopsize, y,
                                       funcType,            // <=
                                       loopFirst != LOOP_SINGLE);
      depth++;
    }
    ....
}

С кем не бывает, забыли инициализировать переменную. Держим в голове, что MSVC считает использование неинициализированных переменных неопределённым поведением, поэтому сыграть в тетрис или чихнуть в колонки дозволительно.

Если это не так, то можно предположить, что переменная funcType должна была быть инициализирована со значением Function_none из перечисления Function_t, либо, как чуть выше, присвоить значение в блоке switch.

Скоропортящиеся продукты

Предупреждение PVS-Studio:

V1091 The 'addrText.toUtf8().constData()' pointer is cast to an integer type of a larger size. The result of this conversion is implementation-defined. Breakpoints.cpp 506

bool Breakpoints::editBP(BPXTYPE type, 
                         const QString & addrText,
                         QWidget* widget,
                         const QString & createCommand)
{
    BRIDGEBP bridgebp = {};
    bool found = false;
    if(type == bp_dll)
    {
        found = DbgFunctions()->GetBridgeBp(
            type,
            reinterpret_cast<duint>(addrText.toUtf8().constData()),    // <=
            &bridgebp
        );
    }
    else
    {
        found = DbgFunctions()->GetBridgeBp(
            type,
            (duint)addrText.toULongLong(nullptr, 16),
            &bridgebp
        );
    }
    ....
}

"Искал одно, нашёл другое" — как раз про случай на экране. Меня напрягло не то, что здесь неправильно произведена протяжка указателя с 32 до 64 бит, а то, что идёт работа с временным объектом QByteArray (создаётся из addrText.toUtf8()), жизненный цикл которого кончится по достижении reinterpret_cast. Из-за этого в функцию GetBridgeBp придёт или может прийти вместо строки с адресом точки останова что-то недействительное.

Ещё минутка занимательной математики

Предупреждение PVS-Studio:

V557 Array overrun is possible. The value of 'registerName - ZYDIS_REGISTER_XMM0' index could reach 47. TraceInfoBox.cpp 310

typedef enum ZydisRegister_
{
  ....
  ZYDIS_REGISTER_XMM0,  // 88
  ....
  ZYDIS_REGISTER_YMM0,  // 120
  ....
  ZYDIS_REGISTER_YMM15, // 135
}

#ifdef _WIN64
#define ArchValue(x32value, x64value) x64value
#else
#define ArchValue(x32value, x64value) x32value
#endif //_WIN64

void TraceInfoBox::update(unsigned long long selection,
              TraceFileReader* traceFile,
              const REGDUMP & registers)
{
  ....
  else if(   registerName >= ZYDIS_REGISTER_YMM0
          && registerName <= ArchValue(ZYDIS_REGISTER_YMM7,
                                       ZYDIS_REGISTER_YMM15))
  {
    //TODO: Untested
    registerLine += CPUInfoBox::formatSSEOperand(
      QByteArray((const char*)&registers.regcontext
        .YmmRegisters[registerName - ZYDIS_REGISTER_XMM0], 32),           // <=
      zydis.getVectorElementType(opindex)
    );
  }
  ....
}

TODO, казалось бы. Возможно, описывать это срабатывание было бы не к месту из-за этого, но найду в себе смелость разобраться в происходящем. Рассмотрим поближе вызывающий вопрос фрагмент:

registerLine += CPUInfoBox::formatSSEOperand(
  QByteArray((const char*)&registers.regcontext
    .YmmRegisters[registerName - ZYDIS_REGISTER_XMM0], 32),           // <=
  zydis.getVectorElementType(opindex)
);

Развернём проверки в условии:

else if(   registerName >= ZYDIS_REGISTER_YMM0
        && registerName <= ZYDIS_REGISTER_YMM15)

И ещё раз:

else if(registerName >= 88 && registerName <= 135)

А сколько может быть регистров YMM при условии, что процессор работает в 64-битном режиме? Шестнадцать!

typedef struct {
    ....
    DWORD MxCsr;
#ifdef _WIN64
    XMMREGISTER XmmRegisters[16];
    YMMREGISTER YmmRegisters[16];
#else // x86
    XMMREGISTER XmmRegisters[8];
    YMMREGISTER YmmRegisters[8];
#endif
} REGISTERCONTEXT;

135 - 88 = 47, и мы оказываемся в жуткой ситуации выхода за границу массива. На этом вопросы не заканчиваются: спустя какое-то время комментарий TODO пропал, а код остался неизменным. Чтение blame тоже не помогло установить причину пропажи комментария, как будто код так и был написан все эти три года.

Избыточно, но окей

Сообщение PVS-Studio:

V826 Consider replacing the 'values' std::vector with std::array. The size is known at compile time. CPUDisassembly.cpp 1855

bool CPUDisassembly::getLabelsFromInstruction(duint addr,
                                              QSet<QString> & labels)
{
    BASIC_INSTRUCTION_INFO basicinfo;
    DbgDisasmFastAt(addr, &basicinfo);
    std::vector<duint> values = { addr,
                                  basicinfo.addr,
                                  basicinfo.value.value,
                                  basicinfo.memory.value }; // <=
    for(auto value : values)
    {
        char label_[MAX_LABEL_SIZE] = "";
        if(DbgGetLabelAt(value, SEG_DEFAULT, label_))
        {
            //TODO: better cleanup of names
            QString label(label_);
            if(label.endsWith("A") || label.endsWith("W"))
                label = label.left(label.length() - 1);
            if(label.startsWith("&"))
                label = label.right(label.length() - 1);
            labels.insert(label);
        }
    }
    return labels.size() != 0;
}

Это чтобы дать вашему углеродному процессору под названием "мозг" чуть-чуть остыть после вникания в сложные логические процессы кремниевого собрата :)

Диагностика из группы микрооптимизаций подсказывает, что не нужно использовать сущность, у которой можно менять количество элементов, если мы не заполняем её динамически, а сразу инициализировали с нужными значениями и никак не модифицируем в будущем.

Кто куда?

Срабатывания анализатора:

V703 It is odd that the 'mCipBackgroundColor' field in derived class 'BreakpointsView' overwrites field in base class 'AbstractStdTable'. Check lines: BreakpointsView.h:59, AbstractStdTable.h:124. BreakpointsView.h 59

V703 It is odd that the 'mCipColor' field in derived class 'BreakpointsView' overwrites field in base class 'AbstractStdTable'. Check lines: BreakpointsView.h:60, AbstractStdTable.h:125. BreakpointsView.h 60

class AbstractStdTable : public AbstractTableView
{
    ....
protected:
    ....
    QColor mCipBackgroundColor;                                  // <=
    QColor mCipColor;                                            // <=
    QColor mBreakpointBackgroundColor;
    QColor mBreakpointColor;

class StdTable : public AbstractStdTable
{
....
}

class BreakpointsView : public StdTable
{
    ....
private:
    ....
    std::unordered_map<duint, const char*> mExceptionMap;
    QStringList mExceptionList;
    int mExceptionMaxLength;
    std::vector<BRIDGEBP> mBps;
    std::vector<std::pair<RichTextPainter::List, RichTextPainter::List>> mRich;
    QColor mDisasmBackgroundColor;
    QColor mDisasmSelectionColor;
    QColor mCipBackgroundColor;                                   // <=
    QColor mCipColor;                                             // <=
    QColor mSummaryParenColor;
    ....
}

Если захочется создать головоломку из ничего — назовите элемент наследующего класса тем же именем, что и в наследуемом. Если слишком просто, добавьте ещё один. Одинаковые названия переменных могут сбить с толку разработчиков, потому что они предполагают, что работают с одним объектом, а на деле может оказаться, что это другой объект. Не надо так.

Наши дни

Я бы сейчас пошутил про Time Travel Debugging из нового WinDbg, ведь теперь мы посмотрим, что поменялось в коде интерфейса x64dbg с июля и сколько исчезло ошибок, найденных PVS-Studio. Но так как TTD никак не связан со статическим анализом, мы просто делаем Time Travel на шесть месяцев вперёд и...

Четыре месяца назад классу Breakpoints пришло время пройти рефакторинг. Общие черты прослеживаются, но ошибки, найденной диагностикой V1091, больше нет. Вопрос временного объекта QByteArray остаётся открытым...

bool Breakpoints::editBP(BPXTYPE type,
                         const QString & module,
                         duint address,
                         QWidget* widget,
                         const QString & createCommand)
{
    QString addrText;
    BP_REF ref;
    switch(type)
    {
    case bp_dll:
        addrText = module;
        DbgFunctions()->BpRefDll(&ref, module.toUtf8().constData()); // <=
        break;
    ....
    }
    ....
}

Рефакторинг — процесс небыстрый, но результат его работы может вынести за борт огромный ворох проблем старой кодовой базы. Разработчики отладчика также пользуются Coverity для статического анализа кодовой базы, поэтому не исключено, что общее количество срабатываний в ядре и графической оболочке x64dbg уменьшилось в том числе по этой причине.

Итоги

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

Желаю команде разработчиков x64dbg довести до конца процесс перехода на актуальные сборочные инструменты и скорейшего выхода кроссплатформенной версии отладчика. И поменьше потраченного времени на отладку отладчика :)

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Taras Shevchenko. Debugging bugs in x64dbg debugger. Step out to GUI.