Our website uses cookies to enhance your browsing experience.
How can you avoid overlooking dangerous code parts during reviews? You can use static analysis tools. Let's take as an example OrcaSlicer, a popular slicing software designed to prepare 3D models for printing. We'll take a peek under its hood and see what surprises await us.
There's a cool project on GitHub that helps prepare 3D models for printing. It can convert a 3D model into machine instructions for creating an object layer by layer.
OrcaSlicer is one of the most popular free open-source slicers. The project is quite large, and its primary development language is C++, which is exactly why it caught my attention. Today, let's take a look under the hood and explore some of the code's highlights.
Note. The purpose of this content is to promote static analysis technology, not to offend the authors of OrcaSlicer.
The article simply shows that even functioning projects can contain bugs. Developers may overlook issues during code reviews or testing, and they accumulate over the years, which can ultimately lead to defects in the product. However, using additional tools, such as static analyzers, helps developers detect bugs before they reach production.
If you still doubt the value of such tools, I recommend reading the article "Why you should choose the PVS-Studio static analyzer to integrate into your development process."
I analyzed this project commit using PVS-Studio plugin for Visual Studio 2022. The article doesn't cover every warning the analyzer issued, only the ones I found the most interesting.
Note. To improve readability, the code snippets have been formatted, and some have been hidden behind comments marked with // .....
Example N1
PVS-Studio warning: 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;
}
// ....
}Here's a typo that developers missed during code review. The code uses + instead of +=, so the string literal never gets appended to the message variable. The code compiles, but it makes no sense. This warning reminded me of an error we caught in qbEngine, where devs forgot to add the # character in the #else macro.
When looking at this code, one might think: "No way! I'd never write something like this." It's simply a mistake a human can do, though. This code has been in the project for four years, yet it still hasn't been fixed. At the same time, the fragment where the message string is initialized was modified just a couple of months ago.
Example N2
PVS-Studio warning: 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;
};
// ....
}The lambda expression captures the do_stop local variable by reference and then stores it in params.on_packed. However, do_stop is destroyed when the process method exits because the lifetime of the local object ends. If the lambda runs after the member function returns, it will access a destroyed object, resulting in undefined behavior.
Capturing by value would solve this issue, but the lambda modifies the do_stop variable, so this approach won't work. Instead, do_stop can be turned into a member of the FillBedJob class.
This pattern repeats two more times in the code:
Example N3
PVS-Studio warning: 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}; } // <=
// ....
};
// ....
}The analyzer detected an enum value being used as the first operand in a ternary operator. The roPortrait constant has a value of 1, so the condition is always true. Most likely, the developers should've used another variable here.
Alternatively, an additional parameter may have been needed to compare it with roPortrait, as done in the constructor:
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)
{}Example N4
PVS-Studio warning: 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;
}
// ....
}These errors are especially intriguing because they seem impossible to make at first glance. The case may seem simple, but it's actually quite confusing. If we look through the list of issues the V781 diagnostic rule detected, we can see that even developers working on fairly large projects make these mistakes quite often.
What's the problem, though? It's array index out of bounds. First, the m_load_slot_index index is used to access an element of the m_amsinfo.cans vector, only then it's checked whether the iterator is out of bounds or negative. The check exists, but it happens too late. We can fix the code by placing the check earlier or handling this case differently.
The same warning:
Example N5
PVS-Studio warning: 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);
}
}
// ....
}This fragment contains another check that also appears after the iterator is used. Theit iterator is dereferenced to calculate z_shift, and the next line contains a check whether it differs from the end() iterator. If the element isn't found, dereferencing the resulting iterator leads to undefined behavior.
The fixed code:
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);
}Example N6
PVS-Studio warnings:
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;
}This is a long snippet, so let's break it down step by step. The AddOutsidePlate member function calls AddPlate and passes nullptr as the first argument. The part_plate parameter is checked two out of the three times where it's used. If a check is missing somewhere, the compiler may assume that the other checks are unnecessary as well and remove them for optimization purposes.
We can fix the code by adding the missing check. Here's a fun fact: if we look at the commit history, we can see that there was originally a ternary operator there, but the code was later rewritten.
Example N7
PVS-Studio warning: 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;
}The code contains a comment explaining that a tag needs to be found. Below is the condition where the tag is searched for. However, it doesn't work as intended:
find returns 0, which is converted to false;find returns a positive number, which is converted to true;find returns std::string::npos, which is also converted to true.As a result, the condition doesn't check whether the tag was found. Instead, it checks whether the tag isn't located at the beginning of the string. The fix may look like this:
if (wxGetApp().app_config->get("filament_type").find(tag) != std::string::npos)Example N8
PVS-Studio warning: 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++);
// ....
}The analyzer reports a suspicious range-based loop: the face loop variable is declared as a copy of the current element from the out.faces() range, but the code modifies it on every iteration. Most likely, the code was intended to modify elements inside the out.faces() range.
If you need to modify elements within a range, declare the loop variable as an lvalue reference:
for (auto &face : out.faces())
face = CGAL::SM_Face_index(index++);Let's also make sure that the elements in the out.faces() range aren't the same as 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());
}
// ....
};There's a lot of code here, so let's try to break it down. In the triangle_mesh_to_cgal function template, the out parameter has the _Mesh template type. Most often, function template arguments are specializations of the Surface_mesh class template.
In the private class section, the template of the Index_iterator iterator, which inherits from boost::iterator_facade, is defined. Essentially, this class template simplifies iterator implementation. Let's take a look at its template parameters:
template <
class Derived // The derived iterator type being constructed
, class Value
, class CategoryOrTraversal
, class Reference = Value&
, class Difference = std::ptrdiff_t
>
class iterator_facadeThe fourth template parameter specifies the reference type, which is the value the iterator returns when dereferenced. If it isn't specified explicitly, reference defaults to Value &.
The Index_iterator class template we're interested in passes its own template type as the fourth argument. This means dereferencing the iterator returns a copy of Index_. The Index_ template parameter is of the Face_index type. This is an alias of the SM_Face_index type, which doesn't define any data members and inherits from the SM_Index class template. The SM_Index class template defines a single idx data member of the boost::uint32_t type, which is a built-in integral type.
I find it difficult to say what the code should've looked like here, since changing the face variable type to auto & doesn't seem like a valid fix. Moreover, the code should stop compiling because an lvalue reference can't bind to a prvalue object.
When Face_iterator is dereferenced, it returns a copy of the Face_index object. To modify anything within the out.faces() range, the iterator would need to return a reference and somehow be connected to the Surface_mesh internal data. In that case, the range-based for loop would work correctly. However, Surface_mesh and its iterator come from a third-party library, and I doubt we can make any changes there.
Example N9
PVS-Studio warning: 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;
// ....
}The analyzer reports a division by zero. To understand what's happening, we'll start from the end. The step_size variable equals safe_step_size, which means the division by zero occurs if safe_step_size == 0.
At the beginning of the function, there's a safe_step_size < 0 check, which catches only negative values and doesn't check for zero. This means the function doesn't return early when safe_step_size == 0, which eventually leads to a division by zero.
This issue appears quite often and is related to using the constants 0, 1, and 2. We noticed this pattern and even wrote an article about it: "Zero, one, two, Freddy's coming for you."
Other V609 warnings:
Example N10
PVS-Studio warning: 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;
}
}
// ....
}The devs wanted to write clean code, but overlooked operator precedence. The < operator has higher precedence than assignment, so the d variable receives the result of the comparison—a bool value instead of double. As a result, min_dist gets either 0 or 1 instead of the actual distance. Here's how we can fix this:
double d = lower_layer_aabb_tree.distance_from_lines<true>(p);
if (d < min_dist)Alternatively, we can fix it like this if the compiler supports the if statement with an initializer (C++17):
if (double d = lower_layer_aabb_tree.distance_from_lines<true>(p);
d < min_dist)Example N11
PVS-Studio warning: 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}, // <=
//....
}Two elements with the open key were added to the initialization of std::map. A container can't store multiple elements with the same key, so the second element won't be added.
Looking at the values, we can see that the key for wxID_PRINT wasn't updated after copying. As a result, the wrong value will be used. The fix may look like this:
{"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
Example N12
PVS-Studio warning: 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 "";
}The analyzer detected an unnecessary call in the condition. If key contains the xplus substring, it also contains plus. The second find("plus") check is redundant because it will always evaluate to true whenever the first one does. Checking plus is enough:
if (key.find("plus") != std::string::npos && key.find("4") != std::string::npos)The code also looks rather inefficient: the string is iterated over three times.
Example N13
PVS-Studio warning: 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;
}
// ....
}There's a typo in the ternary operator: both branches return wxEmptyString. The condition doesn't affect the result. The commented-out line above indicates that the code previously had different logic, but it works differently in its current form.
The exact same code was copied to another snippet as well:
V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: wxEmptyString. Search.cpp 348
Example N14
PVS-Studio warning: 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)
{
// ....
}An uninitialized variable is used in the constructor initialization list. Class data members initialize in the order they're declared, and in this code, m_options_z_corrector initializes before m_result.
Currently, the code works correctly because memory for the object gets allocated before initialization begins. In this case, it's possible to retrieve the address and read it, which is exactly what happens in the OptionsZCorrector constructor. However, if the constructor starts using the m_result variable, it will lead to undefined behavior.
To fix the code, the data member declarations in the class should be reordered so that m_result appears before m_options_z_corrector.
If you wish to minimize errors in your projects, incorporate static analysis into your regular workflow—and PVS-Studio can help you do just that.
You can request pricing, a trial version, or learn about free licensing terms in the relevant sections.
Look ahead to your project's future and catch errors as early as possible :)
0
0
0
0