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

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

Существует проект 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’. При выходе из функции, эта переменная будет уничтожена. Использование ссылки на разрушенный объект приведёт к непредсказуемым последствиям.

Мне кажется, надо было не создавать новый итератор, а сохранить ссылку:

Iter<TStringSet, ConcatVirtual<TSpec> > &before = me;

Аналогичная беда имеется у оператора ‘—‘: 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.