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

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

Введение

NCBI Genome Workbench предлагает исследователям большой набор инструментов для изучения и анализа генетических данных. Пользователи могут исследовать и сравнивать данные из нескольких источников, включая базы данных NCBI (National Center for Biotechnology Information) или собственных личных данных.

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

Данные для обзора (или даже исследования) кода были предоставлены статическим анализатором кода для C/C++/C#/Java — PVS-Studio.

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

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

1. Эффект последней строки;
2. Самая опасная функция в мире С/С++;
3. Логические выражения в C/C++. Как ошибаются профессионалы;
4. Зло живёт в функциях сравнения.

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

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

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

V501 There are identical sub-expressions '(!loc1.IsInt() &&!loc1.IsWhole())' to the left and to the right of the '||' operator. nw_aligner.cpp 480

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 A part of conditional expression is always false: s1.IsSet(). valid_biosource.cpp 3073

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

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

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, но сделал опечатку в имени одного из сравниваемых объектов. В итоге сравниваемые объекты ошибочно могут быть признаны равными в некоторых ситуациях.

Этот пример достоин статьи "Зло живёт в функциях сравнения".

V501 There are identical sub-expressions 'CFieldHandler::QualifierNamesAreEquivalent(field, kFieldTypeSeqId)' to the left and to the right of the '||' operator. field_handler.cpp 152

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 An item with the same key 'kArgRemote' has already been added. blast_args.cpp 3262

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 The 'then' statement is equivalent to the subsequent code fragment. vcf_reader.cpp 1105

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 */

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

/**
 * 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, на самом деле не будут затёрты нулями. Более подробно об этом неочевидном механизме компилятора можно узнать из статьи "Безопасная очистка приватных данных".

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

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

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

V534 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i'. taxFormat.cpp 569

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 The variable 'i' is being used for this loop and for the outer loop. Check lines: 302, 309. sls_alp.cpp 309

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

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

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

V520 The comma operator ',' in array index expression '[-- i2, — k]'. nw_spliced_aligner16.cpp 564

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). Рассмотрим следующую строку:

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

Слово 'matrix' и двойная индексация могут навести на мысль, что массив двумерный, но это не так. Это обычный указатель на массив целых чисел. Но диагностика V520 не просто так появилась. Программисты действительно путаются в способах индексации двумерных массивов.

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

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

V661 A suspicious expression 'A[B == C]'. Probably meant 'A[B] == C'. ncbi_service_connector.c 180

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 Array overrun is possible. The 'row' index is pointing beyond array bound. aln_reader.cpp 412

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 должна быть такой:

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

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

V570 The 'm_onClickFunction' variable is assigned to itself. alngraphic.hpp 103

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 Parameter 'w1' is always rewritten in function body before being used. bmfunc.h 5363

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

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

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

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

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 Uninitialized variable 'm_BitSet' used. SnpBitAttributes.hpp 187

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

//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).

V591 Non-void function should return a value. bio_tree.hpp 266

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

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

return *this;

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

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 Object slicing. An exception should be caught by reference rather than by value. cobalt.cpp 247

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

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

V779 Unreachable code detected. It is possible that an error is present. merge_tree_core.cpp 627

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.

V519 The 'interval_width' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 454, 456. aln_writer.cpp 456

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. Хоть и классическая, но очень нехорошая ошибка.

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

• 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

V571 Recurring check. The 'if (m_QueryOpts->filtering_options)' condition was already verified in line 703. blast_options_local_priv.hpp 713

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

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

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

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

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().

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

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

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 :-/*/;
  }
  ....
}

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

!local_host? SOCK_gethostbyname©: SOCK_GetLocalHostAddress(eDefault))

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

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

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

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

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 It's odd to compare a bool type value with a value of 0: (((status) & 0x7f) == 0) != 0. ncbi_process.cpp 111

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 оказался макросом, раскрывающимся таким образом:

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 The 'dst_len' pointer was utilized before it was verified against nullptr. Check lines: 309, 315. zlib.cpp 309

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 Consider inspecting the 'ch != '' && ch == ' '' expression. The expression is excessive or contains a misprint. cleanup_utils.cpp 580

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

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

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

Заключение

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

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

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. NCBI Genome Workbench: Scientific Research under Threat

Автор: Святослав

Источник