Ошибки в коде

Волею судьбы мы проверили большинство библиотек, входящих в коллекцию под названием "Awesome hpp". Это небольшие проекты на языке C++, состоящие только из заголовочных файлов. Надеемся, найденные ошибки помогут сделать эти библиотеки немного лучше. Также мы будем рады, если их авторы начнут бесплатно использовать анализатор PVS-Studio на регулярной основе.

Предлагаю вашему вниманию обзор результатов проверки различных библиотек, перечисленных в списке awesome-hpp (A curated list of awesome header-only C++ libraries).

Впервые про этот список я узнал из подкаста "Cross Platform Mobile Telephony". Пользуясь случаем, рекомендую всем C++ программистам познакомиться с CppCast. CppCast is the first podcast for C++ developers by C++ developers!

Несмотря на большое количество проектов в списке, ошибок нашлось совсем немного. Тому есть три причины:

• Это очень маленькие проекты. Многие буквально состоят из одного заголовочного файла.
• Мы проверили не все проекты. С компиляцией некоторых из них возникли проблемы, и мы решили их пропустить.
• Часто, чтобы понять, есть ли ошибки в шаблонных классах/функциях или нет, они должны инстанцироваться. Соответственно многие ошибки смогут быть выявлены анализатором только в настоящем проекте, когда библиотека активно используется. Мы же просто включали заголовочные файлы в пустой .cpp файл и проверяли, что делает проверку малоэффективной.

Тем не менее в процессе изучения предупреждений их набралось достаточно, чтобы написать эту статью и парочку дополнительных.



Примечание для разработчиков библиотек. Желающие могут бесплатно использовать анализатор PVS-Studio для проверки открытых проектов. Для получения лицензии для вашего открытого проекта заполните, пожалуйста, эту форму.

Теперь давайте наконец посмотрим, что нашлось в некоторых библиотеках.

Найденные ошибки

Библиотека iutest

Краткое описание библиотеки iutest:

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: 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

Этот код потенциально может привести к утечке памяти. В случае, если контейнеру понадобится реаллокация и он не сможет выделить память под новый массив, то он бросит исключение и указатель будет потерян.

Пожалуй, для тестов эта ситуация маловероятна и некритична. Однако я решил упомянуть этот недостаток в образовательных целях :).

Правильный вариант:

pools.emplace_back(std::make_unique<pool_handler<Event>>{})

Ещё одно такое же место: 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

Библиотека jsoncons

Краткое описание библиотеки jsoncons:

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.

Первая ошибка

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: 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

Эта ошибка подробно уже рассматривалась в статье "Почему важно проводить статический анализ открытых библиотек, которые вы добавляете в свой проект". Если совсем кратко, то, чтобы получать корректные значения маски, нужно написать так:

uint64_t mask = (static_cast<uint64_t>(1) << k) - 1;

Или так:

uint64_t mask = (1ull << k) - 1;

Точно такую же ошибку, как первая, можно увидеть здесь: 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

Вторая ошибка

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: V571 Recurring check. The 'if (begin_ != last_)' condition was already verified in line 1138. unicode_traits.hpp 1140

Странная повторная проверка. Есть подозрение, что здесь какая-то опечатка и второе условие должно выглядеть как-то иначе.

Библиотека clipp

Краткое описание библиотеки clipp:

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 boolfwd_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: V547 Expression 's[0] == '-'' is always false. clipp.h 303

Ну на самом деле это не ошибка, а просто избыточный код. Проверка на наличие минуса выполняется дважды.

Библиотека SimpleIni

Краткое описание библиотеки SimpleIni:

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: V1040 Possible typo in the spelling of a pre-defined macro name. The '_linux' macro is similar to '__linux'. SimpleIni.h 2923

Скорее всего, в имени макроса _linux не хватает одного подчёркивания и должно использоваться имя __linux. Впрочем, в POSIX этот макрос объявлен устаревшим и лучше использовать __linux__.

Библиотека CSV Parser

Краткое описание библиотеки CSV Parser:

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: 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

Интересная ситуация, которая требует внимательного рассмотрения. Поэтому я решил, что напишу про это отдельную маленькую заметку. Плюс, ставя эксперименты с аналогичным кодом, я выявил недоработку в самом PVS-Studio :). В некоторых случаях он молчит, хотя должен выдавать предупреждения.

Если совсем кратко, работает этот код или нет, зависит от порядка вычисления аргументов. А в каком порядке вычисляются аргументы, зависит от компилятора.

Библиотека PPrint

Краткое описание библиотеки PPrint:.

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: V523 The 'then' statement is equivalent to the 'else' statement. pprint.hpp 715

Очень странно, что независимо от условия выполняется одно и то же действие. Нет и какого-то специального поясняющего комментария. Всё это очень похоже на Copy-Paste ошибку.

Аналогичные предупреждения:

• V523 The 'then' statement is equivalent to the 'else' statement. pprint.hpp 780
• V523 The 'then' statement is equivalent to the 'else' statement. pprint.hpp 851
• V523 The 'then' statement is equivalent to the 'else' statement. pprint.hpp 927
• V523 The 'then' statement is equivalent to the 'else' statement. pprint.hpp 1012

Библиотека Strf

Краткое описание библиотеки Strf:

A fast C++ formatting library that supports encoding conversion.

Первая ошибка

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: V591 Non-void function should return a value. numpunct.hpp 402

В конце функции забыли написать "return *this;".

Вторая аналогичная ошибка

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: V591 Non-void function should return a value. numpunct.hpp 528.

Библиотека Indicators

Краткое описание библиотеки Indicators:

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: V524 It is odd that the body of 'move_down' function is fully equivalent to the body of 'move_up' function. indicators.hpp 983

Я не уверен, что это ошибка. Но код очень подозрительный. Высока вероятность, что была скопирована функция move_up и заменено её имя на move_down. А вот минус удалить забыли. Стоит проверить этот код.

Примечание. Если код верен, надо понимать, что он вводит в заблуждение не только анализаторы кода, но и сторонних программистов, которые захотят использовать или развивать этот код. Полезно сопровождать такой код комментариями.

Библиотека manif

Краткое описание библиотеки manif:

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: V591 Non-void function should return a value. lie_group_base.h 347

Неконстантная функция реализована правильно, а константная нет. Интересно даже, как так получилось

Библиотека FakeIt

Краткое описание библиотеки FakeIt:

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: V522 There might be dereferencing of a potential null pointer 'matcher'. fakeit.hpp 6720

Указатель matcher инициализируется значением, которое возвращает оператор dynamic_cast. А этот оператор может возвращать nullptr, и это весьма вероятный сценарий. Иначе вместо dynamic_cast эффективнее использовать static_cast.

Есть подозрение, что в условии допущена опечатка и на самом деле должно быть написано:

if (matcher)  _matching = matcher->matches(actualArg);

Библиотека GuiLite

Краткое описание библиотеки GuiLite:

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: V529 Odd semicolon ';' after 'while' operator. GuiLite.h 3413

К какой-то проблеме ошибка в макросе не приводит. Но всё равно это ошибка, поэтому я решил описать её в статье.

В макросе планировалось использовать классический паттерн do { } while(....). Это позволяет выполнить несколько действий в одном блоке и при этом иметь возможность для красоты после макроса писать точку с запятой ';', как будто это вызов функции.

Но в рассмотренном макросе случайно забыли написать ключевое слово do. В результате макрос как-бы разделился на две части. Первая это блок. Вторая пустой не выполняющийся цикл: while (0);.

А в чём, собственно, проблема?

Например, такой макрос нельзя использовать в конструкции вида:

if (A)  CORRECT(y_min, m_wave_bottom, m_wave_top);else  Foo();

Этот код не скомпилируется, так как он будет раскрыт в:

if (A)  { ..... }while(0);else  Foo();

Согласитесь, такую проблему лучше найти и исправить на этапе разработки библиотеки, а не на этапе её использования. Применяйте статический анализ кода :).

Библиотека PpluX

Краткое описание библиотеки PpluX:

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: V591 Non-void function should return a value. px_render.h 398

Библиотека Universal

Краткое описание библиотеки Universal:

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.

Первая ошибка

template<typename Scalar>vector<Scalar> operator*(double scalar, const vector<Scalar>& v) {  vector<Scalar> scaledVector(v);  scaledVector *= scalar;  return v;}

Предупреждение PVS-Studio: V1001 The 'scaledVector' variable is assigned but is not used by the end of the function. vector.hpp 124

Опечатка. Вместо исходного вектора v из функции нужно вернуть новый вектор scaledVector.

Аналогичную опечатку можно увидеть здесь: V1001 The 'normalizedVector' variable is assigned but is not used by the end of the function. vector.hpp 131

Вторая ошибка

template<typename Scalar>class matrix {  ....  matrix& diagonal() {  }  ....};

Предупреждение PVS-Studio: V591 Non-void function should return a value. matrix.hpp 109

Третья ошибка

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: V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 789, 790. value.hpp 790

Классическая ошибка, возникшая из-за Copy-Paste. Взяли и размножили строчку:

if (r1_sign) r1 = twos_complement(r1);

Поменяли в ней r1 на r2:

if (r1_sign) r2 = twos_complement(r2);

А поменять r1_sign забыли. Правильный вариант:

if (r2_sign) r2 = twos_complement(r2);

Библиотека Chobo Single-Header Libraries

Краткое описание библиотеки Chobo Single-Header Libraries:

A collection of small single-header C++11 libraries by Chobolabs.

Первая ошибка

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: V591 Non-void function should return a value. vector_view.hpp 163

Вторая ошибка

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: V591 Non-void function should return a value. vector_view.hpp 184

Библиотека PGM-index

Краткое описание библиотеки PGM-index:

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.

Первая ошибка

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: 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

Неправильное временное отключение предупреждения компилятора. Подобные неаккуратности ещё как-то простительны пользовательскому коду. Но это точно недопустимо в header-only библиотеках.

Вторая ошибка

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: 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

Один из правильных вариантов:

mask = ((uint64_t)(1)<<log_s)-1;

Библиотека Hnswlib

Краткое описание библиотеки Hnswlib:

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: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); bruteforce.h 26

Забыли перед std::runtime_error написать оператор throw.

Ещё одна такая ошибка: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); bruteforce.h 161

Библиотека tiny-dnn

Краткое описание библиотеки tiny-dnn:

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.

Первая ошибка

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: 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 это не функция, генерирующая исключение, а просто класс. Поэтому правильно его использовать так:

throw nn_error("TinyDNN has not been compiled with OpenCL or CUDA support.");

Ещё одно неправильное использование этого класса: 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

Вторая ошибка

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: 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

Библиотека Dlib

Краткое описание библиотеки Dlib:

TDLib (Telegram Database library) is a cross-platform library for building Telegram clients. It can be easily used from almost any programming language.

Первая ошибка

Ради интереса попробуйте найти эту ошибку самостоятельно.

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;    }  ....};

Нашли?


Она здесь:

if ( res & NO_KEYWORD )

Предупреждение PVS-Studio: V616 The 'NO_KEYWORD' named constant with the value of 0 is used in the bitwise operation. fonts.cpp 288

Именованная константа NO_KEYWORD имеет значение 0. А следовательно условие не имеет смысла. Правильно было бы написать:

if ( res == NO_KEYWORD )

Ещё одна неправильная проверка находится здесь: V616 The 'NO_KEYWORD' named constant with the value of 0 is used in the bitwise operation. fonts.cpp 334

Вторая ошибка

void set(std::vector<tensor*> items){  ....  epa.emplace_back(new enable_peer_access(*g[0], *g[i]));  ....}

Предупреждение PVS-Studio: 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

Чтобы понять, в чём тут заковыка, предлагаю познакомиться с документацией на диагностику V1023.

Третья ошибка

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: V547 Expression 'samples.size() > 0' is always true. svm.h 360

Это очень, очень странный код! Если запускается цикл, то значит условие (samples.size() > 0) всегда истинно. Следовательно, цикл можно упростить:

for (unsigned long i = 0; i < samples.size(); ++i){  ++num_nonzero_elements;}

После этого становится понятно, что цикл вообще не нужен. Можно написать гораздо проще и эффективнее:

unsigned long num_nonzero_elements = samples.size();

Но это ли планировалось сделать? Код явно заслуживает внимательного изучения программистом.

Четвёртая ошибка

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: V601 The bool type is implicitly cast to the double type. console_progress_indicator.h 136

В член класса, имеющий тип double, записывают значение true. Хм

Пятая ошибка

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: 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

Если найдена директория, то генерируется исключение. Но кто будет закрывать дескриптор?

Шестая ошибка

Ещё одно очень странное место.

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: V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. optimization_line_search.h 211

Планируется инициализировать матрицы. Но ведь все эти aa2, f_x1, d0 и так далее это просто переменные типа double. Значит, запятые не разделяют аргументы, предназначенные для создания матриц, а являются обыкновенными comma operator, которые возвращают значение правой части.

Заключение

В начале статьи я привёл пример, как можно совместить несколько полезных дел сразу. Использование статического анализатора тоже одновременно полезно по нескольким причинам:

• Повышение квалификации. Изучая предупреждения анализатора можно узнать много нового и полезного. Примеры: memset, #pragma warning, emplace_back, strictly aligned.
• Выявление опечаток, ошибок и потенциальных уязвимостей на ранних этапах.
• Код постепенно становится качественнее, проще, понятней.
• Вы можете гордиться и всем рассказывать, что используете современные технологии при разработке проектов :). И это юмор только отчасти. Это настоящее конкурентное преимущество.

Вопрос только в том, как начать, как безболезненно внедрить и как правильно использовать? С этим вам помогут следующие статьи:

• Как быстро посмотреть интересные предупреждения, которые выдает анализатор PVS-Studio для C и C++ кода?
• Как внедрить статический анализатор кода в legacy проект и не демотивировать команду.
• Внедряйте статический анализ в процесс, а не ищите с его помощью баги.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Checking a Header-Only C++ Library Collection (awesome-hpp).

Относительно недавно состоялся релиз фреймворка Qt 6, и это стало поводом вновь проверить его с помощью PVS-Studio. В статье будут рассмотрены различные интересные ошибки, например, связанные с обработкой дат. Обнаружение всех этих ошибок хорошо демонстрирует пользу, которую может получить проект от использования таких инструментов, как PVS-Studio, особенно если они применяются регулярно.

Это классическая статья о проверке открытого проекта, которая пополнит нашу "доказательную базу" полезности и эффективности использования PVS-Studio для контроля качества кода. Хотя мы уже писали про поверку проекта Qt (в 2011, в 2014 и в 2018), очень полезно сделать это вновь. Так, мы на практике демонстрируем простую, но очень важную мысль: статический анализ должен применяться регулярно!

Наши статьи показывают, что анализатор PVS-Studio умеет находить множество разнообразнейших ошибок. И, как правило, авторы проектов быстро исправляют описанные нами ошибки. Однако всё это не имеет ничего общего с правильной и полезной методикой регулярного применения статических анализаторов. Когда анализатор встроен в процесс разработки, это позволяет быстро находить ошибки в новом или изменённом коде и тем самым их исправление обходится максимально дёшево.

Всё, теория заканчивается. Давайте посмотрим, что интересного нас ждёт в коде. А пока вы будете читать статью, предлагаю скачать PVS-Studio и запросить демонстрационный ключ, чтобы посмотреть, что интересного найдётся в ваших собственных проектах :).

Даты

Кажется, намечается ещё один паттерн кода, где любят собираться ошибки. Он, конечно, не такой масштабный, как функции сравнения или последние строки в однотипных блоках. Речь идёт про фрагменты кода, работающие с датами. Наверное, такой код сложно тестировать, и в итоге эти недотестированные функции будут давать некорректный результат на определённых наборах входных данных. Про пару таких случаев уже было рассказано в статье "Почему PVS-Studio не предлагает автоматические правки кода".

Встретились ошибки обработки дат и в Qt. Давайте с них и начнём.

Фрагмент N1: неправильная интерпретация статуса ошибки

Для начала нам следует посмотреть, как устроена функция, возвращающая номер месяца по его сокращённому названию.

static const char qt_shortMonthNames[][4] = {    "Jan", "Feb", "Mar", "Apr", "May", "Jun",    "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"};static int fromShortMonthName(QStringView monthName){  for (unsigned int i = 0;       i < sizeof(qt_shortMonthNames) / sizeof(qt_shortMonthNames[0]); ++i)  {    if (monthName == QLatin1String(qt_shortMonthNames[i], 3))      return i + 1;  }  return -1;}

В случае успеха функция возвращает номер месяца (значение от 1 до 12). Если имя месяца некорректно, то функция возвращает отрицательное значение (-1). Обратите внимание, что функция не может вернуть значение 0.

Но там, где только что рассмотренная функция используется, программист как раз рассчитывает, что в качестве статуса ошибки ему будет возвращено нулевое значение. Фрагмент кода, некорректно использующий функцию fromShortMonthName:

QDateTime QDateTime::fromString(QStringView string, Qt::DateFormat format){  ....  month = fromShortMonthName(parts.at(1));  if (month)    day = parts.at(2).toInt(&ok);  // If failed, try day then month  if (!ok || !month || !day) {    month = fromShortMonthName(parts.at(2));    if (month) {      QStringView dayPart = parts.at(1);      if (dayPart.endsWith(u'.'))        day = dayPart.chopped(1).toInt(&ok);    }  }  ....}

Проверки номера месяца на равенство нулю никогда не сработают, и программа продолжит выполняться с некорректным отрицательным номером месяца. Анализатор PVS-Studio видит здесь целый букет несостыковок, о чем сообщает сразу четырьмя сообщениями:

• V547 [CWE-571] Expression 'month' is always true. qdatetime.cpp 4907
• V560 [CWE-570] A part of conditional expression is always false: !month. qdatetime.cpp 4911
• V547 [CWE-571] Expression 'month' is always true. qdatetime.cpp 4913
• V560 [CWE-570] A part of conditional expression is always false: !month. qdatetime.cpp 4921

Фрагмент N2: ошибка логики обработки даты

Для начала посмотрим на реализацию функции, возвращающей количество секунд.

enum {  ....  MSECS_PER_DAY = 86400000,  ....  SECS_PER_MIN = 60,};int QTime::second() const{    if (!isValid())        return -1;    return (ds() / 1000)%SECS_PER_MIN;}

Рассмотренная функция может вернуть значение в диапазоне [0..59] или статус ошибки -1.

В одном месте эта функция используется очень странным образом:

static qint64 qt_mktime(QDate *date, QTime *time, ....){  ....  } else if (yy == 1969 && mm == 12 && dd == 31             && time->second() == MSECS_PER_DAY - 1) {      // There was, of course, a last second in 1969, at time_t(-1); we won't      // rescue it if it's not in normalised form, and we don't know its DST      // status (unless we did already), but let's not wantonly declare it      // invalid.  } else {  ....}

Предупреждение PVS-Studio: V560 [CWE-570] A part of conditional expression is always false: time->second() == MSECS_PER_DAY 1. qdatetime.cpp 2488

Согласно комментарию, если что-то пошло не так, то лучше ничего не делать. Однако, условие всегда будет ложным, поэтому всегда выполняется else-ветка.

Ошибочно вот это сравнение:

time->second() == MSECS_PER_DAY - 1

MSECS_PER_DAY 1 это 86399999. Функция second, как мы уже знаем, никак не может вернуть такое значение. Таким образом, здесь какая-то логическая ошибка и код заслуживает пристального внимания разработчиков.

Сильная сторона статических анализаторов кода в том, что они проверяют все ситуации, независимо от частоты их выполнения. Таким образом, статический анализ хорошо дополняет юнит-тесты и другие методы контроля качества кода.

Опечатки

Фрагмент N3: неожиданно, мы поговорим о HTML!

QString QPixelTool::aboutText() const{  const QList<QScreen *> screens = QGuiApplication::screens();  const QScreen *windowScreen = windowHandle()->screen();  QString result;  QTextStream str(&result);  str << "<html></head><body><h2>Qt Pixeltool</h2><p>Qt " << QT_VERSION_STR    << "</p><p>Copyright (C) 2017 The Qt Company Ltd.</p><h3>Screens</h3><ul>";  for (const QScreen *screen : screens)    str << "<li>" << (screen == windowScreen ? "* " : "  ")        << screen << "</li>";  str << "<ul></body></html>";  return result;}

Предупреждение PVS-Studio: V735 Possibly an incorrect HTML. The "</ body>" closing tag was encountered, while the "</ ul>" tag was expected. qpixeltool.cpp 707

В PVS-Studio есть диагностики, которые не только проверят сам код, но и выискивают аномалии в строковых константах. Здесь как раз сработала одна из таких диагностик. Это достаточно редкий случай, зато этим он и примечательный.

Дважды используется тег открытия списка. Это явная опечатка. Первый тег, должен открывать список, а второй закрывать. Правильный код:

str << "</ul></body></html>";

Фрагмент N4: повторная проверка в условии

class Node{  ....  bool isGroup() const { return m_nodeType == Group; }  ....};void DocBookGenerator::generateDocBookSynopsis(const Node *node){  ....  if (node->isGroup() || node->isGroup()      || node->isSharedCommentNode() || node->isModule()      || node->isJsModule() || node->isQmlModule() || node->isPageNode())    return;  ....}

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions to the left and to the right of the '||' operator: node->isGroup() || node->isGroup() docbookgenerator.cpp 2599

Простая опечатка, но её исправление зависит от того, чего на самом деле хотели достичь в этом коде. Если проверка просто дублируется, то её стоит просто удалить. Но возможен и другой сценарий: не проверено какое-то другое нужное условие.

Фрагмент N5: создание лишней локальной переменной

void MainWindow::addToPhraseBook(){  ....  QString selectedPhraseBook;  if (phraseBookList.size() == 1) {    selectedPhraseBook = phraseBookList.at(0);    if (QMessageBox::information(this, tr("Add to phrase book"),          tr("Adding entry to phrasebook %1").arg(selectedPhraseBook),           QMessageBox::Ok | QMessageBox::Cancel, QMessageBox::Ok)                          != QMessageBox::Ok)      return;  } else {    bool okPressed = false;    QString selectedPhraseBook =       QInputDialog::getItem(this, tr("Add to phrase book"),                            tr("Select phrase book to add to"),                            phraseBookList, 0, false, &okPressed);    if (!okPressed)      return;  }  MessageItem *currentMessage = m_dataModel->messageItem(m_currentIndex);  Phrase *phrase = new Phrase(currentMessage->text(),                              currentMessage->translation(),                              QString(), nullptr);  phraseBookHash.value(selectedPhraseBook)->append(phrase);}

Кстати, если хотите, можете проверить свою внимательность и самостоятельно поискать ошибку. А чтобы вы сразу случайно не увидели ответ, добавлю сюда единорожку из старой коллекции. Возможно, вы даже его ещё не видели :).

Предупреждение PVS-Studio: V561 [CWE-563] It's probably better to assign value to 'selectedPhraseBook' variable than to declare it anew. Previous declaration: mainwindow.cpp, line 1303. mainwindow.cpp 1313

В обоих ветках условного оператора формируется текст, который должен быть записан в переменную selectedPhraseBook. Имя переменной длинное, и программисту было лень писать его заново. Поэтому он скопировал имя переменной из того места, где она объявляется. При этом он чуть поспешил и вместе с именем скопировал и тип переменной:

QString selectedPhraseBook =

В результате в else-блоке появилась ещё одна локальная строковая переменная, которая инициализируется, но не используется. Тем временем переменная с таким же именем во внешнем блоке останется пустой.

Фрагмент N6: приоритет операций

Классический паттерн ошибки, который встречается весьма часто.

bool QQmlImportInstance::resolveType(....){  ....  if (int icID = containingType.lookupInlineComponentIdByName(typeStr) != -1)  {    *type_return = containingType.lookupInlineComponentById(icID);  } else {    auto icType = createICType();    ....  }  ....}

Предупреждение PVS-Studio: V593 [CWE-783] Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. qqmlimport.cpp 754

Значение переменной icID всегда будет иметь значение 0 или 1. Это явно не то, что задумывалось. Причина: в начале происходит сравнение с -1, а только затем инициализация переменной icID.

Современный синтаксис C++ позволяет корректно записать это условие следующим образом:

if (int icID = containingType.lookupInlineComponentIdByName(typeStr);    icID != -1)

Кстати, очень похожую ошибку мы уже обнаруживали в Qt:

char ch;while (i < dataLen && ((ch = data.at(i) != '\n') && ch != '\r'))  ++i;

Но пока на вооружение не будет взят такой анализатор кода, как PVS-Studio, программисты вновь и вновь будут допускать такие ошибки. Никто не совершенен. И да, это тонкий намёк внедрить PVS-Studio :).

Фрагмент N7: коварное деление по модулю

Часто бывает необходимо определить, делится число без остатка на 2 или нет. Правильный вариант это поделить по модулю два и проверить результат:

if (A % 2 == 1)

Но программисты вновь и вновь ошибаются и пишут что-то типа этого:

if (A % 1 == 1)

Это естественно неправильно, так как остаток от деления по модулю на один это всегда ноль. Не обошлось без этой ошибки и в Qt:

bool loadQM(Translator &translator, QIODevice &dev, ConversionData &cd){  ....  case Tag_Translation: {    int len = read32(m);    if (len % 1) {                                             // <=      cd.appendError(QLatin1String("QM-Format error"));      return false;    }    m += 4;    QString str = QString((const QChar *)m, len/2);  ....}

Предупреждение PVS-Studio: V1063 The modulo by 1 operation is meaningless. The result will always be zero. qm.cpp 549

Фрагмент N8: перезаписывание значения

QString Node::qualifyQmlName(){  QString qualifiedName = m_name;  if (m_name.startsWith(QLatin1String("QML:")))    qualifiedName = m_name.mid(4);  qualifiedName = logicalModuleName() + "::" + m_name;  return qualifiedName;}

Предупреждение PVS-Studio: V519 [CWE-563] The 'qualifiedName' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1227, 1228. node.cpp 1228

Как я понимаю, случайно использовали не то имя переменной. Предполагаю, здесь должно быть написано так:

QString qualifiedName = m_name;if (m_name.startsWith(QLatin1String("QML:")))  qualifiedName = m_name.mid(4);qualifiedName = logicalModuleName() + "::" + qualifiedName;return qualifiedName;

Фрагмент N9: copy-paste

class Q_CORE_EXPORT QJsonObject{  ....  bool operator<(const iterator& other) const  { Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }  bool operator<=(const iterator& other) const  { Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }  ....}

Прудпреждение PVS-Studio: V524 It is odd that the body of '<=' function is fully equivalent to the body of '<' function. qjsonobject.h 155

Такие скучные функции, как операторы сравнения, никто не проверяет. На них обычно не пишут тесты. Их не просматривают или делают это очень быстро на code review. А зря. Статический анализ кода здесь как нельзя кстати. Анализатор не устаёт и с удовольствием проверяет даже такой скучный код.

Реализация операторов < и <= совпадают. Это явно неправильно. Скорее всего, код писался методом Copy-Paste, и затем забыли изменить всё что нужно в скопированном коде. Правильно:

bool operator<(const iterator& other) const{ Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }bool operator<=(const iterator& other) const{ Q_ASSERT(item.o == other.item.o); return item.index <= other.item.index; }

Фрагмент N10: static_cast / dynamic_cast

void QSGSoftwareRenderThread::syncAndRender(){  ....  bool canRender = wd->renderer != nullptr;  if (canRender) {     auto softwareRenderer = static_cast<QSGSoftwareRenderer*>(wd->renderer);     if (softwareRenderer)       softwareRenderer->setBackingStore(backingStore);  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'softwareRenderer' is always true. qsgsoftwarethreadedrenderloop.cpp 510

В начале рассмотрим вот эту проверку:

bool canRender = wd->renderer != nullptr;if (canRender) {

Благодаря ей можно быть уверенным, что внутри тела условного оператора значение указателя wd->renderer всегда точно ненулевое. Тогда непонятно, что же хотят проверить следующим кодом?

auto softwareRenderer = static_cast<QSGSoftwareRenderer*>(wd->renderer);if (softwareRenderer)

Если указатель wd->renderer ненулевой, то и указатель softwareRenderer точно ненулевой. Есть подозрение, что здесь опечатка, которая состоит в том, что на самом деле следовало использовать dynamic_cast. В этом случае код начинает приобретать смысл. Если преобразование типа невозможно, оператор dynamic_cast возвращает nullptr, и это возвращенное значение, естественно, следует проверять. Впрочем, возможно, я неправильно интерпретировал ситуацию и код нужно исправлять другим способом.

Фрагмент N11: скопировали блок кода и забыли изменить

void *QQuickPath::qt_metacast(const char *_clname){  if (!_clname) return nullptr;  if (!strcmp(_clname, qt_meta_stringdata_QQuickPath.stringdata0))    return static_cast<void*>(this);  if (!strcmp(_clname, "QQmlParserStatus"))    return static_cast< QQmlParserStatus*>(this);  if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus"))   // <=    return static_cast< QQmlParserStatus*>(this);  if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus"))   // <=    return static_cast< QQmlParserStatus*>(this);  return QObject::qt_metacast(_clname);}

Предупреждение PVS-Studio: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 2719, 2721. moc_qquickpath_p.cpp 2721

Эти две строчки:

if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus"))  return static_cast< QQmlParserStatus*>(this);

Были размножены с помощью Copy-Paste. После чего они не были модифицированы и не имеют смысла.

Фрагмент N12: переполнение из-за не там поставленной скобки

int m_offsetFromUtc;....void QDateTime::setMSecsSinceEpoch(qint64 msecs){  ....  if (!add_overflow(msecs, qint64(d->m_offsetFromUtc * 1000), &msecs))    status |= QDateTimePrivate::ValidWhenMask;  ....}

Предупреждение PVS-Studio: V1028 [CWE-190] Possible overflow. Consider casting operands of the 'd->m_offsetFromUtc * 1000' operator to the 'qint64' type, not the result. qdatetime.cpp 3922

Программист предвидит ситуацию, что при умножении переменной типа int на 1000 может произойти переполнение. Чтобы этого избежать, он планирует использовать при умножении 64-битный тип qint64. И использует явное приведение типа.

Вот только толку от этого приведения типа никакого нет. В начале всё равно произойдёт переполнение. И только затем выполнится приведение типа. Правильный вариант:

add_overflow(msecs, qint64(d->m_offsetFromUtc) * 1000, &msecs)

Фрагмент N13: не полностью инициализированный массив

class QPathEdge{  ....private:  int m_next[2][2];  ....};inline QPathEdge::QPathEdge(int a, int b)    : flag(0)    , windingA(0)    , windingB(0)    , first(a)    , second(b)    , angle(0)    , invAngle(0){    m_next[0][0] = -1;    m_next[1][0] = -1;    m_next[0][0] = -1;    m_next[1][0] = -1;}

Предупреждения PVS-Studio:

• V1048 [CWE-1164] The 'm_next[0][0]' variable was assigned the same value. qpathclipper_p.h 301
• V1048 [CWE-1164] The 'm_next[1][0]' variable was assigned the same value. qpathclipper_p.h 302

Перед нами неудачная попытка инициализировать массив размером 2x2. Два элемента инициализируются повторно, и два остаются неинициализированными. Правильный вариант:

m_next[0][0] = -1;m_next[0][1] = -1;m_next[1][0] = -1;m_next[1][1] = -1;

Я очень люблю такие примеры ошибок, которые встречаются в коде профессиональных разработчиков. Это как раз такой случай. Он показывает, что любой может опечататься, и поэтому статический анализ является вашим другом. Дело в том, что я уже десяток лет веду бой со скептиками, которые уверены, что такие ошибки можно встретить только в лабораторных работах студентов и что они такие ошибки никогда не делают :). Ещё 10 лет назад я написал заметку "Миф второй профессиональные разработчики не допускают глупых ошибок", и с тех пор, естественно, ничего не изменилось. Люди всё также делают такие ошибки и всё также утверждают, что это не так :).

Ошибки в логике

Фрагмент N14: недостижимый код

void QmlProfilerApplication::tryToConnect(){  Q_ASSERT(!m_connection->isConnected());  ++ m_connectionAttempts;  if (!m_verbose && !(m_connectionAttempts % 5)) {// print every 5 seconds    if (m_verbose) {      if (m_socketFile.isEmpty())        logError(          QString::fromLatin1("Could not connect to %1:%2 for %3 seconds ...")          .arg(m_hostName).arg(m_port).arg(m_connectionAttempts));      else        logError(          QString::fromLatin1("No connection received on %1 for %2 seconds ...")          .arg(m_socketFile).arg(m_connectionAttempts));    }  }  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'm_verbose' is always false. qmlprofilerapplication.cpp 495

Этот код никогда ничего не запишет в лог. Причиной являются противоположные условия:

if (!m_verbose && ....) {  if (m_verbose) {

Фрагмент N15: перетирание значения переменной

void QRollEffect::scroll(){  ....  if (currentHeight != totalHeight) {      currentHeight = totalHeight * (elapsed/duration)          + (2 * totalHeight * (elapsed%duration) + duration)          / (2 * duration);      // equiv. to int((totalHeight*elapsed) / duration + 0.5)      done = (currentHeight >= totalHeight);  }  done = (currentHeight >= totalHeight) &&         (currentWidth >= totalWidth);  ....}

Предупреждение PVS-Studio: V519 [CWE-563] The 'done' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 509, 511. qeffects.cpp 511

Весь условный оператор не имеет смысла, так как значение переменной done всё равно тут же перезаписывается. Возможно, здесь не хватает ключевого слова else.

Фрагмент N16-N20: перетирание значения переменной

Альтернативный вариант перетирания значения переменной.

bool QXmlStreamWriterPrivate::finishStartElement(bool contents){  ....  if (inEmptyElement) {    ....    lastNamespaceDeclaration = tag.namespaceDeclarationsSize;   // <=    lastWasStartElement = false;  } else {    write(">");  }  inStartElement = inEmptyElement = false;  lastNamespaceDeclaration = namespaceDeclarations.size();      // <=  return hadSomethingWritten;}

Предупреждение PVS-Studio: V519 [CWE-563] The 'lastNamespaceDeclaration' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3030, 3036. qxmlstream.cpp 3036

Возможно, первая запись значения в переменную lastNamespaceDeclaration является лишней и её можно удалить. А возможно, перед нами серьезная логическая ошибка.

Есть ещё 4 предупреждения, указывающих на такой же паттерн ошибки:

• V519 [CWE-563] The 'last' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 609, 637. qtextengine.cpp 637
• V519 [CWE-563] The 'm_dirty' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1014, 1017. qquickshadereffect.cpp 1017
• V519 [CWE-563] The 'changed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 122, 128. qsgdefaultspritenode.cpp 128
• V519 [CWE-563] The 'eaten' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 299, 301. qdesigner.cpp 301

Фрагмент N21: путаница между нулевым указателем и пустой строкой

// this could become a list of all languages used for each writing// system, instead of using the single most common language.static const char languageForWritingSystem[][6] = {    "",     // Any    "en",  // Latin    "el",  // Greek    "ru",  // Cyrillic    ...... // Нулевых указателей нет. Используются пустые строковые литералы.    "", // Symbol    "sga", // Ogham    "non", // Runic    "man" // N'Ko};static void populateFromPattern(....){  ....  for (int j = 1; j < QFontDatabase::WritingSystemsCount; ++j) {    const FcChar8 *lang = (const FcChar8*) languageForWritingSystem[j];    if (lang) {  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'lang' is always true. qfontconfigdatabase.cpp 462

В массиве languageForWritingSystem нет нулевых указателей. Поэтому проверка if(lang) не имеет смысла. Зато в массиве есть пустые строки. Быть может, хотелось сделать проверку именно на пустую строку? Если да, тогда корректный код должен выглядеть так:

if (strlen(lang) != 0) {

Или можно ещё проще написать:

if (lang[0] != '\0') {

Фрагмент N22: странная проверка

bool QNativeSocketEnginePrivate::createNewSocket(....){  ....  int socket = qt_safe_socket(domain, type, protocol, O_NONBLOCK);  ....  if (socket < 0) {    ....    return false;  }  socketDescriptor = socket;  if (socket != -1) {    this->socketProtocol = socketProtocol;    this->socketType = socketType;  }  return true;}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'socket != 1' is always true. qnativesocketengine_unix.cpp 315

Условие socket != -1 всегда истинно, так как выше происходит выход из функции, если значение переменной socket отрицательное.

Фрагмент N23: так что же всё-таки должна вернуть функция?

bool QSqlTableModel::removeRows(int row, int count, const QModelIndex &parent){  Q_D(QSqlTableModel);  if (parent.isValid() || row < 0 || count <= 0)    return false;  else if (row + count > rowCount())    return false;  else if (!count)    return true;  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression '!count' is always false. qsqltablemodel.cpp 1110

Для упрощения выделю самое главное:

if (.... || count <= 0)  return false;....else if (!count)  return true;

Первая проверка говорит нам, что если значение count меньше или равно 0, то это ошибочное состояние и функция должна вернуть false. Однако ниже мы видим точное сравнение этой переменной с нулём, и этот случай уже интерпретируется по-другому: функция должна вернуть true.

Здесь явно что-то не так. Я подозреваю, что на самом деле проверка должна быть не <=, а просто <. Тогда код обретает смысл:

bool QSqlTableModel::removeRows(int row, int count, const QModelIndex &parent){  Q_D(QSqlTableModel);  if (parent.isValid() || row < 0 || count < 0)    return false;  else if (row + count > rowCount())    return false;  else if (!count)    return true;  ....}

Фрагмент N24: лишний статус?

В следующем коде переменная identifierWithEscapeChars выглядит просто как лишняя сущность. Или это логическая ошибка? Или код не дописан? К моменту второй проверки эта переменная в любом случае всегда будет равна true.

int Lexer::scanToken(){  ....  bool identifierWithEscapeChars = false;  ....  if (!identifierWithEscapeChars) {    identifierWithEscapeChars = true;    ....  }  ....  if (identifierWithEscapeChars) {    // <=    ....  }  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'identifierWithEscapeChars' is always true. qqmljslexer.cpp 817

Фрагмент N25: что делать с девятью объектами?

bool QFont::fromString(const QString &descrip){  ....  const int count = l.count();  if (!count || (count > 2 && count < 9) || count == 9 || count > 17 ||      l.first().isEmpty()) {    qWarning("QFont::fromString: Invalid description '%s'",             descrip.isEmpty() ? "(empty)" : descrip.toLatin1().data());    return false;  }  setFamily(l[0].toString());  if (count > 1 && l[1].toDouble() > 0.0)    setPointSizeF(l[1].toDouble());  if (count == 9) {                           // <=    setStyleHint((StyleHint) l[2].toInt());    setWeight(QFont::Weight(l[3].toInt()));    setItalic(l[4].toInt());    setUnderline(l[5].toInt());    setStrikeOut(l[6].toInt());    setFixedPitch(l[7].toInt());  } else if (count >= 10) {  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'count == 9' is always false. qfont.cpp 2142

Как должна себя вести функция, если переменная count равна 9? С одной стороны, функция должна выдать предупреждение и завершить свою работу. Ведь явно написано:

if (.... || count == 9 || ....) {  qWarning(....);  return false;}

С другой стороны, для 9 объектов предусмотрено выполнение специального кода:

if (count == 9) {  setStyleHint((StyleHint) l[2].toInt());  setWeight(QFont::Weight(l[3].toInt()));  setItalic(l[4].toInt());  ....}

Этот код, конечно, никогда не выполняется. Код ждёт, чтобы его пришли и исправили :).

Нулевые указатели

Фрагмент N26-N42: использование указателя до его проверки

class __attribute__((visibility("default"))) QMetaType {  ....  const QtPrivate::QMetaTypeInterface *d_ptr = nullptr;};QPartialOrdering QMetaType::compare(const void *lhs, const void *rhs) const{    if (!lhs || !rhs)        return QPartialOrdering::Unordered;    if (d_ptr->flags & QMetaType::IsPointer)        return threeWayCompare(*reinterpret_cast<const void * const *>(lhs),                               *reinterpret_cast<const void * const *>(rhs));    if (d_ptr && d_ptr->lessThan) {        if (d_ptr->equals && d_ptr->equals(d_ptr, lhs, rhs))            return QPartialOrdering::Equivalent;        if (d_ptr->lessThan(d_ptr, lhs, rhs))            return QPartialOrdering::Less;        if (d_ptr->lessThan(d_ptr, rhs, lhs))            return QPartialOrdering::Greater;        if (!d_ptr->equals)            return QPartialOrdering::Equivalent;    }    return QPartialOrdering::Unordered;}

Предупреждение PVS-Studio: V595 [CWE-476] The 'd_ptr' pointer was utilized before it was verified against nullptr. Check lines: 710, 713. qmetatype.cpp 710

Ошибка на первый взгляд может быть не заметна. Но на самом деле всё просто. Проследим, как работают с указателем d_ptr:

if (d_ptr->flags & ....)if (d_ptr && ....)

Вначале выполняется условие, в котором указатель разыменовывается. И только из следующей проверки становится ясно, что этот указатель, оказывается, может быть нулевой.

Это один из самых распространённых паттернов ошибки в языке C и С++. Пруфы. В исходных Qt кодах тоже встречается немало ошибок этой разновидности:

• V595 [CWE-476] The 'self' pointer was utilized before it was verified against nullptr. Check lines: 1346, 1351. qcoreapplication.cpp 1346
• V595 [CWE-476] The 'currentTimerInfo' pointer was utilized before it was verified against nullptr. Check lines: 636, 641. qtimerinfo_unix.cpp 636
• V595 [CWE-476] The 'lib' pointer was utilized before it was verified against nullptr. Check lines: 325, 333. qlibrary.cpp 325
• V595 [CWE-476] The 'fragment.d' pointer was utilized before it was verified against nullptr. Check lines: 2262, 2266. qtextcursor.cpp 2262
• V595 [CWE-476] The 'window' pointer was utilized before it was verified against nullptr. Check lines: 1581, 1583. qapplication.cpp 1581
• V595 [CWE-476] The 'window' pointer was utilized before it was verified against nullptr. Check lines: 1593, 1595. qapplication.cpp 1593
• V595 [CWE-476] The 'newHandle' pointer was utilized before it was verified against nullptr. Check lines: 873, 879. qsplitter.cpp 873
• V595 [CWE-476] The 'targetModel' pointer was utilized before it was verified against nullptr. Check lines: 454, 455. qqmllistmodel.cpp 454
• V595 [CWE-476] The 'childIface' pointer was utilized before it was verified against nullptr. Check lines: 102, 104. qaccessiblequickitem.cpp 102
• V595 [CWE-476] The 'e' pointer was utilized before it was verified against nullptr. Check lines: 94, 98. qquickwindowmodule.cpp 94
• V595 [CWE-476] The 'm_texture' pointer was utilized before it was verified against nullptr. Check lines: 235, 239. qsgplaintexture.cpp 235
• V595 [CWE-476] The 'm_unreferencedPixmaps' pointer was utilized before it was verified against nullptr. Check lines: 1140, 1148. qquickpixmapcache.cpp 1140
• V595 [CWE-476] The 'camera' pointer was utilized before it was verified against nullptr. Check lines: 263, 264. assimpimporter.cpp 263
• V595 [CWE-476] The 'light' pointer was utilized before it was verified against nullptr. Check lines: 273, 274. assimpimporter.cpp 273
• V595 [CWE-476] The 'channel' pointer was utilized before it was verified against nullptr. Check lines: 337, 338. assimpimporter.cpp 337
• V595 [CWE-476] The 'm_fwb' pointer was utilized before it was verified against nullptr. Check lines: 2492, 2500. designerpropertymanager.cpp 2492

Фрагмент N43: использование указателя до его проверки в рамках одного выражения

Та же самая ситуация, что и выше. Однако в этот раз разыменование и проверка указателя находятся в одном выражении. Классическая ошибка, возникающая из-за невнимательности при написании кода и при последующем code-review.

void QFormLayoutPrivate::updateSizes(){  ....  QFormLayoutItem *field = m_matrix(i, 1);  ....  if (userHSpacing < 0 && !wrapAllRows && (label || !field->fullRow) && field)  ....}

Предупреждение PVS-Studio: V713 [CWE-476] The pointer 'field' was utilized in the logical expression before it was verified against nullptr in the same logical expression. qformlayout.cpp 405

Минутка отдыха

Я устал. Думаю, устали и читатели. Тут устанешь, даже если просто бегло просматривать текст статьи :). Поэтому я пошёл за второй чашечкой кофе. Первую я выпил где-то на рубеже 12-ого фрагмента. Приглашаю и читателей сходить за любимым напитком.

И пока все ушли на кухню, рекламная пауза. Приглашаю команду, занимающуюся разработкой проекта Qt рассмотреть вопрос приобретения лицензии на анализатор кода PVS-Studio. Запросить прайс можно здесь. С нашей стороны поддержка и помощь в настройке. Да, согласен, я сегодня более навязчив, чем обычно. Это эксперимент :).

Фрагмент N44-N72: нет проверки, что вернула функция malloc

void assignData(const QQmlProfilerEvent &other){  if (m_dataType & External) {    uint length = m_dataLength * (other.m_dataType / 8);    m_data.external = malloc(length);                          // <=    memcpy(m_data.external, other.m_data.external, length);    // <=  } else {    memcpy(&m_data, &other.m_data, sizeof(m_data));  }}

Предупреждение PVS-Studio: V575 [CWE-628] The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 277, 276. qqmlprofilerevent_p.h 277

Нельзя просто взять и использовать указатель, который вернула функция malloc. Нужно обязательно проверить, не является ли этот указатель нулевым, даже если очень лень всем этим заниматься. Для этого есть 4 причины, которые описаны в статье "Почему важно проверять, что вернула функция malloc".

Перед нами один из случаев, где отсутствует необходимая проверка. Есть и другие предупреждения, но из-за их количества включать весь список в статью не хочется. На всякий случай, я выписал 28 предупреждений в файл: qt6-malloc.txt. Но на самом деле разработчикам, конечно, лучше самим перепроверить проект и самостоятельно изучить предупреждения. У меня не было задачи выявить как можно больше ошибок.

Что интересно, на фоне важных забытых проверок есть совершенно ненужные. Речь идёт о вызове оператора new, который в случае ошибки выделения памяти сгенерирует исключение std::bad_alloc. Вот один из примеров такой избыточной проверки:

static QImageScaleInfo* QImageScale::qimageCalcScaleInfo(....){  ....  QImageScaleInfo *isi;  ....  isi = new QImageScaleInfo;  if (!isi)    return nullptr;  ....}

Предупреждение PVS-Studio: V668 [CWE-570] There is no sense in testing the 'isi' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. qimagescale.cpp 245

P.S. Здесь читатели всегда задают вопрос, учитывает ли анализатор placement new или "new (std::nothrow) T"? Да, учитывает и не выдаёт для них ложные срабатывания.

Избыточный код ("код с запахом")

Встречаются ситуации, когда анализатор выдаёт предупреждения на код, который является корректным, но избыточным. Например, есть повторная проверка одной и той же переменной. При этом даже затруднительно сказать, ложное это срабатывание или нет. Формально анализатор прав, но и настоящую ошибку он не нашёл.

Пожалуй, можно сказать, что это "код с запахом". Раз этот код смущает анализатор, он может смущать и программистов, усложняя поддержку такого кода. Приходится тратить больше времени, чтобы понять, что происходит.

Часто я вообще не рассматриваю в статьях такие предупреждения. Это неинтересно. Однако в проекте Qt вдруг таких случаев оказалось удивительно много. Явно больше, чем обычно. Поэтому я решил уделить этому немного внимания и разобрать несколько таких случаев. Думаю, будет полезно провести рефакторинг этих и многих других аналогичных мест. Для этого потребуется использовать полный отчёт, не руководствоваться тем, что я выпишу в статью.

Итак, взглянем на несколько показательных случаев.

Фрагмент N73: "код с запахом" обратная проверка

void QQuick3DSceneManager::setWindow(QQuickWindow *window){  if (window == m_window)    return;  if (window != m_window) {    if (m_window)      disconnect(....);    m_window = window;    connect(....);    emit windowChanged();  }}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'window != m_window' is always true. qquick3dscenemanager.cpp 60

Если window==m_window, то функция завершает работу. Последующая обратная проверка не имеет никакого смысла и только загромождает код.

Фрагмент N74: "код с запахом" странная инициализация

QModelIndex QTreeView::moveCursor(....){  ....  int vi = -1;  if (vi < 0)    vi = qMax(0, d->viewIndex(current));  ....}

Предупреждение PVS-Stduio: V547 [CWE-571] Expression 'vi < 0' is always true. qtreeview.cpp 2219

Что это? Зачем так писать? Код можно упростить до одной строки:

int vi = qMax(0, d->viewIndex(current));

Фрагмент N75: "код с запахом" недостижимый код

bool copyQtFiles(Options *options){  ....  if (unmetDependencies.isEmpty()) {    if (options->verbose) {      fprintf(stdout, "  -- Skipping %s, architecture mismatch.\n",              qPrintable(sourceFileName));    }  } else {    if (unmetDependencies.isEmpty()) {      if (options->verbose) {        fprintf(stdout, "  -- Skipping %s, architecture mismatch.\n",                  qPrintable(sourceFileName));      }    } else {      fprintf(stdout, "  -- Skipping %s. It has unmet dependencies: %s.\n",              qPrintable(sourceFileName),              qPrintable(unmetDependencies.join(QLatin1Char(','))));    }  }  ....}

Предупреждение PVS-Studio: V571 [CWE-571] Recurring check. The 'if (unmetDependencies.isEmpty())' condition was already verified in line 2203. main.cpp 2209

На первый взгляд перед нами респектабельный код, формирующий подсказку. Но давайте приглядимся. Если первый раз условие unmetDependencies.isEmpty() выполнилось, то второй раз этого уже не произойдёт. Это нестрашно, так как автор планировал вывести то же самое сообщение. Настоящей ошибки нет, но код переусложнён. Он может быть упрощен до следующего варианта:

bool copyQtFiles(Options *options){  ....  if (unmetDependencies.isEmpty()) {    if (options->verbose) {      fprintf(stdout, "  -- Skipping %s, architecture mismatch.\n",              qPrintable(sourceFileName));    }  } else {    fprintf(stdout, "  -- Skipping %s. It has unmet dependencies: %s.\n",            qPrintable(sourceFileName),            qPrintable(unmetDependencies.join(QLatin1Char(','))));  }  ....}

Фрагмент N76: "код с запахом" сложный тернарный оператор

bool QDockAreaLayoutInfo::insertGap(....){  ....  QDockAreaLayoutItem new_item    = widgetItem == nullptr      ? QDockAreaLayoutItem(subinfo)      : widgetItem ? QDockAreaLayoutItem(widgetItem)                    : QDockAreaLayoutItem(placeHolderItem);  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'widgetItem' is always true. qdockarealayout.cpp 1167

Возможно, здесь мы имеем дело с настоящей ошибкой. Но я больше склоняюсь к тому, что этот код несколько раз менялся и в результате получился бессмысленно сложным (избыточным). Его можно сократить до:

  QDockAreaLayoutItem new_item    = widgetItem == nullptr      ? QDockAreaLayoutItem(subinfo) : QDockAreaLayoutItem(widgetItem);

Фрагмент N77: "код с запахом" избыточная защита

typedef unsigned int uint;ReturnedValue TypedArrayCtor::virtualCallAsConstructor(....){  ....  qint64 l = argc ? argv[0].toIndex() : 0;  if (scope.engine->hasException)    return Encode::undefined();  // ### lift UINT_MAX restriction  if (l < 0 || l > UINT_MAX)    return scope.engine->throwRangeError(QLatin1String("Index out of range."));  uint len = (uint)l;  if (l != len)    scope.engine->throwRangeError(      QStringLiteral("Non integer length for typed array."));  ....}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'l != len' is always false. qv4typedarray.cpp 306

Кто-то очень переживает, что значение 64-битной переменной не вмещается в 32-битную переменную unsigned. И использует сразу две проверки корректности. При этом вторая проверка избыточна.

Вот этого условия более чем достаточно:

if (l < 0 || l > UINT_MAX)

Приведенный ниже фрагмент можно смело удалить, и программа менее надёжной не станет:

uint len = (uint)l;if (l != len)  scope.engine->throwRangeError(    QStringLiteral("Non integer length for typed array."));

Дальше продолжать не буду. Думаю, идею вы поняли.

Здесь можно сделать маленький вывод: результатом использования анализатора PVS-Studio будет не только устранение ошибок, но и упрощение кода.

Другие ошибки

Я остановился после того, как описал 77 дефектов. Это красивое число и выписанного более чем достаточно, чтобы написать статью. Однако это не значит, что нет других ошибок, которые способен выявить PVS-Studio. При изучении лога я был весьма поверхностен и пропускал всё, где нужно было разбираться более пары минут, ошибка это или нет :).

Поэтому я всегда прошу не полагаться на ошибки, которые мы описываем в наших статьях, а выполнять анализ проектов самостоятельно.

Заключение

Статический анализ это круто! После внедрения PVS-Studio будет экономить время и нервы, выявляя множество ошибок сразу после написания кода. Намного лучше искать на code review с коллегами не опечатки, а высокоуровневые ошибки и обсуждать эффективность реализованного алгоритма. Тем более, как показывает практика, эти дурацкие опечатки всё равно отлично прячутся при просмотре кода глазами. Так что пусть их лучше ищет программа, а не человек.

Если у вас ещё остались вопросы или возражения, приглашаю познакомиться со статьёй "Причины внедрить в процесс разработки статический анализатор кода PVS-Studio". С вероятность 90 % вы найдете в ней ответ на ваши вопросы :). В оставшихся 10 % случаев напишите нам, пообщаемся :).

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Date Processing Attracts Bugs or 77 Defects in Qt 6.

Пользователи иногда спрашивают, как появляются новые диагностики в статическом анализаторе PVS-Studio. Мы отвечаем, что черпаем вдохновение из разнообразнейших источников: книг, стандартов кодирования, собственных ошибок, писем наших пользователей и так далее. Сегодня мы придумали новую интересную диагностику и решили рассказать историю, как это произошло.

Всё началось с проверки проекта COVID-19 CovidSim Model и статьи про неинициализированную переменную. Проект оказался маленьким и написанным с использованием современного стандарта языка C++. Это значит, что он отлично может пополнить базу тестовых проектов для регрессионного тестирования ядра анализатора PVS-Studio.

Однако, прежде чем пополнить базу новым проектом, полезно просмотреть выдаваемые предупреждения, чтобы выявить новые закономерности (паттерны) ложных срабатываний и выписать их для дальнейшей доработки анализатора. Также это дополнительная возможность заметить, что ещё что-то не так. Например, сообщение неудачно описывает ошибку для определённой конструкции кода.

Программист, которому поручили добавить проект в тестовую базу, подошёл к задаче основательно и решил заглянуть в раздел MISRA-диагностик. Вообще, это делать было необязательно, так как это весьма специфичная группа диагностик, которую нет смысла включать для таких проектов, как CovidSim.

MISRA С и MISRA C++ диагностики предназначены для разработчиков встраиваемых систем, и их суть сводится к ограничению использования небезопасных конструкций программирования. Например, не рекомендуется применять оператор goto (V2502), так как он провоцирует создание сложного кода, в котором легко допустить логическую ошибку. Подробнее с философией стандарта кодирования MISRA можно познакомиться в статье "Что такое MISRA и как её готовить".

Для прикладного программного обеспечения, которым как раз и является проект CovidSim, включать набор MISRA диагностик не имеет смысла. Пользователь просто утонет в огромном количестве малополезных для него сообщений. Например, экспериментируя с этим набором диагностик, мы получали более миллиона предупреждений для некоторых открытых проектов среднего размера. Грубо говоря, с точки зрения MISRA, в каждой третьей строчке кода может быть что-то не так :). Естественно, никто всё это смотреть и тем более править не будет. Проект или сразу разрабатывается с учётом MISRA рекомендаций, или для него этот стандарт кодирования неактуален.

Но мы отвлеклись от темы. Так вот, бегло просматривая MISRA предупреждения, коллега зацепился взглядом за предупреждение V2507, выданное для этого фрагмента кода.

if (radiusSquared > StateT[tn].maxRad2) StateT[tn].maxRad2 = radiusSquared;{  SusceptibleToLatent(a->pcell);  if (a->listpos < Cells[a->pcell].S)  {    UpdateCell(Cells[a->pcell].susceptible, a->listpos, Cells[a->pcell].S);    a->listpos = Cells[a->pcell].S;    Cells[a->pcell].latent[0] = ai;  }}StateT[tn].cumI_keyworker[a->keyworker]++;

Правило V2507 заставляет оборачивать тела условных операторов в фигурные скобки.

В первый момент наш дотошный коллега подумал, что анализатор дал сбой. Ведь здесь есть блок текста в фигурных скобках! Перед нами ложное срабатывание?

Давайте присмотримся. Код только кажется корректным, но таковым не является! Фигурные скобки не относятся к оператору if.

Отформатируем код для наглядности:

if (radiusSquared > StateT[tn].maxRad2)  StateT[tn].maxRad2 = radiusSquared;{  SusceptibleToLatent(a->pcell);  ....}

Согласитесь, это красивый баг. Он наверняка войдёт в Top10 C++ ошибок, найденных нами в 2021 году.

И что из этого следует? Подход, диктуемый стандартом MISRA, работает! Да, он заставляет писать везде фигурные скобки. Да, это утомительно. Но это оправданная плата за повышение надёжности встроенных приложений, используемых в медицинской технике, автомобилях и других системах высокой ответственности.

Хорошо, у разработчиков, использующих стандарт MISRA, всё нормально. Однако рекомендовать всем подряд использовать фигурные скобки это плохая идея. Используя такой подход, очень легко довести анализатор до состояния, когда им станет невозможно пользоваться. Будет так много сообщений, что их просто никто не будет смотреть.

Вот мы и добрались для появления идеи для новой диагностики общего назначения. Можно сформулировать следующее правило.

Выдать предупреждение, если для оператора if выполняются следующие условия:

• весь условный оператор if записан в одну строчку и имеет только then-ветку;
• следующий statement после if это compound statement, и он находится не на той же строке, что и if.

Заранее можно прогнозировать, что получится хорошее правило с низким количеством ложных срабатываний.

Именно так эта идея сейчас оформлена в нашей системе учёта задач. Возможно, в процессе реализации что-то будет сделано по-другому, но это уже неважно. Главное, появится хорошее диагностическое правило, которое начнёт выявлять новый паттерн ошибки. Далее мы распространим его на C# и Java ядра анализатора PVS-Studio.

Только что мы рассмотрели интересный пример, как было сформулировано новое диагностическое правило, которое затем будет реализовано в PVS-Studio. Скажем спасибо проекту CovidSim, стандарту кодирования MISRA и наблюдательности нашего коллеги.

Спасибо за внимание и следуйте за мной в мир С++ и багов :). Twitter. Facebook.

Дополнительный ссылки:

1. Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей.
2. Под капотом PVS-Studio для Java: разработка диагностик.
3. Использование машинного обучения в статическом анализе исходного кода программ.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Example of How New Diagnostics Appear in PVS-Studio.

Хотя только недавно была заметка про проект CovidSim, есть хороший повод вновь про него вспомнить и продемонстрировать пользу регулярного использования PVS-Studio. Бывает, что все мы спешим и вносим правки в код, потеряв сосредоточенность. Статический анализатор может оказаться здесь хорошим помощником.

Всё началось с написания вот этих двух небольших заметок про открытый проект COVID-19 CovidSim Model:

• Пример, как в PVS-Studio появляются новые диагностики;
• Исследование COVID-19 и неинициализированная переменная.

После этого мы подумали и решили мониторить и этот проект с целью популяризации регулярного использования статических анализаторов кода. Подробнее про эту затею мой коллега писал в этой статье.

Только мы начали проверять этот проект, как результаты не заставили себя ждать :). Сейчас я покажу свежую ошибку, вызванную недавними неаккуратными правками кода. Конечно, мы не станем описывать в дальнейшем каждый баг и недочёт, обнаруженный анализатором. Будем писать только про что-то интересное.

Вот что получилось, после недавних модификаций файла CovidSim.cpp:

Кто-то решил создавать массивы не на стеке, а в куче. Но был невнимателен, внося изменения. Обратите внимание, что освобождение памяти находится после оператора return:

int GetXMLNode(....){  char* buf = new char[65536];  char* CloseNode = new char[2048];  char* CloseParent = new char[2048];  ....  if (ResetFilePos) fseek(dat, CurPos, 0);  return ret;  delete[] buf;  delete[] CloseNode;  delete[] CloseParent;}

В результате перед нами фрагмент недостижимого кода (unreachable code). И заодно утечка памяти.

Хорошо, что PVS-Studio тут же сообщает про эту ошибку: V779 Unreachable code detected. It is possible that an error is present. CovidSim.cpp 675

Используйте статический анализ регулярно и сможете устранить множество ошибок на самом раннем этапе! Согласитесь, лучше сразу исправить этот мелкий баг, чем гадать, почему вдруг программа начала потреблять слишком много оперативной памяти.

И последнее, что хочется отметить. Эта ошибка была бы невозможна, если не вручную управлять выделением и освобождением памяти, а применить RAII подход и использовать умные указатели.

Правильный и надёжный вариант кода:

std::unique_ptr<char[]> buf(new char[65536]);std::unique_ptr<char[]> CloseNode(new char[2048]);std::unique_ptr<char[]> CloseParent(new char[2048]);

Спасибо за внимание. Следуйте за мной в мир С++ и багов :). Twitter. Facebook.