Неправильный код, который работает

Размер выделяемой памяти

int main(int argc, char *argv[]){  ....  struct pool *pop = malloc(sizeof(pop));  ....}

struct pool *pop = malloc(sizeof(pool));

struct pool *pop = malloc(sizeof(*pop));

struct pool {  struct ctl *ctl;};

Длина строки

typedef void *(*pmem2_memcpy_fn)(void *pmemdest, const void *src, size_t len,    unsigned flags);static const char *initial_state = "No code.";static inttest_rwx_prot_map_priv_do_execute(const struct test_case *tc,  int argc, char *argv[]){  ....  char *addr_map = pmem2_map_get_address(map);  map->memcpy_fn(addr_map, initial_state, sizeof(initial_state), 0);  ....}

static const char initial_state [] = "No code.";

UT_ASSERTeq(memcmp(addr_map, initial_state, strlen(initial_state)), 0);

Побитовый сдвиг

static intclo_parse_single_uint(struct benchmark_clo *clo, const char *arg, void *ptr){  ....  uint64_t tmax = ~0 >> (64 - 8 * clo->type_uint.size);  ....}

Приоритет операций

#define BTT_CREATE_DEF_SIZE  (20 * 1UL << 20) /* 20 MB */

• Сдвинул 1 на 20 разрядов, чтобы получить значение 1048576, т.е. 1 MB.
• Умножил 1 MB на 20.

• (20 * 1UL << 20)
• (20 * (1UL << 20))
• ((20 * 1UL) << 20)

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

Не там поставленная скобка

#define STATUS_INFO_LENGTH_MISMATCH 0xc0000004static voidenum_handles(int op){  ....  NTSTATUS status;  while ((status = NtQuerySystemInformation(      SystemExtendedHandleInformation,      hndl_info, hi_size, &req_size)        == STATUS_INFO_LENGTH_MISMATCH)) {    hi_size = req_size + 4096;    hndl_info = (PSYSTEM_HANDLE_INFORMATION_EX)REALLOC(hndl_info,        hi_size);  }  UT_ASSERT(status >= 0);  ....}

while ((status = NtQuerySystemInformation(....) == STATUS_INFO_LENGTH_MISMATCH))

while ((status = NtQuerySystemInformation(....)) == STATUS_INFO_LENGTH_MISMATCH)

Потенциальная утечка памяти

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  if (!oidp)    return POCLI_ERR_PARS;  ....}

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  if (!oidp)    return POCLI_ERR_PARS;  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  ....}

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  if (!oidp)  {    free(input);    return POCLI_ERR_PARS;  }  ....}

Потенциальное переполнение

typedef long long os_off_t;voiddo_memcpy(...., int dest_off, ....., size_t mapped_len, .....){  ....  LSEEK(fd, (os_off_t)(dest_off + (int)(mapped_len / 2)), SEEK_SET);  ....}

LSEEK(fd, dest_off + (os_off_t)(mapped_len) / 2, SEEK_SET);

Неправильная защита от переполнения

static DWORDget_rel_wait(const struct timespec *abstime){  struct __timeb64 t;  _ftime64_s(&t);  time_t now_ms = t.time * 1000 + t.millitm;  time_t ms = (time_t)(abstime->tv_sec * 1000 +    abstime->tv_nsec / 1000000);  DWORD rel_wait = (DWORD)(ms - now_ms);  return rel_wait < 0 ? 0 : rel_wait;}

Отсутствие проверки, что память успешно выделена

static voidremove_extra_node(TOID(struct tree_map_node) *node){  ....  unsigned char *new_key = (unsigned char *)malloc(new_key_size);  assert(new_key != NULL);  memcpy(new_key, D_RO(tmp)->key, D_RO(tmp)->key_size);  ....}

static voidcalc_pi_mt(void){  ....  HANDLE *workers = (HANDLE *) malloc(sizeof(HANDLE) * pending);  for (i = 0; i < pending; ++i) {    workers[i] = CreateThread(NULL, 0, calc_pi,      &tasks[i], 0, NULL);    if (workers[i] == NULL)      break;  }  ....}

Код с запахом

Двойной вызов CloseHandle

static voidprepare_map(struct pmem2_map **map_ptr,  struct pmem2_config *cfg, struct pmem2_source *src){  ....  HANDLE mh = CreateFileMapping(....);  ....  UT_ASSERTne(CloseHandle(mh), 0);  ....}

#define UT_ASSERTne(lhs, rhs)\  do {\    /* See comment in UT_ASSERT. */\    if (__builtin_constant_p(lhs) && __builtin_constant_p(rhs))\      UT_ASSERT_COMPILE_ERROR_ON((lhs) != (rhs));\    UT_ASSERTne_rt(lhs, rhs);\  } while (0)

do {  if (0 && 0) (void)((CloseHandle(mh)) != (0));  ((void)(((CloseHandle(mh)) != (0)) ||    (ut_fatal(".....", 76, __FUNCTION__, "......: %s (0x%llx) != %s (0x%llx)",              "CloseHandle(mh)", (unsigned long long)(CloseHandle(mh)), "0",              (unsigned long long)(0)), 0))); } while (0);

((void)(((CloseHandle(mh)) != (0)) ||  (ut_fatal(...., "assertion failure: %s (0x%llx) != %s (0x%llx)",            ....., (unsigned long long)(CloseHandle(mh)), .... ), 0)));

Несоответствие интерфейса реализации (снятие константности)

static intstatus_push(PMEMpoolcheck *ppc, struct check_status *st, uint32_t question){  ....  } else {    status_msg_info_and_question(st->msg);            // <=    st->question = question;    ppc->result = CHECK_RESULT_ASK_QUESTIONS;    st->answer = PMEMPOOL_CHECK_ANSWER_EMPTY;    PMDK_TAILQ_INSERT_TAIL(&ppc->data->questions, st, next);  }  ....}

static inline intstatus_msg_info_and_question(const char *msg){  char *sep = strchr(msg, MSG_SEPARATOR);  if (sep) {    *sep = ' ';    return 0;  }  return -1;}

char * strchr ( const char *, int );

static inline intstatus_msg_info_and_question(char *msg){  char *sep = strchr(msg, MSG_SEPARATOR);  if (sep) {    *sep = ' ';    return 0;  }  return -1;}

Переусложненный код

static struct memory_blockheap_coalesce(struct palloc_heap *heap,  const struct memory_block *blocks[], int n){  struct memory_block ret = MEMORY_BLOCK_NONE;  const struct memory_block *b = NULL;  ret.size_idx = 0;  for (int i = 0; i < n; ++i) {    if (blocks[i] == NULL)      continue;    b = b ? b : blocks[i];    ret.size_idx += blocks[i] ? blocks[i]->size_idx : 0;  }  ....}

....for (int i = 0; i < n; ++i) {  if (blocks[i] == NULL)    continue;  b = b ? b : blocks[i];  ret.size_idx += blocks[i]->size_idx;}....

Подозрительное использование нулевого указателя

void win_mmap_fini(void){  ....  if (mt->BaseAddress != NULL)    UnmapViewOfFile(mt->BaseAddress);  size_t release_size =    (char *)mt->EndAddress - (char *)mt->BaseAddress;  void *release_addr = (char *)mt->BaseAddress + mt->FileLen;  mmap_unreserve(release_addr, release_size - mt->FileLen);  ....}

if (mt->BaseAddress != NULL)

size_t release_size =  (char *)mt->EndAddress - (char *)mt->BaseAddress;

Короткие имена глобальных переменных

static struct critnib *c;

• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'ri' variable. map.c 131
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'c' variable. obj_critnib_mt.c 56
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.h 68
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.c 34

Странное

voiddo_memmove(char *dst, char *src, const char *file_name,    size_t dest_off, size_t src_off, size_t bytes,    memmove_fn fn, unsigned flags, persist_fn persist){  ....  /* do the same using regular memmove and verify that buffers match */  memmove(dstshadow + dest_off, dstshadow + dest_off, bytes / 2);  verify_contents(file_name, 0, dstshadow, dst, bytes);  verify_contents(file_name, 1, srcshadow, src, bytes);  ....}

• Хотелось "потрогать" блок памяти. Но произойдёт ли это в реальности? Не удалит ли оптимизирующий компилятор код, который копирует блок памяти сам в себя?
• Это какой-то юнит-тест на работу функции memmove.
• Код содержит опечатку.

voiddo_memmove(char *dst, char *src, const char *file_name,    size_t dest_off, size_t src_off, size_t bytes,    memmove_fn fn, unsigned flags, persist_fn persist){  ....  /* do the same using regular memmove and verify that buffers match */  memmove(dstshadow + dest_off, srcshadow + src_off, 0);  verify_contents(file_name, 2, dstshadow, dst, bytes);  verify_contents(file_name, 3, srcshadow, src, bytes);  ....}

Зачем использовать анализаторы кода?

static inline bool is_valid_number(const std::string &input) {  if (is_binary_notation(input) ||      is_hex_notation(input) ||      is_octal_notation(input)) {    return true;  }  if (input.empty()) {    return false;  }  std::size_t i = 0, j = input.length() - 1;  // Handling whitespaces  while (i < input.length() && input[i] == ' ')    i++;  while (input[j] == ' ')    j--;  if (i > j)    return false;  // if string is of length 1 and the only  // character is not a digit  if (i == j && !(input[i] >= '0' && input[i] <= '9'))    return false;  // If the 1st char is not '+', '-', '.' or digit  if (input[i] != '.' && input[i] != '+' && input[i] != '-' &&      !(input[i] >= '0' && input[i] <= '9'))    return false;  // To check if a '.' or 'e' is found in given  // string. We use this flag to make sure that  // either of them appear only once.  bool dot_or_exp = false;  for (; i <= j; i++) {    // If any of the char does not belong to    // {digit, +, -, ., e}    if (input[i] != 'e' && input[i] != '.' &&        input[i] != '+' && input[i] != '-' &&        !(input[i] >= '0' && input[i] <= '9'))      return false;    if (input[i] == '.') {      // checks if the char 'e' has already      // occurred before '.' If yes, return false;.      if (dot_or_exp == true)        return false;      // If '.' is the last character.      if (i + 1 > input.length())        return false;      // if '.' is not followed by a digit.      if (!(input[i + 1] >= '0' && input[i + 1] <= '9'))        return false;    }    else if (input[i] == 'e') {      // set dot_or_exp = 1 when e is encountered.      dot_or_exp = true;      // if there is no digit before 'e'.      if (!(input[i - 1] >= '0' && input[i - 1] <= '9'))        return false;      // If 'e' is the last Character      if (i + 1 > input.length())        return false;      // if e is not followed either by      // '+', '-' or a digit      if (input[i + 1] != '+' && input[i + 1] != '-' &&          (input[i + 1] >= '0' && input[i] <= '9'))        return false;    }  }  /* If the string skips all above cases, then  it is numeric*/  return true;}

else if (input[i] == 'e') {  ....  if (input[i + 1] != '+' && input[i + 1] != '-' &&      (input[i + 1] >= '0' && input[i] <= '9'))      return false;}

else if (input[i] == 'e') {  ....  if (input[i + 1] != '+' && input[i + 1] != '-' &&      (input[i + 1] >= '0' && input[i + 1] <= '9'))      return false;}

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

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

Библиотека 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);  ....}

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

Библиотека 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;}

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

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

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_)        {  ....}

Библиотека 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;}

Библиотека 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))

Библиотека 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())); // <=  ....}

Библиотека 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");  }  ....}

• 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

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

template <int Base>class no_grouping final{  constexpr STRF_HD no_grouping& operator=(const no_grouping& other) noexcept  {    decimal_point_ = other.decimal_point_;  }  ....}

Библиотека 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); }

Библиотека 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(); // <=}

Библиотека 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;};

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

Библиотека 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);  ....}

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

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

Библиотека PpluX

Single header C++ Libraries for Thread Scheduling, Rendering, and so on...

struct DisplayList {  DisplayList& operator=(DisplayList &&d) {    data_ = d.data_;    d.data_ = nullptr;  }  ....}

Библиотека 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;}

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

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);  // <=  ....}

if (r1_sign) r1 = twos_complement(r1);

if (r1_sign) r2 = twos_complement(r2);

if (r2_sign) r2 = twos_complement(r2);

Библиотека 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);  }  ....}

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

Библиотека 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}

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

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

Библиотека 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;  }  ....}

Библиотека 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}

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

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

Библиотека 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 )

if ( res == NO_KEYWORD )

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

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

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

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

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

Заключение

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

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

static const int kDaysInMonth[13] = {  0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};bool ValidateDateTime(const DateTime& time) {  if (time.year < 1 || time.year > 9999 ||      time.month < 1 || time.month > 12 ||      time.day < 1 || time.day > 31 ||      time.hour < 0 || time.hour > 23 ||      time.minute < 0 || time.minute > 59 ||      time.second < 0 || time.second > 59) {    return false;  }  if (time.month == 2 && IsLeapYear(time.year)) {    return time.month <= kDaysInMonth[time.month] + 1;  } else {    return time.month <= kDaysInMonth[time.month];  }}

bool ValidateDateTime(const DateTime& time) {  if (time.year < 1 || time.year > 9999 ||      time.month < 1 || time.month > 12 ||      time.day < 1 || time.day > 31 ||      time.hour < 0 || time.hour > 23 ||      time.minute < 0 || time.minute > 59 ||      time.second < 0 || time.second > 59) {    return false;  }  if (time.month == 2 && IsLeapYear(time.year)) {    return true;  } else {    return true;  }}

bool ValidateDateTime(const DateTime& time) {  if (time.year < 1 || time.year > 9999 ||      time.month < 1 || time.month > 12 ||      time.day < 1 || time.day > 31 ||      time.hour < 0 || time.hour > 23 ||      time.minute < 0 || time.minute > 59 ||      time.second < 0 || time.second > 59) {    return false;  }  return true;}

static DWORDget_rel_wait(const struct timespec *abstime){  struct __timeb64 t;  _ftime64_s(&t);  time_t now_ms = t.time * 1000 + t.millitm;  time_t ms = (time_t)(abstime->tv_sec * 1000 +    abstime->tv_nsec / 1000000);  DWORD rel_wait = (DWORD)(ms - now_ms);  return rel_wait < 0 ? 0 : rel_wait;}

Один из наших читателей обратил наше внимание на Espressif IoT Development Framework. Он нашёл ошибку в коде проекта и поинтересовался, смог бы её найти статический анализатор PVS-Studio. Именно эту ошибку анализатор пока найти не может, зато нашёл множество других. По мотивам этой истории и найденных ошибок, мы решили написать классическую статью про проверку открытого проекта. Приятного изучения того, из-за чего IoT устройства могут "выстрелить вам в ногу".

Программно-аппаратные системы

Отец языка C++ Бьярне Страуструп как-то сказал:

"Си" позволяет очень просто выстрелить себе в ногу. На "Си++" сделать это сложнее, но, когда вы это делаете, отрывает всю ногу.

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

Такие проекты, как Espressif IoT Development Framework, служат для реализации программно-аппаратных систем, взаимодействующих с человеком и управляющие объектами в реальном мире. Всё это накладывает дополнительные требования к качеству и надёжности программного кода. Именно отсюда берут основы такие стандарты как MISRA или AUTOSAR. Впрочем, это уже другая тема.

Вернёмся к Espressif IoT Development Framework (исходный код на сайте GitHub: esp-idf). Вот его краткое описание:

ESP-IDF is Espressif's official IoT Development Framework for the ESP32 and ESP32-S series of SoCs. It provides a self-sufficient SDK for any generic application development on these platforms, using programming languages such as C and C++. ESP-IDF currently powers millions of devices in the field, and enables building a variety of network-connected products, ranging from simple light bulbs and toys to big appliances and industrial devices.

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

Предыстория

Ещё хочется рассказать, как появилась эта статья. Мне написал Юрий Попов (Hardcore IoT fullstack dev & CTO), который с интересом следит за нашими публикациями. Незадолго до этого он самостоятельно вручную нашёл ошибку в Espressif IoT Development Framework и поинтересовался, может ли выявить этот дефект PVS-Studio. Ошибка связана с опечаткой коде, а PVS-Studio всегда славился тем, что хорошо выявляет подобные ошибки.

Некорректный код находился в файле mdns.c:

mdns_txt_linked_item_t * txt = service->txt;while (txt) {  data_len += 2 + strlen(service->txt->key) + strlen(service->txt->value);  txt = txt->next;}

Происходит обход списка и определённым образом должны суммироваться длины строк, на которые ссылаются различные объекты, хранящиеся в списке. Вот только всё время считается длина строк, хранящаяся в первом объекте.

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

data_len += 2 + strlen(txt->key) + strlen(txt->value);

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

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

По итогам всего этого, Юра сам написал небольшую заметку про эту ошибку, как он её искал и про PVS-Studio: "Баг в ESP-IDF: MDNS, Wireshark и при чём тут единороги". Плюс он уведомил авторов проекта о найденной ошибке: Spurious MDNS collision detection (IDFGH-4263).

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

Правда проверку мы повели достаточно неуклюже. К сожалению, нет примера "собрать всё". Ну или мы не разобрались. Мы начали с getting_started\hello_world. Вроде бы он использует часть фреймворка, но не полностью. Так что можно найти и другие ошибки, добившись компиляции большего количества файлов фреймворка. Другими словами, то, что в статье будет описана только 71 ошибка, это наша недоработка :).

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

К счастью, у Юрия Попова, который заварил всю эту кашу, гораздо больше энтузиазма, чем у меня. Он сообщил, что смог добиться более полной компиляции фреймворка и проверил гораздо больше файлов. И, очень вероятно, вслед за моей статьей, выйдет его статья, где он рассмотрит дополнительную порцию ошибок.

Примеры, откуда берутся ложные/бессмысленные срабатывания

Всех исследователей, которые захотят проверить Espressif IoT Development Framework, я хочу предупредить, что понадобится предварительная настройка анализатора. Без неё вы утоните в большом количестве ложных/бесполезных срабатываний. Но анализатор не виноват.

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

Предупреждение PVS-Studio: V547 Expression 'ret != 0' is always true. esp_hidd.c 45

esp_err_t esp_hidd_dev_init(....){  esp_err_t ret = ESP_OK;  ....  switch (transport) {#if CONFIG_GATTS_ENABLE  case ESP_HID_TRANSPORT_BLE:    ret = esp_ble_hidd_dev_init(dev, config, callback);    break;#endif /* CONFIG_GATTS_ENABLE */  default:    ret = ESP_FAIL;    break;  }  if (ret != ESP_OK) {    free(dev);    return ret;  }  ....}

Выбран такой режим компиляции, при котором макрос CONFIG_GATTS_ENABLE не объявлен. Поэтому, для анализатора этот код выглядит так:

esp_err_t ret = ESP_OK;....switch (transport) {default:  ret = ESP_FAIL;  break;}if (ret != ESP_OK) {

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

Рассмотрим другой пример. В коде активно используется своя разновидность assert-макросов. К сожалению, они тоже сбивают анализатор с толку. Предупреждение PVS-Studio: V547 Expression 'sntp_pcb != NULL' is always true. sntp.c 664

#define LWIP_PLATFORM_ASSERT(x) do \  {printf("Assertion \"%s\" failed at line %d in %s\n", \    x, __LINE__, __FILE__); fflush(NULL); abort();} while(0)#ifndef LWIP_NOASSERT#define LWIP_ASSERT(message, assertion) do { if (!(assertion)) { \  LWIP_PLATFORM_ASSERT(message); }} while(0)#else  /* LWIP_NOASSERT */#define LWIP_ASSERT(message, assertion)#endif /* LWIP_NOASSERT */sntp_pcb = udp_new_ip_type(IPADDR_TYPE_ANY);LWIP_ASSERT("Failed to allocate udp pcb for sntp client", sntp_pcb != NULL);if (sntp_pcb != NULL) {

Анализатор видит, что код в которой раскрывается LWIP_ASSERT остановит выполнение программы (см. вызов функции abort), если указатель sntp_pcb будет нулевой. Поэтому PVS-Studio предупреждает, что следующая проверка (sntp_pcb != NULL) не имеет смысла.

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

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

Security

Начнём с предупреждений, которые, на мой взгляд, связаны с темой security. Разработчики операционных систем, фреймворков и других подобных проектов, должны с особенным вниманием относиться к поиску слабостей кода, которые потенциально могут приводить к возникновению уязвимостей.

Для удобства классификации слабостей кода можно использовать CWE (Common Weakness Enumeration). В PVS-Studio можно включить отображение CWE ID для предупреждений. Для предупреждений этой главы я дополнительно приведу соответствующий CWE ID.

Подробнее тема поиска потенциальных уязвимостей раскрыта в статье "Статический анализатор кода PVS-Studio как защита от уязвимостей нулевого дня".

Ошибка N1; Порядок аргументов

Предупреждение PVS-Studio: V764 Possible incorrect order of arguments passed to 'crypto_generichash_blake2b__init_salt_personal' function: 'salt' and 'personal'. blake2b-ref.c 457

int blake2b_init_salt_personal(blake2b_state *S, const uint8_t outlen,                               const void *personal, const void *salt);intblake2b_salt_personal(uint8_t *out, const void *in, const void *key,                      const uint8_t outlen, const uint64_t inlen,                      uint8_t keylen, const void *salt, const void *personal){  ....  if (blake2b_init_salt_personal(S, outlen, salt, personal) < 0)    abort();  ....}

При вызове функции blake2b_init_salt_personal перепутаны местами аргументы personal и salt. Мне кажется, вряд ли это задумано специально и, скорее всего, это ошибка, возникшая по невнимательности. Я не ориентируюсь в коде проекта и в криптографии, но что-то мне подсказывает, что такая путаница может иметь нехорошие последствия.

Согласно CWE эта ошибка классифицируется как CWE-683: Function Call With Incorrect Order of Arguments.

Ошибка N2; Отбрасывание значащих бит

Предупреждение PVS-Studio: V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. mbc_tcp_master.c 387

static esp_err_t mbc_tcp_master_set_request(  char* name, mb_param_mode_t mode, mb_param_request_t* request,  mb_parameter_descriptor_t* reg_data){  ....  // Compare the name of parameter with parameter key from table  uint8_t comp_result = memcmp((const char*)name,                               (const char*)reg_ptr->param_key,                               (size_t)param_key_len);  if (comp_result == 0) {  ....}

Сохранять результат работы функции memcmp в однобайтовую переменную это очень плохо. Это дефект, который вполне может превратиться в реальную уязвимость, подобную этой: CVE-2012-2122. Подробнее, почему так писать нельзя, описано в документации к диагностике V642.

Если совсем кратко, то некоторые реализации функция memset могут возвращать в случае несовпадения блоков памяти не только значения 1 или -1. Функция, например, может вернуть значение 1024. А это число, записанное в переменную типа uint8_t превратится в 0.

Согласно CWE эта ошибка классифицируется как CWE-197: Numeric Truncation Error.

Ошибка N3 N20; Приватные данные остаются в памяти

Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'prk' buffer. The memset_s() function should be used to erase the private data. dpp.c 854

#ifndef os_memset#define os_memset(s, c, n) memset(s, c, n)#endifstatic int dpp_derive_k1(const u8 *Mx, size_t Mx_len, u8 *k1,       unsigned int hash_len){  u8 salt[DPP_MAX_HASH_LEN], prk[DPP_MAX_HASH_LEN];  const char *info = "first intermediate key";  int res;  /* k1 = HKDF(<>, "first intermediate key", M.x) */  /* HKDF-Extract(<>, M.x) */  os_memset(salt, 0, hash_len);  if (dpp_hmac(hash_len, salt, hash_len, Mx, Mx_len, prk) < 0)    return -1;  wpa_hexdump_key(MSG_DEBUG, "DPP: PRK = HKDF-Extract(<>, IKM=M.x)",      prk, hash_len);  /* HKDF-Expand(PRK, info, L) */  res = dpp_hkdf_expand(hash_len, prk, hash_len, info, k1, hash_len);  os_memset(prk, 0, hash_len);             // <=  if (res < 0)    return -1;  wpa_hexdump_key(MSG_DEBUG, "DPP: k1 = HKDF-Expand(PRK, info, L)",                  k1, hash_len);  return 0;}

Очень распространённая ошибка. Компилятор вправе в целях оптимизации удалить вызов функции memset, так как после заполнения буфера нулями, он больше не используется. В результате приватные данные на самом деле не затираются, а продолжат болтаться где-то в памяти. Подробности можно узнать в статье "Безопасная очистка приватных данных".

Согласно CWE эта ошибка классифицируется как CWE-14: Compiler Removal of Code to Clear Buffers.

Другие ошибки этого типа:

• V597 The compiler could delete the 'memset' function call, which is used to flush 'prk' buffer. The memset_s() function should be used to erase the private data. dpp.c 883
• V597 The compiler could delete the 'memset' function call, which is used to flush 'prk' buffer. The memset_s() function should be used to erase the private data. dpp.c 942
• V597 The compiler could delete the 'memset' function call, which is used to flush 'psk' buffer. The memset_s() function should be used to erase the private data. dpp.c 3939
• V597 The compiler could delete the 'memset' function call, which is used to flush 'prk' buffer. The memset_s() function should be used to erase the private data. dpp.c 5729
• V597 The compiler could delete the 'memset' function call, which is used to flush 'Nx' buffer. The memset_s() function should be used to erase the private data. dpp.c 5934
• V597 The compiler could delete the 'memset' function call, which is used to flush 'val' buffer. The memset_s() function should be used to erase the private data. sae.c 155
• V597 The compiler could delete the 'memset' function call, which is used to flush 'keyseed' buffer. The memset_s() function should be used to erase the private data. sae.c 834
• V597 The compiler could delete the 'memset' function call, which is used to flush 'keys' buffer. The memset_s() function should be used to erase the private data. sae.c 838
• V597 The compiler could delete the 'memset' function call, which is used to flush 'pkey' buffer. The memset_s() function should be used to erase the private data. des-internal.c 422
• V597 The compiler could delete the 'memset' function call, which is used to flush 'ek' buffer. The memset_s() function should be used to erase the private data. des-internal.c 423
• V597 The compiler could delete the 'memset' function call, which is used to flush 'finalcount' buffer. The memset_s() function should be used to erase the private data. sha1-internal.c 358
• V597 The compiler could delete the 'memset' function call, which is used to flush 'A_MD5' buffer. The memset_s() function should be used to erase the private data. sha1-tlsprf.c 95
• V597 The compiler could delete the 'memset' function call, which is used to flush 'P_MD5' buffer. The memset_s() function should be used to erase the private data. sha1-tlsprf.c 96
• V597 The compiler could delete the 'memset' function call, which is used to flush 'A_SHA1' buffer. The memset_s() function should be used to erase the private data. sha1-tlsprf.c 97
• V597 The compiler could delete the 'memset' function call, which is used to flush 'P_SHA1' buffer. The memset_s() function should be used to erase the private data. sha1-tlsprf.c 98
• V597 The compiler could delete the 'memset' function call, which is used to flush 'T' buffer. The memset_s() function should be used to erase the private data. sha256-kdf.c 85
• V597 The compiler could delete the 'memset' function call, which is used to flush 'hash' buffer. The memset_s() function should be used to erase the private data. sha256-prf.c 105

Ошибка N21; Не удаляется буфер с приватными данными

Предупреждение PVS-Studio: V575 The null pointer is passed into 'free' function. Inspect the first argument. sae.c 1185

static int sae_parse_password_identifier(struct sae_data *sae,           const u8 *pos, const u8 *end){  wpa_hexdump(MSG_DEBUG, "SAE: Possible elements at the end of the frame",        pos, end - pos);  if (!sae_is_password_id_elem(pos, end)) {    if (sae->tmp->pw_id) {      wpa_printf(MSG_DEBUG,           "SAE: No Password Identifier included, but expected one (%s)",           sae->tmp->pw_id);      return WLAN_STATUS_UNKNOWN_PASSWORD_IDENTIFIER;    }    os_free(sae->tmp->pw_id);    sae->tmp->pw_id = NULL;    return WLAN_STATUS_SUCCESS; /* No Password Identifier */  }  ....}

Если с паролем что-то не так и указатель pw_id не нулевой, то выводится отладочное предупреждение и функция завершает свою работу. Что интересно, далее происходит попытка освободить буфер, используя нулевой указатель. Более того, в нулевой указатель вновь записывается NULL. Всё это не имеет смысла. Скорее всего, строчки освобождения памяти находятся не на своём месте. И мне кажется, код должен быть таким:

if (!sae_is_password_id_elem(pos, end)) {  if (sae->tmp->pw_id) {    wpa_printf(MSG_DEBUG,         "SAE: No Password Identifier included, but expected one (%s)",         sae->tmp->pw_id);    os_free(sae->tmp->pw_id);    sae->tmp->pw_id = NULL;    return WLAN_STATUS_UNKNOWN_PASSWORD_IDENTIFIER;  }  return WLAN_STATUS_SUCCESS; /* No Password Identifier */}

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

Согласно CWE, эта формально ошибка классифицируется как CWE-628: Function Call with Incorrectly Specified Arguments. Так её классифицирует PVS-Studio, но, по сути и последствиям, это какая-то другая слабость кода.

Ошибка N22, N23; Использование неинициализированного буфера в качестве ключа

Предупреждение PVS-Studio: V614 Uninitialized buffer 'hex' used. Consider checking the second actual argument of the 'memcpy' function. wps_registrar.c 1657

int wps_build_cred(struct wps_data *wps, struct wpabuf *msg){  ....  } else if (wps->use_psk_key && wps->wps->psk_set) {    char hex[65];    wpa_printf(MSG_DEBUG,  "WPS: Use PSK format for Network Key");    os_memcpy(wps->cred.key, hex, 32 * 2);    wps->cred.key_len = 32 * 2;  } else if (wps->wps->network_key) {  ....}

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

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

Согласно CWE, эта ошибка классифицируется как CWE-457: Use of Uninitialized Variable.

Аналогичная ошибка: V614 Uninitialized buffer 'hex' used. Consider checking the second actual argument of the 'memcpy' function. wps_registrar.c 1678

Опечатки и Copy-Paste

Ошибка N24; Copy-Paste классический

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. timer.c 292

esp_err_t timer_isr_register(....){  ....  if ((intr_alloc_flags & ESP_INTR_FLAG_EDGE) == 0) {    intr_source = ETS_TG1_T0_LEVEL_INTR_SOURCE + timer_num;  } else {    intr_source = ETS_TG1_T0_LEVEL_INTR_SOURCE + timer_num;  }  ....}

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

Примечание. Бывает конечно, что так и задумано. Например, если пока, значения действительно должны совпадать (т.е. это "todo-код"). Но тогда такой код явно стоит снабдить поясняющим комментарием.

Ошибка N25; Не там поставлена скобка

Предупреждение PVS-Studio: V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. esp_tls_mbedtls.c 446

esp_err_t set_client_config(....){ .... if ((ret = mbedtls_ssl_conf_alpn_protocols(&tls->conf, cfg->alpn_protos) != 0)) {   ESP_LOGE(TAG, "mbedtls_ssl_conf_alpn_protocols returned -0x%x", -ret);   ESP_INT_EVENT_TRACKER_CAPTURE(tls->error_handle, ERR_TYPE_MBEDTLS, -ret);   return ESP_ERR_MBEDTLS_SSL_CONF_ALPN_PROTOCOLS_FAILED; } ....}

Приоритет оператора сравнения выше, чем приоритет оператора присваивания. Поэтому условие вычисляется следующим образом:

TEMP = mbedtls_ssl_conf_alpn_protocols(....) != 0;if ((ret = TEMP))  PRINT(...., -ret);

В принципе, ошибочная ситуация поймается и обработается в коде, но не так, как задумано. Предполагалось распечатывать статус ошибки, который хранится в переменной ret. Но значение ret всегда будет равно 0 или 1. Поэтому, если что-то пойдёт не так, всегда будет распечатываться только одно значение (-1).

Ошибка возникла из-за того, что не там поставлена скобочка. Правильный код:

if ((ret = mbedtls_ssl_conf_alpn_protocols(&tls->conf, cfg->alpn_protos)) != 0)

Теперь всё будет вычисляться как нужно:

ret = mbedtls_ssl_conf_alpn_protocols(....);if (ret != 0)  PRINT(...., -ret);

Рассмотрим ещё один очень похожий случай.

Ошибка N26; MP_MEM превращается в MP_YES

V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. libtommath.h 1660

В начале рассмотрим некоторые константы. Они пригодятся нам чуть ниже.

#define MP_OKAY       0   /* ok result */#define MP_MEM        -2  /* out of mem */#define MP_VAL        -3  /* invalid input */#define MP_YES        1   /* yes response */

Далее следует сказать, что существует функция mp_init_multi, которая может возвращать значения MP_OKAY и MP_MEM:

static int mp_init_multi(mp_int *mp, ...);

И теперь собственно код с ошибкой:

static intmp_div(mp_int * a, mp_int * b, mp_int * c, mp_int * d){  ....  /* init our temps */  if ((res = mp_init_multi(&ta, &tb, &tq, &q, NULL) != MP_OKAY)) {     return res;  }  ....}

Рассмотрим проверку более тщательно:

if ((res = mp_init_multi(....) != MP_OKAY))

Вновь не там поставлена скобка. Поэтому в начале вычисляется:

TEMP = (mp_init_multi(....) != MP_OKAY);

Значение TEMP может быть только 0 или 1. Этим числам соответствуют константы MB_OKAY и MP_YES.

Далее выполняется присваивание и одновременно проверка:

if ((res = TEMP))   return res;

Видите подвох? Статус ошибки MP_MEM (-2) вдруг превратился в статус MB_YES (1). Последствия предсказать не могу, но ничего хорошего в этом нет.

Ошибка N27; Забыли разыменовать указатель

Предупреждение PVS-Studio: V595 The 'outbuf' pointer was utilized before it was verified against nullptr. Check lines: 374, 381. protocomm.c 374

static int protocomm_version_handler(uint32_t session_id,                                     const uint8_t *inbuf, ssize_t inlen,                                     uint8_t **outbuf, ssize_t *outlen,                                     void *priv_data){    protocomm_t *pc = (protocomm_t *) priv_data;    if (!pc->ver) {        *outlen = 0;        *outbuf = NULL;                                  // <=        return ESP_OK;    }    /* Output is a non null terminated string with length specified */    *outlen = strlen(pc->ver);    *outbuf = malloc(*outlen);                           // <=    if (outbuf == NULL) {                                // <=        ESP_LOGE(TAG, "Failed to allocate memory for version response");        return ESP_ERR_NO_MEM;    }    memcpy(*outbuf, pc->ver, *outlen);    return ESP_OK;}

Сообщение анализатора, на первый взгляд, может показаться непонятным. Давайте разбираться.

Если указатель pc->ver является нулевым, то функция досрочно завершает свою работу и при этом записывает значение по адресу, хранящегося в указателе outbuf:

*outbuf = NULL;

Запись по этому адресу происходит и далее:

*outbuf = malloc(*outlen);

А не нравится анализатору то, что затем этот указатель проверяется:

if (outbuf == NULL)

Действительно, это неправильно, в начале разыменовывать указатель, а только потом его проверять. Ляп в том, что на самом то деле должны были проверять не сам указатель, а то, что в него записали. Здесь просто в проверке опечатались и пропустили оператор разыменования (*).

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

*outbuf = malloc(*outlen);if (*outbuf == NULL) {  ESP_LOGE(TAG, "Failed to allocate memory for version response");  return ESP_ERR_NO_MEM;}

Ошибка N28; Повторное присваивание

Предупреждение PVS-Studio: V519 The 'usRegCount' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 186, 187. mbfuncholding.c 187

eMBExceptioneMBFuncReadHoldingRegister( UCHAR * pucFrame, USHORT * usLen ){  ....  USHORT          usRegCount;  ....  usRegCount = ( USHORT )( pucFrame[MB_PDU_FUNC_READ_REGCNT_OFF] << 8 );  usRegCount = ( USHORT )( pucFrame[MB_PDU_FUNC_READ_REGCNT_OFF + 1] );  ....}

Код явно писался методом Copy-Paste. Строчку скопировали, но изменили только частично. По соседству есть вот такой осмысленный код:

usRegCount = ( USHORT )( pucFrame[MB_PDU_FUNC_WRITE_MUL_REGCNT_OFF] << 8 );usRegCount |= ( USHORT )( pucFrame[MB_PDU_FUNC_WRITE_MUL_REGCNT_OFF + 1] );

Видимо, и в рассмотренном коде с ошибкой, следовало в первой строке использовать оператор =, а во второй оператор |=.

Логические ошибки

Ошибка N29 N31; Неправильная работа с кодами возврата (Rare)

Предупреждение PVS-Studio: V547 Expression is always false. linenoise.c 256

static int getColumns(void) {  ....  /* Restore position. */  if (cols > start) {    char seq[32];    snprintf(seq,32,"\x1b[%dD",cols-start);    if (fwrite(seq, 1, strlen(seq), stdout) == -1) {      /* Can't recover... */    }    flushWrite();  }  ....}

Это безобидный вариант неправильной обработки статуса, возвращаемого функцией. Ошибка безобидна в том смысле, что никакой обработки и не предусмотрено. Не получилось записать строчку, так не получилось :). Хотя, как я уже сказал, этот безобидный вариант, такой стиль написания программ явно не является образцом для подражания.

Суть же самой ошибки в том, что функция fwrite не возвращает статус -1. Это физически невозможно, так как функция fwrite возвращает значение целочисленного типа size_t:

size_t fwrite( const void *restrict buffer, size_t size, size_t count,               FILE *restrict stream );

А вот что возвращает эта функция:

The number of objects written successfully, which may be less than count if an error occurs.

If size or count is zero, fwrite returns zero and performs no other action.

Таким образом, проверка статуса является неверной.

Аналогичные места безобидной неправильной проверки статуса:

• V547 Expression is always false. linenoise.c 481
• V547 Expression is always false. linenoise.c 569

Ошибка N32, N33; Неправильная работа с кодами возврата (Medium)

Предупреждение PVS-Studio: V547 Expression is always false. linenoise.c 596

int linenoiseEditInsert(struct linenoiseState *l, char c) {  ....  if (fwrite(&c,1,1,stdout) == -1) return -1;  ....}

Хотя перед нами та же ошибка, что и в предыдущем случае, она более серьезна. Если не удаётся записать символ в файл, то функция linenoiseEditInsert должна прекратить свою работу и вернуть статус -1. Но этого не произойдёт, так как fwrite никогда не вернёт значение -1. Перед нами логическая ошибка обработки ситуации, когда не удаётся что-то записать в файл.

Аналогичную ошибку можно найти здесь: V547 Expression is always false. linenoise.c 742.

Ошибка N34; Неправильная работа с кодами возврата (Well Done)

Предупреждение PVS-Studio: V547 Expression is always false. linenoise.c 828

static int linenoiseEdit(char *buf, size_t buflen, const char *prompt)  ....  while(1) {    ....    if (fread(seq+2, 1, 1, stdin) == -1) break;    ....  }  ....}

Ошибка в том, что, как и в случае с fwrite, функция fread не возвращает в качестве статуса значение -1.

size_t fread( void *restrict buffer, size_t size, size_t count,              FILE *restrict stream );

Return value

Number of objects read successfully, which may be less than count if an error or end-of-file condition occurs.

If size or count is zero, fread returns zero and performs no other action.

fread does not distinguish between end-of-file and error, and callers must use feof and ferror to determine which occurred.

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

Ошибка N35; Использование оператора || там, где нужен оператор &&

Предупреждение PVS-Studio: V547 Expression is always true. essl_sdio.c 209

esp_err_t essl_sdio_init(void *arg, uint32_t wait_ms){  ....  // Set block sizes for functions 1 to given value (default value = 512).  if (ctx->block_size > 0 || ctx->block_size <= 2048) {    bs = ctx->block_size;  } else {    bs = 512;  }  ....}

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

Итак, перед нами всегда истинное условие. Ведь некая переменная всегда или больше 0 или меньше 2048. Из-за этого размер какого-то блока не будет ограничен значением 512.

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

if (ctx->block_size > 0 && ctx->block_size <= 2048) {  bs = ctx->block_size;} else {  bs = 512;}

Ошибка N35 N38; Переменная не изменяется

Предупреждение PVS-Studio: V547 Expression 'depth <= 0' is always false. panic_handler.c 169

static void print_backtrace(const void *f, int core){  XtExcFrame *frame = (XtExcFrame *) f;  int depth = 100;                                          // <=  //Initialize stk_frame with first frame of stack  esp_backtrace_frame_t stk_frame =    {.pc = frame->pc, .sp = frame->a1, .next_pc = frame->a0};  panic_print_str("\r\nBacktrace:");  print_backtrace_entry(esp_cpu_process_stack_pc(stk_frame.pc),                        stk_frame.sp);  //Check if first frame is valid  bool corrupted =    !(esp_stack_ptr_is_sane(stk_frame.sp) &&      (esp_ptr_executable((void *)esp_cpu_process_stack_pc(stk_frame.pc)) ||       /* Ignore the first corrupted PC in case of InstrFetchProhibited */       frame->exccause == EXCCAUSE_INSTR_PROHIBITED));  //Account for stack frame that's already printed  uint32_t i = ((depth <= 0) ? INT32_MAX : depth) - 1;      // <=  ....}

Переменной depth присваивается значение 100, и до момента проверки этой переменной её значение нигде не изменяется. Это весьма подозрительно. Где-то что-то забыли сделать?

Аналогичные случаи:

• V547 Expression 'xAlreadyYielded == ((BaseType_t) 0)' is always true. event_groups.c 260
• V547 Expression 'xAlreadyYielded == ((BaseType_t) 0)' is always true. tasks.c 1475
• V547 Expression 'xAlreadyYielded == ((BaseType_t) 0)' is always true. tasks.c 1520

Ошибка N39; Использование неинициализированного буфера

Предупреждение PVS-Studio: V614 Potentially uninitialized buffer 'k' used. Consider checking the second actual argument of the 'sae_derive_keys' function. sae.c 854

int sae_process_commit(struct sae_data *sae){  u8 k[SAE_MAX_PRIME_LEN];  if (sae->tmp == NULL ||      (sae->tmp->ec && sae_derive_k_ecc(sae, k) < 0) ||      (sae->tmp->dh && sae_derive_k_ffc(sae, k) < 0) ||      sae_derive_keys(sae, k) < 0)    return ESP_FAIL;  return ESP_OK;}

Ошибка в логике. Предположим, что указатели ec и dh являются нулевыми. В этом случае массив k не инициализируется, но функция sae_derive_keys всё равно начнёт его обрабатывать.

Ошибка N40; Всегда ложное условие

Предупреждение PVS-Studio: V547 Expression 'bit_len == 32' is always false. spi_flash_ll.h 371

static inline void spi_flash_ll_set_usr_address(spi_dev_t *dev, uint32_t addr,                                                int bit_len){  // The blank region should be all ones  if (bit_len >= 32) {    dev->addr = addr;    dev->slv_wr_status = UINT32_MAX;  } else {    uint32_t padding_ones = (bit_len == 32? 0 : UINT32_MAX >> bit_len);    dev->addr = (addr << (32 - bit_len)) | padding_ones;  }}

Как легко увидеть, условие bit_len == 32 всегда даст ложный результат. Возможно, выше следовало написать не больше-или-равно (>=), а просто больше (>).

Ошибка N41; Повторное присваивание

Предупреждение PVS-Studio: V519 The '* pad_num' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 46, 48. touch_sensor_hal.c 48

void touch_hal_get_wakeup_status(touch_pad_t *pad_num){  uint32_t touch_mask = 0;  touch_ll_read_trigger_status_mask(&touch_mask);  if (touch_mask == 0) {    *pad_num = -1;  }  *pad_num = (touch_pad_t)(__builtin_ffs(touch_mask) - 1);}

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

void touch_hal_get_wakeup_status(touch_pad_t *pad_num){  uint32_t touch_mask = 0;  touch_ll_read_trigger_status_mask(&touch_mask);  if (touch_mask == 0) {    *pad_num = -1;  } else {    *pad_num = (touch_pad_t)(__builtin_ffs(touch_mask) - 1);  }}

Выход за границу массива

Ошибка N42; Неправильная граничная проверка

Предупреждение PVS-Studio: V557 Array overrun is possible. The value of 'frame->exccause' index could reach 16. gdbstub_xtensa.c 132

int esp_gdbstub_get_signal(const esp_gdbstub_frame_t *frame){  const char exccause_to_signal[] =    {4, 31, 11, 11, 2, 6, 8, 0, 6, 7, 0, 0, 7, 7, 7, 7};  if (frame->exccause > sizeof(exccause_to_signal)) {    return 11;  }  return (int) exccause_to_signal[frame->exccause];}

Возможен выход за границу массива на 1 элемент. Для правильной проверки следует использовать не оператор больше, а больше-или-равно:

if (frame->exccause >= sizeof(exccause_to_signal)) {

Ошибка N43; Длинный пример ошибки :)

В рассматриваемой функции выход за границу массива может произойти в двух местах, поэтому и предупреждений анализатора сразу два:

• V557 Array overrun is possible. The value of 'other_if' index could reach 3. mdns.c 2206
• V557 Array overrun is possible. The '_mdns_announce_pcb' function processes value '[0..3]'. Inspect the first argument. Check lines: 1674, 2213. mdns.c 1674

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

typedef enum mdns_if_internal {    MDNS_IF_STA = 0,    MDNS_IF_AP = 1,    MDNS_IF_ETH = 2,    MDNS_IF_MAX} mdns_if_t;

Обратите внимание, что значение константы MDNS_IF_MAX равно 3.

Теперь взглянем на определение структуры mdns_server_s. Здесь нам важно, что массив interfaces состоит из 3 элементов:

typedef struct mdns_server_s {    struct {        mdns_pcb_t pcbs[MDNS_IP_PROTOCOL_MAX];    } interfaces[MDNS_IF_MAX];    const char * hostname;    const char * instance;    mdns_srv_item_t * services;    SemaphoreHandle_t lock;    QueueHandle_t action_queue;    mdns_tx_packet_t * tx_queue_head;    mdns_search_once_t * search_once;    esp_timer_handle_t timer_handle;} mdns_server_t;mdns_server_t * _mdns_server = NULL;

Это ещё не всё. Нам понадобится заглянуть внутрь функции _mdns_get_other_if. Обратите внимание, что она может вернуть константу MDNS_IF_MAX. Т.е. она может вернуть значение 3.

static mdns_if_t _mdns_get_other_if (mdns_if_t tcpip_if){  if (tcpip_if == MDNS_IF_STA) {    return MDNS_IF_ETH;  } else if (tcpip_if == MDNS_IF_ETH) {     return MDNS_IF_STA;  }  return MDNS_IF_MAX;}

И вот, наконец, мы добрались до ошибок:

static void _mdns_dup_interface(mdns_if_t tcpip_if){    uint8_t i;    mdns_if_t other_if = _mdns_get_other_if (tcpip_if);    for (i=0; i<MDNS_IP_PROTOCOL_MAX; i++) {        if (_mdns_server->interfaces[other_if].pcbs[i].pcb) {        // <=            //stop this interface and mark as dup            if (_mdns_server->interfaces[tcpip_if].pcbs[i].pcb) {                _mdns_clear_pcb_tx_queue_head(tcpip_if, i);                _mdns_pcb_deinit(tcpip_if, i);            }            _mdns_server->interfaces[tcpip_if].pcbs[i].state = PCB_DUP;            _mdns_announce_pcb(other_if, i, NULL, 0, true);          // <=        }    }}

Итак, мы знаем, что функция _mdns_get_other_if может вернуть тройку. Значит переменная other_if может быть равна трём. И вот первый потенциальный выход за границу массива:

if (_mdns_server->interfaces[other_if].pcbs[i].pcb)

Второе место, где опасно используется переменная other_if, - это вызов функции _mdns_announce_pcb:

_mdns_announce_pcb(other_if, i, NULL, 0, true);

Заглянем в эту функцию:

static void _mdns_announce_pcb(mdns_if_t tcpip_if,                               mdns_ip_protocol_t ip_protocol,                               mdns_srv_item_t ** services,                               size_t len, bool include_ip){  mdns_pcb_t * _pcb = &_mdns_server->interfaces[tcpip_if].pcbs[ip_protocol];  ....}

Опять может использоваться индекс 3 для доступа к массиву, состоящего из 3 элементов. А максимальный доступный индекс это двойка.

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

Ошибка N44 N47; Ошибка очерёдности проверки указателей

Предупреждение PVS-Studio: V595 The 'hapd->wpa_auth' pointer was utilized before it was verified against nullptr. Check lines: 106, 113. esp_hostap.c 106

bool hostap_deinit(void *data){  struct hostapd_data *hapd = (struct hostapd_data *)data;  if (hapd == NULL) {    return true;  }  if (hapd->wpa_auth->wpa_ie != NULL) {    os_free(hapd->wpa_auth->wpa_ie);  }  if (hapd->wpa_auth->group != NULL) {    os_free(hapd->wpa_auth->group);  }  if (hapd->wpa_auth != NULL) {    os_free(hapd->wpa_auth);  }  ....}

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

if (hapd->wpa_auth->group != NULL)....if (hapd->wpa_auth != NULL)

Если указатель hapd->wpa_auth окажется нулевым, то всё плохо. Последовательность действий нужно поменять местами и сделать вложенной:

if (hapd->wpa_auth != NULL){  ....  if (hapd->wpa_auth->group != NULL)  ....}

Аналогичные ошибки:

• V595 The 'hapd->conf' pointer was utilized before it was verified against nullptr. Check lines: 118, 125. esp_hostap.c 118
• V595 The 'sm' pointer was utilized before it was verified against nullptr. Check lines: 1637, 1647. esp_wps.c 1637
• V595 The 'sm' pointer was utilized before it was verified against nullptr. Check lines: 1693, 1703. esp_wps.c 1693

Ошибка N48 N64; Нет проверки указателя после выделения памяти

В целом, в проекте принято проверять, удалось выделить память или нет. Т.е. много кода с подобными проверками:

dhcp_data = (struct dhcp *)malloc(sizeof(struct dhcp));if (dhcp_data == NULL) {  return ESP_ERR_NO_MEM;}

Но местами про проверки забыли.

Предупреждение PVS-Studio: V522 There might be dereferencing of a potential null pointer 'exp'. Check lines: 3470, 3469. argtable3.c 3470

TRex *trex_compile(const TRexChar *pattern,const TRexChar **error,int flags){  TRex *exp = (TRex *)malloc(sizeof(TRex));  exp->_eol = exp->_bol = NULL;  exp->_p = pattern;  ....}

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

Другие места, где отсутствуют проверки:

• V522 There might be dereferencing of a potential null pointer 's_ledc_fade_rec[speed_mode][channel]'. Check lines: 668, 667. ledc.c 668
• V522 There might be dereferencing of a potential null pointer 'environ'. Check lines: 108, 107. syscall_table.c 108
• V522 There might be dereferencing of a potential null pointer 'it'. Check lines: 150, 149. partition.c 150
• V522 There might be dereferencing of a potential null pointer 'eth'. Check lines: 167, 159. wpa_auth.c 167
• V522 There might be dereferencing of a potential null pointer 'pt'. Check lines: 222, 219. crypto_mbedtls-ec.c 222
• V522 There might be dereferencing of a potential null pointer 'attr'. Check lines: 88, 73. wps.c 88
• V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 725, 724. coap_mbedtls.c 725
• V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 3504, 3503. argtable3.c 3504
• V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 496, 495. mqtt_client.c 496
• V575 The potential null pointer is passed into 'strcpy' function. Inspect the first argument. Check lines: 451, 450. transport_ws.c 451
• V769 The 'buffer' pointer in the 'buffer + n' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 186, 181. cbortojson.c 186
• V769 The 'buffer' pointer in the 'buffer + len' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 212, 207. cbortojson.c 212
• V769 The 'out' pointer in the 'out ++' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 233, 207. cbortojson.c 233
• V769 The 'parser->m_bufferPtr' pointer in the expression equals nullptr. The resulting value of arithmetic operations on this pointer is senseless and it should not be used. xmlparse.c 2090
• V769 The 'signature' pointer in the 'signature + curve->prime_len' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 4112, 4110. dpp.c 4112
• V769 The 'key' pointer in the 'key + 16' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 634, 628. eap_mschapv2.c 634

Ошибка N65, N66; Нет проверки указателя после выделения памяти (показательный случай)

Следующий код содержит точно такую же ошибку, как мы рассматривали выше, но она более показательная и яркая. Обратите внимания, что для выделения памяти используется функция realloc.

Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'exp->_nodes' is lost. Consider assigning realloc() to a temporary pointer. argtable3.c 3008

static int trex_newnode(TRex *exp, TRexNodeType type){  TRexNode n;  int newid;  n.type = type;  n.next = n.right = n.left = -1;  if(type == OP_EXPR)    n.right = exp->_nsubexpr++;  if(exp->_nallocated < (exp->_nsize + 1)) {    exp->_nallocated *= 2;    exp->_nodes = (TRexNode *)realloc(exp->_nodes,                                      exp->_nallocated * sizeof(TRexNode));  }  exp->_nodes[exp->_nsize++] = n; // NOLINT(clang-analyzer-unix.Malloc)  newid = exp->_nsize - 1;  return (int)newid;}

Во-первых, если функция realloc вернёт NULL, то будет потеряно предыдущее значение указателя exp->_nodes. Возникнет утечка памяти.

Во-вторых, если функция realloc вернёт NULL, то запись значения произойдёт вовсе не по нулевому указателю. Имеется в виду эта строка:

exp->_nodes[exp->_nsize++] = n;

Значение exp->_nsize++ может быть любым, и, если запись произойдёт в какую-то случайную область памяти, доступную для записи, то программа продолжит своё выполнение, как ни в чём не бывало. При этом будут разрушены структуры данных, что приведёт к непредсказуемым последствиям.

Ещё одна такая ошибка: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'm_context->pki_sni_entry_list' is lost. Consider assigning realloc() to a temporary pointer. coap_mbedtls.c 737

Прочие ошибки

Ошибка N67; Лишний или неверный код

Предупреждение PVS-Studio: V547 Expression 'ret != 0' is always false. sdio_slave.c 394

esp_err_t sdio_slave_start(void){  ....  critical_exit_recv();  ret = ESP_OK;  if (ret != ESP_OK) return ret;  sdio_slave_hal_set_ioready(context.hal, true);  return ESP_OK;}

Это странный код, который можно сократить до:

esp_err_t sdio_slave_start(void){  ....  critical_exit_recv();  sdio_slave_hal_set_ioready(context.hal, true);  return ESP_OK;}

Есть здесь ошибка или нет, мне сказать сложно. Возможно, здесь написано совсем не то, что задумано. А возможно, этот код появился в процессе неудачного рефакторинга и, на самом деле, является корректным. В этом случае его действительно достаточно немного упростить, чтобы он смотрелся красивей и понятней. Одно точно этот код заслуживает внимания и проверки автором.

Ошибка N68; Лишний или неверный код

Предупреждение PVS-Studio: V547 Expression 'err != 0' is always false. sdio_slave_hal.c 96

static esp_err_t sdio_ringbuf_send(....){  uint8_t* get_ptr = ....;  esp_err_t err = ESP_OK;  if (copy_callback) {    (*copy_callback)(get_ptr, arg);  }  if (err != ESP_OK) return err;  buf->write_ptr = get_ptr;  return ESP_OK;}

В общем-то, всё то же самое, что и в предыдущем случае. Переменная err является лишней или её забыли изменить.

Ошибка N69; Использование потенциально неинициализированного буфера

Предупреждение PVS-Studio: V614 Potentially uninitialized buffer 'seq' used. Consider checking the first actual argument of the 'strlen' function. linenoise.c 435

void refreshShowHints(struct abuf *ab, struct linenoiseState *l, int plen) {    char seq[64];    if (hintsCallback && plen+l->len < l->cols) {        int color = -1, bold = 0;        char *hint = hintsCallback(l->buf,&color,&bold);        if (hint) {            int hintlen = strlen(hint);            int hintmaxlen = l->cols-(plen+l->len);            if (hintlen > hintmaxlen) hintlen = hintmaxlen;            if (bold == 1 && color == -1) color = 37;            if (color != -1 || bold != 0)                snprintf(seq,64,"\033[%d;%d;49m",bold,color);            abAppend(ab,seq,strlen(seq));                       // <=            abAppend(ab,hint,hintlen);            if (color != -1 || bold != 0)                abAppend(ab,"\033[0m",4);            /* Call the function to free the hint returned. */            if (freeHintsCallback) freeHintsCallback(hint);        }    }}

Буфер seq может быть заполнен, а может быть и не заполнен! Он заполняется только при выполнении условия:

if (color != -1 || bold != 0)  snprintf(seq,64,"\033[%d;%d;49m",bold,color);

Логично предположить, что условие может быть не выполнено, и тогда буфер останется неинициализированным. В этом случае его нельзя использовать для добавления к строке ab.

Чтобы исправить ситуацию, стоит изменить код следующим образом:

if (color != -1 || bold != 0){  snprintf(seq,64,"\033[%d;%d;49m",bold,color);  abAppend(ab,seq,strlen(seq));}

Ошибка N70; Странная маска

Предупреждение PVS-Studio: V547 Expression is always false. tasks.c 896

#ifndef portPRIVILEGE_BIT  #define portPRIVILEGE_BIT ( ( UBaseType_t ) 0x00 )#endifstatic void prvInitialiseNewTask(...., UBaseType_t uxPriority, ....){  StackType_t *pxTopOfStack;  UBaseType_t x;  #if (portNUM_PROCESSORS < 2)  xCoreID = 0;  #endif  #if( portUSING_MPU_WRAPPERS == 1 )    /* Should the task be created in privileged mode? */    BaseType_t xRunPrivileged;    if( ( uxPriority & portPRIVILEGE_BIT ) != 0U )    {      xRunPrivileged = pdTRUE;    }    else    {      xRunPrivileged = pdFALSE;    }  ....}

Константа portPRIVILEGE_BIT имеет значение 0. Поэтому странно использовать его как маску:

if( ( uxPriority & portPRIVILEGE_BIT ) != 0U )

Ошибка N71, Утечка памяти

Предупреждение PVS-Studio: V773 The function was exited without releasing the 'sm' pointer. A memory leak is possible. esp_wpa2.c 753

static int eap_peer_sm_init(void){  int ret = 0;  struct eap_sm *sm;  ....  sm = (struct eap_sm *)os_zalloc(sizeof(*sm));  if (sm == NULL) {    return ESP_ERR_NO_MEM;  }  s_wpa2_data_lock = xSemaphoreCreateRecursiveMutex();  if (!s_wpa2_data_lock) {    wpa_printf(MSG_ERROR, ".......");  // NOLINT(clang-analyzer-unix.Malloc)    return ESP_ERR_NO_MEM;             // <=  }  ....}

Если функция xSemaphoreCreateRecursiveMutex не сможет создать мьютекс, то функция eap_peer_sm_init завершит свою работу и при этом произойдёт утечка памяти. Как я понимаю, следует добавить вызов функции os_free для очистки памяти:

  s_wpa2_data_lock = xSemaphoreCreateRecursiveMutex();  if (!s_wpa2_data_lock) {    wpa_printf(MSG_ERROR, ".......");    os_free(sm);    return ESP_ERR_NO_MEM;  }

Что интересно, компилятор Clang тоже предупреждает об этой ошибке. Однако автор кода почему-то проигнорировал и даже специально подавил соответствующее предупреждение:

// NOLINT(clang-analyzer-unix.Malloc)

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

Заключение

Спасибо за внимание. Как видите, ошибок весьма много. А это ведь был только беглый просмотр неполного отчёта. Надеюсь, Юрий Попов примет эстафету и опишет ещё больше ошибок в своей последующей статье :).

Используйте статический анализатор PVS-Studio регулярно. Это позволит:

1. Находить многие ошибки на раннем этапе, что существенно сократит расходы на их обнаружение и исправление;
2. Находя и исправляя глупые опечатки и прочие ляпы с помощью статического анализа, вы высвободите время, которое можно потратить на более высокоуровневый обзор кода и алгоритмов;
3. Лучше контролировать качество кода новичков и быстрее обучать их писать красивый надежный код;
4. Если речь идёт о программном обеспечении для встраиваемых устройств, то очень важно устранить как можно больше ошибок до выпуска устройств в эксплуатацию. Поэтому любая дополнительно найденная ошибка с помощью анализатора кода, это здорово. Каждая незамеченная ошибка в программно-аппаратном устройстве потенциально несёт репутационные риски и затраты на обновление прошивок.

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Espressif IoT Development Framework: 71 Shots in the Foot.

Относительно недавно состоялся релиз фреймворка 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.

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

Недавно мы настроили регулярную проверку проекта Blender, о чём мой коллега рассказал в статье "Just for fun: команда PVS-Studio придумала мониторить качество некоторых открытых проектов". В дальнейшем планируем начать мониторить ещё некоторые интересные проекты.

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

Итак, давайте посмотрим, что найдено в свежем коде проекта Blender.

Фрагмент первый: double-checked locking

typedef struct bNodeTree {  ....  struct NodeTreeUIStorage *ui_storage;} bNodeTree;static void ui_storage_ensure(bNodeTree &ntree){  /* As an optimization, only acquire a lock if the UI storage doesn't exist,   * because it only needs to be allocated once for every node tree. */  if (ntree.ui_storage == nullptr) {    std::lock_guard<std::mutex> lock(global_ui_storage_mutex);    /* Check again-- another thread may have allocated the storage       while this one waited. */    if (ntree.ui_storage == nullptr) {      ntree.ui_storage = new NodeTreeUIStorage();    }  }}

Предупреждение PVS-Studio. V1036: Potentially unsafe double-checked locking. node_ui_storage.cc 46

Перед нами неправильная реализация блокировки с двойной проверкой. Для пояснения проблемы процитирую фрагмент статьи "C++ and the Perils of Double-Checked Locking", написанной Scott Meyers и Andrei Alexandrescu ещё в 2004 году. Как видите, проблема давно известна, но это не защищает разработчиков от того, чтобы наступать на одни и те же грабли. Хорошо, что анализатор PVS-Studio помогает выявлять подобные проблемы :). Итак, фрагмент из статьи:

Consider again the line that initializes pInstance: pInstance = newSingleton;

This statement causes three things to happen:

Step 1: Allocate memory to hold a Singleton object.

Step 2: Construct a Singleton object in the allocated memory.

Step 3: Make pInstance point to the allocated memory.

Of critical importance is the observation that compilers are not constrainedto perform these steps in this order! In particular, compilers are sometimes allowed to swap steps 2 and 3. Why they might want to do that is a question we'll address in a moment. For now, let's focus on what happens if they do.

Consider the following code, where we've expanded pInstance's initialization line into the three constituent tasks we mentioned above and where we've merged steps 1 (memory allocation) and 3 (pInstance assignment) into a single statement that precedes step 2 (Singleton construction). The idea is not that a human would write this code. Rather, it's that a compiler might generate code equivalent to this in response to the conventional DCLP source code (shown earlier) that a human would write.

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

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

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

Фрагмент второй: realloc

static void icon_merge_context_register_icon(struct IconMergeContext *context,                                             const char *file_name,                                             struct IconHead *icon_head){  context->read_icons = realloc(context->read_icons,    sizeof(struct IconInfo) * (context->num_read_icons + 1));  struct IconInfo *icon_info = &context->read_icons[context->num_read_icons];  icon_info->head = *icon_head;  icon_info->file_name = strdup(path_basename(file_name));  context->num_read_icons++;}

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

Первая: V701: realloc() possible leak: when realloc() fails in allocating memory, original pointer 'context->read_icons' is lost. Consider assigning realloc() to a temporary pointer. datatoc_icon.c 252

Если память не удастся выделить, функция realloc вернёт значение NULL. Нулевой указатель будет записан в переменную context->read_icons, а её предыдущее значение будет потеряно. Раз предыдущее значение указателя потеряно, то и невозможно освободить ранее выделенный блок памяти, на который ссылался этот указатель. Произойдёт утечка памяти.

Вторая: V522: There might be dereferencing of a potential null pointer 'context->read_icons'. Check lines: 255, 252. datatoc_icon.c

Описанная выше ошибка в каком-то смысле и не ошибка с точки зрения автора кода. Он и не собирался писать код, который будет продолжать работу в случае невозможности увеличения блока выделенной памяти. Такой случай просто не рассматривается. Автор предполагает, что, если память выделить не удалось, программа просто аварийно завершится при разыменовании нулевого указателя. И поэтому смело работает с указателем, не выполняя его предварительную проверку. Оставим в стороне вопрос, насколько красивым является такое поведение программы. На мой взгляд, такое поведение библиотек недопустимо.

Интереснее другое. На самом-то деле падение может и не произойти. Запись производится вовсе не по нулевому указателю, а куда-то дальше. Теоретически возможна ситуация, когда этот адрес уже не находится в странице памяти, защищенной от записи, и никакого падения не будет. Будут испорчены какие-то случайные данные в памяти, и программа продолжит своё выполнение. Последствия работы с испорченными данными непредсказуемы. Подробнее всё это разобрано в статье "Почему важно проверять, что вернула функция malloc".

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

static int node_link_invoke(bContext *C, wmOperator *op, const wmEvent *event){  ....  bNodeLinkDrag *nldrag = node_link_init(bmain, snode, cursor, detach);  nldrag->last_picked_multi_input_socket_link = NULL;  if (nldrag) {    op->customdata = nldrag;  ....}

Предупреждение PVS-Studio: V595: The 'nldrag' pointer was utilized before it was verified against nullptr. Check lines: 1037, 1039. node_relationships.c

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

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

Кстати, нашлась ещё одна такая-же ошибка, но описывать её неинтересно. Приведу только сообщение: V595: The 'seq' pointer was utilized before it was verified against nullptr. Check lines: 373, 385. strip_add.c

Заключение

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. PVS-Studio, Blender: Series of Notes on Advantages of Regular Static Analysis of Code.

Хотя только недавно была заметка про проект 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.

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

Всё началось с твита от JetBrains CLion IDE

На днях я увидел в Twitter пост от JetBrains про новые возможности статического анализатора, встроенного в CLion.

Поскольку мы скоро планируем выпустить плагин PVS-Studio для CLion, то я не мог пройти мимо и не написать, что мы тоже не лыком шиты. И что есть смысл попробовать PVS-Studio как плагин для CLion, чтобы находить ещё больше ошибок.

Ну и ещё немного с ними мило попереписывался:

• А это можно найти с помощью clang-analyzer;
• А это нельзя :)

После всего этого я подумал. А ведь они молодцы! Улучшили анализ потока данных и рассказывают миру. А мы чем хуже? Мы ведь тоже постоянно что-то улучшаем внутри анализатора, в том числе и тот же механизм анализа потока данных. И вот я уже пишу эту заметку.

А что у нас интересного с Data Flow

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

bool foo(){  unsigned N = 2;  for (unsigned i = 0; i < N; ++i)  {    bool stop = (i - 1 == N);    if (stop)      return true;  }  return false;}

Анализатор не мог понять, что переменной stop всегда присваивается значение false.

Почему false? Давайте быстро посчитаем:

• диапазон значения переменной i = [0; 1];
• возможные значения выражения i-1 = [0; 0] U [UINT_MAX; UINT_MAX];
• значение переменной N, равное двойке, не входит в множество { 0, UINT_MAX };
• условие всегда ложно.

Примечание. Неопределённого поведения здесь нет, так как происходит переполнение (wrap) при работе с беззнаковым типом.

Теперь мы научили PVS-Studio правильно работать с такими выражениями и выдавать соответствующее предупреждение. Что интересно, это изменение повлекло каскад других доработок.

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

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

int Assemble(unsigned char *output, int addr, char *str) {  output[0] = output[1] = output[2] = 0;  char astr[128],ins[4];  if ((!strlen(str)) || (strlen(str) > 0x127)) return 1;  strcpy(astr,str);  ....}

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

Предупреждение PVS-Studio: V512 A call of the 'strcpy' function will lead to overflow of the buffer 'astr'. asm.cpp 21

Всё равно не видите ошибку? Давайте внимательно разберём код. Для начала уберём всё не относящееся к делу:

int Assemble(char *str) {  char astr[128];  if ((!strlen(str)) || (strlen(str) > 0x127)) return 1;  strcpy(astr,str);  ....}

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

Пока всё логично и правильно? На первый взгляд, да. Но что это?! Что это за константа 0x127?!

Это вовсе не 127. Совсем не 127 :)

Константа задана в шестнадцатеричной системе. Если перевести в десятичную, то получается 295.

Итак, написанный код эквивалентен следующему:

int Assemble(char *str) {  char astr[128];  if ((!strlen(str)) || (strlen(str) > 295)) return 1;  strcpy(astr,str);  ....}

Как видите, проверка никак не защищает от переполнения буфера, и анализатор совершенно правильно предупреждает о проблеме.

Раньше анализатор не мог найти ошибку, будучи не в состоянии понять, что две функции strlen работают с одной строкой. И эта строка не меняется между двумя вызовами strlen. С точки зрения программиста, всё это очевидно, а вот анализатор нужно учить всё это понимать :).

Теперь PVS-Studio выводит из выражения, что длина строки str лежит в диапазоне [1..295], а значит, может возникнуть выход за границу массива, если попытаться его скопировать в буфер astr.

Новые вызовы

Описанная ошибка присутствует и в текущей версии кодовой базы проекта FCEUX. Но мы её не найдём, так как код изменился и теперь длина строки сохраняется в переменной. Это разрывает взаимосвязь между строкой и её длиной. Анализатор пока, к сожалению, молчит на новый вариант кода:

int Assemble(unsigned char *output, int addr, char *str) {  output[0] = output[1] = output[2] = 0;  char astr[128],ins[4];  int len = strlen(str);  if ((!len) || (len > 0x127)) return 1;  strcpy(astr,str);  ....}

Человеку такой код может показаться даже проще, но, с точки зрения статического анализа, он труден для отслеживания значений. Нужно учитывать, что значение переменной len является длиной строки str. Дополнительно требуется аккуратно отслеживать, когда разоврётся эта взаимосвязь при модификации содержимого строки или переменной len.

Пока это анализатор PVS-Studio делать не умеет. Зато видно, куда можно и нужно развиваться! Со временем научимся находить ошибку и в этом новом коде.

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

А чтобы тестировать анализатор на современном коде, написанном на C++14, C++17 и т.д., мы постепенно пополняем базу новыми проектами. Например, относительно недавно мы добавили коллекцию header-only C++ библиотек (awesome-hpp).

Заключение

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

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

Приглашаю скачать анализатор PVS-Studio и проверить свои проекты.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. PVS-Studio Learns What strlen is All About.

Зачем нужен статический анализ?

auto x = obj.x;auto y = obj.y;auto z = obj.z;

auto x = obj.x;auto y = obj.y;auto z = obj.x;

• Топ 10 ошибок в проектах C++ за 2019 год
• Топ 10 ошибок в проектах C# за 2019 год
• Топ 10 ошибок в проектах Java за 2019 год

0. Знакомство с инструментом

• Какие есть способы взаимодействия с анализатором;
• Совместим ли анализатор с вашей средой разработки;
• Какие проблемы есть сейчас в ваших проектах.

1. Автоматизация

• Какие варианты автоматизации предоставляет инструмент;
• Совместим ли анализатор с вашей сборочной системой.

• PVS-Studio_Cmd.exe (анализ решений, C#, C++ проектов на Windows)
• CLMonitor.exe (мониторинг компиляции)
• pvs-studio-analyzer (анализ C++ проектов на Linux / macOS)
• pvs-studio-dotnet (анализ решений, C# проектов на Linux / macOS)
• pvs-studio.jar (анализ Java проектов)
• PlogConverter (конвертер файлов отчёта)

• Установить анализатор;
• Запустить анализ;
• Доставить результаты.

wget -q -O - https://files.viva64.com/etc/pubkey.txt \    | sudo apt-key add -sudo wget -O /etc/apt/sources.list.d/viva64.list \  https://files.viva64.com/etc/viva64.list  sudo apt-get update -qqsudo apt-get install -qq pvs-studio

PVS-Studio_setup.exe /verysilent /suppressmsgboxes /norestart /nocloseapplications

pvs-studio-analyzer analyze -j8 \                            -o PVS-Studio.logplog-converter -t errorfile PVS-Studio.log --cerr -w

git diff --name-only HEAD origin/$MERGE_BASE > .pvs-pr.list

pvs-studio-analyzer analyze -j8 \                            -o PVS-Studio.log \                            -S .pvs-pr.list

2. Интеграция на машины разработчиков

3. Ежедневное использование

• Как запустить PVS-Studio в Windows (C, C++, C#)
• Как запустить PVS-Studio в Linux и macOS (C, C++)
• Как запустить PVS-Studio Java

После интеграции

Порядок вычисления аргументов

#include <cstdio>int main(){  int i = 1;  printf("%d, %d\n", i, i++);  return 0;}

• программа, скомпилированная с помощью Clang 11.0.0, выдаёт "1, 1".
• программа, скомпилированная с помощью GCC 10.2, выдаёт "2, 1".

Код из проекта CSV Parser

std::unique_ptr<char[]> buffer(new char[BUFFER_UPPER_LIMIT]);....this->feed_state->feed_buffer.push_back(    std::make_pair<>(std::move(buffer), line_buffer - buffer.get()));

Foo(std::move(buffer), line_buffer - buffer.get());

#include <iostream>#include <memory>   void Print(std::unique_ptr<char[]> p, ptrdiff_t diff){    std::cout << diff << std::endl;} void Print2(ptrdiff_t diff, std::unique_ptr<char[]> p){    std::cout << diff << std::endl;} int main(){    {        std::unique_ptr<char[]> buffer(new char[100]);        char *ptr = buffer.get() + 22;        Print(std::move(buffer), ptr - buffer.get());    }    {        std::unique_ptr<char[]> buffer(new char[100]);        char *ptr = buffer.get() + 22;        Print2(ptr - buffer.get(), std::move(buffer));    }    return 0;}

2338784622

2226640070

std::make_pair

template< class T1, class T2 >
std::pair<T1,T2> make_pair( T1 t, T2 u );

template< class T1, class T2 >
std::pair<V1,V2> make_pair( T1&& t, T2&& u );

template< class T1, class T2 >
constexpr std::pair<V1,V2> make_pair( T1&& t, T2&& u );

Король умер, да здравствует король!

auto delta = line_buffer - buffer.get();this->feed_state->feed_buffer.push_back(  std::make_pair(std::move(buffer), delta));

auto delta = line_buffer - buffer.get();this->feed_state->feed_buffer.emplace_back(std::move(buffer), delta);

Заключение

Foo(std::move(buffer), line_buffer - buffer.get());

А что, если я скажу вам, что линтеры для Go можно создавать вот таким декларативным способом?

func alwaysTrue(m dsl.Matcher) {    m.Match(`strings.Count($_, $_) >= 0`).Report(`always evaluates to true`)    m.Match(`bytes.Count($_, $_) >= 0`).Report(`always evaluates to true`)}func replaceAll() {    m.Match(`strings.Replace($s, $d, $w, $n)`).        Where(m["n"].Value.Int() <= 0).        Suggest(`strings.ReplaceAll($s, $d, $w)`)}

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

Основные нововведения:

• Поддержка установки наборов правил через Go модули (bundles)
• Программируемые фильтры (компилируются в байт-код)
• Добавлен режим отладки фильтров
• Появился хороший обучающий материал: ruleguard by example
• У проекта появились реальные пользователи и внешние наборы правил
• Онлайн песочница, позволяющая попробовать ruleguard прямо в браузере

Небольшое введение

ruleguard это платформа для запуска динамических диагностик. Что-то вроде интерпретатора для скриптов, специализирующихся на статическом анализе.

Вы описываете на DSL свой набор правил (или используете уже готовые наборы) и запускаете их через утилиту ruleguard.

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

Если называть наиболее близкие к этой концепции проекты, то в голову приходят CodeQL и Semgrep. Некоторое время назад я проводил сравнение, хотя часть информации из того доклада уже устарела (все проекты получают новые фичи).

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

Большая часть нововведений адресует какую-то конкретную проблему, отсюда и формат заголовков.

Проблема: переиспользование наборов правил

Раньше всё было относительно просто: есть файл с правилами, утилита принимает его на вход и применяет его к проверяемой кодовой базе.

Далее мы понимаем, что хранить всё в одном файле не очень удобно, и я добавляю поддержку множественных файлов правил.

Затем появился хороший набор правил, написанный Damian Gryski. Единственный способ его использовать на своих проектах это копировать в свой репозиторий.

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

Новый механизм бандлов для правил позволит решить сразу несколько проблем:

• Установка бандлов через go get
• Версионирование с помощью Go модулей: удобно делать релизы и закреплять версию
• Культура оформления правил в модули упрощает тестирование

Всё это возможно благодаря тому, что ruleguard файлы, в которых пишутся правила это обычный Go код (по этой же причине мы имеем нормальный autocomplete и поддержку редакторов).

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

package gorulesimport (    "github.com/quasilyte/go-ruleguard/dsl"    damianrules "github.com/dgryski/semgrep-go")func init() {    // Импорт всех правил, без префикса.    dsl.ImportRules("", damianrules.Bundle)}func emptyStringTest(m dsl.Matcher) {    m.Match(`len($s) == 0`).        Where(m["s"].Type.Is("string")).        Report(`maybe use $s == "" instead?`)    m.Match(`len($s) != 0`).        Where(m["s"].Type.Is("string")).        Report(`maybe use $s != "" instead?`)}

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

Проблема: недостаточная выразительность DSL

dsl.Matcher предоставляет несколько фильтров, которые часто нужны в типичных для ruleguard правилах.

Но бывают моменты, когда требуется создать довольно сложное условие или фильтр, имеющий промежуточные результаты. В этой ситуации можно использовать новый метод Filter(), который принимает Go функцию-предикат в качестве аргумента. Эта функция будет вызываться во время применения фильтра.

package gorulesimport (    "github.com/quasilyte/go-ruleguard/dsl"    "github.com/quasilyte/go-ruleguard/dsl/types")// implementsStringer является пользовательским фильтром.// Этот фильтр проверяет, реализуют ли T или *T интерфейс `fmt.Stringer`.func implementsStringer(ctx *dsl.VarFilterContext) bool {    stringer := ctx.GetInterface(`fmt.Stringer`)    return types.Implements(ctx.Type, stringer) ||        types.Implements(types.NewPointer(ctx.Type), stringer)}func sprintStringer(m dsl.Matcher) {    // Если бы мы использовали m["x"].Type.Implements(`fmt.Stringer`), тогда    // мы бы не получили все желаемые результаты: если тип $x реализует    // fmt.Stringer как *T, то значения типа T не будут считаться реализациями.    // Наш кастомный фильтр примеряет обе версии: с указателем и без укатателя.    m.Match(`fmt.Sprint($x)`).        Where(m["x"].Filter(implementsStringer) && m["x"].Addressable).        Report(`can use $x.String() directly`)}

Запускать эти правила будем на следующем файле:

package mainimport "fmt"func main() {    fooPtr := &Foo{}    foo := Foo{}    println(fmt.Sprint(foo))    println(fmt.Sprint(fooPtr))    println(fmt.Sprint(0))    // Не fmt.Stringer    println(fmt.Sprint(&foo)) // Отбрасывается условием addressable}type Foo struct{}func (*Foo) String() string { return "Foo" }

Результат запуска:

$ ruleguard -rules rules.go main.gomain.go:9:10: can use foo.String() directlymain.go:10:10: can use fooPtr.String() directly

Флаг -debug-filter позволяет посмотреть, во что скомпилировался выбранный фильтр:

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

Проблема: правила сложно отлаживать

Поскольку в Where() может использоваться довольно сложное выражение, не всегда понятно, почему правило не срабатывает на анализируемых фрагментах кода.

На помощь приходит новый флаг debug-group, включающий детальную информацию о неуспешно выполнившихся фильтрах для выбранной группы правил.

Допустим, вы описали следующее правило:

func offBy1(m dsl.Matcher) {    m.Match(`$s[len($s)]`).        Where(m["s"].Type.Is(`[]$elem`) && m["s"].Pure).        Report(`index expr always panics; maybe you wanted $s[len($s)-1]?`)}

И запустили его на следующем файле:

func lastByte(s string) byte {    return s[len(s)]}func f() byte {    return randString()[len(randString())]}

И не получили ни одного предупреждения Давайте попробуем включить отладочную печать.

$ ruleguard -rules rules.go -debug-group offBy1 test.gotest.go:6: [rules.go:6] rejected by m["s"].Type.Is(`[]$elem`)  $s string: stest.go:10: [rules.go:6] rejected by m["s"].Pure  $s []byte: randBytes()

Мы видим конкретное выражение из Where(), которое не дало сработать правилу. Мы также видим все захваченные Go выражения в именованных частях AST шаблона (в данном случае это $s), а также их тип.

В первом случае условие типа []$elem требует произвольного слайса, а в коде строка. Во втором случае правило не срабатывает из-за вызова функции (нарушается условие pure).

Скорее всего, мы не хотим убирать условие на чистоту выражений, а вот добавить тип string в диагностику можно:

- Where(m["s"].Type.Is(`[]$elem`) && m["s"].Pure).+ Where((m["s"].Type.Is(`[]$elem`) || m["s"].Type.Is(`string`)) && m["s"].Pure).

Повторный запуск с обновлённой версией найдёт ошибку в индексировании строки:

test.go:6:9: offBy1: index expr always panics; maybe you wanted s[len(s)-1]?

Проблема: трудности изучения DSL

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

Мне нравится подход Go by Example. В нём введение производится через набор примеров с пояснениями, от простого к более продвинутому. Это полезно как начинающим, так и продолжающим.

Ruleguard by Example написан в таком же стиле. Он позволяет достаточно быстро получить все необходимые знания в наглядной форме.

Как начать использовать ruleguard?

Внимание! Лучше всего ruleguard работает с проектами, которые используют Go модули.

Лучше всего дождаться момента, когда в golangci-lint появится новая версия.

Однако, если вы не используете golangci-lint или хотите попробовать уже сегодня, то можно скачать бинарник ruleguard со страницы релиза {linux/amd64, linux/arm64, darwin/amd64, windows/amd64}.

Вам также понадобится набор правил. Здесь есть как минимум два варианта: использовать минималистичный набор github.com/quasilyte/go-ruleguard/rules или более обширный github.com/dgryski/semgrep-go. Вы также можете импортировать оба этих бандла или не импортировать ничего и использовать лишь свои наработки.

Допустим, вы выбрали github.com/quasilyte/go-ruleguard/rules, тогда:

1. Скачиваем ruleguard для своей платформы (или собираем из исходников)
2. Выполняем go get -v github.com/quasilyte/go-ruleguard/dsl внутри модуля вашего проекта
3. Выполняем go get -v github.com/quasilyte/go-ruleguard/rules внутри модуля вашего проекта
4. Создаём свой файл правил rules.go, импортируем там установленный бандл
5. Запускаем ruleguard с параметром -rules rules.go на вашем проекте

$ ruleguard -rules rules.go ./...

Если у вас возникают проблемы с запуском или установкой ruleguard, сообщите об этом.

Создаём свой бандл

Есть только два требования:

1. Бандл должен быть отдельным Go модулем
2. Пакет должен определять экспортируемую переменную Bundle

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

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

package gorulesimport "github.com/quasilyte/go-ruleguard/dsl"// Bundle содержит метаданные о наборе правил.var Bundle = dsl.Bundle{}func boolComparison(m dsl.Matcher) {    m.Match(`$x == true`,        `$x != true`,        `$x == false`,        `$x != false`).        Report(`omit bool literal in expression`)}

В качестве примера, можно посмотреть на репозиторий ruleguard-rules-test.

Тестируем свой бандл

Тестирование основано на фреймворке go/analysis и вспомогательном пакете analysistest.

Рядом с модулем создаётся директория testdata, куда складываются Go файлы, на которых будут запускаться ваши диагностики.

Для запуска тестов нужно написать некоторый шаблонный код:

// file rules_test.gopackage gorules_testimport (    "testing"    "github.com/quasilyte/go-ruleguard/analyzer"    "golang.org/x/tools/go/analysis/analysistest")func TestRules(t *testing.T) {    // Если у вас несколько файлов с правилами, то вместо "rules.go"    // нужно указать имена всех файлов через запятую, например: "style.go,perf.go".    if err := analyzer.Analyzer.Flags.Set("rules", "rules.go"); err != nil {        t.Fatalf("set rules flag: %v", err)    }    analysistest.Run(t, analysistest.TestData(), analyzer.Analyzer, "./...")}

Структура бандла будет выглядеть примерно так:

mybundle/  go.mod        -- файл, создаваемый "go mod init"  rules.go      -- здесь ваши правила (можно назвать файл иначе)  rules_test.go -- запускатель тестов  testdata/     -- файлы, на которых будем запускать анализ    target1.go    target2.go    ...

Тестовые файлы будут содержать магические комментарии:

// file testdata/target1.gopackage testfunc f(cond bool) {    if cond == true { // want `omit bool literal in expression`    }}

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

Тест запускается обычным go test из директории бандла.

Ссылки и дополнительные материалы

• Список похожих проектов
• Сайт проекта ruleguard
• Телеграм чатик, где обсуждается go-critic и ruleguard
• Использование ruleguard из golangci-lint
• DSL мануал
• go-critic линтер, в который встроен ruleguard
• Введение в бандлы

Существует открытый проект COVID-19 CovidSim Model, написанный на языке C++. Существует статический анализатор кода PVS-Studio, который умеет хорошо находить ошибки. Однажды они встретились. Познайте хрупкость алгоритмов математического моделирования и почему нужно прикладывать максимум усилий к качеству программного кода.

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

Проект оказался совсем крошечным. В нём всего 13 000 строк кода, если не считать пустые строки и комментарии. И ошибок там тоже почти нет. Но одна ошибка настолько проста и красива, что я не могу пройти мимо!

void CalcLikelihood(int run, std::string const& DataFile,                    std::string const& OutFileBase){  ....  double m = Data[row][col]; // numerator  double N = Data[row][col + 1]; // denominator  double ModelValue;  // loop over all days of infection up to day of sample  for (int k = offset; k < day; k++)  {    // add P1 to P2 to prevent degeneracy    double prob_seroconvert = P.SeroConvMaxSens *      (1.0 - 0.5 * ((exp(-((double)(_I64(day) - k)) * P.SeroConvP1) + 1.0) *      exp(-((double)(_I64(day) - k)) * P.SeroConvP2)));    ModelValue += c * TimeSeries[k - offset].incI * prob_seroconvert;  }  ModelValue += c * TimeSeries[day - offset].S * (1.0 - P.SeroConvSpec);  ModelValue /= ((double)P.PopSize);  // subtract saturated likelihood  LL += m * log((ModelValue + 1e-20) / (m / N + 1e-20)) +        (N - m) * log((1.0 - ModelValue + 1e-20) / (1.0 - m / N + 1e-20));  ....}

Серьёзный научный код. Что-то считается. Формулы. Выглядит всё умно и обстоятельно.

Вот только все эти вычисления разбиваются о человеческую невнимательность. Хорошо, что на помощь может прийти анализатор кода PVS-Studio и указать на баг: V614 [CWE-457] Uninitialized variable 'ModelValue' used. CovidSim.cpp 5412

И действительно, посмотрим внимательнее на это:

double ModelValue;for (int k = offset; k < day; k++){  double prob_seroconvert = ....;  ModelValue += c * TimeSeries[k - offset].incI * prob_seroconvert;}

Перед нами простая и одновременно страшная ошибка: неинициализированная переменная. Этот алгоритм может насчитать всё что угодно.

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

Это уже не первая наша статья на эту тему:

• Анализ кода ROOT фреймворка для анализа данных научных исследований
• NCBI Genome Workbench: научные исследования под угрозой
• Большой Калькулятор выходит из-под контроля

Используйте статический анализатор кода PVS-Studio! Польза от своевременно найденных ошибок может быть колоссальной. Спасибо за внимание.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. COVID-19 Research and Uninitialized Variable.

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

Всё началось с проверки свежей версии библиотеки Qt 6. Про это была отдельная классическая статья, где я описал 77 найденных ошибок. Так получилось, что вначале я решил бегло полистать отчёт, ещё не пряча предупреждения, относящиеся к сторонним библиотекам. Другими словами, я не отключил в настройках предупреждения, относящиеся к \src\3rdparty. И так вышло, что я сразу наткнулся на интересный пример ошибки в библиотеки Open Asset Import Library, про которую я решил сделать эту отдельную маленькую заметку.

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

Теперь перейдём, собственно, к ошибке, обнаруженной в Open Asset Import Library (assimp). Файл: \src\3rdparty\assimp\src\code\FBX\FBXUtil.cpp.

std::string EncodeBase64(const char* data, size_t length){    // calculate extra bytes needed to get a multiple of 3    size_t extraBytes = 3 - length % 3;    // number of base64 bytes    size_t encodedBytes = 4 * (length + extraBytes) / 3;    std::string encoded_string(encodedBytes, '=');    // read blocks of 3 bytes    for (size_t ib3 = 0; ib3 < length / 3; ib3++)    {        const size_t iByte = ib3 * 3;        const size_t iEncodedByte = ib3 * 4;        const char* currData = &data[iByte];        EncodeByteBlock(currData, encoded_string, iEncodedByte);    }    // if size of data is not a multiple of 3,    // also encode the final bytes (and add zeros where needed)    if (extraBytes > 0)    {        char finalBytes[4] = { 0,0,0,0 };        memcpy(&finalBytes[0], &data[length - length % 3], length % 3);        const size_t iEncodedByte = encodedBytes - 4;        EncodeByteBlock(&finalBytes[0], encoded_string, iEncodedByte);        // add '=' at the end        for (size_t i = 0; i < 4 * extraBytes / 3; i++)            encoded_string[encodedBytes - i - 1] = '=';    }    return encoded_string;}

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

1. 31 февраля;
2. Использование машинного обучения в статическом анализе исходного кода программ;
3. Как внедрить статический анализатор кода в legacy проект и не демотивировать команду.

Ok, продолжим. Перед нами реализация алгоритма кодирования строки байт в кодировку Base64. Это стандарт кодирования двоичных данных при помощи только 64 символов. Алфавит кодирования содержит текстово-цифровые латинские символы A-Z, a-z и 0-9 (62 знака) и 2 дополнительных символа, зависящих от системы реализации. Каждые 3 исходных байта кодируются 4 символами.

Если осталось закодировать только один или два байта, то в результате получаются только первые два или три символа строки, а выходная строка дополняется двумя или одним символами "=". Это предотвращает добавление дополнительных битов к восстановленным данным. Вот этот момент как раз реализован в рассматриваемой функции неправильно.

Если вы нашли ошибку, вы молодец. Если нет, то это тоже нормально. Нужно вникать в код, чтобы заметить, что что-то идёт не так. Анализатор про это "что-то не то" сообщает предупреждением: V547 [CWE-571] Expression 'extraBytes > 0' is always true. FBXUtil.cpp 224

Чтобы понять причину беспокойства анализатора, давайте посмотрим, как инициализируется переменная extraBytes:

// calculate extra bytes needed to get a multiple of 3size_t extraBytes = 3 - length % 3;

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

size_t extraBytes = length % 3;

Тогда, если обрабатывается, например, 5 байт, то получаем 5 % 3 = 2, и нужно дополнительно обработать 2 байта. Если на вход поступило 6 байт, то ничего отдельно обрабатывать не нужно, так как 6 % 3 = 0.

Но программист перемудрил и написал бессмысленный код:

size_t extraBytes = 3 - length % 3;

И как раз при анализе этого кода анализатору и понадобился механизм анализа потока данных. Какое бы значение не находилось в переменной length, после деления по модулю будет получено значение в диапазоне [0..2]. Анализатор PVS-Studio умеет работать с диапазонами, точными значениями и множествами. Т. е. речь идёт про Value Range Analysis. В данном случае будет использован именно диапазон значений.

Продолжим вычисления:

size_t extraBytes = 3 - [0..2];

Получается, что переменная extraBytes никогда не будет равна нулю. Анализатор вычислит следующий возможный диапазон её значений: [1..3].

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

if (extraBytes > 0)

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

Конечно, некорректность работы функции не ограничивается тем, что будет выполнять фрагмент кода, который выполняться не должен. Там вообще всё идёт вкривь и вкось. Предположим, что требуется закодировать 6 символов. В этом случае выходная строка должна содержать 8 символов. Давайте быстренько прикинем, как поведёт себя рассмотренная функция.

// calculate extra bytes needed to get a multiple of 3size_t extraBytes = 3 - length % 3; // 3-6%3 = 3// number of base64 bytessize_t encodedBytes = 4 * (length + extraBytes) / 3; // 4*(6+3)/3 = 12std::string encoded_string(encodedBytes, '=');

Уже получилось, что выходная строка будет содержать 12 символов, а не 8. Дальше тоже всё будет работать неправильно даже нет смысла вдаваться в подробности.

Вот так легко и просто статический анализ нашёл ошибку в коде. А представьте, ведь кто-то будет мучиться и отлаживаться, чтобы понять, почему неправильно происходит кодирование символов в кодировку Base64. Здесь мы, кстати, подходим к вопросу качества используемых сторонних библиотек, который я рассматривал в публикации "Почему важно проводить статический анализ открытых библиотек, которые вы добавляете в свой проект".

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Why PVS-Studio Uses Data Flow Analysis: Based on Gripping Error in Open Asset Import Library.

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

Код, который мы рассмотрим далее, был выписан в процессе работы над статьёй "Обработка дат притягивает ошибки или 77 дефектов в Qt 6".

Анализатор PVS-Studio обратил внимание на этот фрагмент кода, выдав предупреждение: V575 [CWE-628] The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. qplaintestlogger.cpp 253. Собственно, вот он:

const char *msgFiller = msg[0] ? " " : "";QTestCharBuffer testIdentifier;QTestPrivate::generateTestIdentifier(&testIdentifier);QTest::qt_asprintf(&messagePrefix, "%s: %s%s%s%s\n",                   type, testIdentifier.data(), msgFiller, msg,                   failureLocation.data());// In colored mode, printf above stripped our nonprintable control characters.// Put them back.memcpy(messagePrefix.data(), type, strlen(type));outputMessage(messagePrefix.data());

Обратите внимание на вызов функции memcpy. Сам по себе этот код вызывает сразу два вопроса:

1. Зачем что-то записывается в буфер, содержимое которого было только что сформировано с помощью printf-подобной функции?
2. Точно не ошибка, что не копируется терминальный ноль? Это как раз и не нравится анализатору.

К счастью, комментарий сразу всё проясняет. Нужно восстановить некие непечатаемые символы.

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

Для сравнения рассмотрим другой фрагмент кода из этого же файла:

char buf[1024];if (result.setByMacro) {  qsnprintf(buf, sizeof(buf), "%s%s%s%s%s%s\n", buf1, bufTag, fill,            buf2, buf2_, buf3);} else {  qsnprintf(buf, sizeof(buf), "%s%s%s%s\n", buf1, bufTag, fill, buf2);}memcpy(buf, bmtag, strlen(bmtag));outputMessage(buf);

Здесь забыли сделать аналогичный комментарий. И картина радикально меняется. Этот код способен ввести в замешательство нового члена команды, который будет его сопровождать или модифицировать. Совершенно не понятно, зачем нужен этот memcpy. Более того, непонятно, почему в начало строки печаталось содержимое некоего буфера buf1, а затем в начало строки помещается содержимое буфера bmtag. Как много вопросов, как мало ответов. Не стоит писать такой код.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. One Useful Comment.

Десятое место: Деление по модулю на единицу

void Act() override {  ....  // If the value type is a vector, and we allow vector select,  // then in 50% of the cases generate a vector select.  if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 1)) {    unsigned NumElem =        cast<FixedVectorType>(Val0->getType())->getNumElements();    CondTy = FixedVectorType::get(CondTy, NumElem);  }  ....}

if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 2))

Девятое место: Четыре проверки

• V560 A part of conditional expression is always true: x >= 0. editor.cpp 1137
• V560 A part of conditional expression is always true: y >= 0. editor.cpp 1137
• V560 A part of conditional expression is always true: x < 40. editor.cpp 1137
• V560 A part of conditional expression is always true: y < 30. editor.cpp 1137

int editorclass::at( int x, int y ){  if(x<0) return at(0,y);  if(y<0) return at(x,0);  if(x>=40) return at(39,y);  if(y>=30) return at(x,29);  if(x>=0 && y>=0 && x<40 && y<30)  {      return contents[x+(levx*40)+vmult[y+(levy*30)]];  }  return 0;}

Восьмое место: delete вместо delete[]

BOOL Send_Data_To_DDE_Server (char *data, int length, int packet_type){  ....  char *poke_data = new char [length + 2*sizeof(int)]; // <=  ....  if(DDE_Class->Poke_Server( .... ) == FALSE) {    CCDebugString("C&C95 - POKE failed!\n");    DDE_Class->Close_Poke_Connection();    delete poke_data;                                  // <=    return (FALSE);  }  DDE_Class->Close_Poke_Connection();  delete poke_data;                                    // <=  return (TRUE);}

Седьмое место: Выход за границу буфера

#if defined(CONFIG_NET_HOSTNAME_ENABLE)const char *net_hostname_get(void);#elsestatic inline const char *net_hostname_get(void){  return "zephyr";}#endif

static inline const char *net_hostname_get(void){  return "zephyr";}

static int do_net_init(void){  ....  (void)memcpy(hostname, net_hostname_get(), MAX_HOSTNAME_LEN);  ....}

(void)memcpy(hostname, net_hostname_get(),    sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx"));

Шестое место: Что-то очень странное

static char *mntpt_prepare(char *mntpt){  char *cpy_mntpt;  cpy_mntpt = k_malloc(strlen(mntpt) + 1);  if (cpy_mntpt) {    ((u8_t *)mntpt)[strlen(mntpt)] = '\0';    memcpy(cpy_mntpt, mntpt, strlen(mntpt));  }  return cpy_mntpt;}

((u8_t *)mntpt)[strlen(mntpt)] = '\0';

((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\0';

static char *mntpt_prepare(char *mntpt){  char *cpy_mntpt;  cpy_mntpt = k_malloc(strlen(mntpt) + 1);  if (cpy_mntpt) {    strcpy(cpy_mntpt, mntpt);  }  return cpy_mntpt;}

Пятое место: Неправильная защита от переполнения

static DWORDget_rel_wait(const struct timespec *abstime){  struct __timeb64 t;  _ftime64_s(&t);  time_t now_ms = t.time * 1000 + t.millitm;  time_t ms = (time_t)(abstime->tv_sec * 1000 +    abstime->tv_nsec / 1000000);  DWORD rel_wait = (DWORD)(ms - now_ms);  return rel_wait < 0 ? 0 : rel_wait;}

Четвёртое место: Не пиши в std, брат

// Dirty hack because g++ 4.6 at least wants// to do a bunch of copy operations.namespace std {inline void iter_swap(util::SizedIterator first,                      util::SizedIterator second){  util::swap(*first, *second);}} // namespace std

Третье место: Скроллбар, который не смог

bool TermControl::_InitializeTerminal(){  ....  auto bottom = _terminal->GetViewport().BottomExclusive();  auto bufferHeight = bottom;  ScrollBar().Maximum(bufferHeight - bufferHeight);  ScrollBar().Minimum(0);  ScrollBar().Value(0);  ScrollBar().ViewportSize(bufferHeight);  ....}

Второе место: перепутали радиус и высоту

• V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 791
• V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 833
• V764 Possible incorrect order of arguments passed to 'CreateWheel' function: 'height' and 'radius'. StandardJoints.cpp 884

NewtonBody* const wheel = CreateWheel (scene, origin, height, radius);

static NewtonBody* CreateWheel (DemoEntityManager* const scene,  const dVector& location, dFloat radius, dFloat height)

Первое место: Затирание результата

static bool parseNamedColorString(const std::string &value,                                  video::SColor &color){  std::string color_name;  std::string alpha_string;  size_t alpha_pos = value.find('#');  if (alpha_pos != std::string::npos) {    color_name = value.substr(0, alpha_pos);    alpha_string = value.substr(alpha_pos + 1);  } else {    color_name = value;  }  color_name = lowercase(value); // <=  std::map<const std::string, unsigned>::const_iterator it;  it = named_colors.colors.find(color_name);  if (it == named_colors.colors.end())    return false;  ....}

color_name = lowercase(color_name);

Заключение

Опасная трясина MISRA и подобных

Статический анализ не панацея

Формальная верификация кода

#include "pi_control.h"/* Global variable definitions */float inp_volt[2];float integral_state;float duty_cycle;float direction;float normalized_error;/* Static functions */static void pi_alg(float Kp, float Ki);static void process_inputs(void);/* control_task implements a PI controller algorithm that ../  *  * - reads inputs from hardware on actual and desired position  * - determines error between actual and desired position  * - obtains controller gains  * - calculates direction and duty cycle of PWM output using PI control algorithm  * - sets PWM output to hardware  *  */void control_task(void){  float Ki;  float Kp;  /* Read inputs from hardware */  read_inputs();  /* Convert ADC values to their respective voltages provided read failure did not occur, otherwise do not update input values */  if (!read_failure)  {    inp_volt[0] = 0.0048828125F * (float) inp_val[0];    inp_volt[1] = 0.0048828125F * (float) inp_val[1];  }    /* Determine error */  process_inputs();    /* Determine integral and proprortional controller gains */  get_control_gains(&Kp,&Ki);    /* PI control algorithm */  pi_alg(Kp, Ki);  /* Set output pins on hardware */  set_outputs();}/* process_inputs  computes the error between the actual and desired position by  * normalizing the input values using lookup tables and then taking the difference */static void process_inputs(void){  /* local variables */  float rtb_AngleNormalization;  float rtb_PositionNormalization;  /* Normalize voltage values */  look_up_even( &(rtb_AngleNormalization), inp_volt[1], angle_norm_map, angle_norm_vals);   look_up_even( &(rtb_PositionNormalization), inp_volt[0], pos_norm_map, pos_norm_vals);   /* Compute error */  normalized_error = rtb_PositionNormalization - rtb_AngleNormalization;}/* look_up_even provides a lookup table algorithm that works for evenly spaced values.  *   * Inputs to the function are...  *     pY - pointer to the output value  *     u - input value  *     map - structure containing the static lookup table data...  *         valueLo - minimum independent axis value  *         uSpacing - increment size of evenly spaced independent axis  *         iHi - number of increments available in pYData  *         pYData - pointer to array of values that make up dependent axis of lookup table   *   */void look_up_even( float *pY, float u, map_data map, float *pYData){  /* If input is below range of lookup table, output is minimum value of lookup table (pYData) */  if (u <= map.valueLo )   {    pY[1] = pYData[1];  }   else   {    /* Determine index of output into pYData based on input and uSpacing */    float uAdjusted = u - map.valueLo;    unsigned int iLeft = uAdjusted / map.uSpacing;/* If input is above range of lookup table, output is maximum value of lookup table (pYData) */    if (iLeft >= map.iHi ) {      (*pY) = pYData[map.iHi];    } /* If input is in range of lookup table, output will interpolate between lookup values */else {      {        float lambda;  // fractional part of difference between input and nearest lower table value        {          float num = uAdjusted - ( iLeft * map.uSpacing );          lambda = num / map.uSpacing;        }        {          float yLeftCast;  // table value that is just lower than input          float yRghtCast;  // table value that is just higher than input          yLeftCast = pYData[iLeft];          yRghtCast = pYData[((iLeft)+1)];          if (lambda != 0) {            yLeftCast += lambda * ( yRghtCast - yLeftCast );          }          (*pY) = yLeftCast;        }      }    }  }}static void pi_alg(float Kp, float Ki){  {    float control_output;float abs_control_output;    /*  y = integral_state + Kp*error   */    control_output = Kp * normalized_error + integral_state;/* Determine direction of torque based on sign of control_output */    if (control_output >= 0.0F) {      direction = TRUE;    } else {      direction = FALSE;    }/* Absolute value of control_output */    if (control_output < 0.0F) {      abs_control_output = -control_output;    } else if (control_output > 0.0F) {  abs_control_output = control_output;}    /* Saturate duty cycle to be less than 1 */    if (abs_control_output > 1.0F) {  duty_cycle = 1.0F;} else {  duty_cycle = abs_control_output;}    /* integral_state = integral_state + Ki*Ts*error */    integral_state = Ki * normalized_error * 1.0e-002F + integral_state;    }}

/* Lookup table structure */typedef struct {  float valueLo;  unsigned int iHi;  float uSpacing;} map_data;/* Macro definitions */#define TRUE 1#define FALSE 0/* Global variable declarations */extern unsigned short inp_val[];extern map_data angle_norm_map;extern float angle_norm_vals[11];extern map_data pos_norm_map;extern float pos_norm_vals[11];extern float inp_volt[2];extern float integral_state;extern float duty_cycle;extern float direction;extern float normalized_error;extern unsigned char read_failure;/* Function declarations */void control_task(void);void look_up_even( float *pY, float u, map_data map, float *pYData);extern void read_inputs(void);extern void set_outputs(void);extern void get_control_gains(float* c_prop, float* c_int);

Доверие к инструментам

Итоги

• АНБ, Ghidra и единороги
• Под капотом PVS-Studio для Java: разработка диагностик
• Проверка кода XMage и почему недоступны специальные редкие карточки для коллекции Dragon's Maze
• Проверка WildFly сервера JavaEE приложений
• Единороги на страже вашей безопасности: исследуем код Bouncy Castle
• Big/Bug Data: анализируем исходный код Apache Flink

Десятое место: "Обманчивое равенство"

@Overridepublic boolean equals(Object o) {  ....  CheckpointStatistics that = (CheckpointStatistics) o;  return id == that.id &&    savepoint == that.savepoint &&    triggerTimestamp == that.triggerTimestamp &&    latestAckTimestamp == that.latestAckTimestamp &&    stateSize == that.stateSize &&    duration == that.duration &&    alignmentBuffered == that.alignmentBuffered &&    processedData == processedData &&                // <=    persistedData == that.persistedData &&    numSubtasks == that.numSubtasks &&    numAckSubtasks == that.numAckSubtasks &&    status == that.status &&    Objects.equals(checkpointType, that.checkpointType) &&    Objects.equals(      checkpointStatisticsPerTask,       that.checkpointStatisticsPerTask);}

Девятое место: "Недостижимый код"

public void testSignSHA256CompleteEvenHeight2() {    ....    int height = 10;    ....    for (int i = 0; i < (1 << height); i++) {        byte[] signature = xmss.sign(new byte[1024]);        switch (i) {            case 0x005b:                assertEquals(signatures[0], Hex.toHexString(signature));                break;            case 0x0822:                assertEquals(signatures[1], Hex.toHexString(signature));                break;            ....        }    }}

Восьмое место: "Проаннотированный метод"

protected void after(){  this.metricsById.clear();  this.metricsById.clear();}

• Чистый ли это метод,
• Какие накладываются ограничения на аргументы,
• Возвращаемый результат,
• и всякое прочее.

private final Map<String, Metric> metricsByKey = new HashMap<>();private final Map<Long, Metric> metricsById = new HashMap<>();

Седьмое место: "Ожидание / реальность"

// Send value to RESTEasy only if it's not null, empty string, or the // default value.private boolean isTransmittable(AttributeDefinition attribute,                                ModelNode modelNode) {  if (modelNode == null || ModelType      .UNDEFINED.equals(modelNode.getType())) {    return false;  }  String value = modelNode.asString();  if ("".equals(value.trim())) {    return false;  }  return !value.equals(attribute.getDefaultValue());        // <=}

• modelNode не null,
• строковое представление modelNode не пустое,
• modelNode не значение по умолчанию.

Шестое место: "Копипаст-ориентированное программирование"

@Testpublic void testArcaneAdaptationGiveType() {    addCard(Zone.HAND, playerA, "Arcane Adaptation", 1);    addCard(Zone.BATTLEFIELD, playerA, "Island", 3);    addCard(Zone.HAND, playerA, "Silvercoat Lion");    addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=    addCard(Zone.HAND, playerB, "Silvercoat Lion");    addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=    ....    for (Card card : playerB.getGraveyard().getCards(currentGame)) {        if (card.isCreature()) {            Assert.assertEquals(card.getName() + " should not have ORC type",                    false, card.getSubtype(currentGame).contains(SubType.ORC));            Assert.assertEquals(card.getName() + " should have CAT type",                    true, card.getSubtype(currentGame).contains(SubType.CAT));        }    }}

Пятое место: "Ненормальное распределение"

public static <T> Collection<List<T>> partition(Collection<T> elements, int numBuckets) {  Map<Integer, List<T>> buckets = new HashMap<>(numBuckets);    int initialCapacity = elements.size() / numBuckets;  int index = 0;  for (T element : elements)   {    int bucket = index % numBuckets;                                 // <=    buckets.computeIfAbsent(bucket,                             key -> new ArrayList<>(initialCapacity))           .add(element);   }  return buckets.values();}

Четвертое место: "Бомба замедленного действия"

private void processSelection(OptionsTreeNode selectedNode) {  if (selectedNode == null) {    setViewPanel(defaultPanel, selectedNode); // <=    return;  }  ....}private void setViewPanel(JComponent component, OptionsTreeNode selectedNode) {  ....  setHelpLocation(component, selectedNode);  ....}private void setHelpLocation(JComponent component, OptionsTreeNode node) {  Options options = node.getOptions();  ....}

Третье место: "Всегда false"

@Overridepublic String getText(Mode mode) {  StringBuilder sb = new StringBuilder();  ....  if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) {    sb.append(" has base power and toughness ");  } else {    sb.append(" have base power and toughness ");  }  ....  return sb.toString();}

Второе место: "2-в-1"

public void setValueAt(Object aValue, int row, int column) {  ....  int index = indexOf(newName);  if (index >= 0) {                  // <=    Window window = tool.getActiveWindow();    Msg.showInfo(getClass(), window, "Duplicate Name",                 "Name already exists: " + newName);    return;  }  ExternalPath path = paths.get(row); // <=  ....}private int indexOf(String name) {  for (int i = 0; i < paths.size(); i++) {    ExternalPath path = paths.get(i);    if (path.getName().equals(name)) {      return i;    }  }  return 0;}

Первое место: "А то ли мы проверили?"

public class Menu{  private Map<String, List<String>> menus = new HashMap<String, List<String>>();  public void putMenuItem(String menu, String item)  {    List<String> items = menus.get(menu);    if (item == null)                      // <=    {      items = new ArrayList<String>();      menus.put(menu, items);    }    items.add(item);  }  ....}

Заключение