Clever Geek Handbook

NCBIゲノムワークベンチ：絶滅危Research研究

最新のコンピューター技術、技術、ソフトウェアソリューション-これらはすべて、さまざまな科学研究の実施を大幅に促進および加速します。 多くの場合、コンピューターシミュレーションが多くの理論をテストする唯一の方法です。 科学ソフトウェアには独自の特徴があります。 たとえば、このようなソフトウェアは多くの場合非常に徹底的なテストの対象となりますが、文書化は不十分です。 しかし、ソフトウェアは人によって書かれており、人は間違いを犯します。 科学プログラムの誤りは、研究全体に疑問を投げかける可能性があります。 この記事では、NCBI Genome Workbenchソフトウェアパッケージのコードで見つかった多くの問題をリストします。



はじめに



NCBI Genome Workbenchは、遺伝データを研究および分析するための多数のツールセットを研究者に提供します。 ユーザーは、NCBI（国立バイオテクノロジー情報センター）データベースまたは自分の個人データを含むいくつかのソースからのデータを調査および比較できます。



前述のように、科学ソフトウェアは通常、単体テストで十分にカバーされています。 このプロジェクトをチェックすると、テストファイルのある85個のディレクトリが分析から除外されました。 これは約1,000個のファイルです。 これはおそらく、さまざまな研究のために発明されたさまざまな複雑なアルゴリズムをテストするための要件によるものです。 しかし、残りのコード（テスト用ではない）の品質は、私たちが望むほど高いレベルではありません。 ただし、静的コード分析ツールの導入をまだ考慮していないプロジェクトの場合と同様に:)。



C / C ++ / C＃/ Java- PVS-Studioの静的コードアナライザーによって、コードレビュー（または研究）用のデータが提供されました。



プロジェクトを台無しにする2つの数字















現在12,000以上の選択された例に相当するエラーのデータベースに基づいて、多数のエラーにつながるコードを記述するための特定のパターンに気付き、説明します。 たとえば、次の調査を実施しました。



  1. 最後の行の効果 。
  2. C / C ++の世界で最も危険な関数 。
  3. C / C ++の論理式。 専門家はどの程度間違っていますか。
  4. 悪は比較機能に住んでいます 。


このプロジェクトは、新しいパターンの説明の始まりを示しました。 たとえば、 file1やfile2など、変数の名前に含まれる数字1と2について話します。 このような2つの変数を混同するのは非常に簡単です。 これはコードのタイプミスの特殊なケースですが、そのようなエラーの1つは、名前の末尾の数字1と2のみが異なる、同じ名前の変数を使用したいという欲求につながります。



少し先を見据えて、このプロジェクトのコードで上記のすべての研究が確認されたと言います：D.



Genome Workbenchプロジェクトの最初の例を考えてみましょう。



V501 「||」の左側と右側には、同一の副次式「（！Loc1.IsInt（）&&！Loc1.IsWhole（））」があります。 演算子。 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という名前の2つの変数があります 。 また、コードのエラー： loc2変数は使用されません。代わりにloc1が再度使用されるためです。



別の例：



V560条件式の一部は常に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に名前を付けることで、プログラマーは将来間違いなく間違いを犯すからです。 また、関数でこのような名前を使用するほど、エラーの可能性が高くなります。



その他のタイプミスとコピーアンドペースト















V501 '！='演算子の左右に同じ部分式があります：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; } } .... }


すべてのチェックの後、 bd.bit_およびib_db.bit_オブジェクトの ビット配列のサイズは等しいと信じています 。 したがって、コードの作成者は、 ビット配列の要素ごとの比較のために1サイクルを記述しましたが、比較対象オブジェクトの1つの名前にタイプミスを作成しました。 その結果、状況によっては、比較されたオブジェクトが誤って等しいと見なされる場合があります。



この例は、「 悪は比較機能に生きている 」という記事に値します 。



V501 「||」の左と右に同一のサブ式「CFieldHandler :: QualifierNamesAreEquivalent（field、kFieldTypeSeqId）」があります 演算子。 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; } }


ほとんどの場合、チェックの1つは不要です。 kFieldTypeSeqIdに似たコード変数は見つかりませんでした 。 それでも、「||」演算子のために余分な関数呼び出しが可能になり、パフォーマンスが低下します。



検証が必要なアナライザー警告のある同じタイプの場所がさらに2つあります。





V766同じキー「kArgRemote」を持つアイテムがすでに追加されています。 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つの同一の値の追加を検出しました。 このコンテナは一意の値のみを保存するため、重複はコンテナに追加されないことを思い出してください。



上記のようなコードは、多くの場合、コピーと貼り付けの方法を使用して記述されます。 単に追加の値があるか、または作成者がコピーしたときに変数の1つの名前を変更するのを忘れた可能性があります。 insertの余分な呼び出しを削除すると、コードはわずかに最適化されますが、重要ではありません。 さらに重要なのは、セット内の要素が欠落しているために、重大なエラーがここに隠れている可能性があることです。



V523 「then」ステートメントは、後続のコードフラグメントと同等です。 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 ( ie [], 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ステートメントを含む疑わしい場所のリストは次のようになります。





/ *セキュリティを確保するのが最善です* /













V597コンパイラは、「passwd_buf」バッファーのフラッシュに使用される「memset」関数呼び出しを削除できました。 プライベートデータを消去するには、memset_s（）関数を使用する必要があります。 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コンパイラーは、「memset」関数呼び出しを削除できます。これは、「answer」オブジェクトをフラッシュするために使用されます。 プライベートデータを消去するには、memset_s（）関数を使用する必要があります。 challenge.c 561 



 static TDSRET tds7_send_auth(....) { .... /* for security reason clear structure */ memset(&answer, 0, sizeof(TDSANSWER)); return tds_flush_packet(tds); }


「セキュリティ」に関するコメントがある例はこれだけではありません。 コメントから判断すると、プロジェクトにとってセキュリティは本当に重要であると考えられます。 したがって、特定された問題の小さなリスト全体を囲みます。





疑わしいサイクル















V534 「for」演算子内で間違った変数が比較されている可能性があります。 「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変数 'i'は、このループと外側のループに使用されています。 行を確認してください：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++) // <= { .... .... }


グローバルカウンタもリセットされる2つのネストされた同一のサイクルは、非常に疑わしく見えます。 開発者は、ここで何が起こるかを確認する必要があります。



配列のインデックス付けが異常です















V520配列インデックス式 '[-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); } .... }


私はすぐにエラーがないようだと言わなければなりません（今のところ、笑）。 次の行を検討してください。 



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


単語「マトリックス」と二重索引付けは、配列が2次元であることを示唆する場合がありますが、そうではありません。 これは整数の配列への通常のポインターです。 しかし、 V520診断は表示されませんでした。 プログラマは、2次元配列のインデックス付け方法について本当に混乱しています。



この場合、作者は次のように書くこともできますが、1行のコードで保存することにしました。 



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


V661疑わしい表現「A [B == C]」。 おそらく「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配列のオーバーランが可能です。 「行」インデックスは、配列の境界を超えています。 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) { .... } .... }


ここに重大な間違いがあります。 正しい行インデックスチェックは次のようになります。 



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


それ以外の場合、 MiddleSectionsベクトルの外部のデータにアクセスできます。



さらに多くのそのような場所：





機能の不信を獲得する方法















V570 「m_onClickFunction」変数はそれ自体に割り当てられます。 alngraphic.hpp 103 



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


コメントすることすらありません。 何かをクリックした人、クリックした人だけに同情できますが、何も変わりません。



変数を自分に割り当てる場合、さらに2つのケースがリストになります。





V763パラメーター 'w1'は、使用される前に常に関数本体で書き換えられます。 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 'm_qsrc'関数の引数は、クラスメンバーの1つと同じ名前を持っているため、混乱を招く可能性があります。 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初期化されていない変数「m_BitSet」が使用されました。 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); }


コンストラクターの1つは、 m_BitSet変数と不正確です。 実際、変数は初期化されていません。 その「ガベージ」値は、ループの最初の繰り返しで使用され、その後初期化が行われます。 これは非常に重大な間違いであり、未定義のプログラムの動作につながります。



V603オブジェクトは作成されましたが、使用されていません。 コンストラクターを呼び出す場合は、「this-> SIntervalComparisonResult :: SIntervalComparisonResult（....）」を使用する必要があります。 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); } .... };


ずいぶん前に、プロジェクトをチェックするときにこのようなエラーに遭遇しませんでした。 しかし、問題は依然として関連しています。 エラーは、この方法でパラメーター化されたコンストラクターを呼び出すと、一時オブジェクトが作成および削除されることです。 そして、クラスのフィールドは初期化されません。 初期化リストから別のコンストラクターを呼び出す必要があります（ コンストラクターの委任を参照）。



V591非void関数は値を返す必要があります。 bio_tree.hpp 266 



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


アナライザーは、オーバーロードされたステートメントに行がないと見なします。 



 return *this;


V670初期化されていないクラスメンバー「m_OutBlobIdOrData」は、「m_StdOut」メンバーを初期化するために使用されます。 メンバーは、クラス内の宣言の順序で初期化されることに注意してください。 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オブジェクトのスライス。 例外は、値ではなく参照によってキャッチする必要があります。 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); .... }


値で例外をキャッチすると、新しいオブジェクトの作成により、例外に関する一部の情報が失われる可能性があります。 参照により例外をキャッチする方がはるかに安全です。



同様の場所：





到達不能コードおよびその他のコード実行の問題について













V779到達不能コードが検出されました。 エラーが存在する可能性があります。 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ループで数行の到達不能コードが形成されました。 これらの行は非常に疑わしく見えます。 ほとんどの場合、この問題はコードのリファクタリング後に発生し、現在では慎重なコードレビューが必要です。



V519 「interval_width」変数には、連続して2回値が割り当てられます。 おそらくこれは間違いです。 確認行：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ステートメントはありません。 古典的ではありますが、非常に悪い間違いです。



さらにいくつかの疑わしい場所：





V571定期的なチェック。 「if（m_QueryOpts-> filtering_options）」条件は、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ポインターを使用してやりたいアイデアがいくつかありますが、コードはまだ混乱しています。 コードの作者に訴えます。



さて、問題は単独では発生しません。





データ読み取りエラー













V739 EOFは、「char」タイプの値と比較しないでください。 「ラインストリング[0]」は「int」タイプである必要があります。 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無限ループが可能です。 「cin.eof（）」条件は、ループから抜けるには不十分です。 「cin.fail（）」関数呼び出しを条件式に追加することを検討してください。 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おそらく、「?:」演算子は予想とは異なる方法で動作します。 「？：」演算子の優先順位は、「&&」演算子よりも低くなっています。 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(c) : SOCK_GetLocalHostAddress(eDefault)




式全体が次のようになるため、プログラマーが予想したとおりに計算されません。 



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




&&演算子の優先順位は？：よりも高くなっています。このため、コードは意図したとおりに実行されません。



V561新たに宣言するよりも、 'seq'変数に値を割り当てるほうがおそらく良いでしょう。前の宣言：validator.cpp、行490。validator.cpp492 



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


プログラマがtry / catchセクション内で新しいseq変数を宣言するため、別のseq変数は初期化されずに残り、以下のコードで使用されます。



V562ブール型の値を値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;


関数が反対の値を返すことがわかりました。



別のそのような関数がコードで見つかり、警告を発行しました：





V595 nullptrに対して検証される前に、「dst_len」ポインターが使用されました。行を確認してください：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 'ch！=' \ 0 '&& ch ==' ''式の検査を検討してください。表現が過剰であるか、誤植が含まれています。cleanup_utils.cpp 580 



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


ループを停止する条件は、文字chがスペースかどうかのみに依存します。式は次のように簡略化できます。 



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


おわりに



科学研究におけるコンピュータープログラムの使用は、発見を支援し、支援するでしょう。誤字が原因で特に重要なものを見逃さないことを期待しましょう。



NCBI Genome Workbenchプロジェクトの開発者にご連絡ください。PVS-Studioアナライザーによって発行された完全なレポートを提供します。



この小さなコードの研究が多くのバグを修正し、一般にプロジェクトの信頼性を向上させることを願っています。まだ実行していない場合は、プロジェクトのコードでPVS-Studioを実行してみてください。あなたはそれを好きかもしれません:)。











英語を話す聴衆とこの記事を共有したい場合は、翻訳へのリンクを使用してください：Svyatoslav Razmyslov。 NCBI Genome Workbench：脅威にさらされている科学研究


More articles:


All Articles