Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
GBP
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

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

Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Идем по грибы после Cppcheck

Идем по грибы после Cppcheck

09 Сен 2013

После горячих обсуждений про "Большой Калькулятор", мне захотелось проверить ещё что-то из проектов, связанных с проведением исследований. Первое что нашлось, оказался открытый проект OpenMS, связанный с protein mass spectrometry. Этот проект оказалось написан с серьёзным подходом. При разработке используется как минимум Cppcheck. Так что ничего сенсационного ждать не приходилось. Однако был интерес, какие ошибки сможет найти PVS-Studio после Cppcheck. Заинтересовавшихся приглашаю продолжить чтение статьи.

Поддержка OpenMP была прекращена в PVS-Studio после версии 5.20. По всем возникшим вопросам вы можете обратиться в нашу поддержку.

0213_Picking_Mushrooms_after_Cppcheck_ru/image1.png

Существует проект OpenMS. Для чего он нужен, я не берусь пересказать своими словами. Ещё ляпну глупость. Просто процитирую фрагмент описания с сайта Wikipedia:

OpenMS is an open-source project for data analysis and processing in protein mass spectrometry and is released under the 2-clause BSD licence. OpenMS has tools for many common data analysis pipelines used in proteomics, providing algorithms for signal processing, feature finding (including de-isotoping), visualization in 1D (spectra or chromatogram level), 2D and 3D, map mapping and peptide identification. It supports label-free and isotopic-label based quantification (such as iTRAQ and TMT and SILAC). Furthermore, it also supports metabolomics workflows and DIA/SWATH targeted analysis.

Первоисточник: Wikipedia. OpenMS.

Проект среднего размера, но весьма сложный. Размер исходного кода составляет более 20 мегабайт. Плюс большое количество сторонних библиотек (Boost, Qt, Zlib и так далее). В проекте очень активно используются шаблоны. Исходный код проекта можно скачать с сайта SourceForge.

При разработке проекта OpenMS применяется статический анализ кода. Наличие "cppcheck.cmake" и комментариев в духе:

if (i != peptide.size()) // added for cppcheck

свидетельствует, что используются как минимум Cppcheck. Также встречается упоминание Cpplint и есть файл "cpplint.py". Профессиональный подход. Молодцы.

Давайте теперь посмотрим, что удастся найти с помощью анализатора PVS-Studio.

Примечание. Отчего-то Си++ файлы носят расширение '*.C'. Так что пусть вас не смущает, когда вы увидите пример кода на Си++, находящийся в '*.C' файле.

1. Недочёты с OpenMP

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

Среди выданных предупреждений есть ложные срабатывания, но нашлись предупреждения и по делу.

DoubleReal ILPDCWrapper::compute(....) const
{
  ....
  DoubleReal score = 0;
  ....
  #pragma omp parallel for schedule(dynamic, 1)
  for (SignedSize i = 0; i < (SignedSize)bins.size(); ++i)
  {
    score += computeSlice_(fm, pairs, bins[i].first,
                           bins[i].second, verbose_level);
  }
  return score;
}

Предупреждение PVS-Studio: V1205 Data race risk. Unprotected concurrent operation with the 'score' variable. ilpdcwrapper.c 213

Неправильно считается сумма. Переменная 'score' не защищена от одновременного использования в разных потоках.

Другие замечания не столько критичны, но всё же на них стоит обратить внимание. Внутри параллельных секций все исключения должны быть перехвачены. Выход исключения за пределы параллельной секции, скорее всего, приведет к аварийному завершению программы. Подробнее про эту тему можно прочитать в заметках: "OpenMP и исключения (exceptions)", "Обработка исключений внутри параллельных секций".

Исключение можно сгенерировать явно, используя оператор throw. Также оно может возникнуть при вызове оператора new (std::bad_alloc).

Первый вариант. Функция getTheoreticalmaxPosition() может сгенерировать исключение.

Size getTheoreticalmaxPosition() const
{
  if (!this->size())
  {
    throw Exception::Precondition(__FILE__, __LINE__,
      __PRETTY_FUNCTION__,
      "There must be at least one trace to ......");
  }
  ....
}

virtual void run()
{
  ....
  #pragma omp parallel for
  for (SignedSize i = 0; i < (SignedSize)seeds.size(); ++i)
  {
    ....
    f.setMZ(
      traces[traces.getTheoreticalmaxPosition()].getAvgMZ());
    ....
  }
  ....
}

Предупреждение PVS-Studio: V1301 The 'throw' keyword cannot be used outside of a try..catch block in a parallel section. featurefinderalgorithmpickedhelperstructs.h 199

Второй вариант. Вызов оператор 'new' может привести к возникновению исключения.

TraceFitter<PeakType>* chooseTraceFitter_(double& tau)
{
  // choose fitter
  if (param_.getValue("feature:rt_shape") == "asymmetric")
  {
    LOG_DEBUG << "use asymmetric rt peak shape" << std::endl;
    tau = -1.0;
    return new EGHTraceFitter<PeakType>();
  }
  ....
}

virtual void run()
{
  ....
  #pragma omp parallel for
  for (SignedSize i = 0; i < (SignedSize)seeds.size(); ++i)
  {
    ....
    TraceFitter<PeakType>* fitter = chooseTraceFitter_(egh_tau);
    ....
  }
  ....
}

Предупреждение PVS-Studio: V1302 The 'new' operator cannot be used outside of a try..catch block in a parallel section. featurefinderalgorithmpicked.h 1926

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

  • V1301 featurefinderalgorithmpicked.h 1261
  • V1301 mzmlfile.h 114
  • V1301 rawmssignalsimulation.c 598
  • V1301 rawmssignalsimulation.c 1152
  • V1301 chromatogramextractor.h 103
  • V1301 chromatogramextractor.h 118
  • V1302 featurefinderalgorithmpicked.h 1931
  • V1302 rawmssignalsimulation.c 592
  • V1302 rawmssignalsimulation.c 601
  • V1302 openswathanalyzer.c 246

2. Опечатки

std::vector< std::pair<std::string, long> > spectra_offsets;
std::vector< std::pair<std::string, long> > chromatograms_offsets;

template <typename MapType>
void MzMLHandler<MapType>::writeFooter_(std::ostream& os)
{
  ....
  int indexlists;
  if (spectra_offsets.empty() && spectra_offsets.empty() )
  {
    indexlists = 0;
  }
  else if (!spectra_offsets.empty() && !spectra_offsets.empty() )
  {
    indexlists = 2;
  }
  else
  {
    indexlists = 1;
  }
  ....
}

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

V501 There are identical sub-expressions 'spectra_offsets.empty()' to the left and to the right of the '&&' operator. mzmlhandler.h 5288

V501 There are identical sub-expressions '!spectra_offsets.empty()' to the left and to the right of the '&&' operator. mzmlhandler.h 5292

Очень странные проверки. Два раза проверяется контейнер 'spectra_offsets'. Скорее всего, это опечатка и следует проверять два разных контейнера: spectra_offsets и chromatograms_offsets.

template <typename MapType>
void MzMLHandler<MapType>::characters(
  const XMLCh* const chars, const XMLSize_t)
{
  ....
  if (optionalAttributeAsString_(data_processing_ref,
                                 attributes,
                                 s_data_processing_ref))
  {
    data_.back().meta.setDataProcessing(
                        processing_[data_processing_ref]);
  }
  else
  {
    data_.back().meta.setDataProcessing(
                        processing_[data_processing_ref]);
  }
  ....
}

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

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

  • processing_[data_processing_ref]
  • processing_[default_processing_]

Много опечаток оказалось связано с генерацией исключений. Ошибки банальны. Забыто ключевое слово 'throw'. Просто создается временный объект и сразу уничтожается. Пример такой ошибки:

inline UInt asUInt_(const String & in)
{
  UInt res = 0;
  try
  {
    Int tmp = in.toInt();
    if (tmp < 0)
    {
      Exception::ConversionError(
        __FILE__, __LINE__, __PRETTY_FUNCTION__, "");
    }
    res = UInt(tmp);
  }
  catch (Exception::ConversionError)
  {
    error(LOAD, 
          String("UInt conversion error of \"") + in + "\"");
  }
  return res;
}

Предупреждение PVS-Studio: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw ConversionError(FOO); xmlhandler.h 247

Идентичные опечатки здесь:

  • inclusionexclusionlist.c 281
  • inclusionexclusionlist.c 285
  • precursorionselectionpreprocessing.c 257
  • modificationsdb.c 419
  • modificationsdb.c 442
  • svmtheoreticalspectrumgeneratorset.c 103
  • logconfighandler.c 285
  • logconfighandler.c 315
  • suffixarraytrypticcompressed.c 488
  • tooldescription.c 147
  • tofcalibration.c 147

И последняя замеченная мной опечатка:

inline typename Value<Pipe>::Type const & operator*() {
  tmp.i1 = *in.in1;
  tmp.i2 = *in.in2;
  tmp.i3 = *in.in2;
  return tmp;
}

Предупреждение PVS-Studio: V525 The code containing the collection of similar blocks. Check items 'in1', 'in2', 'in2' in lines 112, 113, 114. pipe_joiner.h 112

Должно быть написано:

tmp.i1 = *in.in1;
tmp.i2 = *in.in2;
tmp.i3 = *in.in3;

3. Странное условие

CompressedInputSource::CompressedInputSource(
  const String & file_path, const char * header,
  MemoryManager * const manager) 
  : xercesc::InputSource(manager)
{
  if (sizeof(header) / sizeof(char) > 1)
  {
    head_[0] = header[0];
    head_[1] = header[1];
  }
  else
  {
    head_[0] = '\0';
    head_[1] = '\0';
  }
  ....
}

Предупреждение PVS-Studio: V514 Dividing sizeof a pointer 'sizeof (header)' by another value. There is a probability of logical error presence. compressedinputsource.c 52

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

Аналогичная странная проверка имеется здесь: compressedinputsource.c 104

4. Возвращение ссылки на локальный объект

template <typename TStringSet, typename TSpec>
inline Iter<TStringSet, ConcatVirtual<TSpec> > const &
operator++(Iter<TStringSet, ConcatVirtual<TSpec> > & me, int)
{
    Iter<TStringSet, ConcatVirtual<TSpec> > before = me;
    goNext(me);
    return before;
}

Предупреждение PVS-Studio: V558 Function returns the reference to temporary local object: before. iter_concat_virtual.h 277

Функция возвращает ссылку на временную переменную 'before'. При выходе из функции, эта переменная будет уничтожена. Использование ссылки на разрушенный объект приведёт к непредсказуемым последствиям.

Мне кажется, надо было написать так:

template <typename TStringSet, typename TSpec>
inline Iter<TStringSet, ConcatVirtual<TSpec> > const
operator++(Iter<TStringSet, ConcatVirtual<TSpec> > & me, int)
{ ... }

Аналогичная беда имеется у оператора '--': iter_concat_virtual.h 310

5. Неаккуратные вычисления

typedef size_t Size;
typedef double DoubleReal;
void updateMeanEstimate(const DoubleReal & x_t,
  DoubleReal & mean_t, Size t)
{
  DoubleReal tmp(mean_t);
  tmp = mean_t + (1 / (t + 1)) * (x_t - mean_t);
  mean_t = tmp;
}

Предупреждение PVS-Studio: V636 The '1 / (t + 1)' expression was implicitly casted from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. masstracedetection.c 129

Выражение "(1 / (t + 1))" всегда равно нулю или единице. Это связано с тем, что это выражение целочисленное. Возможно, задумывалось получить совсем иной результат иное. Я не знаю логику работы программы, но мне кажется, имелось в виду следующее:

tmp = mean_t + (1.0 / (t + 1)) * (x_t - mean_t);

Ещё мне не понравилось, что вместо константы M_PI используются явные значения, причем не очень точные. Это конечно не ошибка, но не очень хорошо. Пример:

bool PosteriorErrorProbabilityModel::fit(
  std::vector<double> & search_engine_scores)
{
  ....
  incorrectly_assigned_fit_param_.A =
    1 / sqrt(2 * 3.14159 *
             pow(incorrectly_assigned_fit_param_.sigma, 2));
  ....
}

Предупреждение PVS-Studio: V624 The constant 3.14159 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. posteriorerrorprobabilitymodel.c 92

Аналогично:

  • posteriorerrorprobabilitymodel.c 101
  • posteriorerrorprobabilitymodel.c 110
  • posteriorerrorprobabilitymodel.c 155
  • posteriorerrorprobabilitymodel.c 162

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

static const Int CHANNELS_FOURPLEX[4][1];
static const Int CHANNELS_EIGHTPLEX[8][1];
ExitCodes main_(int, const char **)
{
  ....
  if (itraq_type == ItraqQuantifier::FOURPLEX)
  {
    for (Size i = 0; i < 4; ++i)
    {
      std::vector<std::pair<String, DoubleReal> > one_label;
      one_label.push_back(std::make_pair<String, DoubleReal>(
        String("Channel ") +
          String(ItraqConstants::CHANNELS_FOURPLEX[i][0]),
        DoubleReal(ItraqConstants::CHANNELS_FOURPLEX[i][0])));
      labels.push_back(one_label);
    }
  }
  else //ItraqQuantifier::EIGHTPLEX
  {
    for (Size i = 0; i < 8; ++i)
    {
      std::vector<std::pair<String, DoubleReal> > one_label;
      one_label.push_back(std::make_pair<String, DoubleReal>(
        String("Channel ") +
          String(ItraqConstants::CHANNELS_FOURPLEX[i][0]),
        DoubleReal(ItraqConstants::CHANNELS_FOURPLEX[i][0])));
      labels.push_back(one_label);
    }
  }
  ....
}

Предупреждение PVS-Studio: V557 Array overrun is possible. The value of 'i' index could reach 7. itraqanalyzer.c 232

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

Здесь, в ветке 'else' надо было работать с массивов 'CHANNELS_EIGHTPLEX'. Об этом свидетельствует комментарий:

else //ItraqQuantifier::EIGHTPLEX

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

Аналогичная ошибка здесь (ещё один Copy-Paste): tmtanalyzer.c 225

Рассмотрим ещё один пример.

DoubleReal masse_[255]; ///< mass table

EdwardsLippertIterator::EdwardsLippertIterator(const
 EdwardsLippertIterator & source) :
  PepIterator(source),
  f_file_(source.f_file_),
  actual_pep_(source.actual_pep_),
  spec_(source.spec_),
  tol_(source.tol_),
  is_at_end_(source.is_at_end_),
  f_iterator_(source.f_iterator_),
  f_entry_(source.f_entry_),
  b_(source.b_),
  e_(source.e_),
  m_(source.m_),
  massMax_(source.massMax_)
{
  for (Size i = 0; i < 256; i++)
  {
    masse_[i] = source.masse_[i];
  }
}

Предупреждение PVS-Studio: V557 Array overrun is possible. The value of 'i' index could reach 255. edwardslippertiterator.c 134

В конструкторе копирования неправильно работают с массивом masse_. Он состоит из 255 элементов. А копируется 256 элементов.

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

for (Size i = 0; i < 255; i++)
{
  masse_[i] = source.masse_[i];
}

А ещё лучше вообще не использовать магические константы.

7. Устаревший вызов оператора 'new'

svm_problem * LibSVMEncoder::encodeLibSVMProblem(....)
{
  ....
  node_vectors = new svm_node *[problem->l];
  if (node_vectors == NULL)
  {
    delete[] problem->y;
    delete problem;
    return NULL;
  }
  ....
}

Предупреждение PVS-Studio: V668 There is no sense in testing the 'node_vectors' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. libsvmencoder.c 177

Проверка "if (node_vectors == NULL)" не имеет смысла. Если не удастся выделить память, возникнет исключение. В результате программа поведет себя не так, как планировал программист. Например, произойдет утечка памяти.

Аналогичные устаревшие провреки:

  • file_page.h 728
  • libsvmencoder.c 160

Заключение

Я думаю, будет полезно использовать не только Cppcheck, Cpplint, но PVS-Studio. Особенно, если делать это регулярно. Приглашаю разработчиков проекта написать нам на support@viva64.com. На некоторое время мы выделим ключ, для более подробной проверки OpenMS.

Популярные статьи по теме
Бесплатный PVS-Studio для тех, кто развивает открытые проекты

Дата: 22 Дек 2018

Автор: Андрей Карпов

В канун празднования нового 2019 года команда PVS-Studio решила сделать приятный подарок всем контрибьюторам open-source проектов, хостящихся на GitHub, GitLab или Bitbucket. Им предоставляется возмо…
Главный вопрос программирования, рефакторинга и всего такого

Дата: 14 Апр 2016

Автор: Андрей Карпов

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

Дата: 21 Ноя 2018

Автор: Андрей Карпов

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

Дата: 17 Янв 2019

Автор: Андрей Карпов

В седьмой версии статического анализатора PVS-Studio мы добавили поддержку языка Java. Пришло время немного рассказать, как мы начинали делать поддержку языка Java, что у нас получилось и какие дальн…
Эффект последней строки

Дата: 31 Май 2014

Автор: Андрей Карпов

Я изучил множество ошибок, возникающих в результате копирования кода. И утверждаю, что чаще всего ошибки допускают в последнем фрагменте однотипного кода. Ранее я не встречал в книгах описания этого …
Статический анализ как часть процесса разработки Unreal Engine

Дата: 27 Июн 2017

Автор: Андрей Карпов

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

Дата: 20 Мар 2017

Автор: Андрей Карпов

В своей предыдущей статье я писал, что мне не нравится подход, при котором статические анализаторы кода оцениваются с помощью синтетических тестов. В статье приводился пример, воспринимаемый анализат…
Характеристики анализатора PVS-Studio на примере EFL Core Libraries, 10-15% ложных срабатываний

Дата: 31 Июл 2017

Автор: Андрей Карпов

После большой статьи про проверку операционной системы Tizen мне было задано много вопросов о проценте ложных срабатываний и о плотности ошибок (сколько ошибок PVS-Studio выявляет на 1000 строк кода)…
Зло живёт в функциях сравнения

Дата: 19 Май 2017

Автор: Андрей Карпов

Возможно, читатели помнят мою статью под названием "Эффект последней строки". В ней идёт речь о замеченной мной закономерности: ошибка чаще всего допускается в последней строке однотипных блоков текс…
Как PVS-Studio оказался внимательнее, чем три с половиной программиста

Дата: 22 Окт 2018

Автор: Андрей Карпов

PVS-Studio, как и другие статические анализаторы кода, часто выдаёт ложные срабатывания. Но не стоит спешить считать странные срабатывания ложными. Это короткая история о том, как PVS-Studio вновь ок…

Комментарии (0)

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