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.
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 " using The strings were concatenated but are not utilized. Consider inspecting the 'message + "\n\nApplication will close."' expression. , where devs forgot to add the
#character in the#elsemacro.
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
messagestring is initialized was Lifetime of the lambda is greater than lifetime of the local variable 'do_stop' captured by reference.
V1047 Lifetime of the lambda is greater than lifetime of the local variable 'do_stop' captured by reference. The enumeration constant 'roPortrait' is used as a variable of a Boolean-type. 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. 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_indexindex is used to access an element of them_amsinfo.cansvector, 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:
- V781 The value of the 'i' index is checked after it was used. Perhaps there is a mistake in program logic. ViewerImpl.cpp 56
Example N5
PVS-Studio warning:
CODEvoid 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. The
ititerator is dereferenced to calculatez_shift, and the next line contains a check whether it differs from theend()iterator. If the element isn't found, dereferencing the resulting iterator leads to undefined behavior.
The fixed code:
CODEif (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:
CODEwxDataViewItem 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
AddOutsidePlatemember function callsAddPlateand passesnullptras the first argument. Thepart_plateparameter 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 originally a ternary operator there, but the code was later rewritten.
Example N7
PVS-Studio warning:
CODETagCheckResult 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:
- if the tag is found at the beginning,
findreturns0, which is converted tofalse;
- if the tag is found somewhere in the middle,
findreturns a positive number, which is converted totrue;
- if the tag is not found,
findreturnsstd::string::npos, which is also converted totrue.
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:
CODEif (wxGetApp().app_config->get("filament_type").find(tag) != std::string::npos)
Example N8
PVS-Studio warning:
CODEtemplate<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
faceloop variable is declared as a copy of the current element from theout.faces()range, but the code modifies it on every iteration. Most likely, the code was intended to modify elements inside theout.faces()range.
If you need to modify elements within a range, declare the loop variable as an lvalue reference:
CODEfor (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 asstd::reference_wrapper.
Spoiler: no, they aren't
CODEtemplate <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_cgalfunction template, the out parameter has the_Meshtemplate type. Most often, function template arguments are specializations of theSurface_meshclass template.
In the private class section, the template of the
Index_iteratoriterator, which inherits from[boost::iterator_facade](https://www.boost.org/doc/libs/latest/libs/iterator/doc/facade-and-adaptor.html), is defined. Essentially, this class template simplifies iterator implementation. Let's take a look at its template parameters:
CODEtemplate <
class Derived // The derived iterator type being constructed
, class Value
, class CategoryOrTraversal
, class Reference = Value&
, class Difference = std::ptrdiff_t
>
class iterator_facade
The fourth template parameter specifies the
[reference](https://cppreference.com/cpp/iterator/iterator_traits)type, which is the value the iterator returns when dereferenced. If it isn't specified explicitly,referencedefaults toValue &.
The
Index_iteratorclass template we're interested in passes its own template type as the fourth argument. This means dereferencing the iterator returns a copy ofIndex_. TheIndex_template parameter is of theFace_indextype. This is an alias of theSM_Face_indextype, which doesn't define any data members and inherits from theSM_Indexclass template. TheSM_Indexclass template defines a singleidxdata member of theboost::uint32_ttype, which is a built-in integral type.
I find it difficult to say what the code should've looked like here, since changing the
facevariable type toauto &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_iteratoris dereferenced, it returns a copy of theFace_indexobject. To modify anything within theout.faces()range, the iterator would need to return a reference and somehow be connected to theSurface_meshinternal data. In that case, the range-basedforloop would work correctly. However,Surface_meshand its iterator come from a third-party library, and I doubt we can make any changes there.
Example N9
PVS-Studio warning:
CODE[[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_sizevariable equalssafe_step_size, which means the division by zero occurs ifsafe_step_size == 0.
At the beginning of the function, there's a
safe_step_size < 0check, which catches only negative values and doesn't check for zero. This means the function doesn't return early whensafe_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: " Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. An item with the same key '"open"' has already been added. wasn't updated after copying. As a result, the wrong value will be used. The fix may look like this:
CODE{"print" , wxID_PRINT},
{"open" , wxID_OPEN},
Other V766 warnings
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:
CODEstd::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
keycontains thexplussubstring, it also containsplus. The secondfind("plus")check is redundant because it will always evaluate to true whenever the first one does. Checkingplusis enough:
CODEif (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:
CODEstatic 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:
Example N14
PVS-Studio warning:
CODEclass 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_correctorinitializes —and , a in the relevant sections.
Look ahead to your project's future and catch errors as early as possible :)
↗ Original-Artikel auf dev.to lesenVollständiger Original-BerichtAusführliche Details, Code-Beispiele & Hersteller-Stellungnahme auf dev.to.
SOCIAL SHARE CARD GENERATOR