From 4d2ef58e6a2e2c6efd1d35db1a4325a1c8079f44 Mon Sep 17 00:00:00 2001 From: karliss Date: Thu, 16 Apr 2020 20:32:24 +0300 Subject: [PATCH] Fix errors reported by UB sanitizer (#2150) * Use qt parent for deleting completer Manually deleting causes some UAF due to it being installed as event filter. Qt seems to destroy things in correct order. Sanitizer doesn't report completer as being leaked. * Fix sanitizer problems in HexWidget * Initialize size properties to somewhat sane values to avoid unrealized variable use when calculating them first time. * Change AbstractData interface. Old one returned pointer to unknown sized block of data which was difficult to use correctly. Adjust bound checking to avoid out of bounds access when comparing with oldData and scrolling. --- src/widgets/ConsoleWidget.cpp | 2 -- src/widgets/HexWidget.cpp | 60 +++++++++++++++++------------------ src/widgets/HexWidget.h | 49 ++++++++++++++++++---------- 3 files changed, 62 insertions(+), 49 deletions(-) diff --git a/src/widgets/ConsoleWidget.cpp b/src/widgets/ConsoleWidget.cpp index b1e10243..9999950a 100644 --- a/src/widgets/ConsoleWidget.cpp +++ b/src/widgets/ConsoleWidget.cpp @@ -133,8 +133,6 @@ ConsoleWidget::ConsoleWidget(MainWindow *main, QAction *action) : ConsoleWidget::~ConsoleWidget() { - delete completer; - #ifndef Q_OS_WIN ::close(stdinFile); remove(stdinFifoPath.toStdString().c_str()); diff --git a/src/widgets/HexWidget.cpp b/src/widgets/HexWidget.cpp index 599ba8ad..f206b819 100644 --- a/src/widgets/HexWidget.cpp +++ b/src/widgets/HexWidget.cpp @@ -262,13 +262,11 @@ void HexWidget::setItemGroupSize(int size) * It is assumed that the current read data buffer contains the address. */ bool HexWidget::isItemDifferentAt(uint64_t address) { - if (address >= oldData->minIndex() && address < oldData->maxIndex()) { - uint64_t itemOffset = address - startAddress; - QVariant curItem = readItem(itemOffset); - data.swap(oldData); - QVariant oldItem = readItem(itemOffset); - data.swap(oldData); - return (oldItem != curItem); + char oldItem[sizeof(uint64_t)] = {}; + char newItem[sizeof(uint64_t)] = {}; + if (data->copy(newItem, address, static_cast(itemByteLen)) && + oldData->copy(oldItem, address, static_cast(itemByteLen))) { + return memcmp(oldItem, newItem, sizeof(oldItem)) != 0; } return false; } @@ -1373,15 +1371,20 @@ QVariant HexWidget::readItem(int offset, QColor *color) quint16 word; quint32 dword; quint64 qword; - float *ptrFloat32; - double *ptrFloat64; + float float32; + double float64; - const void *dataPtr = data->dataPtr(startAddress + offset); + quint8 bytes[sizeof(uint64_t)]; + data->copy(bytes, startAddress + offset, static_cast(itemByteLen)); const bool signedItem = itemFormat == ItemFormatSignedDec; + if (color) { + *color = defColor; + } + switch (itemByteLen) { case 1: - byte = *static_cast(dataPtr); + byte = bytes[0]; if (color) *color = itemColor(byte); if (!signedItem) @@ -1389,38 +1392,34 @@ QVariant HexWidget::readItem(int offset, QColor *color) return QVariant(static_cast(static_cast(byte))); case 2: if (itemBigEndian) - word = fromBigEndian(dataPtr); + word = fromBigEndian(bytes); else - word = fromLittleEndian(dataPtr); - if (color) - *color = defColor; + word = fromLittleEndian(bytes); + if (!signedItem) return QVariant(static_cast(word)); return QVariant(static_cast(static_cast(word))); case 4: if (itemBigEndian) - dword = fromBigEndian(dataPtr); + dword = fromBigEndian(bytes); else - dword = fromLittleEndian(dataPtr); - if (color) - *color = defColor; + dword = fromLittleEndian(bytes); + if (itemFormat == ItemFormatFloat) { - ptrFloat32 = static_cast(static_cast(&dword)); - return QVariant(*ptrFloat32); + memcpy(&float32, &dword, sizeof(float32)); + return QVariant(float32); } if (!signedItem) return QVariant(static_cast(dword)); return QVariant(static_cast(static_cast(dword))); case 8: if (itemBigEndian) - qword = fromBigEndian(dataPtr); + qword = fromBigEndian(bytes); else - qword = fromLittleEndian(dataPtr); - if (color) - *color = defColor; + qword = fromLittleEndian(bytes); if (itemFormat == ItemFormatFloat) { - ptrFloat64 = static_cast(static_cast(&qword)); - return QVariant(*ptrFloat64); + memcpy(&float64, &qword, sizeof(float64)); + return QVariant(float64); } if (!signedItem) return QVariant(qword); @@ -1462,7 +1461,8 @@ QString HexWidget::renderItem(int offset, QColor *color) QChar HexWidget::renderAscii(int offset, QColor *color) { - uchar byte = *static_cast(data->dataPtr(startAddress + offset)); + uchar byte; + data->copy(&byte, startAddress + offset, sizeof(byte)); if (color) { *color = itemColor(byte); } @@ -1539,7 +1539,7 @@ BasicCursor HexWidget::mousePosToAddr(const QPoint &point, bool middle) const return asciiArea.contains(point) ? asciiPosToAddr(point, middle) : screenPosToAddr(point, middle); } -QRectF HexWidget::itemRectangle(uint offset) +QRectF HexWidget::itemRectangle(int offset) { qreal x; qreal y; @@ -1565,7 +1565,7 @@ QRectF HexWidget::itemRectangle(uint offset) return QRectF(x, y, width, lineHeight); } -QRectF HexWidget::asciiRectangle(uint offset) +QRectF HexWidget::asciiRectangle(int offset) { QPointF p; diff --git a/src/widgets/HexWidget.h b/src/widgets/HexWidget.h index 0cc603eb..368b111c 100644 --- a/src/widgets/HexWidget.h +++ b/src/widgets/HexWidget.h @@ -75,7 +75,7 @@ class AbstractData public: virtual ~AbstractData() {} virtual void fetch(uint64_t addr, int len) = 0; - virtual const void *dataPtr(uint64_t addr) = 0; + virtual bool copy(void *out, uint64_t adr, size_t len) = 0; virtual uint64_t maxIndex() = 0; virtual uint64_t minIndex() = 0; }; @@ -101,9 +101,12 @@ public: void fetch(uint64_t, int) override { } - const void *dataPtr(uint64_t addr) override - { - return m_buffer.constData() + addr; + bool copy(void *out, uint64_t addr, size_t len) override { + if (addr < static_cast(m_buffer.size()) && (static_cast(m_buffer.size()) - addr) < len) { + memcpy(out, m_buffer.constData() + addr, len); + return true; + } + return false; } uint64_t maxIndex() override @@ -120,6 +123,7 @@ class MemoryData : public AbstractData public: MemoryData() {} ~MemoryData() override {} + static constexpr size_t BLOCK_SIZE = 4096; void fetch(uint64_t address, int length) override { @@ -141,12 +145,23 @@ public: } } - const void *dataPtr(uint64_t addr) override - { + bool copy(void *out, uint64_t addr, size_t len) override { + if (addr < m_firstBlockAddr || addr > m_lastValidAddr || + (m_lastValidAddr - addr + 1) < len /* do not merge with last check to handle overflows */) { + return false; + } + int totalOffset = addr - m_firstBlockAddr; - int blockId = totalOffset / 4096; - int blockOffset = totalOffset % 4096; - return static_cast(m_blocks.at(blockId).constData() + blockOffset); + int blockId = totalOffset / BLOCK_SIZE; + int blockOffset = totalOffset % BLOCK_SIZE; + size_t first_part = BLOCK_SIZE - blockOffset; + if (first_part >= len) { + memcpy(out, m_blocks.at(blockId).constData() + blockOffset, len); + } else { + memcpy(out, m_blocks.at(blockId).constData() + blockOffset, first_part); + memcpy(static_cast(out) + first_part, m_blocks.at(blockId + 1).constData(), len - first_part); + } + return true; } virtual uint64_t maxIndex() override @@ -353,13 +368,13 @@ private: * @param offset relative to first byte on screen * @return */ - QRectF itemRectangle(uint offset); + QRectF itemRectangle(int offset); /** * @brief Rectangle for single item in ascii area. * @param offset relative to first byte on screen * @return */ - QRectF asciiRectangle(uint offset); + QRectF asciiRectangle(int offset); QVector rangePolygons(RVA start, RVA last, bool ascii); void updateWidth(); @@ -450,12 +465,12 @@ private: QRectF itemArea; QRectF asciiArea; - int itemByteLen; - int itemGroupSize; ///< Items per group (default: 1), 2 in case of hexpair mode - int rowSizeBytes; ///< Line size in bytes - int itemColumns; ///< Number of columns, single column consists of itemGroupSize items - int itemCharLen; - int itemPrefixLen; + int itemByteLen = 1; + int itemGroupSize = 1; ///< Items per group (default: 1), 2 in case of hexpair mode + int rowSizeBytes = 16; ///< Line size in bytes + int itemColumns = 16; ///< Number of columns, single column consists of itemGroupSize items + int itemCharLen = 2; + int itemPrefixLen = 0; ColumnMode columnMode; ItemFormat itemFormat;