# NCBI Genome Workbench: научные исследования под угрозой

Современные компьютерные технологии, технические и программные решения \- всё это сильно облегчает и ускоряет проведение различных научных исследований\. Зачастую компьютерное моделирование \- единственный способ проверки многих теорий\. Научный софт имеет свои особенности\. Например, такой софт зачастую подвергается очень тщательному тестированию, но слабо документирован\. Тем не менее программное обеспечение пишется людьми, а люди допускают ошибки\. Ошибки в научных программах могут ставить под сомнение целые исследования\. В этой статье будут приведены десятки проблем, обнаруженных в коде пакета программ NCBI Genome Workbench\.

![0591_NCBI_GenomeWorkbench_ru/image1.png](https://import.viva64.com/docx/blog/0591_NCBI_GenomeWorkbench_ru/image1.png)

## Введение

[NCBI Genome Workbench](https://www.ncbi.nlm.nih.gov/tools/gbench/) предлагает исследователям большой набор инструментов для изучения и анализа генетических данных\. Пользователи могут исследовать и сравнивать данные из нескольких источников, включая базы данных NCBI \(National Center for Biotechnology Information\) или собственных личных данных\.

Как уже говорилось ранее, научный софт обычно хорошо покрыт unit\-тестами\. При проверке этого проекта из анализа было исключено 85 каталогов с файлами тестов\. Это около тысячи файлов\. Наверное, это обусловлено требованиями к тестированию разных сложных алгоритмов, которые изобретаются для тех или иных исследований\. Но качество остального кода \(не тестового\) находится не на таком высоком уровне, как хотелось бы\. Впрочем, как и в любом проекте, в котором ещё не позаботились о внедрении инструментов статического анализа кода :\)\.

Данные для обзора \(или даже исследования\) кода были предоставлены статическим анализатором кода для C/C\+\+/C\#/Java \- [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\.

## Всего две цифры, которые испортят ваш проект

![0591_NCBI_GenomeWorkbench_ru/image2.png](https://import.viva64.com/docx/blog/0591_NCBI_GenomeWorkbench_ru/image2.png)

На основе нашей базы ошибок, которая на данный момент составляет более 12 тысяч отборных примеров, мы замечаем и описываем особые паттерны написания кода, которые приводят к многочисленным ошибкам\. Например, мы проводили следующие исследования:

1. [Эффект последней строки](https://pvs-studio.ru/ru/blog/posts/cpp/0260/);
1. [Самая опасная функция в мире С/С\+\+](https://pvs-studio.ru/ru/blog/posts/cpp/0360/);
1. [Логические выражения в C/C\+\+\. Как ошибаются профессионалы](https://pvs-studio.ru/ru/blog/posts/cpp/0390/);
1. [Зло живёт в функциях сравнения](https://pvs-studio.ru/ru/blog/posts/cpp/0509/)\.

Этот проект положил начало описанию нового паттерна\. Речь идёт о цифрах **1** и **2** в названиях переменных, например, _file1_ и _file2_ и т\.п\. Очень легко перепутать две таких переменных\. Это частный случай опечаток в коде, но к появлению таких ошибок приводит одно \- желание работать с одноимёнными переменными, различающихся только цифрами 1 и 2 в конце имени\.

Немного забегая вперёд, скажу, что все перечисленные исследования нашли своё подтверждение в коде этого проекта :D\.

Рассмотрим первый пример из проекта Genome Workbench:

[V501](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions '\(\!loc1\.IsInt\(\) &&\!loc1\.IsWhole\(\)\)' to the left and to the right of the '\|\|' operator\. nw\_aligner\.cpp 480

```cpp
CRef<CSeq_align> CNWAligner::Run(CScope &scope, const CSeq_loc &loc1,
                                 const CSeq_loc &loc2,
                                 bool trim_end_gaps)
{
  if ((!loc1.IsInt() && !loc1.IsWhole()) ||
      (!loc1.IsInt() && !loc1.IsWhole()))
  {
    NCBI_THROW(CException, eUnknown,
               "Only whole and interval locations supported");
  }
  ....
}
```

Мы видим две переменные с именами _loc1_ и _loc2_\. А также ошибку в коде: переменная _loc2_ не используется, потому что вместо неё лишний раз используется _loc1_\.

Другой пример:

[V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always false: s1\.IsSet\(\)\. valid\_biosource\.cpp 3073

```cpp
static bool s_PCRPrimerSetLess(const CPCRPrimerSet& s1, const CPCRPrimerSet& s2)
{
  if (!s1.IsSet() && s1.IsSet()) {
    return true;
  } else if (s1.IsSet() && !s2.IsSet()) {
    return false;
  } else if (!s1.IsSet() && !s2.IsSet()) {
    return false;
  } else if (s1.Get().size() < s2.Get().size()) {
    return true;
  } else if (s1.Get().size() > s2.Get().size()) {
    return false;
  } else {
  ....
}
```

В первой же строке кода перепутали переменные _s1_ и _s2_\. Исходя из названия, это функция сравнения\. Но такая ошибка может быть где угодно, потому что, назвав переменные _Номер1_ и _Номер2_, программист почти наверняка сделает ошибку в будущем\. И чем больше использований таких имен в функции, тем выше вероятность совершить ошибку\.

## Другие опечатки и copy\-paste

![0591_NCBI_GenomeWorkbench_ru/image3.png](https://import.viva64.com/docx/blog/0591_NCBI_GenomeWorkbench_ru/image3.png)

[V501](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions to the left and to the right of the '\!\=' operator: bd\.bit\_\.bits\[i\] \!\= bd\.bit\_\.bits\[i\] bm\.h 296

```cpp
bool compare_state(const iterator_base& ib) const
{
  ....
  if (this->block_type_ == 0
  {
    if (bd.bit_.ptr != ib_db.bit_.ptr) return false;
    if (bd.bit_.idx != ib_db.bit_.idx) return false;
    if (bd.bit_.cnt != ib_db.bit_.cnt) return false;
    if (bd.bit_.pos != ib_db.bit_.pos) return false;
    for (unsigned i = 0; i < bd.bit_.cnt; ++i)
    {
      if (bd.bit_.bits[i] != bd.bit_.bits[i]) return false;
    }
  }
  ....
}
```

Я полагаю, что после всех проверок размеры массивов _bits_ у объектов _bd\.bit\__ и _ib\_db\.bit\__ равны\. Поэтому автор кода написал один цикл для поэлементного сравнения массивов _bits_, но сделал опечатку в имени одного из сравниваемых объектов\. В итоге сравниваемые объекты ошибочно могут быть признаны равными в некоторых ситуациях\.

Этот пример достоин статьи "[Зло живёт в функциях сравнения](https://pvs-studio.ru/ru/blog/posts/cpp/0509/)"\.

[V501](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions 'CFieldHandler::QualifierNamesAreEquivalent\(field, kFieldTypeSeqId\)' to the left and to the right of the '\|\|' operator\. field\_handler\.cpp 152

```cpp
bool CFieldHandlerFactory::s_IsSequenceIDField(const string& field)
{
  if (   CFieldHandler::QualifierNamesAreEquivalent(field, kFieldTypeSeqId)
      || CFieldHandler::QualifierNamesAreEquivalent(field, kFieldTypeSeqId)) {
    return true;
  } else {
    return false;
  }
}
```

Скорее всего, одна из проверок является лишней\. Я не нашёл в коде переменных, похожих на _kFieldTypeSeqId_\. Тем не менее тут возможен лишний вызов функции из\-за оператора "\|\|", что ухудшает производительность\.

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

* V501 There are identical sub\-expressions 'uf\-\>GetData\(\)\.IsBool\(\)' to the left and to the right of the '&&' operator\. variation\_utils\.cpp 1711
* V501 There are identical sub\-expressions 'uf\-\>GetData\(\)\.IsBool\(\)' to the left and to the right of the '&&' operator\. variation\_utils\.cpp 1735

[V766](https://pvs-studio.ru/ru/docs/warnings/v766/) An item with the same key 'kArgRemote' has already been added\. blast\_args\.cpp 3262

```cpp
void
CBlastAppArgs::x_IssueWarningsForIgnoredOptions(const CArgs& args)
{
  set<string> can_override;
  ....
  can_override.insert(kArgOutputFormat);
  can_override.insert(kArgNumDescriptions);
  can_override.insert(kArgNumAlignments);
  can_override.insert(kArgMaxTargetSequences);
  can_override.insert(kArgRemote);               // <=
  can_override.insert(kArgNumThreads);
  can_override.insert(kArgInputSearchStrategy);
  can_override.insert(kArgRemote);               // <=
  can_override.insert("remote_verbose");
  can_override.insert("verbose");
  ....
}
```

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

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

[V523](https://pvs-studio.ru/ru/docs/warnings/v523/) The 'then' statement is equivalent to the subsequent code fragment\. vcf\_reader\.cpp 1105

```cpp
bool
CVcfReader::xAssignFeatureLocationSet(....)
{
  ....
  if (data.m_SetType == CVcfData::ST_ALL_DEL) {
    if (data.m_strRef.size() == 1) {
      //deletion of a single base
      pFeat->SetLocation().SetPnt().SetPoint(data.m_iPos-1);
      pFeat->SetLocation().SetPnt().SetId(*pId);
    }
    else {
      pFeat->SetLocation().SetInt().SetFrom(data.m_iPos-1);
      //-1 for 0-based, 
      //another -1 for inclusive end-point ( i.e. [], not [) )
      pFeat->SetLocation().SetInt().SetTo( 
         data.m_iPos -1 + data.m_strRef.length() - 1); 
      pFeat->SetLocation().SetInt().SetId(*pId);
    }
    return true;
  }

  //default: For MNV's we will use the single starting point
  //NB: For references of size >=2, this location will not
  //match the reference allele.  Future Variation-ref
  //normalization code will address these issues,
  //and obviate the need for this code altogether.
  if (data.m_strRef.size() == 1) {
    //deletion of a single base
    pFeat->SetLocation().SetPnt().SetPoint(data.m_iPos-1);
    pFeat->SetLocation().SetPnt().SetId(*pId);
  }
  else {
    pFeat->SetLocation().SetInt().SetFrom(data.m_iPos-1);
    pFeat->SetLocation().SetInt().SetTo( 
      data.m_iPos -1 + data.m_strRef.length() - 1); 
    pFeat->SetLocation().SetInt().SetId(*pId);
  }
  return true;
}
```

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

Весь список подозрительных мест с оператором if\-else выглядит так:

* V523 The 'then' statement is equivalent to the 'else' statement\. blk\.c 2142
* V523 The 'then' statement is equivalent to the subsequent code fragment\. odbc\.c 379
* V523 The 'then' statement is equivalent to the subsequent code fragment\. odbc\.c 1414
* V523 The 'then' statement is equivalent to the 'else' statement\. seqdbvol\.cpp 1922
* V523 The 'then' statement is equivalent to the 'else' statement\. seqdb\_demo\.cpp 466
* V523 The 'then' statement is equivalent to the subsequent code fragment\. blast\_engine\.c 1917
* V523 The 'then' statement is equivalent to the 'else' statement\. blast\_filter\.c 420
* V523 The 'then' statement is equivalent to the 'else' statement\. blast\_parameters\.c 636
* V523 The 'then' statement is equivalent to the 'else' statement\. unordered\_spliter\.cpp 684
* V523 The 'then' statement is equivalent to the 'else' statement\. bme\.cpp 333
* V523 The 'then' statement is equivalent to the 'else' statement\. gme\.cpp 484

## /\* with security is best be pedantic \*/

![0591_NCBI_GenomeWorkbench_ru/image4.png](https://import.viva64.com/docx/blog/0591_NCBI_GenomeWorkbench_ru/image4.png)

[V597](https://pvs-studio.ru/ru/docs/warnings/v597/) The compiler could delete the 'memset' function call, which is used to flush 'passwd\_buf' buffer\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 366

```cpp
/**
 * Crypt a given password using schema required for NTLMv1 authentication
 * @param passwd clear text domain password
 * @param challenge challenge data given by server
 * @param flags NTLM flags from server side
 * @param answer buffer where to store crypted password
 */
void
tds_answer_challenge(....)
{
#define MAX_PW_SZ 14
  ....
  if (ntlm_v == 1) {
    ....
    /* with security is best be pedantic */
    memset(hash, 0, sizeof(hash));
    memset(passwd_buf, 0, sizeof(passwd_buf));
    memset(ntlm2_challenge, 0, sizeof(ntlm2_challenge));
  } else {
    ....
  }
}
```

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

Если вкратце, то функция _memset_ будет удалена компилятором, потому что очищаемые буферы больше не используются\. И такие данные, как _hash_ или _passwd\_buf_, на самом деле не будут затёрты нулями\. Более подробно об этом неочевидном механизме компилятора можно узнать из статьи "[Безопасная очистка приватных данных](https://pvs-studio.ru/ru/blog/posts/cpp/0388/)"\.

[V597](https://pvs-studio.ru/ru/docs/warnings/v597/) The compiler could delete the 'memset' function call, which is used to flush 'answer' object\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 561

```cpp
static TDSRET
tds7_send_auth(....)
{
  ....
  /* for security reason clear structure */
  memset(&answer, 0, sizeof(TDSANSWER));

  return tds_flush_packet(tds);
}
```

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

* V597 The compiler could delete the 'memset' function call, which is used to flush 'heap' object\. The memset\_s\(\) function should be used to erase the private data\. ncbi\_heapmgr\.c 1300
* V597 The compiler could delete the 'memset' function call, which is used to flush 'context' object\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 167
* V597 The compiler could delete the 'memset' function call, which is used to flush 'ks' object\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 339
* V597 The compiler could delete the 'memset' function call, which is used to flush 'md5\_ctx' object\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 353
* 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\. challenge\.c 365
* V597 The compiler could delete the 'memset' function call, which is used to flush 'ks' object\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 406
* V597 The compiler could delete the 'memset' function call, which is used to flush 'ntlm\_v2\_response' object\. The memset\_s\(\) function should be used to erase the private data\. login\.c 795
* V597 The compiler could delete the 'memset' function call, which is used to flush 'answer' object\. The memset\_s\(\) function should be used to erase the private data\. login\.c 801
* V597 The compiler could delete the 'memset' function call, which is used to flush 'packet' buffer\. The memset\_s\(\) function should be used to erase the private data\. numeric\.c 256
* V597 The compiler could delete the 'memset' function call, which is used to flush 'packet' buffer\. The memset\_s\(\) function should be used to erase the private data\. numeric\.c 110
* V597 The compiler could delete the 'memset' function call, which is used to flush 'pwd' buffer\. The memset\_s\(\) function should be used to erase the private data\. getpassarg\.c 50
* V597 The compiler could delete the 'memset' function call, which is used to flush 'context' object\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 188
* V597 The compiler could delete the 'memset' function call, which is used to flush 'buf' buffer\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 243
* V597 The compiler could delete the 'memset' function call, which is used to flush 'ntlm\_v2\_hash' buffer\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 309
* V597 The compiler could delete the 'memset' function call, which is used to flush 'md5\_ctx' object\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 354
* V597 The compiler could delete the 'memset' function call, which is used to flush 'passwd\_buf' buffer\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 380
* V597 The compiler could delete the 'memset' function call, which is used to flush 'ks' object\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 393
* 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\. challenge\.c 394
* V597 The compiler could delete the 'memset' function call, which is used to flush 'ntlm2\_challenge' buffer\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 395
* V597 The compiler could delete the 'memset' function call, which is used to flush 'ks' object\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 419
* V597 The compiler could delete the 'memset' function call, which is used to flush 'ntlm\_v2\_response' object\. The memset\_s\(\) function should be used to erase the private data\. challenge\.c 556

## Подозрительные циклы

![0591_NCBI_GenomeWorkbench_ru/image5.png](https://import.viva64.com/docx/blog/0591_NCBI_GenomeWorkbench_ru/image5.png)

[V534](https://pvs-studio.ru/ru/docs/warnings/v534/) It is likely that a wrong variable is being compared inside the 'for' operator\. Consider reviewing 'i'\. taxFormat\.cpp 569

```cpp
void CTaxFormat::x_LoadTaxTree(void)
{
  ....
  for(size_t i = 0; i < alignTaxids.size(); i++) {
    int tax_id = alignTaxids[i];
    ....
    for(size_t j = 0; i < taxInfo.seqInfoList.size(); j++) {
      SSeqInfo* seqInfo = taxInfo.seqInfoList[j];
      seqInfo->taxid = newTaxid;
    }
    ....
  }
  ....
}
```

Я думаю, в условие внутреннего цикла переменная _i_ затесалась случайно\. Вместо неё должна использоваться переменная _j_\.

[V535](https://pvs-studio.ru/ru/docs/warnings/v535/) The variable 'i' is being used for this loop and for the outer loop\. Check lines: 302, 309\. sls\_alp\.cpp 309

```cpp
alp::~alp()
{
  ....
  if(d_alp_states)
  {
    for(i=0;i<=d_nalp;i++)           // <=
    {
      if(i<=d_alp_states->d_dim)
      {
        if(d_alp_states->d_elem[i])
        {
          for(i=0;i<=d_nalp;i++)     // <=
          {
            ....
  ....
}
```

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

## Ненормальная индексация массивов

![0591_NCBI_GenomeWorkbench_ru/image6.png](https://import.viva64.com/docx/blog/0591_NCBI_GenomeWorkbench_ru/image6.png)

[V520](https://pvs-studio.ru/ru/docs/warnings/v520/) The comma operator ',' in array index expression '\[\-\- i2, \-\- k\]'\. nw\_spliced\_aligner16\.cpp 564

```cpp
void CSplicedAligner16::x_DoBackTrace (
    const Uint2* backtrace_matrix, CNWAligner::SAlignInOut* data,
    int i_global_max, int j_global_max)
{
  ....
  while(intron_length < m_IntronMinSize || (Key & donor) == 0) {
      Key = backtrace_matrix[--i2, --k];
      ++intron_length;
      data->m_transcript.push_back(eTS_Intron);
  }
  ....
}
```

Сразу скажу, что ошибки тут вроде нет \(пока, lol\)\. Рассмотрим следующую строку:

```cpp
Key = backtrace_matrix[--i2, --k];
```

Слово 'matrix' и двойная индексация могут навести на мысль, что массив двумерный, но это не так\. Это обычный указатель на массив целых чисел\. Но диагностика [V520](https://pvs-studio.ru/ru/docs/warnings/v520/) не просто так появилась\. Программисты действительно путаются в способах индексации двумерных массивов\.

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

```cpp
--i2;
Key = backtrace_matrix[--k];
```

[V661](https://pvs-studio.ru/ru/docs/warnings/v661/) A suspicious expression 'A\[B \=\= C\]'\. Probably meant 'A\[B\] \=\= C'\. ncbi\_service\_connector\.c 180

```cpp
static EHTTP_HeaderParse s_ParseHeader(const char* header, ....)
{
  ....
  if (sscanf(header, "%u.%u.%u.%u%n", &i1, &i2, &i3, &i4, &n) < 4
      ||  sscanf(header + n, "%hu%x%n", &uuu->port, &tkt, &m) < 2
      || (header[m += n]  &&  !(header[m] == '$')  &&
          !isspace((unsigned char)((header + m)
                                   [header[m] == '$'])))) {
      break/*failed - unreadable connection info*/;
  }
  ....
}
```

Ещё один пример кода, в котором я долго пытался понять, что происходит :D\. Функцией _isspace\(\)_ проверяется символ с индексом _m_, но если этот символ '$', то в функцию передаётся символ с индексом _m \+ 1_\. При этом сравнение с '$' уже было заранее\. Возможно, ошибки тут нет, но код точно можно переписать понятнее\.

[V557](https://pvs-studio.ru/ru/docs/warnings/v557/) Array overrun is possible\. The 'row' index is pointing beyond array bound\. aln\_reader\.cpp 412

```cpp
bool CAlnReader::x_IsGap(TNumrow row, TSeqPos pos, const string& residue)
{
  if (m_MiddleSections.size() == 0) {
    x_CalculateMiddleSections();
  }
  
  if (row > m_MiddleSections.size()) {
      return false;
  }
  if (pos < m_MiddleSections[row].first) {
    ....
  }
  ....
}
```

Вот тут присутствует серьёзная ошибка\. Правильная проверка индекса _row_ должна быть такой:

```cpp
if (row >= m_MiddleSections.size()) {
  return false;
}
```

Иначе возможно обращение к данным за пределами вектора _MiddleSections_\.

Ещё много таких мест:

* V557 Array overrun is possible\. The 'i' index is pointing beyond array bound\. resource\_pool\.hpp 388
* V557 Array overrun is possible\. The 'row' index is pointing beyond array bound\. aln\_reader\.cpp 418
* V557 Array overrun is possible\. The 'fmt\_idx' index is pointing beyond array bound\. seq\_writer\.cpp 384
* V557 Array overrun is possible\. The 'fmt\_idx' index is pointing beyond array bound\. blastdb\_formatter\.cpp 183
* V557 Array overrun is possible\. The 'num' index is pointing beyond array bound\. newcleanupp\.cpp 13035

## Как заработать недоверие к функциям

![0591_NCBI_GenomeWorkbench_ru/image7.png](https://import.viva64.com/docx/blog/0591_NCBI_GenomeWorkbench_ru/image7.png)

[V570](https://pvs-studio.ru/ru/docs/warnings/v570/) The 'm\_onClickFunction' variable is assigned to itself\. alngraphic\.hpp 103

```cpp
void SetOnClickFunctionName(string onClickFunction) {
  m_onClickFunction = m_onClickFunction;
}
```

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

Ещё два случая присваивания переменных самим себе приведу списком:

* V570 The 'iter\-\>level' variable is assigned to itself\. align\_format\_util\.cpp 189
* V570 The 'd\_elements\_values\[ind\]' variable is assigned to itself\. sls\_alp\_data\.cpp 1416

[V763](https://pvs-studio.ru/ru/docs/warnings/v763/) Parameter 'w1' is always rewritten in function body before being used\. bmfunc\.h 5363

```cpp
/// Bit COUNT functor
template<typename W> struct bit_COUNT
{
  W operator()(W w1, W w2) 
  {
    w1 = 0;
    BM_INCWORD_BITCOUNT(w1, w2);
    return w1;
  }
};
```

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

## Ошибки при проектировании классов

![0591_NCBI_GenomeWorkbench_ru/image8.png](https://import.viva64.com/docx/blog/0591_NCBI_GenomeWorkbench_ru/image8.png)

[V688](https://pvs-studio.ru/ru/docs/warnings/v688/) The 'm\_qsrc' function argument possesses the same name as one of the class members, which can result in a confusion\. compart\_matching\.cpp 873

```cpp
class CElementaryMatching: public CObject
{
  ....
  ISequenceSource * m_qsrc;
  ....
  void x_CreateIndex (ISequenceSource *m_qsrc, EIndexMode index_more, ....);
  void x_CreateRemapData(ISequenceSource *m_qsrc, EIndexMode mode);
  void x_LoadRemapData (ISequenceSource *m_qsrc, const string& sdb);
  ....
};
```

Сразу 3 функции класса содержат аргументы, имена которых совпадают с полем класса\. Это может приводить к ошибкам в телах функций: программист может думать, что работает с членом класса, на самом деле изменяя значение локальной переменной\.

[V614](https://pvs-studio.ru/ru/docs/warnings/v614/) Uninitialized variable 'm\_BitSet' used\. SnpBitAttributes\.hpp 187

```cpp
/// SNP bit attribute container.
class CSnpBitAttributes
{
public:
  ....
private:
  /// Internal storage for bits.
  Uint8 m_BitSet;
};

inline CSnpBitAttributes::CSnpBitAttributes(Uint8 bits) : m_BitSet(bits)
{
}

inline CSnpBitAttributes::CSnpBitAttributes(const vector<char>& octet_string)
{
  auto count = sizeof(m_BitSet);
  auto byte = octet_string.end();

  do
    m_BitSet = (m_BitSet << 8) | *--byte;
  while (--count > 0);
}
```

Один из конструкторов неаккуратно работает с переменной _m\_BitSet_\. Дело в том, что переменная неинициализированная\. Её "мусорное" значение используется на первой итерации цикла, после чего происходит инициализация\. Это очень серьёзная ошибка, приводящая к неопределённому поведению программы\.

[V603](https://pvs-studio.ru/ru/docs/warnings/v603/) The object was created but it is not being used\. If you wish to call constructor, 'this\-\>SIntervalComparisonResult::SIntervalComparisonResult\(\.\.\.\.\)' should be used\. compare\_feats\.hpp 100

```cpp
//This struct keeps the result of comparison of two exons
struct SIntervalComparisonResult : CObject
{
public:
  SIntervalComparisonResult(unsigned pos1, unsigned pos2,
                            FCompareLocs result, int pos_comparison = 0) 
  : m_exon_ordinal1(pos1), m_exon_ordinal2(pos2),
    m_result(result), m_position_comparison(pos_comparison) {}
    
  SIntervalComparisonResult()
  {
    SIntervalComparisonResult(0, 0, fCmp_Unknown, 0);
  }
  ....
};
```

Очень давно я не встречал таких ошибок при проверке проектов\. Но проблема всё ещё актуальна\. Ошибка в том, что вызов параметризированного конструктора таким образом приводит к созданию и удалению временного объекта\. А поля класса остаются неинициализированными\. Вызывать другой конструктор следует через список инициализации \(см\. [Delegating constructor](https://en.cppreference.com/w/cpp/language/constructor)\)\.

[V591](https://pvs-studio.ru/ru/docs/warnings/v591/) Non\-void function should return a value\. bio\_tree\.hpp 266

```cpp
/// Recursive assignment
CBioNode& operator=(const CBioNode& tree)
{
  TParent::operator=(tree);
  TBioTree* pt = (TBioTree*)tree.GetParentTree();
  SetParentTree(pt);
}
```

Анализатор считает, что в перегруженном операторе не хватает строки:

```cpp
return *this;
```

[V670](https://pvs-studio.ru/ru/docs/warnings/v670/) The uninitialized class member 'm\_OutBlobIdOrData' is used to initialize the 'm\_StdOut' member\. Remember that members are initialized in the order of their declarations inside a class\. remote\_app\.hpp 215

```cpp
class NCBI_XCONNECT_EXPORT CRemoteAppResult
{
public:
  CRemoteAppResult(CNetCacheAPI::TInstance netcache_api,
          size_t max_inline_size = kMaxBlobInlineSize) :
      m_NetCacheAPI(netcache_api),
      m_RetCode(-1),
      m_StdOut(netcache_api, m_OutBlobIdOrData, m_OutBlobSize),
      m_OutBlobSize(0),
      m_StdErr(netcache_api, m_ErrBlobIdOrData, m_ErrBlobSize),
      m_ErrBlobSize(0),
      m_StorageType(eBlobStorage),
      m_MaxInlineSize(max_inline_size)
  {
  }
  ....
};
```

На этот фрагмент кода выдаётся сразу 3 предупреждения анализатора\. Поля класса инициализируются не в том порядке, в котором перечислены в списке инициализации, а в том, в котором они объявлены в классе\. Классическая причина ошибки в том, что не все программисты помнят или знают об этом правиле\. Здесь и в списке инициализации как раз неверный порядок\. Складывается ощущение, что список полей вводили в случайном порядке\. 

[V746](https://pvs-studio.ru/ru/docs/warnings/v746/) Object slicing\. An exception should be caught by reference rather than by value\. cobalt\.cpp 247

```cpp
void 
CMultiAligner::SetQueries(const vector< CRef<objects::CBioseq> >& queries)
{
  ....
  try {
    seq_loc->SetId(*it->GetSeqId());
  }
  catch (objects::CObjMgrException e) {
    NCBI_THROW(CMultiAlignerException, eInvalidInput,
               (string)"Missing seq-id in bioseq. " + e.GetMsg());
  }
  m_tQueries.push_back(seq_loc);
  ....
}
```

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

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

* V746 Object slicing\. An exception should be caught by reference rather than by value\. agp\_validate\_reader\.cpp 562
* V746 Object slicing\. An exception should be caught by reference rather than by value\. aln\_build\_app\.cpp 320
* V746 Object slicing\. An exception should be caught by reference rather than by value\. aln\_test\_app\.cpp 458
* V746 Object slicing\. An exception should be caught by reference rather than by value\. cobalt\.cpp 691
* V746 Object slicing\. An exception should be caught by reference rather than by value\. cobalt\.cpp 719
* V746 Object slicing\. An exception should be caught by reference rather than by value\. cobalt\.cpp 728
* V746 Object slicing\. An exception should be caught by reference rather than by value\. cobalt\.cpp 732

## О недостижимом коде и других проблемах выполнения кода

![0591_NCBI_GenomeWorkbench_ru/image9.png](https://import.viva64.com/docx/blog/0591_NCBI_GenomeWorkbench_ru/image9.png)

[V779](https://pvs-studio.ru/ru/docs/warnings/v779/) Unreachable code detected\. It is possible that an error is present\. merge\_tree\_core\.cpp 627

```cpp
bool CMergeTree::x_FindBefores_Up_Iter(....)
{
    ....
    FirstFrame->Curr = StartCurr;
    FirstFrame->Returned = false;
    FirstFrame->VisitCount = 0;
    FrameStack.push_back(FirstFrame);

    while(!FrameStack.empty()) {

        ....
        if(Rel == CEquivRange::eAfter) {
            Frame->Returned = false;
            FrameStack.pop_back();
            continue;
        } 
        else if(Rel == CEquivRange::eBefore) {
            ....
            continue;
        }
        else {
            if(Frame->VisitCount == 0) {
                ....
                continue;
            } else {
                ....
                continue;
            }
        }
        
        Frame->Returned = false; // <=
        FrameStack.pop_back();
        continue;
    }  // end stack loop
    
    FirstFrame->ChildFrames.clear();
    return FirstFrame->Returned;
}
```

Код условного оператора написан так, что абсолютно все ветви кода заканчиваются оператором _continue_\. Это привело к тому, что в цикле _while_ образовались несколько строк недостижимого кода\. Выглядят эти строки очень подозрительно\. Скорее всего, такая проблема возникла после рефакторинга кода, и теперь тут требуется внимательный code\-review\.

Ещё несколько подозрительных мест:

* V779 Unreachable code detected\. It is possible that an error is present\. dbapi\_driver\_utils\.cpp 351
* V779 Unreachable code detected\. It is possible that an error is present\. net\.c 780
* V779 Unreachable code detected\. It is possible that an error is present\. bcp\.c 1495
* V779 Unreachable code detected\. It is possible that an error is present\. remote\_blast\.cpp 1470
* V779 Unreachable code detected\. It is possible that an error is present\. remote\_blast\.cpp 1522

[V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'interval\_width' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 454, 456\. aln\_writer\.cpp 456

```cpp
void CAlnWriter::AddGaps(....)
{
  ....
  switch(exon_chunk->Which()) {
  case CSpliced_exon_chunk::e_Match:
      interval_width = exon_chunk->GetMatch();
  case CSpliced_exon_chunk::e_Mismatch:
      interval_width = exon_chunk->GetMismatch();
  case CSpliced_exon_chunk::e_Diag:
      interval_width = exon_chunk->GetDiag();
      genomic_string.append(....);
      product_string.append(....);
      genomic_pos += interval_width;
      product_pos += interval_width/res_width;
      break;
    ....
  }
  ....
}
```

Переменная _interval\_width_ перезаписывается несколько раз, т\.к\. в ветках _case_ отсутствуют операторы _break_\. Хоть и классическая, но очень нехорошая ошибка\.

[V571](https://pvs-studio.ru/ru/docs/warnings/v571/) Recurring check\. The 'if \(m\_QueryOpts\-\>filtering\_options\)' condition was already verified in line 703\. blast\_options\_local\_priv\.hpp 713

```cpp
inline void
CBlastOptionsLocal::SetFilterString(const char* f)
{
  ....
  if (m_QueryOpts->filtering_options)      // <=
  {
    SBlastFilterOptions* old_opts = m_QueryOpts->filtering_options;
    m_QueryOpts->filtering_options = NULL;
    SBlastFilterOptionsMerge(&(m_QueryOpts->filtering_options),
      old_opts, new_opts);
    old_opts = SBlastFilterOptionsFree(old_opts);
    new_opts = SBlastFilterOptionsFree(new_opts);
  } 
  else
  {
     if (m_QueryOpts->filtering_options)   // <=
         m_QueryOpts->filtering_options = 
             SBlastFilterOptionsFree(m_QueryOpts->filtering_options);
     m_QueryOpts->filtering_options = new_opts;
     new_opts = NULL;
  }
  ....
}
```

Очевидно, ветвь _else_ требует переписывания\. У меня есть несколько идей, что хотели сделать с указателем _m\_QueryOpts\-\>filtering\_options_, но код всё равно какой\-то запутанный\. Взываю к авторам кода\.

Ну и проблема не приходит одна:

* V571 Recurring check\. The 'if \(sleeptime\)' condition was already verified in line 205\. request\_control\.cpp 208
* V571 Recurring check\. The 'if \(assignValue\.empty\(\)\)' condition was already verified in line 712\. classstr\.cpp 718

## Ошибки чтения данных

![0591_NCBI_GenomeWorkbench_ru/image10.png](https://import.viva64.com/docx/blog/0591_NCBI_GenomeWorkbench_ru/image10.png)

[V739](https://pvs-studio.ru/ru/docs/warnings/v739/) EOF should not be compared with a value of the 'char' type\. The 'linestring\[0\]' should be of the 'int' type\. alnread\.c 3509

```cpp
static EBool
s_AfrpInitLineData(
  ....
  char* linestring = readfunc (pfile);
  ....
  while (linestring != NULL  &&  linestring [0] != EOF) {
    s_TrimSpace (&linestring);
    ....
  }
  ....
}
```

Символы, которые планируется сравнивать с EOF, не должны храниться в переменных типа _char_\. Иначе есть риск, что символ со значением 0xFF \(255\) превратится в \-1 и будет интерпретироваться точно так же, как конец файла \(EOF\)\. Также \(на всякий случай\) стоит проверить реализацию функции _readfunc_\.

[V663](https://pvs-studio.ru/ru/docs/warnings/v663/) Infinite loop is possible\. The 'cin\.eof\(\)' condition is insufficient to break from the loop\. Consider adding the 'cin\.fail\(\)' function call to the conditional expression\. ncbicgi\.cpp 1564

```cpp
typedef std::istream CNcbiIstream;
void CCgiRequest::Serialize(CNcbiOstream& os) const
{
  ....
  CNcbiIstream* istrm = GetInputStream();
  if (istrm) {
    char buf[1024];
    while(!istrm->eof()) {
      istrm->read(buf, sizeof(buf));
      os.write(buf, istrm->gcount());
    }
  }
}
```

Анализатор обнаружил потенциальную ошибку, из\-за которой может возникнуть бесконечный цикл\. В случае возникновения сбоя при чтении данных вызов функции _eof\(\)_ будет всегда возвращать значение _false_\. Для завершения цикла в этом случае необходима дополнительная проверка значения, возвращаемого функцией _fail\(\)_\.

## Разные ошибки

![0591_NCBI_GenomeWorkbench_ru/image11.png](https://import.viva64.com/docx/blog/0591_NCBI_GenomeWorkbench_ru/image11.png)

[V502](https://pvs-studio.ru/ru/docs/warnings/v502/) Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '&&' operator\. ncbi\_connutil\.c 1135

```cpp
static const char* x_ClientAddress(const char* client_host,
                                   int/*bool*/ local_host)
{
  ....
  if ((client_host == c  &&  x_IsSufficientAddress(client_host))
      ||  !(ip = *c  &&  !local_host
            ? SOCK_gethostbyname(c)
            : SOCK_GetLocalHostAddress(eDefault))
      ||  SOCK_ntoa(ip, addr, sizeof(addr)) != 0
      ||  !(s = (char*) malloc(strlen(client_host) + strlen(addr) + 3))) {
      return client_host/*least we can do :-/*/;
  }
  ....
}
```

Обратите внимание на выражение:

```cpp
!local_host ? SOCK_gethostbyname(c) : SOCK_GetLocalHostAddress(eDefault)
```

Оно не вычисляется так, как ожидал программист, потому что всё выражение выглядит так:

```cpp
ip = *c  && !local_host ? SOCK_gethostbyname(c) : SOCK_GetLocalHostAddress(...)
```

Приоритет оператора _&&_ выше, чем у _?:_\. По этой причине код выполняется не так, как задумывалось\.

[V561](https://pvs-studio.ru/ru/docs/warnings/v561/) It's probably better to assign value to 'seq' variable than to declare it anew\. Previous declaration: validator\.cpp, line 490\. validator\.cpp 492

```cpp
bool CValidator::IsSeqLocCorrectlyOrdered(const CSeq_loc& loc, CScope& scope)
{
  CBioseq_Handle seq;
  try {
    CBioseq_Handle seq = scope.GetBioseqHandle(loc);
  } catch (CObjMgrException& ) {
    // no way to tell
    return true;
  } catch (const exception& ) {
    // no way to tell
    return true;
  }
  if (seq  &&  seq.GetInst_Topology() == CSeq_inst::eTopology_circular) {
    // no way to check if topology is circular
    return true;
  }

  return CheckConsecutiveIntervals(loc, scope, x_IsCorrectlyOrdered);
}
```

Из\-за того, что программист объявил новую переменную _seq_ внутри секции try/catch, другая переменная _seq_ остаётся неинициализированной и используется ниже по коду\.

[V562](https://pvs-studio.ru/ru/docs/warnings/v562/) It's odd to compare a bool type value with a value of 0: \(\(\(status\) & 0x7f\) \=\= 0\) \!\= 0\. ncbi\_process\.cpp 111

```cpp
bool CProcess::CExitInfo::IsExited(void) const
{
    EXIT_INFO_CHECK;
    if (state != eExitInfo_Terminated) {
        return false;
    }
#if   defined(NCBI_OS_UNIX)
    return WIFEXITED(status) != 0;
#elif defined(NCBI_OS_MSWIN)
    // The process always terminates with exit code
    return true;
#endif
}
```

Ничего не предвещало беды, но WIFEXITED оказался макросом, раскрывающимся таким образом:

```cpp
return (((status) & 0x7f) == 0) != 0;
```

Получается так, что функция возвращает противоположное значение\.

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

* V562 It's odd to compare a bool type value with a value of 0\. ncbi\_process\.cpp 126

[V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'dst\_len' pointer was utilized before it was verified against nullptr\. Check lines: 309, 315\. zlib\.cpp 309

```cpp
bool CZipCompression::CompressBuffer(
  const void* src_buf, size_t  src_len,
  void*       dst_buf, size_t  dst_size,
  /* out */   size_t* dst_len)
{
  *dst_len = 0;

  // Check parameters
  if (!src_len  &&  !F_ISSET(fAllowEmptyData)) {
    src_buf = NULL;
  }
  if (!src_buf || !dst_buf || !dst_len) {
    SetError(Z_STREAM_ERROR, "bad argument");
    ERR_COMPRESS(48, FormatErrorMessage("CZipCompression::CompressBuffer"));
    return false;
  }
  ....
}
```

Указатель _dst\_len_ разыменовывается в самом начале функции, при этом далее по коду проверяется на равенство нулю\. В коде допущена ошибка, которая приводит к неопределённому поведению, если указатель _dst\_len_ окажется равен _nullptr_\.

[V590](https://pvs-studio.ru/ru/docs/warnings/v590/) Consider inspecting the 'ch \!\= '\\0' && ch \=\= ' '' expression\. The expression is excessive or contains a misprint\. cleanup\_utils\.cpp 580

```cpp
bool Asn2gnbkCompressSpaces(string& val)
{
  ....
  while (ch != '\0' && ch == ' ') {
    ptr++;
    ch = *ptr;
  }
  ....
}
```

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

```cpp
while (ch == ' ') {
  ....
}
```

## Заключение

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

Приглашаю разработчиков проекта NCBI Genome Workbench связаться с нами, и мы предоставим полный отчёт, выданный анализатором PVS\-Studio\.

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