introduction
Nous avons recherché à plusieurs reprises des bogues dans des projets utilisant l'apprentissage automatique, et DeepSpeech n'a pas fait exception pour nous. Pas étonnant, car ce projet est assez populaire: au moment d'écrire ces lignes, il compte déjà plus de 15k étoiles sur GitHub.
Comme d'habitude, la recherche d'erreurs que je citerai dans cet article a été réalisée à l'aide de l'analyseur de code statique PVS-Studio.
Pour son travail, DeepSpeech utilise la bibliothèque TensorFlow. J'ai désactivé l'analyse du code de cette bibliothèque, car nous avons déjà écrit un article séparé à ce sujet.cependant, je n'ai pas désactivé l'analyse du reste des bibliothèques utilisées. Quelle est la raison pour ça? Les erreurs dans une bibliothèque que vous incluez dans votre projet deviennent également des erreurs dans votre projet. Par conséquent, il est utile d'analyser non seulement votre code, mais également tout code tiers que vous utilisez. Vous pouvez lire un avis détaillé à ce sujet dans notre récent article .
Ceci conclut la brève introduction - il est temps de passer à l'analyse des erreurs. D'ailleurs, si vous êtes venu ici pour trouver la réponse à la question que j'ai posée dans le titre de l'article (pourquoi vous ne devriez pas écrire dans namespace std), vous pouvez immédiatement regarder à la fin de l'article. Un exemple particulièrement intéressant vous y attend!
Un aperçu de 10 avertissements intéressants émis par l'analyseur
Avertissement 1
V773 La fonction a été quittée sans relâcher le pointeur 'data'. Une fuite de mémoire est possible. edit-fst.h 311
// EditFstData method implementations: just the Read method.
template <typename A, typename WrappedFstT, typename MutableFstT>
EditFstData<A, WrappedFstT, MutableFstT> *
EditFstData<A, WrappedFstT, MutableFstT>::Read(std::istream &strm,
const FstReadOptions &opts)
{
auto *data = new EditFstData<A, WrappedFstT, MutableFstT>();
// next read in MutabelFstT machine that stores edits
FstReadOptions edits_opts(opts);
....
std::unique_ptr<MutableFstT> edits(MutableFstT::Read(strm, edits_opts));
if (!edits) return nullptr; // <=
....
}Cet extrait de code contient un exemple classique de fuite de mémoire: la fonction Read appelle ' return nullptr ' sans libérer la mémoire allouée avec l'expression ' new EditFstData '. Avec une telle sortie de la fonction (sans appeler les données de suppression ), seul le pointeur lui-même sera supprimé et le destructeur de l'objet vers lequel il pointe ne sera pas appelé. Ainsi, l'objet continuera à être stocké en mémoire, et il ne sera plus possible de le supprimer ou de l'utiliser.
En plus d'une erreur, ce code contient également une autre pas très bonne pratique: le code d'une fonction utilise simultanément des pointeurs intelligents et ordinaires. Par exemple, si les donnéesétait également un pointeur intelligent, alors une telle erreur ne se produirait pas: si nécessaire, en sortant de la portée, les pointeurs intelligents appellent automatiquement le destructeur de l'objet stocké.
Avertissement 2
V1062 La classe 'DfsState' définit un opérateur 'new' personnalisé. L'opérateur «supprimer» doit également être défini. dfs-visit.h 62
// An FST state's DFS stack state.
template <class FST>
struct DfsState {
public:
....
void *operator new(size_t size,
MemoryPool<DfsState<FST>> *pool) {
return pool->Allocate();
}
....
}PVS-Studio ne s'arrête pas et continue d'ajouter de nouveaux diagnostics. Cet extrait de code est un excellent exemple pour montrer le travail du dernier diagnostic numéroté V1062 .
La règle que ce diagnostic surveille est simple: si vous définissez votre propre opérateur «nouvel», vous devez également définir votre propre opérateur «supprimer». Le contraire fonctionne de la même manière: si vous définissez votre propre «suppression», alors votre propre «nouveau» doit également être défini.
Dans l'exemple ci-dessus, cette règle est violée: l'objet sera créé en utilisant le «nouveau» défini par nous, et supprimé - en utilisant le standard «supprimer». Voyons voir ce que la Allouer fonction de la MemoryPool classe ne ,ce que nos propres `` nouveaux '' appellent:
void *Allocate() {
if (free_list_ == nullptr) {
auto *link = static_cast<Link *>(mem_arena_.Allocate(1));
link->next = nullptr;
return link;
} else {
auto *link = free_list_;
free_list_ = link->next;
return link;
}
}Cette fonction crée un élément et l'ajoute à la liste liée. Il est logique qu'une telle allocation ait dû être écrite dans son propre «nouveau».
Mais attendez une minute! Quelques lignes ci-dessous contiennent la fonction suivante:
void Free(void *ptr) {
if (ptr) {
auto *link = static_cast<Link *>(ptr);
link->next = free_list_;
free_list_ = link;
}
}Cela signifie que nous avons déjà des fonctions prêtes à l'emploi pour l'allocation et la libération. Très probablement, le programmeur a dû écrire son propre opérateur 'delete', en utilisant la fonction Free () pour le libérer .
L'analyseur a détecté au moins trois autres erreurs de ce type:
- V1062 La classe 'VectorState' définit un opérateur 'new' personnalisé. L'opérateur «supprimer» doit également être défini. vector-fst.h 31
- V1062 La classe 'CacheState' définit un opérateur 'new' personnalisé. L'opérateur «supprimer» doit également être défini. cache.h 65
Avertissement 3
V703 Il est étrange que le champ 'first_path' de la classe dérivée 'ShortestPathOptions' écrase le champ de la classe de base 'ShortestDistanceOptions'. Vérifiez les lignes: shortest-path.h: 35, shortest-distance.h: 34. le plus court chemin.h 35
// Base class
template <class Arc, class Queue, class ArcFilter>
struct ShortestDistanceOptions {
Queue *state_queue; // Queue discipline used; owned by caller.
ArcFilter arc_filter; // Arc filter (e.g., limit to only epsilon graph).
StateId source; // If kNoStateId, use the FST's initial state.
float delta; // Determines the degree of convergence required
bool first_path; // For a semiring with the path property (o.w.
// undefined), compute the shortest-distances along
// along the first path to a final state found
// by the algorithm. That path is the shortest-path
// only if the FST has a unique final state (or all
// the final states have the same final weight), the
// queue discipline is shortest-first and all the
// weights in the FST are between One() and Zero()
// according to NaturalLess.
ShortestDistanceOptions(Queue *state_queue, ArcFilter arc_filter,
StateId source = kNoStateId,
float delta = kShortestDelta)
: state_queue(state_queue),
arc_filter(arc_filter),
source(source),
delta(delta),
first_path(false) {}
};
// Derived class
template <class Arc, class Queue, class ArcFilter>
struct ShortestPathOptions
: public ShortestDistanceOptions<Arc, Queue, ArcFilter> {
using StateId = typename Arc::StateId;
using Weight = typename Arc::Weight;
int32 nshortest; // Returns n-shortest paths.
bool unique; // Only returns paths with distinct input strings.
bool has_distance; // Distance vector already contains the
// shortest distance from the initial state.
bool first_path; // Single shortest path stops after finding the first
// path to a final state; that path is the shortest path
// only when:
// (1) using the ShortestFirstQueue with all the weights
// in the FST being between One() and Zero() according to
// NaturalLess or when
// (2) using the NaturalAStarQueue with an admissible
// and consistent estimate.
Weight weight_threshold; // Pruning weight threshold.
StateId state_threshold; // Pruning state threshold.
ShortestPathOptions(Queue *queue, ArcFilter filter, int32 nshortest = 1,
bool unique = false, bool has_distance = false,
float delta = kShortestDelta, bool first_path = false,
Weight weight_threshold = Weight::Zero(),
StateId state_threshold = kNoStateId)
: ShortestDistanceOptions<Arc, Queue, ArcFilter>(queue, filter,
kNoStateId, delta),
nshortest(nshortest),
unique(unique),
has_distance(has_distance),
first_path(first_path),
weight_threshold(std::move(weight_threshold)),
state_threshold(state_threshold) {}
};D'accord, ce n'est pas si facile de trouver une erreur potentielle, non?
Le problème réside dans le fait que la classe de base et la classe dérivée contiennent des champs portant le même nom: first_path . Cela entraînera la classe dérivée d'avoir son propre champ différent, qui remplace le champ de la classe de base par son nom. De telles erreurs peuvent conduire à une grave confusion.
Pour mieux comprendre ce que je veux dire, je propose de considérer un court exemple synthétique tiré de notre documentation. Disons que nous avons le code suivant:
class U {
public:
int x;
};
class V : public U {
public:
int x; // <= V703 here
int z;
};Ici, le nom x est remplacé dans la classe dérivée. Maintenant, la question est: quelle valeur le code suivant imprimera-t-il?
int main() {
V vClass;
vClass.x = 1;
U *uClassPtr = &vClass;
std::cout << uClassPtr->x << std::endl;
....
}Si vous pensez qu'une valeur non définie sera sortie, alors vous avez raison. Dans cet exemple, l'unité sera écrite dans le champ de la classe dérivée, mais la lecture se fera à partir du champ de la classe de base, qui au moment de la sortie n'est toujours pas définie.
Le chevauchement des noms dans la hiérarchie des classes est une erreur potentielle et doit être évité :)
Avertissement 4
V1004 Le pointeur 'aiter' a été utilisé de manière non sécurisée après avoir été vérifié par rapport à nullptr. Vérifiez les lignes: 107, 119. visit.h 119
template <....>
void Visit(....)
{
....
// Deletes arc iterator if done.
auto *aiter = arc_iterator[state];
if ((aiter && aiter->Done()) || !visit) {
Destroy(aiter, &aiter_pool);
arc_iterator[state] = nullptr;
state_status[state] |= kArcIterDone;
}
// Dequeues state and marks black if done.
if (state_status[state] & kArcIterDone) {
queue->Dequeue();
visitor->FinishState(state);
state_status[state] = kBlackState;
continue;
}
const auto &arc = aiter->Value(); // <=
....
}Le pointeur aiter est utilisé une fois qu'il a été vérifié pour nullptr . L'analyseur fait une hypothèse: si un pointeur est vérifié pour nullptr , alors pendant la vérification, il peut avoir une telle valeur.
Dans ce cas, voyons ce qui arrive à aiter s'il est vraiment égal à zéro. Tout d'abord, ce pointeur sera vérifié dans l' instruction ' if ((aiter && aiter-> Done ()) ||! Visit) '. Cette condition sera fausse , et nous n'entrerons pas dans la branche alors de ceci si . Et puis, selon tous les canons des erreurs classiques, un pointeur nul sera déréférencé: ' aiter-> Value ();». Ce déréférencement entraîne un comportement indéfini.
Avertissement 5
L'exemple suivant contient deux erreurs à la fois:
- V595 Le pointeur 'istrm' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 60, 61. mapped-file.cc 60
- V595 Le pointeur 'istrm' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 39, 61. mapped-file.cc 39
MappedFile *MappedFile::Map(std::istream *istrm, bool memorymap,
const string &source, size_t size) {
const auto spos = istrm->tellg(); // <=
....
istrm->seekg(pos + size, std::ios::beg); // <=
if (istrm) { // <=
VLOG(1) << "mmap'ed region of " << size
<< " at offset " << pos
<< " from " << source
<< " to addr " << map;
return mmf.release();
}
....
}L'erreur trouvée ici est plus claire que l'erreur de l'exemple précédent. Le pointeur istrm est d' abord déréférencé (deux fois), et seulement après cela, une vérification du zéro et une journalisation des erreurs suivent. Cela indique clairement: si un pointeur nul arrive sur cette fonction en tant que istrm , un comportement indéfini (ou, plus probablement, un plantage du programme) se produira sans aucune journalisation. Désordre ... des bugs comme celui-ci ne doivent pas être négligés.
Avertissement 6
V730 Tous les membres d'une classe ne sont pas initialisés à l'intérieur du constructeur. Pensez à inspecter: stones_written_. ersatz_progress.cc 14
ErsatzProgress::ErsatzProgress()
: current_(0)
, next_(std::numeric_limits<uint64_t>::max())
, complete_(next_)
, out_(NULL)
{}L'analyseur nous avertit que le constructeur n'initialise pas tous les champs de la structure ErzatzProgress . Comparons ce constructeur avec la liste des champs de cette structure:
class ErsatzProgress {
....
private:
void Milestone();
uint64_t current_, next_, complete_;
unsigned char stones_written_;
std::ostream *out_;
};En effet, vous pouvez voir que le constructeur initialise tous les champs sauf stones_written_ .
Remarque : cet exemple ne peut pas être une erreur. La véritable erreur ne se produira que lorsque la valeur d'un champ non initialisé est utilisée .
Cependant, les diagnostics V730 vous aident à déboguer de tels cas d'utilisation à l'avance. Après tout, une question naturelle se pose: si le programmeur a décidé d'initialiser spécifiquement tous les champs de la classe, alors pourquoi aurait-il une raison de laisser un champ sans valeur?
Ma supposition que le champ stones_written_ n'a pas été initialisé par erreur a été confirmée lorsque j'ai vu un autre constructeur quelques lignes ci-dessous:
ErsatzProgress::ErsatzProgress(uint64_t complete,
std::ostream *to,
const std::string &message)
: current_(0)
, next_(complete / kWidth)
, complete_(complete)
, stones_written_(0)
, out_(to)
{
....
}Ici tous les champs de la classe sont initialisés, ce qui confirme: le programmeur a vraiment prévu d'initialiser tous les champs, mais a accidentellement oublié une chose.
Avertissement 7
V780 L'objet '& params' de type non passif (non PDS) ne peut pas être initialisé à l'aide de la fonction memset. binary_format.cc 261
/* Not the best numbering system,
but it grew this way for historical reasons
* and I want to preserve existing binary files. */
typedef enum
{
PROBING=0,
REST_PROBING=1,
TRIE=2,
QUANT_TRIE=3,
ARRAY_TRIE=4,
QUANT_ARRAY_TRIE=5
}
ModelType;
....
struct FixedWidthParameters {
unsigned char order;
float probing_multiplier;
// What type of model is this?
ModelType model_type;
// Does the end of the file
// have the actual strings in the vocabulary?
bool has_vocabulary;
unsigned int search_version;
};
....
// Parameters stored in the header of a binary file.
struct Parameters {
FixedWidthParameters fixed;
std::vector<uint64_t> counts;
};
....
void BinaryFormat::FinishFile(....)
{
....
// header and vocab share the same mmap.
Parameters params = Parameters();
memset(¶ms, 0, sizeof(Parameters)); // <=
....
}Pour comprendre cet avertissement, je vous suggère de comprendre d'abord ce qu'est un type PDS. PDS signifie Passive Data Structure, une structure de données simple. Parfois, au lieu de "PDS", ils disent "POD" - "Plain Old Data". En termes simples (je cite le Wikipedia russe ), le type PDS est un type de données qui a un emplacement défini de manière rigide des champs en mémoire, ce qui ne nécessite pas de restrictions d'accès et de contrôle automatique. En termes simples, il s'agit d'un type de données composé uniquement de types intégrés.
Une caractéristique distinctive des types POD est que les variables de ces types peuvent être modifiées et traitées à l'aide de fonctions de gestion de mémoire primitives (memset, memcpy, etc.). Cependant, cela ne peut pas être dit à propos des types "non PDS": une telle gestion de bas niveau de leurs valeurs peut conduire à de graves erreurs. Par exemple, des fuites de mémoire, un double vidage de la même ressource ou un comportement non défini.
PVS-Studio émet un avertissement pour le code ci-dessus: vous ne pouvez pas gérer une structure de type Paramètres de cette manière. Si vous regardez la définition de cette structure, vous pouvez voir que son deuxième membre est de type std :: vector... Ce type utilise activement la gestion automatique de la mémoire et, en plus des données de contenu, stocke des variables de service supplémentaires. La remise à zéro d'un tel champ à l'aide de memset peut casser la logique de la classe et constitue une grave erreur.
Avertissement 8
V575 Le pointeur nul potentiel est passé dans la fonction 'memcpy'. Inspectez le premier argument. Vérifier les lignes: 73, 68.modelstate.cc 73
Metadata*
ModelState::decode_metadata(const DecoderState& state,
size_t num_results)
{
....
Metadata* ret = (Metadata*)malloc(sizeof(Metadata));
....
memcpy(ret, &metadata, sizeof(Metadata));
return ret;
}L'avertissement suivant nous indique qu'un pointeur nul est passé à la fonction memcpy . Oui, en effet, si la fonction malloc ne parvient pas à allouer de la mémoire, elle retournera NULL . Dans ce cas, ce pointeur sera passé à la fonction memset , où il sera déréférencé - et, par conséquent, un plantage du programme enchanteur.
Cependant, certains de nos lecteurs peuvent être indignés: si la mémoire est tellement débordée / fragmentée que malloc ne peut pas allouer de mémoire, est-ce vraiment important ce qui se passe ensuite? Le programme plantera de toute façon, car en raison du manque de mémoire, il ne pourra pas fonctionner normalement.
Nous avons rencontré à plusieurs reprises cette opinion et pensons qu'elle est incorrecte. Je vous expliquerais en détail pourquoi il en est vraiment ainsi, mais ce sujet mérite un article séparé. Tellement méritant que nous l'avons écrit il y a quelques années :) Si vous vous demandez pourquoi vous devriez toujours vérifier un pointeur renvoyé par les fonctions de malloc , alors je vous invite à lire: Pourquoi est-il important de vérifier ce que malloc a renvoyé .
Avertissement 9
L'avertissement suivant est causé par les mêmes raisons que le précédent. Certes, cela indique une erreur légèrement différente.
V769Le pointeur 'middle_begin_' dans l'expression 'middle_begin_ + (count.size () - 2)' pourrait être nullptr. Dans ce cas, la valeur résultante sera insensée et ne doit pas être utilisée. Vérifiez les lignes: 553, 552. search_trie.cc 553
template <class Quant, class Bhiksha> class TrieSearch {
....
private:
....
Middle *middle_begin_, *middle_end_;
....
};
template <class Quant, class Bhiksha>
uint8_t *TrieSearch<Quant, Bhiksha>::SetupMemory(....)
{
....
middle_begin_
= static_cast<Middle*>(malloc(sizeof(Middle) * (counts.size() - 2)));
middle_end_ = middle_begin_ + (counts.size() - 2);
....
}Tout comme dans l'exemple précédent, la mémoire est allouée ici à l'aide de la fonction malloc . Le pointeur renvoyé est utilisé dans l'expression arithmétique sans aucune vérification de nullptr . Hélas, le résultat d'une telle expression n'aura aucun sens, et une valeur complètement inutile sera stockée dans le champ middle_end_ .
Avertissement 10
Et enfin, l'exemple le plus intéressant à mon avis a été trouvé dans la bibliothèque kenlm incluse dans DeepSpeech:
V1061 L'extension de l'espace de noms 'std' peut entraîner un comportement indéfini. size_iterator.hh 210
// Dirty hack because g++ 4.6 at least wants
// to do a bunch of copy operations.
namespace std {
inline void iter_swap(util::SizedIterator first,
util::SizedIterator second)
{
util::swap(*first, *second);
}
} // namespace stdLe truc appelé "sale tour" dans le commentaire est vraiment sale. Le fait est qu'une telle expansion de l' espace de noms std peut conduire à un comportement indéfini.
Pourquoi? Parce que le contenu de l' espace de noms std est déterminé exclusivement par le comité des normes. C'est pourquoi la norme internationale du langage C ++ interdit explicitement d'étendre std de cette manière.
Le dernier standard pris en charge dans g ++ 4.6 est C ++ 03. Voici une citation traduite du projet de travail final de C ++ 03(voir le point 17.6.4.2.1): "Le comportement d'un programme C ++ n'est pas défini s'il ajoute des déclarations ou des définitions à l'espace de noms std ou à l'espace de noms imbriqué std, sauf indication contraire." Cette citation s'applique à toutes les normes ultérieures (C ++ 11, C ++ 14, C ++ 17 et C ++ 20).
Je propose d'examiner comment vous pourriez résoudre le code problématique de notre exemple. La première question logique: quels sont ces «cas pour lesquels le contraire est indiqué»? Il existe plusieurs situations dans lesquelles l'expansion std ne conduit pas à un comportement indéfini. Vous pouvez en savoir plus sur toutes ces situations sur la page de documentation des diagnostics V1061 , mais maintenant il est important pour nous que l'un de ces cas soit l'ajout d'une spécialisation du modèle de fonction.
Parce questd a déjà une fonction appelée iter_swap (note: une fonction template), il est logique de supposer que le programmeur voulait étendre ses capacités pour pouvoir travailler avec le type util :: SizedIterator . Mais voici la malchance: au lieu d'ajouter une spécialisation au modèle de fonction , le programmeur a simplement écrit une surcharge ordinaire . Cela aurait dû être écrit comme ceci:
namespace std {
template <>
inline void iter_swap(util::SizedIterator first,
util::SizedIterator second)
{
util::swap(*first, *second);
}
} // namespace stdCependant, ce code n'est pas non plus si simple. Le fait est que ce code ne sera valide que jusqu'au standard C ++ 20. Oui, il a également noté que les spécialisations des modèles de fonctions conduisaient à un comportement indéfini (voir le projet de travail final de C ++ 20 , section 16.5.4.2.1). Et comme ce code appartient à une bibliothèque, il est probable que tôt ou tard il sera construit avec l' indicateur -std = C ++ 20 . À propos, PVS-Studio distingue la version de la norme utilisée dans le code et, en fonction de cela, émet ou n'émet pas d'avertissement. Voyez par vous-même: exemple pour C ++ 17 , exemple pour C ++ 20 .
En fait, vous pouvez faire beaucoup plus facilement. Pour corriger l'erreur, il vous suffit de transférer votre propre définition de iter_swapdans le même espace de noms qui définit la classe SizedIterator . Dans ce cas, aux endroits où iter_swap est appelé , vous devez ajouter "using std :: iter_swap;". Il s'avère que ceci (la définition de la classe SizedIterator et la fonction util :: swap () ont été modifiées pour plus de simplicité):
namespace util
{
class SizedIterator
{
public:
SizedIterator(int i) : m_data(i) {}
int& operator*()
{
return m_data;
}
private:
int m_data;
};
....
inline void iter_swap(SizedIterator first,
SizedIterator second)
{
std::cout << "we are inside util::iter_swap" << std::endl;
swap(*first, *second);
}
}
int main()
{
double d1 = 1.1, d2 = 2.2;
double *pd1 = &d1, *pd2 = &d2;
util::SizedIterator si1(42), si2(43);
using std::iter_swap;
iter_swap(pd1, pd2);
iter_swap(si1, si2); // "we are inside util::iter_swap"
return 0;
}Maintenant, le compilateur sélectionnera indépendamment la surcharge requise de la fonction iter_swap basée sur une recherche d' arguments (ADL). Pour la classe SizedIterator , la version de namespace util sera appelée , et pour les autres types, la version de namespace std sera appelée . La preuve est sur le lien . De plus, il n'est pas nécessaire d'ajouter des fonctions "using" à l'intérieur de la bibliothèque: puisque leur code est déjà dans std , le compilateur choisira toujours la surcharge correcte.
Et puis - voila - la fonction personnalisée iter_swap fonctionnera comme elle le devrait sans aucun "sale truc" et autre sorcellerie :)
Conclusion
Ceci conclut mon article. J'espère que les erreurs que j'ai trouvées vous ont intéressées et que vous avez appris quelque chose de nouveau et d'utile pour vous-même. Si vous avez lu jusqu'à présent, je vous souhaite sincèrement un code propre et ordonné sans erreurs. Laissez les bugs contourner vos projets!
PS Nous pensons que c'est une mauvaise pratique d'écrire votre propre code dans l'espace de noms std. Qu'est-ce que tu penses? J'attends avec impatience vos réponses dans les commentaires.
Si vous développez en C, C ++, C # ou Java et, comme moi, vous êtes intéressé par le sujet de l'analyse statique, alors je vous suggère d'essayer PVS-Studio vous-même. Vous pouvez le télécharger sur le lien .
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: George Gribkov. Vérification du code de DeepSpeech, ou pourquoi vous ne devriez pas écrire dans l'espace de noms std .