Somehow, we've happened to check most of the libraries making up a collection called "Awesome hpp". These are small header-only projects in C++. Hopefully, the information about the bugs we've found will help make the libraries better. We'll also be happy to see the developers use PVS-Studio on a regular basis thanks to the free-license option we provide.
What follows is an overview of bugs found in the different libraries on the curated list of awesome header-only C++ libraries: awesome-hpp.
I learned about this list from the "Cross Platform Mobile Telephony" podcast. While we're at it, I recommend that all C++ programmers check out CppCast. It is the first podcast for C++ developers by C++ developers!
Despite the large number of projects making up the list, bugs were few. There are three reasons for that:
Even so, we've gathered enough warnings to write the article you are reading right now and a couple of additional ones.
A note for my teammates :). When I do something, I like to set and achieve a number of goals at once, and I urge you to follow my example. After learning about the awesome-hpp collection, I managed to accomplish the following useful tasks:
A note for library developers. You can use PVS-Studio to check open-source projects for free. To get a free license to use with your open-source project, please fill in this form.
Okay, let's move on to our overview.
Brief description of the iutest library:
iutest is framework for writing C++ tests.
template<typename Event>
pool_handler<Event> & assure() {
....
return static_cast<pool_handler<Event> &>(it == pools.cend() ?
*pools.emplace_back(new pool_handler<Event>{}) : **it);
....
}PVS-Studio diagnostic message: V1023 A pointer without owner is added to the 'pools' container by the 'emplace_back' method. A memory leak will occur in case of an exception. entt.hpp 17114
This code may end up with a memory leak. If the container needs reallocation and fails to allocate storage for a new array, it will throw an exception and the pointer will be lost.
Well, when found in tests, this kind of bug isn't very critical and likely to occur, but I still had to mention it for educational purposes :).
Fixed code:
pools.emplace_back(std::make_unique<pool_handler<Event>>{})Another problem spot: V1023 A pointer without owner is added to the 'pools' container by the 'emplace_back' method. A memory leak will occur in case of an exception. entt.hpp 17407
Brief description of the jsoncons library:
A C++, header-only library for constructing JSON and JSON-like data formats, with JSON Pointer, JSON Patch, JSONPath, JMESPath, CSV, MessagePack, CBOR, BSON, UBJSON.
Bug 1
static constexpr uint64_t basic_type_bits = sizeof(uint64_t) * 8;
uint64_t* data()
{
return is_dynamic() ? dynamic_stor_.data_ : short_stor_.values_;
}
basic_bigint& operator<<=( uint64_t k )
{
size_type q = (size_type)(k / basic_type_bits);
....
if ( k ) // 0 < k < basic_type_bits:
{
uint64_t k1 = basic_type_bits - k;
uint64_t mask = (1 << k) - 1; // <=
....
data()[i] |= (data()[i-1] >> k1) & mask;
....
}
reduce();
return *this;
}PVS-Studio diagnostic message: V629 Consider inspecting the '1 << k' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. bigint.hpp 744
This bug was already discussed in detail in the article "Why it is important to apply static analysis for open libraries that you add to your project". In a nutshell, to get correct values of the mask, you should write the following:
uint64_t mask = (static_cast<uint64_t>(1) << k) - 1;Here's an alternative version:
uint64_t mask = (1ull << k) - 1;Another similar bug was found here: V629 Consider inspecting the '1 << k' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. bigint.hpp 779
Bug 2
template <class CharT = typename std::iterator_traits<Iterator>::value_type>
typename std::enable_if<sizeof(CharT) == sizeof(uint16_t)>::type
next() UNICONS_NOEXCEPT
{
begin_ += length_;
if (begin_ != last_)
{
if (begin_ != last_)
{
....
}PVS-Studio diagnostic message: V571 Recurring check. The 'if (begin_ != last_)' condition was already verified in line 1138. unicode_traits.hpp 1140
This is a strange duplicate check. I suspect the second condition contains a typo and was meant to check some other value.
Brief description of the clipp library:
clipp - command line interfaces for modern C++. Easy to use, powerful and expressive command line argument handling for C++11/14/17 contained in a single header file.
inline bool
fwd_to_unsigned_int(const char*& s)
{
if(!s) return false;
for(; std::isspace(*s); ++s);
if(!s[0] || s[0] == '-') return false;
if(s[0] == '-') return false;
return true;
}PVS-Studio diagnostic message: V547 Expression 's[0] == '-'' is always false. clipp.h 303
This one isn't actually a bug – just redundant code. The element is checked twice for being the minus character.
Brief description of the SimpleIni library:
A cross-platform library that provides a simple API to read and write INI-style configuration files. It supports data files in ASCII, MBCS and Unicode.
#if defined(SI_NO_MBSTOWCS_NULL) || (!defined(_MSC_VER) && !defined(_linux))PVS-Studio diagnostic message: V1040 Possible typo in the spelling of a pre-defined macro name. The '_linux' macro is similar to '__linux'. SimpleIni.h 2923
It seems an underscore is missing in the _linux macro's name: __linux. Anyway, this macro is deprecated in POSIX, so you should use __linux__ instead.
Brief description of the CSV Parser library:
A modern C++ library for reading, writing, and analyzing CSV (and similar) files.
CSV_INLINE void CSVReader::read_csv(const size_t& bytes) {
const size_t BUFFER_UPPER_LIMIT = std::min(bytes, (size_t)1000000);
std::unique_ptr<char[]> buffer(new char[BUFFER_UPPER_LIMIT]);
auto * HEDLEY_RESTRICT line_buffer = buffer.get();
line_buffer[0] = '\0';
....
this->feed_state->feed_buffer.push_back(
std::make_pair<>(std::move(buffer), line_buffer - buffer.get())); // <=
....
}PVS-Studio diagnostic message: V769 The 'buffer.get()' pointer in the 'line_buffer - buffer.get()' expression equals nullptr. The resulting value is senseless and it should not be used. csv.hpp 4957
It's an interesting case, which calls for careful investigation, so I decided to write a small separate post about it. Besides, while experimenting with similar code, I discovered a flaw in the code of PVS-Studio itself :). Because of that flaw, the analyzer will sometimes keep silent when it must issue a warning.
In a nutshell, whether or not this code will work depends on the order of argument evaluation – and that depends on the compiler.
Brief description of the PPrint library:
Pretty Printer for Modern C++.
template <typename Container>
typename std::enable_if<......>::type print_internal(......) {
....
for (size_t i = 1; i < value.size() - 1; i++) {
print_internal(value[i], indent + indent_, "", level + 1);
if (is_container<T>::value == false)
print_internal_without_quotes(", ", 0, "\n");
else
print_internal_without_quotes(", ", 0, "\n");
}
....
}PVS-Studio diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. pprint.hpp 715
It's very strange to have the same logic executed regardless of the check's result. No clarifying comment is provided either. It looks very much like a copy-paste mistake.
Similar warnings:
Brief description of the Strf library:
A fast C++ formatting library that supports encoding conversion.
Bug 1
template <int Base>
class numpunct: private strf::digits_grouping
{
....
constexpr STRF_HD numpunct& operator=(const numpunct& other) noexcept
{
strf::digits_grouping::operator=(other);
decimal_point_ = other.decimal_point_;
thousands_sep_ = other.thousands_sep_;
}
....
};PVS-Studio diagnostic message: V591 Non-void function should return a value. numpunct.hpp 402
The "return *this;" statement is missing at the end.
Bug 2 (of the same nature)
template <int Base>
class no_grouping final
{
constexpr STRF_HD no_grouping& operator=(const no_grouping& other) noexcept
{
decimal_point_ = other.decimal_point_;
}
....
}PVS-Studio diagnostic message: V591 Non-void function should return a value. numpunct.hpp 528.
Brief description of the Indicators library:
Activity Indicators for Modern C++.
static inline void move_up(int lines) { move(0, -lines); }
static inline void move_down(int lines) { move(0, -lines); } // <=
static inline void move_right(int cols) { move(cols, 0); }
static inline void move_left(int cols) { move(-cols, 0); }PVS-Studio diagnostic message: V524 It is odd that the body of 'move_down' function is fully equivalent to the body of 'move_up' function. indicators.hpp 983
I'm not sure this one is an error, but the code is highly suspicious. The developer must have copied the move_up function and changed the copy's name to move_down but forgotten to delete the minus character. In any case, this snippet needs checking.
Note. If the code is correct, you should understand that it's going to confuse not only static analyzers but third-party developers as well, who may want to use or develop it. Make sure you leave a comment to clarify your point.
Brief description of the manif library:
manif is a header-only C++11 Lie theory library for state-estimation targeted at robotics applications.
template <typename _Derived>
typename LieGroupBase<_Derived>::Scalar*
LieGroupBase<_Derived>::data()
{
return derived().coeffs().data();
}
template <typename _Derived>
const typename LieGroupBase<_Derived>::Scalar*
LieGroupBase<_Derived>::data() const
{
derived().coeffs().data(); // <=
}PVS-Studio diagnostic message: V591 Non-void function should return a value. lie_group_base.h 347
The non-constant function is implemented properly, while the constant one is not. I wonder how it came to be so...
Brief description of the FakeIt library:
FakeIt is a simple mocking framework for C++. It supports GCC, Clang and MS Visual C++. FakeIt is written in C++11 and can be used for testing both C++11 and C++ projects.
template<typename ... arglist>
struct ArgumentsMatcherInvocationMatcher :
public ActualInvocation<arglist...>::Matcher {
....
template<typename A>
void operator()(int index, A &actualArg) {
TypedMatcher<typename naked_type<A>::type> *matcher =
dynamic_cast<TypedMatcher<typename naked_type<A>::type> *>(
_matchers[index]);
if (_matching)
_matching = matcher->matches(actualArg);
}
....
const std::vector<Destructible *> _matchers;
};PVS-Studio diagnostic message: V522 There might be dereferencing of a potential null pointer 'matcher'. fakeit.hpp 6720
The matcher pointer is initialized with the value returned by dynamic_cast. Yet that operator can return nullptr, which is very likely. Otherwise, use the more efficient static_cast instead of dynamic_cast.
I suspect the condition contains a typo and was actually meant to look like this:
if (matcher)
_matching = matcher->matches(actualArg);Brief description of the GuiLite library:
The smallest header-only GUI library(4 KLOC) for all platforms.
#define CORRECT(x, high_limit, low_limit) {\
x = (x > high_limit) ? high_limit : x;\
x = (x < low_limit) ? low_limit : x;\
}while(0)
void refresh_wave(unsigned char frame)
{
....
CORRECT(y_min, m_wave_bottom, m_wave_top);
....
}PVS-Studio diagnostic message: V529 Odd semicolon ';' after 'while' operator. GuiLite.h 3413
This error in the macro doesn't cause any specific problem, but it's still an error, so I included it.
The macro was meant to use the classic pattern do { .... } while(....). This allows executing several operations in one block while enabling you to write a nice semicolon after the macro as if it were a function call.
This macro, however, lacks the do keyword. As a result, it splits into two parts, so to speak: a block of code and an empty, never-running while (0); loop.
But why is it bad, actually?
Well, for one thing, you can't use this macro in constructs like this:
if (A)
CORRECT(y_min, m_wave_bottom, m_wave_top);
else
Foo();This code won't compile as it will expand into the following:
if (A)
{ ..... }
while(0);
else
Foo();I think you'll agree that it's better to find and fix defects like that while the library is still in development than after the release. To ensure this, use static analysis :).
Brief description of the PpluX library:
Single header C++ Libraries for Thread Scheduling, Rendering, and so on...
struct DisplayList {
DisplayList& operator=(DisplayList &&d) {
data_ = d.data_;
d.data_ = nullptr;
}
....
}PVS-Studio diagnostic message: V591 Non-void function should return a value. px_render.h 398
Brief description of the Universal library:
The goal of Universal Numbers, or unums, is to replace IEEE floating-point with a number system that is more efficient and mathematically consistent in concurrent execution environments.
Bug 1
template<typename Scalar>
vector<Scalar> operator*(double scalar, const vector<Scalar>& v) {
vector<Scalar> scaledVector(v);
scaledVector *= scalar;
return v;
}PVS-Studio diagnostic message: V1001 The 'scaledVector' variable is assigned but is not used by the end of the function. vector.hpp 124
This is a typo. The function must return the new scaledVector vector rather than the original v vector.
Another similar typo: V1001 The 'normalizedVector' variable is assigned but is not used by the end of the function. vector.hpp 131
Bug 2
template<typename Scalar>
class matrix {
....
matrix& diagonal() {
}
....
};PVS-Studio diagnostic message: V591 Non-void function should return a value. matrix.hpp 109
Bug 3
template<size_t fbits, size_t abits>
void module_subtract_BROKEN(
const value<fbits>& lhs, const value<fbits>& rhs, value<abits + 1>& result)
{
if (lhs.isinf() || rhs.isinf()) {
result.setinf();
return;
}
int lhs_scale = lhs.scale(),
rhs_scale = rhs.scale(),
scale_of_result = std::max(lhs_scale, rhs_scale);
// align the fractions
bitblock<abits> r1 =
lhs.template nshift<abits>(lhs_scale - scale_of_result + 3);
bitblock<abits> r2 =
rhs.template nshift<abits>(rhs_scale - scale_of_result + 3);
bool r1_sign = lhs.sign(), r2_sign = rhs.sign();
//bool signs_are_equal = r1_sign == r2_sign;
if (r1_sign) r1 = twos_complement(r1);
if (r1_sign) r2 = twos_complement(r2); // <=
....
}PVS-Studio diagnostic message: V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 789, 790. value.hpp 790
It's a classic copy-paste bug. The programmer cloned the following line:
if (r1_sign) r1 = twos_complement(r1);Changed r1 to r2:
if (r1_sign) r2 = twos_complement(r2);But forgot to change r1_sign. Here's the correct version:
if (r2_sign) r2 = twos_complement(r2);Brief description of the Chobo Single-Header Libraries library:
A collection of small single-header C++11 libraries by Chobolabs.
Bug 1
template <typename T, typename U, typename Alloc = std::allocator<T>>
class vector_view
{
....
vector_view& operator=(vector_view&& other)
{
m_vector = std::move(other.m_vector);
}
....
}PVS-Studio diagnostic message: V591 Non-void function should return a value. vector_view.hpp 163
Bug 2
template <typename UAlloc>
vector_view& operator=(const std::vector<U, UAlloc>& other)
{
size_type n = other.size();
resize(n);
for (size_type i = 0; i < n; ++i)
{
this->at(i) = other[i];
}
}PVS-Studio diagnostic message: V591 Non-void function should return a value. vector_view.hpp 184
Brief description of the PGM-index library:
The Piecewise Geometric Model index (PGM-index) is a data structure that enables fast lookup, predecessor, range searches and updates in arrays of billions of items using orders of magnitude less space than traditional indexes while providing the same worst-case query time guarantees.
Bug 1
char* str_from_errno()
{
#ifdef MSVC_COMPILER
#pragma warning(disable:4996)
return strerror(errno);
#pragma warning(default:4996)
#else
return strerror(errno);
#endif
}PVS-Studio diagnostic message: V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 9170, 9172. sdsl.hpp 9172
This code temporarily disables a compiler warning but does that in an incorrect way. Such mistakes can be tolerated in user code, but certainly not in header-only libraries.
Bug 2
template<class t_int_vec>
t_int_vec rnd_positions(uint8_t log_s, uint64_t& mask,
uint64_t mod=0, uint64_t seed=17)
{
mask = (1<<log_s)-1; // <=
t_int_vec rands(1<<log_s ,0);
set_random_bits(rands, seed);
if (mod > 0) {
util::mod(rands, mod);
}
return rands;
}PVS-Studio diagnostic message: V629 Consider inspecting the '1 << log_s' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. sdsl.hpp 1350
Here's one correct version:
mask = ((uint64_t)(1)<<log_s)-1;Brief description of the Hnswlib library:
Header-only C++ HNSW implementation with python bindings. Paper's code for the HNSW 200M SIFT experiment.
template<typename dist_t>
class BruteforceSearch : public AlgorithmInterface<dist_t> {
public:
BruteforceSearch(SpaceInterface <dist_t> *s, size_t maxElements) {
maxelements_ = maxElements;
data_size_ = s->get_data_size();
fstdistfunc_ = s->get_dist_func();
dist_func_param_ = s->get_dist_func_param();
size_per_element_ = data_size_ + sizeof(labeltype);
data_ = (char *) malloc(maxElements * size_per_element_);
if (data_ == nullptr)
std::runtime_error(
"Not enough memory: BruteforceSearch failed to allocate data");
cur_element_count = 0;
}
....
}PVS-Studio diagnostic message: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); bruteforce.h 26
The throw operator is missing before std::runtime_error.
A similar problem: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); bruteforce.h 161
Brief description of the tiny-dnn library:
tiny-dnn is a C++14 implementation of deep learning. It is suitable for deep learning on limited computational resource, embedded systems and IoT devices.
Bug 1
class nn_error : public std::exception {
public:
explicit nn_error(const std::string &msg) : msg_(msg) {}
const char *what() const throw() override { return msg_.c_str(); }
private:
std::string msg_;
};
inline Device::Device(device_t type, const int platform_id, const int device_id)
: type_(type),
has_clcuda_api_(true),
platform_id_(platform_id),
device_id_(device_id) {
....
#else
nn_error("TinyDNN has not been compiled with OpenCL or CUDA support.");
#endif
}PVS-Studio diagnostic message: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw nn_error(FOO); device.h 68
nn_error is not an exception-throwing function but simply a class, and the correct way of using it is as follows:
throw nn_error("TinyDNN has not been compiled with OpenCL or CUDA support.");Here's another case of improper use of this class: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw nn_error(FOO); conv2d_op_opencl.h 136
Bug 2
inline std::string format_str(const char *fmt, ...) {
static char buf[2048];
#ifdef _MSC_VER
#pragma warning(disable : 4996)
#endif
va_list args;
va_start(args, fmt);
vsnprintf(buf, sizeof(buf), fmt, args);
va_end(args);
#ifdef _MSC_VER
#pragma warning(default : 4996)
#endif
return std::string(buf);
}PVS-Studio diagnostic message: V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 139, 146. util.h 146
Brief description of the Dlib library:
Dlib is a modern C++ toolkit containing machine learning algorithms and tools for creating complex software in C++ to solve real world problems.
Bug 1
To keep things interesting, I dare you to find the bug on your own.
class bdf_parser
{
public:
enum bdf_enums
{
NO_KEYWORD = 0,
STARTFONT = 1,
FONTBOUNDINGBOX = 2,
DWIDTH = 4,
DEFAULT_CHAR = 8,
CHARS = 16,
STARTCHAR = 32,
ENCODING = 64,
BBX = 128,
BITMAP = 256,
ENDCHAR = 512,
ENDFONT = 1024
};
....
bool parse_header( header_info& info )
{
....
while ( 1 )
{
res = find_keywords( find | stop );
if ( res & FONTBOUNDINGBOX )
{
in_ >> info.FBBx >> info.FBBy >> info.Xoff >> info.Yoff;
if ( in_.fail() )
return false; // parse_error
find &= ~FONTBOUNDINGBOX;
continue;
}
if ( res & DWIDTH )
{
in_ >> info.dwx0 >> info.dwy0;
if ( in_.fail() )
return false; // parse_error
find &= ~DWIDTH;
info.has_global_dw = true;
continue;
}
if ( res & DEFAULT_CHAR )
{
in_ >> info.default_char;
if ( in_.fail() )
return false; // parse_error
find &= ~DEFAULT_CHAR;
continue;
}
if ( res & NO_KEYWORD )
return false; // parse_error: unexpected EOF
break;
}
....
};Any luck?
Here it is:
if ( res & NO_KEYWORD )PVS-Studio diagnostic message: V616 The 'NO_KEYWORD' named constant with the value of 0 is used in the bitwise operation. fonts.cpp 288
The value of the named constant NO_KEYWORD is 0. Therefore, the condition doesn't make sense. This is what the correct check should look like:
if ( res == NO_KEYWORD )Another incorrect check: V616 The 'NO_KEYWORD' named constant with the value of 0 is used in the bitwise operation. fonts.cpp 334
Bug 2
void set(std::vector<tensor*> items)
{
....
epa.emplace_back(new enable_peer_access(*g[0], *g[i]));
....
}PVS-Studio diagnostic message: V1023 A pointer without owner is added to the 'epa' container by the 'emplace_back' method. A memory leak will occur in case of an exception. tensor_tools.h 1665
To figure out what's wrong here, see the V1023 documentation.
Bug 3
template <
typename detection_type,
typename label_type
>
bool is_track_association_problem (
const std::vector<
std::vector<labeled_detection<detection_type,label_type> > >& samples
)
{
if (samples.size() == 0)
return false;
unsigned long num_nonzero_elements = 0;
for (unsigned long i = 0; i < samples.size(); ++i)
{
if (samples.size() > 0)
++num_nonzero_elements;
}
if (num_nonzero_elements < 2)
return false;
....
}PVS-Studio diagnostic message: V547 Expression 'samples.size() > 0' is always true. svm.h 360
It's a very, very strange piece of code! If the loop starts, then the (samples.size() > 0) condition is always true. And that means the loop can be simplified:
for (unsigned long i = 0; i < samples.size(); ++i)
{
++num_nonzero_elements;
}But now it becomes clear that you don't need the loop at all. The snippet could be rewritten in a much simpler and more efficient way:
unsigned long num_nonzero_elements = samples.size();But was it really the developer's intention? This code certainly needs close inspection.
Bug 4
class console_progress_indicator
{
....
double seen_first_val;
....
};
bool console_progress_indicator::print_status (
double cur, bool always_print)
{
....
if (!seen_first_val)
{
start_time = cur_time;
last_time = cur_time;
first_val = cur;
seen_first_val = true; // <=
return false;
}
....
}PVS-Studio diagnostic message: V601 The bool type is implicitly cast to the double type. console_progress_indicator.h 136
The value true is stored to a class member of type double. Hmm....
Bug 5
void file::init(const std::string& name)
{
....
WIN32_FIND_DATAA data;
HANDLE ffind = FindFirstFileA(state.full_name.c_str(), &data);
if (ffind == INVALID_HANDLE_VALUE ||
(data.dwFileAttributes&FILE_ATTRIBUTE_DIRECTORY) != 0)
{
throw file_not_found("Unable to find file " + name);
}
else
{
....
}
}PVS-Studio diagnostic message: V773 The exception was thrown without closing the file referenced by the 'ffind' handle. A resource leak is possible. dir_nav_kernel_1.cpp 60
An exception is thrown if the directory is found. But how about closing the file handle?
Bug 6
Another extremely strange spot.
inline double poly_min_extrap(double f0, double d0,
double x1, double f_x1,
double x2, double f_x2)
{
....
matrix<double,2,2> m;
matrix<double,2,1> v;
const double aa2 = x2*x2;
const double aa1 = x1*x1;
m = aa2, -aa1,
-aa2*x2, aa1*x1;
v = f_x1 - f0 - d0*x1,
f_x2 - f0 - d0*x2;
....
}PVS-Studio diagnostic message: V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. optimization_line_search.h 211
The plan was to have the matrices initialized. But all those aa2, f_x1, d0, etc. are simply variables of type double. And that means the commas don't separate the arguments, which are used to form the matrices, but simply act as the comma operator, which returns the value of the right operand.
At the beginning of this article, I gave you an example of how you can pursue several goals at once. Using a static analyzer is, too, beneficial for several reasons:
The only question left is how to get started with static analysis, integrate it smoothly, and use it properly. The following articles will give you all the answers you need: