Geekly articles weekly

NCBI Genome Workbench: Endangered Research

Technologies informatiques modernes, solutions techniques et logicielles - tout cela facilite et accélère considérablement la mise en œuvre de diverses recherches scientifiques. Souvent, la simulation informatique est le seul moyen de tester de nombreuses théories. Le logiciel scientifique a ses propres caractéristiques. Par exemple, ces logiciels sont souvent soumis à des tests très poussés, mais sont peu documentés. Cependant, les logiciels sont écrits par des gens et les gens font des erreurs. Des erreurs dans les programmes scientifiques peuvent mettre en doute toute la recherche. Cet article répertorie des dizaines de problèmes trouvés dans le code du progiciel NCBI Genome Workbench.

Présentation


Le NCBI Genome Workbench offre aux chercheurs une large gamme d'outils pour étudier et analyser les données génétiques. Les utilisateurs peuvent rechercher et comparer des données provenant de plusieurs sources, y compris les bases de données NCBI (National Center for Biotechnology Information) ou leurs propres données personnelles.

Comme mentionné précédemment, les logiciels scientifiques sont généralement bien couverts par les tests unitaires. Lors de la vérification de ce projet, 85 répertoires contenant des fichiers de test ont été exclus de l'analyse. C'est environ un millier de fichiers. Cela est peut-être dû aux exigences pour tester divers algorithmes complexes qui sont inventés pour diverses études. Mais la qualité du reste du code (pas celui du test) n'est pas à un niveau aussi élevé que nous le souhaiterions. Cependant, comme dans tout projet dans lequel ils n'ont pas encore pris en charge l'introduction d'outils d'analyse de code statique :).

Les données pour la révision (ou même la recherche) du code ont été fournies par l'analyseur de code statique pour C / C ++ / C # / Java - PVS-Studio .

Juste deux chiffres qui ruinent votre projet




Sur la base de notre base de données d'erreurs, qui représente actuellement plus de 12 000 exemples sélectionnés, nous remarquons et décrivons des modèles spécifiques d'écriture de code qui conduisent à de nombreuses erreurs. Par exemple, nous avons mené les études suivantes:

  1. L'effet de la dernière ligne ;
  2. La fonction la plus dangereuse du monde du C / C ++ ;
  3. Expressions logiques en C / C ++. À quel point les professionnels ont-ils tort ?
  4. Le mal vit dans des fonctions de comparaison .

Ce projet a marqué le début de la description du nouveau modèle. Nous parlons des nombres 1 et 2 dans les noms des variables, par exemple, fichier1 et fichier2 , etc. Il est très facile de confondre deux de ces variables. Il s'agit d'un cas particulier de fautes de frappe dans le code, mais une telle erreur entraîne le désir de travailler avec des variables du même nom, qui ne diffèrent que par les chiffres 1 et 2 à la fin du nom.

En regardant un peu plus loin, je dirai que toutes les études énumérées ci-dessus ont été confirmées dans le code de ce projet: D.

Prenons le premier exemple du projet Genome Workbench:

V501 Il existe des sous-expressions identiques '(! Loc1.IsInt () &&! Loc1.IsWhole ())' à gauche et à droite de '||' opérateur. 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"); } .... }

Nous voyons deux variables nommées loc1 et loc2 . Et aussi une erreur dans le code: la variable loc2 n'est pas utilisée, car au lieu de cela loc1 est utilisé à nouveau.

Un autre exemple:

V560 Une partie de l'expression conditionnelle est toujours fausse: 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 { ..... }

La première ligne de code a mélangé les variables s1 et s2 . Basé sur le nom, il s'agit d'une fonction de comparaison. Mais une telle erreur peut être n'importe où, car en nommant les variables Numéro 1 et Numéro 2 , le programmeur fera certainement une erreur à l'avenir. Et plus il y a d'utilisations de ces noms dans une fonction, plus la probabilité d'erreur est élevée.

Autres fautes de frappe et copier-coller




V501 Il existe des sous-expressions identiques à gauche et à droite de l'opérateur '! =': 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; } } .... }

Je crois qu'après toutes les vérifications, les tailles des tableaux de bits des objets bd.bit_ et ib_db.bit_ sont égales. Par conséquent, l'auteur du code a écrit un cycle pour la comparaison élémentaire des tableaux de bits , mais a fait une faute de frappe au nom de l'un des objets comparés. Par conséquent, les objets comparés peuvent être considérés à tort comme égaux dans certaines situations.

Cet exemple est digne de l'article «Le mal vit dans les fonctions de comparaison ».

V501 Il existe des sous-expressions identiques «CFieldHandler :: QualifierNamesAreEquivalent (field, kFieldTypeSeqId)» à gauche et à droite de «||» opérateur. 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; } }

Très probablement, l'un des contrôles est superflu. Je n'ai pas trouvé dans les variables de code similaires à kFieldTypeSeqId . Néanmoins, un appel de fonction supplémentaire est possible en raison de l'opérateur "||", ce qui dégrade les performances.

Quelques autres endroits du même type avec un avertissement d'analyseur, nécessitant une vérification:

  • V501 Il existe des sous-expressions identiques «uf-> GetData (). IsBool ()» à gauche et à droite de l'opérateur «&&». variation_utils.cpp 1711
  • V501 Il existe des sous-expressions identiques «uf-> GetData (). IsBool ()» à gauche et à droite de l'opérateur «&&». variation_utils.cpp 1735

V766 Un élément avec la même clé 'kArgRemote' a déjà été ajouté. 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"); .... }

L'analyseur a détecté l'ajout de 2 valeurs identiques dans le récipient défini . Rappelez-vous que ce conteneur ne stocke que des valeurs uniques, donc les doublons n'y sont pas ajoutés.

Un code comme celui ci-dessus est souvent écrit à l'aide de la méthode copier-coller. Il peut simplement y avoir une valeur supplémentaire, ou peut-être que l'auteur a oublié de renommer l'une des variables lors de la copie. Lorsque vous supprimez un appel supplémentaire à insérer, le code est légèrement optimisé, ce qui n'est cependant pas significatif. Plus important encore, une erreur grave peut être cachée ici en raison d'un élément manquant dans l'ensemble.

V523 L' instruction 'then' est équivalente au fragment de code suivant. 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; }

La fonction contient de gros fragments de code complètement identiques. Cependant, ils contiennent divers commentaires d'accompagnement. Le code n'est pas écrit de manière optimale, source de confusion, et peut contenir une erreur.

La liste complète des endroits suspects avec l'instruction if-else ressemble à ceci:

  • V523 L'instruction 'then' est équivalente à l'instruction 'else'. blk.c 2142
  • V523 L'instruction 'then' est équivalente au fragment de code suivant. odbc.c 379
  • V523 L'instruction 'then' est équivalente au fragment de code suivant. odbc.c 1414
  • V523 L'instruction 'then' est équivalente à l'instruction 'else'. seqdbvol.cpp 1922
  • V523 L'instruction 'then' est équivalente à l'instruction 'else'. seqdb_demo.cpp 466
  • V523 L'instruction 'then' est équivalente au fragment de code suivant. blast_engine.c 1917
  • V523 L'instruction 'then' est équivalente à l'instruction 'else'. blast_filter.c 420
  • V523 L'instruction 'then' est équivalente à l'instruction 'else'. blast_parameters.c 636
  • V523 L'instruction 'then' est équivalente à l'instruction 'else'. unordered_spliter.cpp 684
  • V523 L'instruction 'then' est équivalente à l'instruction 'else'. bme.cpp 333
  • V523 L'instruction 'then' est équivalente à l'instruction 'else'. gme.cpp 484

/ * avec la sécurité est préférable d'être pédant * /



V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'passwd_buf'. La fonction memset_s () doit être utilisée pour effacer les données privées. 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 { .... } }

Comme vous l'avez probablement déjà deviné, un commentaire amusant sur la sécurité du code a été utilisé dans le titre de la section.

En bref, la fonction memset sera supprimée par le compilateur, car les tampons vidés ne sont plus utilisés. Et les données comme hash ou passwd_buf ne seront pas réellement des zéros. Pour plus d'informations sur ce mécanisme de compilation non évident, consultez l'article « Effacement sécurisé des données privées ».

V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'answer'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 561

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

Ce n'était pas le seul exemple avec des commentaires sur la «sécurité». À en juger par les commentaires, on peut supposer que la sécurité est vraiment importante pour le projet. Par conséquent, je joins toute la liste non petite des problèmes identifiés:

  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'heap'. La fonction memset_s () doit être utilisée pour effacer les données privées. ncbi_heapmgr.c 1300
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'context'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 167
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'ks'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 339
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'md5_ctx'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 353
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'hash'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 365
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'ks'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 406
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'ntlm_v2_response'. La fonction memset_s () doit être utilisée pour effacer les données privées. login.c 795
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'answer'. La fonction memset_s () doit être utilisée pour effacer les données privées. login.c 801
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'packet'. La fonction memset_s () doit être utilisée pour effacer les données privées. numeric.c 256
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'packet'. La fonction memset_s () doit être utilisée pour effacer les données privées. numeric.c 110
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'pwd'. La fonction memset_s () doit être utilisée pour effacer les données privées. getpassarg.c 50
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'context'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 188
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'buf'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 243
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'ntlm_v2_hash'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 309
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'md5_ctx'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 354
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'passwd_buf'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 380
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'ks'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 393
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'hash'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 394
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'ntlm2_challenge'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 395
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'ks'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 419
  • V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider l'objet 'ntlm_v2_response'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 556

Cycles suspects




V534 Il est probable qu'une mauvaise variable soit comparée à l'intérieur de l'opérateur 'for'. Pensez à revoir «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; } .... } .... }

Je pense que, dans l'état de la boucle intérieure, la variable je l' ai obtenue au hasard. Au lieu de cela, la variable j doit être utilisée.

V535 La variable 'i' est utilisée pour cette boucle et pour la boucle externe. Vérifiez les lignes: 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++) // <= { .... .... }

Deux cycles identiques imbriqués, dans lesquels le compteur global est également réinitialisé, semblent très suspects. Les développeurs doivent vérifier ce qui se passe ici.

Indexation de tableau anormale




V520 L'opérateur virgule ',' dans l'expression d'index du tableau '[- 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); } .... }

Je dois dire tout de suite qu'il ne semble pas y avoir d'erreur (pour l'instant, lol). Considérez la ligne suivante:

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

Le mot «matrice» et la double indexation peuvent suggérer que le tableau est bidimensionnel, mais ce n'est pas le cas. Il s'agit d'un pointeur régulier vers un tableau d'entiers. Mais les diagnostics du V520 ne sont pas apparus. Les programmeurs sont vraiment confus quant à la façon d'indexer les tableaux bidimensionnels.

Dans ce cas, l'auteur a simplement décidé d'économiser sur une seule ligne de code, bien qu'il puisse écrire comme ceci:

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

V661 Une expression suspecte 'A [B == C]'. Signifiait probablement «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*/; } .... }

Un autre exemple de code dans lequel j'ai passé beaucoup de temps à essayer de comprendre ce qui se passait: D. La fonction isspace () vérifie le caractère avec l'index m , mais si ce caractère est '$', alors le caractère avec l'index m + 1 est passé à la fonction. De plus, la comparaison avec «$» était déjà en avance. Il n'y a peut-être pas d'erreur ici, mais le code peut certainement être réécrit plus clairement.

V557 Le dépassement de matrice est possible. L'index «ligne» pointe au-delà de la limite du tableau. 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) { .... } .... }

Ici, il y a une grave erreur. La vérification d'index de ligne correcte doit être comme ceci:

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

Sinon, il est possible d'accéder aux données en dehors du vecteur MiddleSections .

Beaucoup plus de tels endroits:

  • V557 Le dépassement de matrice est possible. L'index «i» pointe au-delà de la limite du tableau. resource_pool.hpp 388
  • V557 Le dépassement de matrice est possible. L'index «ligne» pointe au-delà de la limite du tableau. aln_reader.cpp 418
  • V557 Le dépassement de matrice est possible. L'index 'fmt_idx' pointe au-delà de la limite du tableau. seq_writer.cpp 384
  • V557 Le dépassement de matrice est possible. L'index 'fmt_idx' pointe au-delà de la limite du tableau. blastdb_formatter.cpp 183
  • V557 Le dépassement de matrice est possible. L'index 'num' pointe au-delà de la limite du tableau. newcleanupp.cpp 13035

Comment se méfier des fonctions




V570 La variable 'm_onClickFunction' est assignée à elle-même. alngraphic.hpp 103

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

Il n'y a même rien à commenter. Vous ne pouvez sympathiser qu'avec la personne qui a cliqué sur quelque chose, cliqué, mais rien n'a changé.

Deux autres cas d'attribution de variables à moi-même résulteront en une liste:

  • V570 La variable 'iter-> level' est assignée à elle-même. align_format_util.cpp 189
  • V570 La variable 'd_elements_values ​​[ind]' est assignée à elle-même. sls_alp_data.cpp 1416

V763 Le paramètre 'w1' est toujours réécrit dans le corps de la fonction avant d'être utilisé. 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; } };

Une fonction dans laquelle l'argument est effiloché dès son entrée dans la fonction peut être trompeuse pour les développeurs qui l'utilisent. Le code doit être revérifié.

Erreurs de conception de classe




V688 L'argument de la fonction 'm_qsrc' possède le même nom que l'un des membres de la classe, ce qui peut entraîner une 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); .... };

Immédiatement, 3 fonctions de classe contiennent des arguments dont les noms coïncident avec le champ de classe. Cela peut entraîner des erreurs dans les corps de fonction: le programmeur peut penser qu'il travaille avec un membre de la classe, changeant en fait la valeur de la variable locale.

V614 Variable non initialisée 'm_BitSet' utilisée. 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); }

L'un des constructeurs fonctionne de manière bâclée avec la variable m_BitSet . Le fait est que la variable n'est pas initialisée. Sa valeur «garbage» est utilisée à la première itération de la boucle, après quoi l'initialisation a lieu. Il s'agit d'une erreur très grave, conduisant à un comportement de programme indéfini.

V603 L'objet a été créé mais il n'est pas utilisé. Si vous souhaitez appeler le constructeur, 'this-> SIntervalComparisonResult :: SIntervalComparisonResult (....)' doit être utilisé. 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); } .... };

Il y a très longtemps, je n'ai pas rencontré de telles erreurs lors de la vérification des projets. Mais le problème est toujours d'actualité. L'erreur est que l'appel du constructeur paramétré de cette manière crée et supprime un objet temporaire. Et les champs de classe restent non initialisés. Un autre constructeur doit être appelé via la liste d'initialisation (voir Délégation de constructeur ).

V591 La fonction non vide doit renvoyer une valeur. bio_tree.hpp 266

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

L'analyseur considère que la ligne est manquante dans l'instruction surchargée:

 return *this;

V670 Le membre de classe non initialisé 'm_OutBlobIdOrData' est utilisé pour initialiser le membre 'm_StdOut'. N'oubliez pas que les membres sont initialisés dans l'ordre de leurs déclarations à l'intérieur d'une classe. 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 avertissements de l'analyseur sont immédiatement émis pour ce fragment de code. Les champs de classe sont initialisés non dans l'ordre dans lequel ils sont répertoriés dans la liste d'initialisation, mais dans la manière dont ils sont déclarés dans la classe. La cause classique de l'erreur est que tous les programmeurs ne se souviennent pas ou ne connaissent pas cette règle. Ici et dans la liste d'initialisation est juste le mauvais ordre. On a l'impression que la liste des champs a été saisie dans un ordre aléatoire.

V746 Découpe d' objets. Une exception doit être interceptée par référence plutôt que par valeur. 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); .... }

La capture d'exceptions par valeur peut entraîner la perte de certaines informations sur l'exception en raison de la création d'un nouvel objet. Il est beaucoup mieux et plus sûr d'attraper une exception par référence.

Lieux similaires:

  • V746 Découpe d'objets. Une exception doit être interceptée par référence plutôt que par valeur. agp_validate_reader.cpp 562
  • V746 Découpe d'objets. Une exception doit être interceptée par référence plutôt que par valeur. aln_build_app.cpp 320
  • V746 Découpe d'objets. Une exception doit être interceptée par référence plutôt que par valeur. aln_test_app.cpp 458
  • V746 Découpe d'objets. Une exception doit être interceptée par référence plutôt que par valeur. cobalt.cpp 691
  • V746 Découpe d'objets. Une exception doit être interceptée par référence plutôt que par valeur. cobalt.cpp 719
  • V746 Découpe d'objets. Une exception doit être interceptée par référence plutôt que par valeur. cobalt.cpp 728
  • V746 Découpe d'objets. Une exception doit être interceptée par référence plutôt que par valeur. cobalt.cpp 732

À propos du code inaccessible et d'autres problèmes d'exécution de code



V779 Code inaccessible détecté. Il est possible qu'une erreur soit présente. 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; }

Le code de l'instruction conditionnelle est écrit de manière à ce que toutes les branches du code se terminent par l'instruction continue . Cela a conduit à la formation de plusieurs lignes de code inaccessible dans la boucle while . Ces lignes semblent très suspectes. Très probablement, ce problème est survenu après la refactorisation du code, et maintenant il nécessite une révision minutieuse du code.

V519 La variable 'interval_width' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 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; .... } .... }

La variable interval_width est écrasée plusieurs fois, car il n'y a aucune instruction break dans les branches de cas . Bien qu'un classique, mais une très mauvaise erreur.

Quelques endroits plus suspects:

  • V779 Code inaccessible détecté. Il est possible qu'une erreur soit présente. dbapi_driver_utils.cpp 351
  • V779 Code inaccessible détecté. Il est possible qu'une erreur soit présente. net.c 780
  • V779 Code inaccessible détecté. Il est possible qu'une erreur soit présente. bcp.c 1495
  • V779 Code inaccessible détecté. Il est possible qu'une erreur soit présente. remote_blast.cpp 1470
  • V779 Code inaccessible détecté. Il est possible qu'une erreur soit présente. remote_blast.cpp 1522

V571 Contrôle récurrent. La condition 'if (m_QueryOpts-> filtering_options)' a déjà été vérifiée à la ligne 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; } .... }

De toute évidence, la branche else nécessite une réécriture. J'ai quelques idées que je voulais faire avec le pointeur m_QueryOpts-> filtering_options , mais le code est toujours confus. J'en appelle aux auteurs du code.

Eh bien, le problème ne vient pas seul:

  • V571 Contrôle récurrent. La condition «if (sleep time)» a déjà été vérifiée à la ligne 205. request_control.cpp 208
  • V571 Contrôle récurrent. La condition «if (assignValue.empty ())» a déjà été vérifiée à la ligne 712. classstr.cpp 718

Erreurs de lecture des données



V739 EOF ne doit pas être comparé à une valeur de type 'char'. La 'linestring [0]' doit être de type 'int'. alnread.c 3509

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

Les caractères que vous prévoyez de comparer avec EOF ne doivent pas être stockés dans des variables char . Sinon, il existe un risque qu'un caractère avec la valeur 0xFF (255) se transforme en -1 et soit interprété de la même manière que la fin d'un fichier (EOF). Aussi (juste au cas où), il vaut la peine de vérifier l'implémentation de la fonction readfunc .

La boucle infinie V663 est possible. La condition «cin.eof ()» est insuffisante pour rompre la boucle.Pensez à ajouter l'appel de fonction 'cin.fail ()' à l'expression conditionnelle. 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()); } } }

L'analyseur a détecté une erreur potentielle à cause de laquelle une boucle infinie peut se produire. Si une défaillance se produit lors de la lecture des données, l'appel de la fonction eof () renvoie toujours false . Pour terminer la boucle dans ce cas, une vérification supplémentaire de la valeur retournée par la fonction fail () est nécessaire .

Erreurs diverses



V502 L'opérateur '?:' Fonctionne peut-être d'une manière différente de celle attendue. L'opérateur '?:' A une priorité plus faible que l'opérateur '&&'. 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 :-/*/; } .... }

Faites attention à l'expression:

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


Il n'est pas calculé comme prévu par le programmeur, car l'expression entière ressemble à ceci:

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


La priorité de l'opérateur && est supérieure à ?: . Pour cette raison, le code ne s'exécute pas comme prévu.

V561 Il vaut probablement mieux attribuer une valeur à la variable 'seq' que de la déclarer à nouveau. Déclaration précédente: validator.cpp, ligne 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); }

Étant donné que le programmeur a déclaré une nouvelle variable seq dans la section try / catch, une autre variable seq reste non initialisée et est utilisée ci-dessous dans le code.

V562 Il est étrange de comparer une valeur de type booléen avec une valeur de 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 }

Rien de mauvais augure, mais WIFEXITED s'est avéré être une macro ouverture de cette façon:

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

Il s'avère que la fonction renvoie la valeur opposée.

Dans le code, il y avait une autre fonction de ce type, qui a émis un avertissement:

  • V562 Il est étrange de comparer une valeur de type booléen avec une valeur de 0. ncbi_process.cpp 126

V595 Le pointeur 'dst_len' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 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; } .... }

Le pointeur dst_len est déréférencé au tout début de la fonction, tandis que plus loin dans le code est vérifié l'égalité à zéro. Une erreur a été commise dans le code qui conduit à un comportement non défini si le pointeur dst_len est nullptr .

V590 Envisagez d'inspecter l' expression 'ch! =' \ 0 '&& ch ==' ''. L'expression est excessive ou contient une erreur d'impression. cleanup_utils.cpp 580

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

La condition d'arrêt de la boucle ne dépend que de si le caractère ch est un espace ou non. L'expression peut être simplifiée comme suit:

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

Conclusion


L'utilisation de programmes informatiques dans la recherche scientifique aide et contribuera à faire des découvertes. Espérons que ceux qui sont particulièrement importants ne seront pas manqués à cause d'une faute de frappe.

J'invite les développeurs du projet NCBI Genome Workbench à nous contacter et nous fournirons un rapport complet publié par l'analyseur PVS-Studio.

J'espère que cette petite recherche de code permet de corriger de nombreux bugs et d'améliorer généralement la fiabilité du projet. Essayez d'exécuter PVS-Studio sur le code de vos projets, si vous ne l'avez pas déjà fait. Vous pouvez l'aimer :).



Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Svyatoslav Razmyslov. NCBI Genome Workbench: la recherche scientifique menacée

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

More articles:


All Articles