Geekly Articles each Day
📜 ⬆️ ⬇️

NCBI Genome Workbench: Scientific Research Under Threat

Modern computer technology, technical and software solutions - all this greatly facilitates and accelerates the conduct of various scientific studies. Often, computer simulation is the only way to test many theories. Scientific software has its own characteristics. For example, such software is often subjected to very thorough testing, but is poorly documented. However, software is written by people, and people make mistakes. Errors in scientific programs can cast doubt on entire studies. This article will cover dozens of problems found in the code of the NCBI Genome Workbench software package.

Introduction


NCBI Genome Workbench offers researchers a wide range of tools for studying and analyzing genetic data. Users can explore and compare data from multiple sources, including NCBI (National Center for Biotechnology Information) databases or their own personal data.

As mentioned earlier, scientific software is usually well covered by unit tests. When checking this project, 85 directories with test files were excluded from the analysis. This is about a thousand files. Probably, this is due to the requirements for testing various complex algorithms that are invented for various studies. But the quality of the rest of the code (not test) is not at such a high level as we would like. However, as in any project that has not yet taken care of the implementation of static code analysis tools :).

Data for the review (or even research) of the code was provided by a static code analyzer for C / C ++ / C # / Java - PVS-Studio .
')

Just two digits that will ruin your project.




Based on our database of errors, which currently amounts to more than 12 thousand selected examples, we notice and describe special patterns of writing code that lead to numerous errors. For example, we conducted the following studies:

  1. Last line effect ;
  2. The most dangerous feature in the C / C ++ world ;
  3. Boolean expressions in C / C ++. How are the professionals mistaken ;
  4. Evil lives in comparison functions .

This project initiated the description of the new pattern. We are talking about the numbers 1 and 2 in the names of variables, for example, file1 and file2 , etc. It is very easy to confuse two such variables. This is a special case of typos in the code, but one error leads to the appearance of such errors - the desire to work with variables of the same name, differing only in numbers 1 and 2 at the end of the name.

Looking ahead a bit, I’ll say that all the listed studies have been confirmed in the code of this project: D.

Consider the first example from the Genome Workbench project:

V501 There are identical sub-expressions '(! Loc1.IsInt () &&! Loc1.IsWhole ())' 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"); } .... }

We see two variables, loc1 and loc2 . And also an error in the code: the variable loc2 is not used, because instead of it loc1 is used once again.

Another example:

V560 A part of the 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 { ..... }

The first line of the code mixed up the variables s1 and s2 . As the name suggests, this is a comparison function. But such an error can be anywhere, because by calling the variables Number 1 and Number 2 , the programmer will almost certainly make a mistake in the future. And the more uses of such names in the function, the higher the probability of making a mistake.

Other typos and copy-paste




V501 There are identical sub-expressions to the left and the right! ”= '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; } } .... }

I suppose that after all the checks, the sizes of the arrays bits of the bd.bit_ and ib_db.bit_ objects are equal. Therefore, the author of the code wrote one cycle for the elementwise comparison of the bits arrays, but made a typo in the name of one of the compared objects. As a result, the compared objects can be mistakenly considered equal in some situations.

This example is worthy of the article " Evil lives in comparison functions ."

V501 There are identical sub-expressions 'CFieldHandler :: QualifierNamesAreEquivalent (field, kFieldTypeSeqId)' to the left | 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; } }

Most likely, one of the checks is superfluous. I did not find in the code variables similar to kFieldTypeSeqId . However, there may be an extra function call due to the operator "||", which degrades performance.

A couple more of the same type of places with a warning analyzer, requiring verification:


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

The analyzer detected the addition of 2 identical values ​​to the set container. Recall that this container stores only unique values, so duplicates are not added to it.

Code like the one above is often written using the copy-paste method. There may just be an extra value, or perhaps the author forgot to rename one of the variables when copied. When removing an extra call, the insert code is slightly optimized, which, however, is not essential. It is much more important that there may be a serious mistake hiding because of a missing element in the set.

V523 The 'then' statement is 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 ( 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; }

The function contains large and completely identical code fragments. However, they contain different accompanying comments. The code is not optimally written, complicated, and possibly contains an error.

The entire list of suspicious places with an if-else operator looks like this:


/ * with security is best be pedantic * /



V597 The compiler couldn’t need the call memorization function, 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 { .... } }

As you probably already guessed, in the section title there is an amusing comment about security from the code.

In short, the memset function will be deleted by the compiler, because cleared buffers are no longer used. And data such as hash or passwd_buf , in fact, will not be overwritten with zeros. More details about this unobvious compiler mechanism can be found in the article " Safe Private Data Cleansing ".

V597 The compiler couldn’t delete the memset function call, which is used to flush the 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); }

That was not the only example with comments about "security." Judging by the comments, we can assume that security is really important for the project. Therefore, I’m enclosing a whole list of problems that are not small:


Suspicious cycles




V534 It is likely that a 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 think, in the condition of the inner cycle, the variable i got stuck randomly. Instead, the variable j should be used.

V535 The variable 'i' is being used for the 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++) // <= { .... .... }

Two nested identical cycles, in which the global counter is also reset, look well, very suspiciously. Developers should check out what's going on here.

Abnormal array indexing




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

At once I will say that there seems to be no error here (so far, lol). Consider the following line:

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

The word 'matrix' and double indexing may suggest that the array is two-dimensional, but it is not. This is a regular pointer to an array of integers. But the diagnosis of the V520 did not just appear. Programmers are really confused about how to index two-dimensional arrays.

In this case, the author simply decided to save on one line of code, although he could write this:

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

Another example of code in which I tried for a long time to understand what was happening: D. The isspace () function checks the character with the index m , but if this character is '$', then the character with the index m + 1 is passed to the function. In this case, the comparison with '$' was already in advance. Perhaps there is no error here, but the code can be accurately rewritten more clearly.

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

Here there is a serious mistake. Proper check of row index should be like this:

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

Otherwise, it is possible to access data outside of the MiddleSections vector.

Many more places like this:


How to earn distrust of functions




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

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

Here there is nothing to even comment on. One can only sympathize with the person who clicked on something, clicked, but nothing has changed.

Two more cases of assigning variables to myself will give a list:


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

The function in which the argument is ground immediately upon entering the function may be misleading by the developers using it. The code should be double-checked.

Errors when designing classes




V688 The 'm_qsrc' function argument. 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); .... };

Immediately 3 functions of the class contain arguments whose names coincide with the field of the class. This can lead to errors in the function bodies: the programmer may think that he is working with a member of a class, actually changing the value of a local variable.

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

One of the constructors does not work properly with the m_BitSet variable. The fact is that the variable is uninitialized. Its “garbage” value is used at the first iteration of the loop, after which initialization occurs. This is a very serious mistake, leading to an undefined program behavior.

V603 The object was not used. If you wish to call the 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); } .... };

A long time ago I did not encounter such errors when checking projects. But the problem is still relevant. The mistake is that calling a parameterized constructor in this way results in the creation and deletion of a temporary object. And class fields remain uninitialized. Call another constructor via the initialization list (see 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); }

The analyzer considers that there is not enough line in the overloaded operator:

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

On this fragment of the code 3 warnings of the analyzer are issued immediately. Class fields are initialized not in the order in which they are listed in the initialization list, but in how they are declared in the class. The classic reason for the error is that not all programmers remember or know about this rule. Here and in the initialization list is just the wrong order. One gets the feeling that the list of fields was entered in a random order.

V746 Object slicing. 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); .... }

Intercepting exceptions by value can lead to the loss of some information about the exception due to the creation of a new object. It is much better and safer to catch an exception by reference.

Similar places:


About unreachable code and other code execution issues



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

The code of the conditional operator is written in such a way that absolutely all branches of the code end with the continue operator. This led to the fact that in a while loop there were several lines of unreachable code. These lines look very suspicious. Most likely, such a problem arose after the code was refactored, and now a careful code-review is required here.

V519 The 'interval interval' 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; .... } .... }

The variable interval_width is overwritten several times, because There are no break statements in the case branches. Although a classic, but very bad mistake.

A few more suspicious places:


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

Obviously, the else branch requires rewriting. I have a few ideas on what they wanted to do with the m_QueryOpts-> filtering_options pointer, but the code is still some kind of tangled. I appeal to the authors of the code.

Well, the problem does not come alone:


Read errors



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

Characters to be compared with EOF should not be stored in variables of type char . Otherwise, there is a risk that a character with a value of 0xFF (255) becomes -1 and will be interpreted in the same way as the end of file (EOF).Also (just in case) it is worth checking the implementation of the readfunc function .

V663 Infinite loop is possible. The 'cin.eof ()' condition is not a loop 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()); } } }

The analyzer has detected a potential error due to which an infinite loop may occur. In case of a failure in reading the data, the call to the eof () function will always return false . To complete the loop in this case, additional verification of the value returned by the function fail () is necessary .

Mistakes



V502 Perhaps the '?:' Operator was different. The '?:' Operator has a 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 :-/*/; } .... }

Pay attention to the expression:

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


It is not calculated as the programmer expected, because the whole expression looks like this:

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


The priority of the && operator is higher than that of ?: . For this reason, the code is not executed as intended.

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

Due to the fact that the programmer declared the new variable seq inside the try / catch section, the other variable seq remains uninitialized and is used below by code.

V562 It's a bit to compare 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 }

Nothing foreshadowed trouble, but WIFEXITED turned out to be a macro, opening like this:

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

It turns out that the function returns the opposite value.

The code found another such function, for which a warning was issued:


V595 The 'dst_len' pointer was used 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; } .... }

The dst_len pointer is dereferenced at the very beginning of the function, while further on the code is checked for equality to zero. There is an error in the code that leads to undefined behavior if the dst_len pointer turns out to be nullptr .

V590 Consider inspecting the 'ch! =' \ 0 '&& ch ==' '' expression. The expression is misprint. cleanup_utils.cpp 580

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

The condition for stopping the loop depends only on whether the character ch is a space or not. The expression can be simplified to this:

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

Conclusion


The use of computer programs in scientific research helps and will help to make discoveries. Let's hope that the most important of them will not be missed because of any typos.

I invite the developers of the NCBI Genome Workbench project to contact us, and we will provide the full report issued by the PVS-Studio analyzer.

I hope that this small code study will help to correct many errors and, in general, improve the reliability of the project. Try running PVS-Studio on the code of your projects, if you haven’t done so yet. You might like it :).



If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. NCBI Genome Workbench: Scientific Research under Threat

Source: https://habr.com/ru/post/430476/

More articles:


All Articles