Мы используем куки, чтобы пользоваться сайтом
было удобно.
Как не пропустить опасные участки кода при ревью? Можно воспользоваться инструментами статического анализа. Возьмём для примера OrcaSlicer — популярную программу, которая подготавливает 3D-модель к печати. Заглянем внутрь и посмотрим, какие сюрпризы нас ждут.
На просторах GitHub есть интересный проект для подготовки деталей к печати на 3D-принтере. Он позволяет преобразовать 3D-модель в управляющие команды для послойного создания объекта.
OrcaSlicer — это один из самых популярных бесплатных слайсеров с открытым исходным кодом. Проект довольно большой, а основной язык разработки — C++, именно поэтому он и попал в мои руки. Сегодня предлагаю изучить его изнутри и найти интересные места в коде.
Примечание. Данный контент создан с целью популяризации технологии статического анализа и не пытается оскорбить авторов OrcaSlicer.
Эта статья только показывает, что даже в работающем проекте могут быть ошибки. Их пропускают при ревью или тестах, они годами накапливаются, что в итоге может приводить к проблемам в продукте. Однако использование дополнительных средств проверки, например статических анализаторов, позволяет обнаружить ошибки ещё на этапе разработки.
Если вы всё ещё сомневаетесь в пользе таких инструментов, то рекомендую прочитать статью "Причины внедрить в процесс разработки статический анализатор кода PVS-Studio".
Анализ проекта проводился на основе этого коммита с помощью плагина PVS-Studio для Visual Studio 2022. В статье были собраны не все, а только самые интересные, на мой взгляд, предупреждения анализатора.
Примечание. Для упрощения чтения фрагменты кода были отформатированы, а некоторые скрыты за комментариями // .....
Пример N1
Предупреждение PVS-Studio: V655 The strings were concatenated but are not utilized. Consider inspecting the 'message + "\n\nApplication will close."' expression. GUI_App.cpp 7170
bool GUI_App::load_language(wxString language, bool initial)
{
// ....
if (!wxLocale::IsAvailable(locale_language_info->Language)) {
// Loading the language dictionary failed.
wxString message = "Switching Orca Slicer to language "
+ requested_language_code + " failed.";
#if !defined(_WIN32) && !defined(__APPLE__)
// likely some linux system
message += "\nYou may need to reconfigure the missing locales, "
" likely by running the \"locale-gen\" "
"and \"dpkg-reconfigure locales\" commands.\n";
#endif
if (initial)
message + "\n\nApplication will close."; // <=
wxMessageBox(message, "Orca Slicer - Switching language failed",
wxOK | wxICON_ERROR);
if (initial)
std::exit(EXIT_FAILURE);
else
return false;
}
// ....
}Опечатка, которую не заметил разработчик и которую пропустили при код-ревью. Вместо оператора += был использован +, и в результате строковый литерал не добавляется к переменной message. Код компилируется, но не имеет никакого смысла. Это предупреждение напомнило мне ошибку из qbEngine, когда в макросе #else забыли дописать #.
При взгляде на этот код, возможно, у вас тоже возникнут мысли: "Ну нет... Это смешно, я такое точно никогда не напишу". Но это просто человеческий фактор. Это код, который был заложен четыре года назад и до сих пор не был исправлен. При этом место, где инициализируется строка message, изменяли пару месяцев назад.
Пример N2
Предупреждения PVS-Studio: V1047 Lifetime of the lambda is greater than lifetime of the local variable 'do_stop' captured by reference. FillBedJob.cpp 250
void FillBedJob::process(Ctl &ctl)
{
// ....
bool do_stop = false;
// ....
params.on_packed =
[&do_stop] (const ArrangePolygon &ap)
{
do_stop = ap.bed_idx > 0 && ap.priority == 0;
};
// ....
}Лямбда-выражение захватывает локальную переменную do_stop по ссылке, а затем сохраняется в params.on_packed. При этом do_stop уничтожается при выходе из метода process, так как заканчивается время жизни локального объекта. Если лямбда будет вызвана после выхода из этой функции-члена, произойдёт обращение к разрушенному объекту, и поведение в этой ситуации не определено.
Можно было бы сделать захват по значению, но в этой лямбде происходит перезапись переменной do_stop, а значит такой вариант не подходит. Поэтому можно сделать do_stop членом класса FillBedJob.
Ниже по коду аналогичная ситуация происходит ещё два раза:
Пример N3
Предупреждение PVS-Studio: V768 The enumeration constant 'roPortrait' is used as a variable of a Boolean-type. RasterBase.hpp 81
class RasterBase {
public:
enum Orientation { roLandscape, roPortrait };
struct Trafo {
bool mirror_x = false, mirror_y = false, flipXY = false;
TMirroring get_mirror() const
{ return { (roPortrait ? !mirror_x : mirror_x), mirror_y}; } // <=
// ....
};
// ....
}Анализатор обнаружил использование элемента перечисления в тернарном операторе в качестве первого операнда. Константа roPortrait имеет значение 1, поэтому условие всегда будет истинно. Вероятно, здесь должна использоваться другая переменная.
А возможно, нужно было добавить дополнительный параметр, чтобы сравнить его с roPortrait, как это сделано в конструкторе:
Trafo(Orientation o = roLandscape, const TMirroring &mirror = NoMirror)
// XY flipping implicitly does an X mirror
: mirror_x(o == roPortrait ? !mirror[0] : mirror[0]) // <=
, mirror_y(!mirror[1]) // Makes raster origin to be top left corner
, flipXY(o == roPortrait)
{}Пример N4
Предупреждение PVS-Studio: V781 The value of the 'm_load_slot_index' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 2152, 2155. AMSItem.cpp 2152
void AMSRoadUpPart::doRender(wxDC& dc)
{
// ....
dc.SetPen(wxPen(_get_diff_clr(this,
m_amsinfo.cans[m_load_slot_index].material_colour), 4, wxPENSTYLE_SOLID));
auto temp = m_amsinfo;
if (m_load_step != AMSPassRoadSTEP::AMS_ROAD_STEP_NONE){
if (m_amsinfo.cans.size() <= m_load_slot_index || m_load_slot_index < 0){
BOOST_LOG_TRIVIAL(trace) << "up road render error";
return;
}
// ....
}Такие ошибки очень интересны тем, что кажется невозможным здесь ошибиться. Вроде бы простое место, но довольно легко запутаться. Если посмотреть в список найденных диагностическим правилом V781 ошибок, то можно понять, что даже разработчики довольно крупных проектов часто их допускают.
Но в чём проблема? Выход за границы массива. Сначала индекс m_load_slot_index используется для доступа к элементу вектора m_amsinfo.cans, и только после этого проверяется, не выходит ли итератор за границы и не отрицательный ли он. Вроде проверка есть, но она срабатывает слишком поздно. Чтобы код работал корректно, можно поставить проверку выше или обработать такую ситуацию другим способом.
Аналогичное предупреждение:
Пример N5
Предупреждение PVS-Studio: V783 Iterator 'it' is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 3159, 3160. Selection.cpp 3160
void Selection::ensure_not_below_bed()
{
// ....
if (is_any_volume()) {
for (unsigned int i : m_list) {
GLVolume& volume = *(*m_volumes)[i];
const std::pair<int, int> instance =
std::make_pair(volume.object_idx(), volume.instance_idx());
InstancesToZMap::const_iterator it = instances_max_z.find(instance);
const double z_shift = SINKING_MIN_Z_THRESHOLD - it->second;
if (it != instances_max_z.end() && z_shift > 0.0)
volume.set_volume_offset(Z, volume.get_volume_offset(Z) + z_shift);
}
}
// ....
}В этом же фрагменте есть ещё одна проверка, и она тоже почему-то стоит после использования итератора. Для вычисления z_shift разыменовывается итератор it, а строчкой ниже он проверяется на неравенство итератору end(). Если элемент не был найден, то поведение при разыменовании результирующего итератора не определено.
Исправленный вариант кода:
if (it != instances_max_z.end())
{
const double z_shift = SINKING_MIN_Z_THRESHOLD - it->second;
if (z_shift > 0.0)
volume.set_volume_offset(Z, volume.get_volume_offset(Z) + z_shift);
}Пример N6
Предупреждения PVS-Studio:
V595 The 'part_plate' pointer was utilized before it was verified against nullptr. Check lines: 498, 525. ObjectDataViewModel.cpp 498
V522 Dereferencing of the null pointer 'part_plate' might take place. The null pointer is passed into 'AddPlate' function. Inspect the first argument. Check lines: 497, 535. ObjectDataViewModel.cpp 497
wxDataViewItem ObjectDataViewModel::AddOutsidePlate(bool refresh)
{
wxDataViewItem plate_item = AddPlate(nullptr, _L("Outside")); // <=
m_plate_outside = (ObjectDataViewModelNode*)plate_item.GetID();
return plate_item;
}
// ....
wxDataViewItem ObjectDataViewModel::AddPlate
(PartPlate* part_plate, wxString name, bool refresh)
{
int plate_idx = part_plate ? part_plate->get_index() : -1; // <= 1
wxString plate_name = name;
if (name.empty())
{
plate_name = _L("Plate");
plate_name += wxString::Format(" %d", plate_idx + 1);
if (!part_plate->get_plate_name().empty()) // <= 2
{
plate_name += wxString(" (",
wxConvUTF8) + from_u8(part_plate->get_plate_name()) + wxString(")",
wxConvUTF8);
}
}
// ....
for (int obj_idx = 0; obj_idx < m_objects.size(); obj_idx++) {
auto obj_node = m_objects[obj_idx];
if (part_plate && part_plate->contain_instance_totally(obj_idx, 0)){// <= 3
ReparentObject(plate_node, obj_node);
}
}
return plate_item;
}Фрагмент большой, поэтому давайте по порядку. В функции-члене AddOutsidePlate происходит вызов функции-члена AddPlate, где первым аргументом передают nullptr. Соответствующий параметр — part_plate — проверяется на валидность в двух из трёх случаев использования. Если в одном месте проверки нет, то компилятор может посчитать, что и остальные проверки не нужны, и просто уберёт их в целях оптимизации.
Можно исправить код, добавив недостающую проверку. Интересный факт: если посмотреть на историю изменений, можно увидеть, что изначально там стоял тернарный оператор, но потом код переписали.
Пример N7
Предупреждение PVS-Studio: V797 The 'find' function is used as if it returned a bool type. The return value of the function should probably be compared with std::string::npos. HintNotification.cpp 175
TagCheckResult tag_check_material(const std::string& tag)
{
if (const GUI::Tab* tab = wxGetApp().get_tab(Preset::Type::TYPE_FILAMENT))
{
// search PrintConfig filament_type to find if allowed tag
if (wxGetApp().app_config->get("filament_type").find(tag))
{
const Preset& preset = tab->m_presets->get_edited_preset();
const auto* opt =
preset.config.opt<ConfigOptionStrings>("filament_type");
if (opt->values[0] == tag)
return TagCheckAffirmative;
return TagCheckNegative;
}
return TagCheckNotCompatible;
}
return TagCheckNotCompatible;
}В коде есть комментарий, который сообщает, что необходимо найти нужный тег. Ниже находится условие, в котором и происходит поиск тега. Но проблема в том, что оно работает не так, как задумывалось:
find вернёт 0, что будет сконвертировано в false;find вернёт положительное число, что будет сконвертировано в true;find вернёт std::string::npos, что будет сконвертировано в true.В итоге условие проверяет не то, что тег найден, а то, что такой тег не находится в начале строки. Исправить код можно следующим образом:
if (wxGetApp().app_config->get("filament_type").find(tag) != std::string::npos)Пример N8
Предупреждение PVS-Studio: V714 Variable 'face' is not passed into foreach loop by a reference, but its value is changed inside of the loop. MeshBoolean.cpp 153
template<class _Mesh>
void triangle_mesh_to_cgal(const TriangleMesh& M, _Mesh& out)
{
// ....
// Number the faces because 'orient_to_bound_a_volume'
// needs a face <--> index map
unsigned index = 0;
for (auto face : out.faces()) // <=
face = CGAL::SM_Face_index(index++);
// ....
}Анализатор сообщает о странном диапазонном цикле: переменная цикла face объявляется как копия очередного элемента диапазона out.faces() и при этом модифицируется на каждой итерации. По всей видимости, код должен был изменить элементы диапазона out.faces().
Если элементы какого-то диапазона нужно изменить, то переменная цикла должна быть объявлена как lvalue-ссылка:
for (auto &face : out.faces())
face = CGAL::SM_Face_index(index++);Давайте на всякий случай убедимся, что элементы диапазона out.faces() не являются аналогами std::reference_wrapper.
template <typename I>
class Iterator_range
: public std::pair<I,I>
{
public:
I begin() const
{
return this->first;
}
I end() const
{
return this->second;
}
template <typename T>
Iterator_range<T>
make_range(const T& b, const T&e)
{
return Iterator_range<T>(b,e);
}
// ....
};
template <typename T>
class SM_Index
{
// ....
public:
typedef boost::uint32_t size_type;
// ....
protected:
size_type idx_;
};
class SM_Face_index
: public SM_Index<SM_Face_index>
{ /* .... */ };
typedef SM_Face_index Face_index;
template <typename P>
class Surface_mesh
{
// ....
private:
template<typename Index_>
class Index_iterator
: public boost::iterator_facade< Index_iterator<Index_>,
Index_,
std::random_access_iterator_tag,
Index_
>
{
// ....
private:
friend class boost::iterator_core_access;
// ....
Index_ dereference() const { return hnd_; }
Index_ hnd_;
const Surface_mesh* mesh_;
};
// ....
public:
typedef Index_iterator<Face_index> Face_iterator;
typedef Iterator_range<Face_iterator> Face_range;
Face_iterator faces_begin() const
{
return Face_iterator(Face_index(0), this);
}
/// End iterator for faces.
Face_iterator faces_end() const
{
return Face_iterator(Face_index(num_faces()), this);
}
Face_range faces() const {
return make_range(faces_begin(), faces_end());
}
// ....
};Кода много, но попробуем разобраться. В шаблоне функции triangle_mesh_to_cgal параметр out имеет шаблонный тип _Mesh. Чаще всего аргументами шаблона функции выступают специализации шаблона класса Surface_mesh.
Внутри него в приватной части определяется шаблон итератора Index_iterator, который наследуется от boost::iterator_facade. По сути, этот шаблон класса упрощает имплементацию итераторов. Давайте посмотрим на его шаблонные параметры:
template <
class Derived // The derived iterator type being constructed
, class Value
, class CategoryOrTraversal
, class Reference = Value&
, class Difference = std::ptrdiff_t
>
class iterator_facadeЧетвёртый шаблонный параметр задаёт тип reference, т. е. то, что будет возвращать итератор при разыменовании. Если он не указан явно, то считается, что reference — это Value &.
Интересуемый нас шаблон класса Index_iterator передаёт четвёртым аргументом свой шаблонный тип, т. е. при разыменовании итератора будет возвращаться копия Index_. В качестве шаблонного типа Index_ выступает тип Face_index. Это псевдоним типа SM_Face_index, в котором не определяются никакие поля и который наследуется от шаблона класса SM_Index. В самом шаблоне класса SM_Index определяется одно поле idx типа boost::uint32_t, т. е. встроенного интегрального типа.
Я затрудняюсь ответить, каким именно должен был быть код в этой ситуации, т. к. изменение типа переменной face на auto & не выглядит как исправление. Более того, код должен перестать компилироваться, т. к. lvalue-ссылка не может быть привязана к prvalue-объекту.
При разыменовании итератора Face_iterator возвращается копия объекта Face_index. Чтобы модифицировать что-то внутри диапазона out.faces(), нужно, чтобы итератор возвращал ссылку и был каким-то образом ассоциирован с внутренностями Surface_mesh. Тогда бы диапазонный цикл for работал корректно. Однако сам Surface_mesh с итератором находится в third-party библиотеке, и я не уверена, что там можно делать какие-либо изменения.
Пример N9
Предупреждение PVS-Studio: V609 Divide by zero. Denominator range [0x0..0x7FFFFFFFFFFFFFFF]. TreeSupport3D.cpp 811
[[nodiscard]] static Polygons safe_offset_inc(....)
{
if (distance == 0)
return do_final_difference ? diff(ret, collision_trimmed())
: union_(ret);
if (safe_step_size < 0 || last_step_offset_without_check < 0) {
BOOST_LOG_TRIVIAL(warning)
<< "Offset increase got invalid parameter!";
tree_supports_show_error(
"Negative offset distance... How did you manage this ?"sv, true);
return do_final_difference ? diff(ret, collision_trimmed())
: union_(ret);
}
coord_t step_size = safe_step_size;
int steps = distance > last_step_offset_without_check
? (distance - last_step_offset_without_check) / step_size
: 0;
// ....
}Анализатор сообщает, что произошло деление на ноль. Чтобы понять, в чём дело, нужно начать с конца. Переменная step_size равна safe_step_size, а значит, деление на ноль произойдёт, если safe_step_size == 0.
В начале функции есть проверка safe_step_size < 0. Она отлавливает только отрицательные значения, но не проверяет равенство нулю. Значит, раннего выхода не случится, если safe_step_size == 0, и из-за этого произойдёт деление на ноль.
Эта проблема встречается довольно часто и связана с использованием констант 0, 1, 2. Мы обнаружили такую закономерность и даже написали об этом статью "Ноль, один, два, Фредди заберёт тебя".
Другие предупреждения V609:
Пример N10
Предупреждение PVS-Studio: V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. PerimeterGenerator.cpp 1057
template <typename LineType>
class LinesDistancer {
using Floating = typename std::conditional<
std::is_floating_point<Scalar>::value, Scalar, double>::type;
template<bool SIGNED_DISTANCE> Floating distance_from_lines(/*.... */)
//....
}
std::tuple<std::vector<ExtrusionPaths>, Polygons>
generate_extra_perimeters_over_overhangs(....)
{
// ....
for (size_t i = 0; i < overhang_region.front().polyline.size(); i++)
{
Point p = overhang_region.front().polyline.points[i].to_point();
if (double d =
lower_layer_aabb_tree.distance_from_lines<true>(p) < min_dist)
{
min_dist = d;
min_dist_idx = i;
}
}
// ....
}Хотели написать красивый код, но не учли приоритет операций. Оператор < выполняется раньше присваивания, поэтому переменная d получает результат сравнения, то есть значение типа bool, а не double. И в итоге в переменную min_dist записывается либо 0, либо 1, а не реальное расстояние. Исправить код можно так:
double d = lower_layer_aabb_tree.distance_from_lines<true>(p);
if (d < min_dist)Или так, если компилятор поддерживает инструкцию if с инициализатором (C++17):
if (double d = lower_layer_aabb_tree.distance_from_lines<true>(p);
d < min_dist)Пример N11
Предупреждение PVS-Studio: V766 An item with the same key '"open"' has already been added. DialogButtons.hpp 69
class DialogButtons : public wxPanel
{
private:
const std::map<wxString, wxStandardID> m_standardIDs = {
// Choice
{"ok" , wxID_OK},
{"yes" , wxID_YES},
{"apply" , wxID_APPLY},
{"confirm" , wxID_APPLY}, // no id for confirm, reusing wxID_APPLY
{"no" , wxID_NO},
{"cancel" , wxID_CANCEL},
// Action
{"open" , wxID_PRINT}, // <=
{"open" , wxID_OPEN}, // <=
//....
}В инициализацию std::map добавили два элемента с ключом open. Контейнер не может содержать несколько одинаковых элементов, поэтому второй элемент не будет добавлен.
Если посмотреть на значения, то становится понятно, что при копировании забыли заменить ключ для wxID_PRINT. Из-за этого будет использоваться не то значение. Скорее всего, исправить код можно так:
{"print" , wxID_PRINT},
{"open" , wxID_OPEN},V766 An item with the same key '"use_firmware_retraction"' has already been added. Print.cpp 229
V766 An item with the same key '"filament_notes"' has already been added. Print.cpp 237
V766 An item with the same key '"nozzle_volume"' has already been added. PrintConfig.cpp 8201
V766 An item with the same key '"inner-outer-inner wall/infill"' has already been added. PrintConfig.cpp 272
Пример N12
Предупреждение PVS-Studio: V734 An excessive expression. Examine the substrings "plus" and "xplus". QidiPrinterAgent.cpp 375
std::string QidiPrinterAgent::infer_series_id
(const std::string& model_id, const std::string& dev_name)
{
// ....
if ( ( key.find("xplus") != std::string::npos
|| key.find("plus") != std::string::npos)
&& key.find("4") != std::string::npos)
{
return "0";
}
return "";
}Анализатор обнаружил лишний вызов в условии. Если key содержит подстроку xplus, то она содержит и plus. Второе условие find("plus") избыточно, т. к. оно будет истинно всегда, когда истинно первое. Достаточно проверить только plus:
if (key.find("plus") != std::string::npos && key.find("4") != std::string::npos)А ещё сам код выглядит не очень эффективно: строку перебирают три раза.
Пример N13
Предупреждение PVS-Studio: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: wxEmptyString. Search.cpp 96
static Option create_option
(const std::string &opt_key, const wxString &label,
Preset::Type type, const GroupAndCategory &gc)
{
wxString suffix;
wxString suffix_local;
if (gc.category == "Machine limits") {
//suffix = opt_key.back() == '1' ? L("Stealth") : L("Normal");
suffix = opt_key.back() == '1' ? wxEmptyString : wxEmptyString;
suffix_local = " " + _(suffix);
suffix = " " + suffix;
}
// ....
}Опечатка в тернарном операторе: оба варианта возвращают wxEmptyString. Условие не влияет на результат. По закомментированной выше строке видно, что ранее здесь была другая логика, но в текущем виде код работает совершенно по-другому.
Точно такой же код был скопирован и в другое место:
V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: wxEmptyString. Search.cpp 348
Пример N14
Предупреждение PVS-Studio: V670 The uninitialized class member 'm_result' is used to initialize the 'm_options_z_corrector' member. Remember that members are initialized in the order of their declarations inside a class. GCodeProcessor.cpp 1671
class OptionsZCorrector
{
GCodeProcessorResult& m_result;
public:
explicit OptionsZCorrector(GCodeProcessorResult& result) : m_result(result) {
}
// ....
}
class GCodeProcessor
{
// ....
OptionsZCorrector m_options_z_corrector; // line: 811
// ....
GCodeProcessorResult m_result; // line: 843
// ....
}
GCodeProcessor::GCodeProcessor()
: m_options_z_corrector(m_result)
{
// ....
}Использование неинициализированной переменной в списке инициализации конструктора. Поля класса инициализируются в порядке их объявления, и в этом коде m_options_z_corrector инициализируется раньше, чем m_result.
Сейчас код работает корректно, потому что место в памяти под объект выделяется раньше, чем происходит инициализация. В таком случае есть возможность взять адрес и прочитать его, что и было сделано в конструкторе OptionsZCorrector. Но если в нём будет использоваться переменная m_result, то возникнет неопределённое поведение.
Чтобы код работал корректно, нужно поменять местами объявления полей в классе так, чтобы m_result было выше, чем m_options_z_corrector.
Если вы тоже хотите минимизировать ошибки в своих проектах, сделайте статический анализ частью регулярного процесса, а помочь с этим может PVS-Studio.
Ознакомиться с ценами, запросить пробную версию или узнать условия бесплатного лицензирования вы можете в соответствующих разделах.
Смотрите в будущее своего проекта и ловите ошибки как можно раньше :)
0
0
0
0