eteran / edb-debugger

edb is a cross-platform AArch32/x86/x86-64 debugger.
GNU General Public License v2.0
2.7k stars 326 forks source link

uninitialized array in DialogEditSIMDRegister #695

Closed sorokin closed 5 years ago

sorokin commented 5 years ago

At program startup (before anything is run), valgrind reports uninitialized variable with the following callstack (reduced for clarity):

Conditional jump or move depends on uninitialised value(s) ... QString::arg(long long, int, int, QChar) const QString::arg(int, int, int, QChar) const ODbgRegisterView::DialogEditSIMDRegister::formatInteger ODbgRegisterView::DialogEditSIMDRegister::updateIntegralEntries ODbgRegisterView::DialogEditSIMDRegister::updateAllEntriesExcept ODbgRegisterView::DialogEditSIMDRegister::onHexToggled QtPrivate::FunctorCall<...>::call QtPrivate::FunctionPointer<...>::call QtPrivate::QSlotObject<...>::impl QMetaObject::activate(QObject , int, int, void *) QAbstractButton::toggled(bool) ... QAbstractButton::setChecked(bool) ODbgRegisterView::DialogEditSIMDRegister::DialogEditSIMDRegister # radioHex->setChecked(true); ODbgRegisterView::ODBRegView::ODBRegView ODbgRegisterView::Plugin::createRegisterView ODbgRegisterView::Plugin::setupDocks ODbgRegisterView::Plugin::menu Debugger::finish_plugin_setup Debugger::Debugger (anonymous namespace)::start_debugger main

The uninitialized value that is used is DialogEditSIMDRegister::value_.

Proper fix might involve connecting onHexToggled after calling setChecked in DialogEditSIMDRegister constructor. But a comment in the code states that this is done on purpose.

connect(radioHex, &QRadioButton::toggled, this, &DialogEditSIMDRegister::onHexToggled);
radioHex->setChecked(true); // must be after connecting of toggled()

Unfortunately the comment doesn't explain why it must be done this way.

Another possibility is to avoid formatting entries altogether as they are not shown everywhere (the error occurs at program startup, when register view is empty).

This commit fixes the issue in the most conservative way possible (by filling value_ with zeros). This makes it appropriate for stable branch (1.0), but for master perhaps a better solution should be devised.

10110111 commented 5 years ago

Unfortunately the comment doesn't explain why it must be done this way.

One of the side effects of DialogEditSIMDRegister::onHexToggled is setting new validators, so the comment is indeed correct. If radioHex->setChecked(true) is called before connecting to the slot, validators won't be set on integer entries.

The proper fix, I think, is to avoid formatting value_ if !reg.

Not sure though why the value_ variable is actually needed since reg is already stored.

sorokin commented 5 years ago

Thank you for your prompt reply.

Not formatting values works for me. I force pushed an updated version.

eteran commented 5 years ago

Looks good to me. Simple solution.