From 5ea021d57a1b8f8a08510199cf639e42455c917e Mon Sep 17 00:00:00 2001 From: paxcut <53811119+paxcut@users.noreply.github.com> Date: Tue, 15 Jul 2025 23:55:52 -0700 Subject: [PATCH] fix: fix for issue 2334 (#2337) The error was that if `m_bytesPerRow` was not divisible by the number of bytes per column then ImHex would crash but wouldn't crash if it was. When `m_bytesPerRow` is not equal to the resultant bytes per row obtained by the product of column count and bytes per column, then the later bytes per row were be allocated but the former bytes per row were being written causing heap corruption and crashes. Instead of resetting `m_bytePerRow` when it can't be used, a new variable (`bytesPerRow`) is created with the correct value and used in the rest of the function. This way if the user goes back to choose a data size that divides the old `m_bytesPerRow` then the number would still be available and not overwritten. Test indicate that this approach works and previous crashes are eliminated while producing the desired output. --- plugins/ui/source/ui/hex_editor.cpp | 66 +++++++++++++++-------------- 1 file changed, 34 insertions(+), 32 deletions(-) diff --git a/plugins/ui/source/ui/hex_editor.cpp b/plugins/ui/source/ui/hex_editor.cpp index 9716ae7e1..9d9e98b9d 100644 --- a/plugins/ui/source/ui/hex_editor.cpp +++ b/plugins/ui/source/ui/hex_editor.cpp @@ -536,6 +536,8 @@ namespace hex::ui { const auto bytesPerCell = m_currDataVisualizer->getBytesPerCell(); const u64 columnCount = m_bytesPerRow / bytesPerCell; + const auto bytesPerRow = columnCount * bytesPerCell; + auto byteColumnCount = 2 + columnCount + getByteColumnSeparatorCount(columnCount) + 2 + 2; if (byteColumnCount >= IMGUI_TABLE_MAX_COLUMNS) { @@ -554,7 +556,7 @@ namespace hex::ui { m_mode = Mode::Overwrite; Region hoveredCell = Region::Invalid(); - ImGui::PushID(m_bytesPerRow); + ImGui::PushID(bytesPerRow); ON_SCOPE_EXIT { ImGui::PopID(); }; if (ImGui::BeginChild("Hex View", size, ImGuiChildFlags_None, ImGuiWindowFlags_NoScrollbar | ImGuiWindowFlags_NoScrollWithMouse)) { this->drawScrollbar(CharacterSize); @@ -567,7 +569,7 @@ namespace hex::ui { // Row address column ImGui::TableSetupColumn("hex.ui.common.address"_lang, ImGuiTableColumnFlags_WidthFixed, m_provider == nullptr ? 0 : - CharacterSize.x * fmt::formatted_size("{:08X}: ", ((m_scrollPosition + m_visibleRowCount) * m_bytesPerRow) + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress()) + CharacterSize.x * fmt::formatted_size("{:08X}: ", ((m_scrollPosition + m_visibleRowCount) * bytesPerRow) + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress()) ); ImGui::TableSetupColumn(""); @@ -583,7 +585,7 @@ namespace hex::ui { ImGui::TableSetupColumn(""); if (m_showAscii) { - ImGui::TableSetupColumn("hex.ui.common.encoding.ascii"_lang, ImGuiTableColumnFlags_WidthFixed, (CharacterSize.x + m_characterCellPadding * 1_scaled) * m_bytesPerRow); + ImGui::TableSetupColumn("hex.ui.common.encoding.ascii"_lang, ImGuiTableColumnFlags_WidthFixed, (CharacterSize.x + m_characterCellPadding * 1_scaled) * bytesPerRow); } else { ImGui::TableSetupColumn("", ImGuiTableColumnFlags_WidthFixed, 0); } @@ -618,7 +620,7 @@ namespace hex::ui { return currRegionValid; }; - ImS64 numRows = (m_provider->getSize() / m_bytesPerRow) + ((m_provider->getSize() % m_bytesPerRow) == 0 ? 0 : 1); + ImS64 numRows = (m_provider->getSize() / bytesPerRow) + ((m_provider->getSize() % bytesPerRow) == 0 ? 0 : 1); if (numRows == 0) { ImGui::TableNextRow(); @@ -631,8 +633,8 @@ namespace hex::ui { m_visibleRowCount = std::max(m_visibleRowCount, 1); // Loop over rows - std::vector bytes(m_bytesPerRow, 0x00); - std::vector, std::optional>> cellColors(m_bytesPerRow / bytesPerCell); + std::vector bytes(bytesPerRow, 0x00); + std::vector, std::optional>> cellColors(bytesPerRow / bytesPerCell); for (ImS64 y = m_scrollPosition; y < (m_scrollPosition + m_visibleRowCount + 5) && y < numRows && numRows != 0; y++) { // Draw address column ImGui::TableNextRow(); @@ -640,9 +642,9 @@ namespace hex::ui { double addressWidth = ImGui::GetCursorPosX(); { - const auto rowAddress = y * m_bytesPerRow + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); + const auto rowAddress = y * bytesPerRow + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); - if (m_separatorStride > 0 && rowAddress % m_separatorStride < m_bytesPerRow && !ImGui::GetIO().KeyShift) + if (m_separatorStride > 0 && rowAddress % m_separatorStride < bytesPerRow && !ImGui::GetIO().KeyShift) ImGuiExt::TextFormattedColored(ImGui::GetStyleColorVec4(ImGuiCol_SeparatorActive), "{} {}", "hex.ui.common.segment"_lang, rowAddress / m_separatorStride); else ImGuiExt::TextFormattedSelectable("{0}: ", formatAddress(rowAddress, 8)); @@ -651,13 +653,13 @@ namespace hex::ui { ImGui::TableNextColumn(); addressWidth = ImGui::GetCursorPosX() - addressWidth; - const u8 validBytes = std::min(m_bytesPerRow, m_provider->getSize() - y * m_bytesPerRow); + const u8 validBytes = std::min(bytesPerRow, m_provider->getSize() - y * bytesPerRow); - m_provider->read(y * m_bytesPerRow + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(), bytes.data(), validBytes); + m_provider->read(y * bytesPerRow + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(), bytes.data(), validBytes); { for (u64 x = 0; x < std::ceil(float(validBytes) / bytesPerCell); x++) { - const u64 byteAddress = y * m_bytesPerRow + x * bytesPerCell + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); + const u64 byteAddress = y * bytesPerRow + x * bytesPerCell + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); const auto cellBytes = std::min(validBytes, bytesPerCell); @@ -700,7 +702,7 @@ namespace hex::ui { byteCellSize = ImVec2(std::ceil(byteCellSize.x), std::ceil(byteCellSize.y)); for (u64 x = 0; x < columnCount; x++) { - const u64 byteAddress = y * m_bytesPerRow + x * bytesPerCell + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); + const u64 byteAddress = y * bytesPerRow + x * bytesPerCell + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); ImGui::TableNextColumn(); drawSeparatorLine(byteAddress, x != 0); @@ -772,16 +774,16 @@ namespace hex::ui { // Draw ASCII column if (m_showAscii) { ImGui::PushStyleVar(ImGuiStyleVar_CellPadding, ImVec2(0, 0)); - if (ImGui::BeginTable("##ascii_column", m_bytesPerRow)) { - for (u64 x = 0; x < m_bytesPerRow; x++) + if (ImGui::BeginTable("##ascii_column", bytesPerRow)) { + for (u64 x = 0; x < bytesPerRow; x++) ImGui::TableSetupColumn(hex::format("##ascii_cell{}", x).c_str(), ImGuiTableColumnFlags_WidthFixed, CharacterSize.x + (m_characterCellPadding * 1_scaled)); ImGui::TableNextRow(); const auto asciiCellSize = CharacterSize + scaled(ImVec2(m_characterCellPadding, 0)); - for (u64 x = 0; x < m_bytesPerRow; x++) { - const u64 byteAddress = y * m_bytesPerRow + x + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); + for (u64 x = 0; x < bytesPerRow; x++) { + const u64 byteAddress = y * bytesPerRow + x + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); ImGui::TableNextColumn(); drawSeparatorLine(byteAddress, true); @@ -854,29 +856,29 @@ namespace hex::ui { if (singleByteEncoding) { u64 offset = 0; do { - const u64 address = y * m_bytesPerRow + offset + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); + const u64 address = y * bytesPerRow + offset + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); auto result = queryCustomEncodingData(m_provider, *m_currCustomEncoding, address); offset += result.advance; encodingData.emplace_back(address, result); - } while (offset < m_bytesPerRow); + } while (offset < bytesPerRow); } else { - if (m_encodingLineStartAddresses[y] >= m_bytesPerRow) { - encodingData.emplace_back(y * m_bytesPerRow + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(), CustomEncodingData(".", 1, ImGuiExt::GetCustomColorU32(ImGuiCustomCol_AdvancedEncodingUnknown))); + if (m_encodingLineStartAddresses[y] >= bytesPerRow) { + encodingData.emplace_back(y * bytesPerRow + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(), CustomEncodingData(".", 1, ImGuiExt::GetCustomColorU32(ImGuiCustomCol_AdvancedEncodingUnknown))); m_encodingLineStartAddresses.push_back(0); } else { u64 offset = m_encodingLineStartAddresses[y]; do { - const u64 address = y * m_bytesPerRow + offset + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); + const u64 address = y * bytesPerRow + offset + m_provider->getBaseAddress() + m_provider->getCurrentPageAddress(); auto result = queryCustomEncodingData(m_provider, *m_currCustomEncoding, address); offset += result.advance; encodingData.emplace_back(address, result); - } while (offset < m_bytesPerRow); + } while (offset < bytesPerRow); - m_encodingLineStartAddresses.push_back(offset - m_bytesPerRow); + m_encodingLineStartAddresses.push_back(offset - bytesPerRow); } } @@ -893,7 +895,7 @@ namespace hex::ui { const auto cellSize = ImGui::CalcTextSize(data.displayValue.c_str()) * ImVec2(1, 0) + ImVec2(m_characterCellPadding * 1_scaled, CharacterSize.y); const bool cellHovered = ImGui::IsMouseHoveringRect(cellStartPos, cellStartPos + cellSize, true) && ImGui::IsWindowHovered(); - const auto x = address % m_bytesPerRow; + const auto x = address % bytesPerRow; if (x < validBytes && isCurrRegionValid(address)) { auto [foregroundColor, backgroundColor] = cellColors[x / bytesPerCell]; @@ -912,7 +914,7 @@ namespace hex::ui { ImGui::SameLine(0, 0); ImGui::Dummy({ 0, 0 }); - this->handleSelection(address, data.advance, &bytes[address % m_bytesPerRow], cellHovered); + this->handleSelection(address, data.advance, &bytes[address % bytesPerRow], cellHovered); if (cellHovered) { Region newHoveredCell = { address, data.advance }; @@ -937,13 +939,13 @@ namespace hex::ui { if (m_shouldScrollToSelection && isSelectionValid()) { // Make sure simply clicking on a byte at the edge of the screen won't cause scrolling if ((ImGui::IsMouseDragging(ImGuiMouseButton_Left))) { - if ((*m_selectionStart >= (*m_selectionEnd + m_bytesPerRow)) && y == (m_scrollPosition + 1)) { - if (i128(m_selectionEnd.value() - m_provider->getBaseAddress() - m_provider->getCurrentPageAddress()) <= (ImS64(m_scrollPosition + 1) * m_bytesPerRow)) { + if ((*m_selectionStart >= (*m_selectionEnd + bytesPerRow)) && y == (m_scrollPosition + 1)) { + if (i128(m_selectionEnd.value() - m_provider->getBaseAddress() - m_provider->getCurrentPageAddress()) <= (ImS64(m_scrollPosition + 1) * bytesPerRow)) { m_shouldScrollToSelection = false; m_scrollPosition -= 3; } - } else if ((*m_selectionStart <= (*m_selectionEnd - m_bytesPerRow)) && y == ((m_scrollPosition + m_visibleRowCount) - 1)) { - if (i128(m_selectionEnd.value() - m_provider->getBaseAddress() - m_provider->getCurrentPageAddress()) >= (ImS64((m_scrollPosition + m_visibleRowCount) - 2) * m_bytesPerRow)) { + } else if ((*m_selectionStart <= (*m_selectionEnd - bytesPerRow)) && y == ((m_scrollPosition + m_visibleRowCount) - 1)) { + if (i128(m_selectionEnd.value() - m_provider->getBaseAddress() - m_provider->getCurrentPageAddress()) >= (ImS64((m_scrollPosition + m_visibleRowCount) - 2) * bytesPerRow)) { m_shouldScrollToSelection = false; m_scrollPosition += 3; } @@ -958,9 +960,9 @@ namespace hex::ui { auto newSelection = getSelection(); newSelection.address -= pageAddress; - if ((newSelection.getStartAddress()) < u64(m_scrollPosition * m_bytesPerRow)) + if ((newSelection.getStartAddress()) < u64(m_scrollPosition * bytesPerRow)) this->jumpToSelection(0.0F); - if ((newSelection.getEndAddress()) > u64((m_scrollPosition + m_visibleRowCount) * m_bytesPerRow)) + if ((newSelection.getEndAddress()) > u64((m_scrollPosition + m_visibleRowCount) * bytesPerRow)) this->jumpToSelection(1.0F); } } @@ -973,7 +975,7 @@ namespace hex::ui { m_provider->setCurrentPage(m_provider->getPageOfAddress(jumpAddress).value_or(0)); const auto pageAddress = m_provider->getCurrentPageAddress() + m_provider->getBaseAddress(); - const auto targetRowNumber = (jumpAddress - pageAddress) / m_bytesPerRow; + const auto targetRowNumber = (jumpAddress - pageAddress) / bytesPerRow; // Calculate the current top and bottom row numbers of the viewport ImS64 currentTopRow = m_scrollPosition;