# Дебажим баги в дебаггере x64dbg\. "Шаг с выходом" в GUI С отладчиком нужно как\-то взаимодействовать: через текстовый или графический интерфейс\. В "оболочке" программы с хорошо отлаженным ядром может оказаться неприятный баг, и пользователю это вряд ли понравится\. Любому разработчику не хотелось бы допускать такой ситуации\. ![1187_x64dbg_pt2_ru/image1.png](https://import.viva64.com/docx/blog/1187_x64dbg_pt2_ru/image1.png) ## Восстанавливаем контекст В [первой части](https://pvs-studio.ru/ru/blog/posts/cpp/1146/) статьи про автономный отладчик x64dbg мы рассматривали его ядро, держа в одной руке документацию на процессоры Intel Pentium 4, в другой — на Windows API~~, а в третьей учебник математики~~\. Получился неплохой суп из математического сопроцессора, поданный в лёгкой посуде из титана, но без второго блюда из графического интерфейса обед был бы неполным\. С момента публикации прошлой части прошло несколько месяцев, и x64dbg продолжали улучшать функционально, а также появилась задача на [актуализацию средств разработки](https://github.com/x64dbg/x64dbg/issues/3412)\. Здесь же мы продолжим анализ на основе кода коммита [f518e50](https://github.com/x64dbg/x64dbg/tree/f518e507c24a04d9c82161ef1e89a7a70a73c0f2) и, где возможно, проведём сравнение с актуальным на момент написания статьи коммитом [9785d1a](https://github.com/x64dbg/x64dbg/tree/9785d1a62e104a2720c21736db8fc8c6f7311ce3)\. Для сборки нам понадобится Qt 5\.6\.3, о чём уже было сказано в инструкции от разработчиков отладчика\. Эту версию Qt можно без проблем использовать в актуальной версии Qt Creator 14\.0\.2, в которую можно установить [плагин PVS\-Studio](https://pvs-studio.ru/ru/docs/manual/6648/) и провести статический анализ кода\. ![1187_x64dbg_pt2_ru/image2.png](https://import.viva64.com/docx/blog/1187_x64dbg_pt2_ru/image2.png) ## Шагаем по интерфейсу ### Испачкались в п~~р~~обелке Начнём\. Предупреждение PVS\-Studio: [V781](https://pvs-studio.ru/ru/docs/warnings/v781/) The value of the 'post' index is checked after it was used\. Perhaps there is a mistake in program logic\. [Utf8Ini\.h 243](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/bridge/Utf8Ini.h#L243) ```cpp 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) : ""; } ``` Что будет, если вся строка состоит **только** из пробелов? Обрезка пробелов обрежет не только строку, но и [программу](https://godbolt.org/z/4v3bac683) под хоровое пение\. Достаточно поменять местами проверки, и _EXCEPTION\_ACCESS\_VIOLATION_ обойдёт вас стороной\. ```cpp while(post < len && str[len - post - 1] == ' ') post++; ``` Идёт в комплекте с: * [V781](https://pvs-studio.ru/ru/docs/warnings/v781/) The value of the 'len' index is checked after it was used\. Perhaps there is a mistake in program logic\. [Utf8Ini\.h 243](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/bridge/Utf8Ini.h#L243) ### Два по цене одного Предупреждения анализатора: [V1048](https://pvs-studio.ru/ru/docs/warnings/v1048/) The 'accept' variable was assigned the same value\. [HexDump\.cpp 405](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/gui/Src/BasicView/HexDump.cpp#L405) [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'accept' is always true\. [HexDump\.cpp 417](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/gui/Src/BasicView/HexDump.cpp#L417) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v1048/) The 'mUpdateCountLabel' variable was assigned the same value\. [ReferenceView\.cpp 166](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/gui/Src/BasicView/ReferenceView.cpp#L166) ### Надеемся на понимание Предупреждение PVS\-Studio: [V614](https://pvs-studio.ru/ru/docs/warnings/v614/) Potentially uninitialized variable 'funcType' used\. Consider checking the fourth actual argument of the 'paintFunctionGraphic' function\. [Disassembly\.cpp 548](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/gui/Src/BasicView/Disassembly.cpp#L548) ```cpp 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 считает использование [неинициализированных переменных](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-and-level-4-c4700?view=msvc-170) [неопределённым поведением](https://pvs-studio.ru/ru/blog/terms/0066/), поэтому сыграть в тетрис или чихнуть в колонки дозволительно\. Если это не так, то можно предположить, что переменная _funcType_ должна была быть инициализирована со значением _Function\_none_ из перечисления [_Function\_t_](https://github.com/x64dbg/x64dbg/blob/9785d1a62e104a2720c21736db8fc8c6f7311ce3/src/gui/Src/BasicView/Disassembly.h#L182), либо, как [чуть выше](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/gui/Src/BasicView/Disassembly.cpp#L467), присвоить значение в блоке _switch_\. ### Скоропортящиеся продукты Предупреждение PVS\-Studio: [V1091](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/gui/Src/Utils/Breakpoints.cpp#L506) ```cpp 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(addrText.toUtf8().constData()), // <= &bridgebp ); } else { found = DbgFunctions()->GetBridgeBp( type, (duint)addrText.toULongLong(nullptr, 16), &bridgebp ); } .... } ``` "Искал одно, нашёл другое" — как раз про случай на экране\. Меня напрягло не то, что здесь неправильно произведена протяжка указателя с 32 до 64 бит, а то, что идёт работа с временным объектом _QByteArray_ \(создаётся из [_addrText\.toUtf8\(\)_](https://doc.qt.io/qt-5/qstring.html#toUtf8)\), жизненный цикл которого кончится по достижении _reinterpret\_cast_\. Из\-за этого в функцию _GetBridgeBp_ придёт или может прийти вместо строки с адресом точки останова что\-то недействительное\. ### Ещё минутка занимательной математики Предупреждение PVS\-Studio: [V557](https://pvs-studio.ru/ru/docs/warnings/v557/) Array overrun is possible\. The value of 'registerName \- ZYDIS\_REGISTER\_XMM0' index could reach 47\. [TraceInfoBox\.cpp 310](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/gui/Src/Tracer/TraceInfoBox.cpp#L310) ```cpp 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*)®isters.regcontext .YmmRegisters[registerName - ZYDIS_REGISTER_XMM0], 32), // <= zydis.getVectorElementType(opindex) ); } .... } ``` TODO, казалось бы\. Возможно, описывать это срабатывание было бы не к месту из\-за этого, но найду в себе смелость разобраться в происходящем\. Рассмотрим поближе вызывающий вопрос фрагмент: ```cpp registerLine += CPUInfoBox::formatSSEOperand( QByteArray((const char*)®isters.regcontext .YmmRegisters[registerName - ZYDIS_REGISTER_XMM0], 32), // <= zydis.getVectorElementType(opindex) ); ``` Развернём проверки в условии: ```cpp else if( registerName >= ZYDIS_REGISTER_YMM0 && registerName <= ZYDIS_REGISTER_YMM15) ``` И ещё раз: ```cpp else if(registerName >= 88 && registerName <= 135) ``` А сколько может быть регистров _YMM_ при условии, что процессор работает в 64\-битном режиме? [Шестнадцать\!](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/bridge/bridgemain.h#L846) ```cpp 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 пропал](https://github.com/x64dbg/x64dbg/blame/development/src/gui/Src/Tracer/TraceInfoBox.cpp#L190), а код остался неизменным\. Чтение blame тоже не помогло установить причину пропажи комментария, как будто код так и был написан все эти три года\. ### Избыточно, но окей Сообщение PVS\-Studio: [V826](https://pvs-studio.ru/ru/docs/warnings/v826/) Consider replacing the 'values' std::vector with std::array\. The size is known at compile time\. [CPUDisassembly\.cpp 1855](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/gui/Src/Gui/CPUDisassembly.cpp#L1855) ```cpp bool CPUDisassembly::getLabelsFromInstruction(duint addr, QSet & labels) { BASIC_INSTRUCTION_INFO basicinfo; DbgDisasmFastAt(addr, &basicinfo); std::vector 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](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/gui/Src/Gui/BreakpointsView.h#L59) [V703](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/x64dbg/x64dbg/blob/f518e507c24a04d9c82161ef1e89a7a70a73c0f2/src/gui/Src/Gui/BreakpointsView.h#L60) ```cpp 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 mExceptionMap; QStringList mExceptionList; int mExceptionMaxLength; std::vector mBps; std::vector> mRich; QColor mDisasmBackgroundColor; QColor mDisasmSelectionColor; QColor mCipBackgroundColor; // <= QColor mCipColor; // <= QColor mSummaryParenColor; .... } ``` Если захочется создать головоломку из ничего — назовите элемент наследующего класса тем же именем, что и в наследуемом\. Если слишком просто, добавьте ещё один\. Одинаковые названия переменных могут сбить с толку разработчиков, потому что они предполагают, что работают с одним объектом, а на деле может оказаться, что это другой объект\. Не надо так\. ![1187_x64dbg_pt2_ru/image3.png](https://import.viva64.com/docx/blog/1187_x64dbg_pt2_ru/image3.png) ## Наши дни Я бы сейчас пошутил про [_Time Travel Debugging_](https://learn.microsoft.com/en-us/windows-hardware/drivers/debuggercmds/time-travel-debugging-overview) из нового WinDbg, ведь теперь мы посмотрим, что поменялось в коде интерфейса x64dbg с июля и сколько исчезло ошибок, найденных PVS\-Studio\. Но так как TTD никак не связан со статическим анализом, мы просто делаем Time Travel на шесть месяцев вперёд и\.\.\. Четыре месяца назад классу [Breakpoints](https://github.com/x64dbg/x64dbg/blob/9785d1a62e104a2720c21736db8fc8c6f7311ce3/src/gui/Src/Utils/Breakpoints.cpp#L226) пришло время пройти рефакторинг\. Общие черты прослеживаются, но ошибки, найденной диагностикой V1091, больше нет\. Вопрос временного объекта _QByteArray_ остаётся открытым\.\.\. ```cpp 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 уменьшилось в том числе по этой причине\. ## Итоги Сделать хороший отладчик — задача действительно не из лёгких, как и создать удобный и комфортный для использования интерфейс\. Изъяны найдёт каждый, но сможет ли каждый найти красоту в программном коде? Ошибки [тоже бывают красивыми](https://pvs-studio.ru/ru/blog/posts/cpp/1176/), но ещё красивее будет код, который исправляют при регулярном использовании статического анализатора, например, PVS\-Studio\. Для проектов СПО мы предлагаем [бесплатную лицензию](https://pvs-studio.ru/ru/order/open-source-license/) с возможностью продления в будущем\. Желаю команде разработчиков x64dbg довести до конца процесс перехода на актуальные сборочные инструменты и скорейшего выхода кроссплатформенной версии отладчика\. И поменьше потраченного времени на отладку отладчика :\)