Skip to content

Commit

Permalink
Fix invalid memory access when reading colour maps
Browse files Browse the repository at this point in the history
In 6286158, as part of #2911,
ColourMap::maps was converted from a null-terminated C array to a
std::vector. However, the loops iterating over this list weren't updated
and were still relying on null-termination causing an invalid memory
access. This also resulted mrview crashing on MacOS.
  • Loading branch information
daljit46 authored and Lestropie committed Aug 23, 2024
1 parent 10c4993 commit d591658
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 35 deletions.
33 changes: 9 additions & 24 deletions src/colourmap.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#pragma once

#include "exception.h"
#include "types.h"
#include <functional>

Expand Down Expand Up @@ -51,38 +52,22 @@ class Entry {

extern const std::vector<Entry> maps;

inline size_t num() {
size_t n = 0;
while (maps[n].name)
++n;
return n;
}
inline size_t num() { return maps.size(); }

inline size_t num_scalar() {
size_t n = 0, i = 0;
while (maps[i].name) {
if (!maps[i].special)
++n;
++i;
}
return n;
return std::count_if(maps.begin(), maps.end(), [](const Entry &map) { return map.special; });
}

inline size_t index(const std::string &name) {
size_t n = 0;
while (maps[n].name != name)
++n;
return n;
auto it = std::find_if(maps.begin(), maps.end(), [&name](const Entry &map) { return map.name == name; });

if (it == maps.end())
throw MR::Exception("Colour map \"" + name + "\" not found");
return std::distance(maps.begin(), it);
}

inline size_t num_special() {
size_t n = 0, i = 0;
while (maps[i].name) {
if (maps[i].special)
++n;
++i;
}
return n;
return std::count_if(maps.begin(), maps.end(), [](const Entry &map) { return map.special; });
}

} // namespace MR::ColourMap
12 changes: 6 additions & 6 deletions src/gui/mrview/colourmap_button.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ void ColourMapButton::init_core_menu_items(bool create_shortcuts) {
core_colourmaps_actions->setExclusive(true);

size_t n = 0;
for (size_t i = 0; ColourMap::maps[i].name; ++i) {
if (!ColourMap::maps[i].special && !ColourMap::maps[i].is_colour) {
QAction *action = new QAction(ColourMap::maps[i].name, this);
for (const auto &map : ColourMap::maps) {
if (!map.special && !map.is_colour) {
QAction *action = new QAction(map.name, this);
action->setCheckable(true);
core_colourmaps_actions->addAction(action);

Expand Down Expand Up @@ -82,9 +82,9 @@ void ColourMapButton::init_custom_colour_menu_items() {

void ColourMapButton::init_special_colour_menu_items(bool create_shortcuts) {
size_t n = colourmap_actions.size();
for (size_t i = 0; ColourMap::maps[i].name; ++i) {
if (ColourMap::maps[i].special) {
QAction *action = new QAction(ColourMap::maps[i].name, this);
for (const auto &map : ColourMap::maps) {
if (map.special) {
QAction *action = new QAction(map.name, this);
action->setCheckable(true);
core_colourmaps_actions->addAction(action);

Expand Down
9 changes: 6 additions & 3 deletions src/gui/mrview/gui_image.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,12 @@ size_t Image::guess_colourmap() const {
if (header().size(3) == 3)
map = "RGB";
}
for (size_t n = 0; ColourMap::maps[n].name; ++n)
if (ColourMap::maps[n].name == map)
return n;
auto it = std::find_if(
ColourMap::maps.begin(), ColourMap::maps.end(), [&map](const auto &entry) { return entry.name == map; });
if (it != ColourMap::maps.end()) {
return std::distance(ColourMap::maps.begin(), it);
}

return 0;
}

Expand Down
4 changes: 2 additions & 2 deletions src/gui/mrview/tool/fixel/fixel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -415,8 +415,8 @@ void Fixel::update_gui_colour_controls(bool reload_colour_types) {
// how many menu elements were actually created by ColourMap::create_menu()
static size_t colourmap_count = 0;
if (!colourmap_count) {
for (size_t i = 0; ColourMap::maps[i].name; ++i) {
if (!ColourMap::maps[i].special)
for (const auto &map : ColourMap::maps) {
if (!map.special)
++colourmap_count;
}
}
Expand Down

0 comments on commit d591658

Please sign in to comment.