Top 10 des erreurs dans les projets C ++ en 2020

C'est l'hiver dehors, l'année s'achève, ce qui signifie qu'il est temps de considérer les erreurs les plus intéressantes détectées par l'analyseur PVS-Studio en 2020.



Il est à noter que l'année écoulée a été marquée par un grand nombre de nouvelles règles de diagnostic, dont le déclenchement leur a permis d'entrer dans ce top. Nous continuons également à améliorer le cœur de l'analyseur et à ajouter de nouveaux scénarios pour son utilisation, vous pouvez lire tout cela dans notre blog . Si vous êtes intéressé par d'autres langages supportés par notre analyseur (C, C # et Java), jetez un œil aux articles de mes collègues. Passons maintenant directement aux bogues les plus mémorables trouvés par PVS-Studio au cours de l'année écoulée.

Dixième place: division Modulo par un

V1063 L'opération modulo par 1 n'a pas de sens. Le résultat sera toujours nul. llvm-stress.cpp 631

void Act() override {
  ....
  // If the value type is a vector, and we allow vector select,
  // then in 50% of the cases generate a vector select.
  if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 1)) {
    unsigned NumElem =
        cast<FixedVectorType>(Val0->getType())->getNumElements();
    CondTy = FixedVectorType::get(CondTy, NumElem);
  }
  ....
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Le développeur voulait obtenir une valeur aléatoire entre 0 et 1 en utilisant la division modulo. Cependant, une opération comme X% 1 renverra toujours 0. Dans ce cas, il serait correct de réécrire la condition comme suit:

if (isa<FixedVectorType>(Val0->getType()) && (getRandom() % 2))
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Cette erreur a été incluse dans le haut de l'article: " Vérification de Clang 11 avec PVS-Studio ".

Neuvième place: quatre chèques

PVS-Studio a émis quatre avertissements pour la section suivante du code:

• V560 Une partie de l'expression conditionnelle est toujours vraie: x> = 0. editor.cpp 1137
• V560 Une partie de l'expression conditionnelle est toujours vraie: y> = 0. editor.cpp 1137
• V560 Une partie de l'expression conditionnelle est toujours vraie: x <40. Editor.cpp 1137
• V560 Une partie de l'expression conditionnelle est toujours vraie: y <30. Editor.cpp 1137

int editorclass::at( int x, int y )
{
  if(x<0) return at(0,y);
  if(y<0) return at(x,0);
  if(x>=40) return at(39,y);
  if(y>=30) return at(x,29);

  if(x>=0 && y>=0 && x<40 && y<30)
  {
      return contents[x+(levx*40)+vmult[y+(levy*30)]];
  }
  return 0;
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Tous les avertissements concernent la dernière instruction if . Le problème est que les quatre vérifications qui y sont effectuées renverront toujours vrai . Je ne dirais pas que c'est une grave erreur, mais cela s'est avéré assez drôle. En général, ces contrôles sont redondants et peuvent être supprimés.



Cette erreur est entrée en haut de l'article: " VVVVVV ??? VVVVVV !!! ".

Huitième place: supprimer au lieu de supprimer []

V611 La mémoire a été allouée à l'aide de l'opérateur 'new T []' mais a été libérée à l'aide de l'opérateur 'delete'. Pensez à inspecter ce code. Il est probablement préférable d'utiliser 'delete [] poke_data;'. CCDDE.CPP 410

BOOL Send_Data_To_DDE_Server (char *data, int length, int packet_type)
{
  ....
  char *poke_data = new char [length + 2*sizeof(int)]; // <=
  ....
  if(DDE_Class->Poke_Server( .... ) == FALSE) {
    CCDebugString("C&C95 - POKE failed!\n");
    DDE_Class->Close_Poke_Connection();
    delete poke_data;                                  // <=
    return (FALSE);
  }

  DDE_Class->Close_Poke_Connection();

  delete poke_data;                                    // <=

  return (TRUE);
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

L'analyseur a détecté une erreur liée au fait que la mémoire a été allouée et libérée de manière incompatible. Pour libérer de la mémoire allouée à un tableau, utilisez l'opérateur delete [] et non supprimer .



Cette erreur est arrivée en haut de l'article: " Code du jeu Command & Conquer: bugs des années 90. Volume deux ".

Septième place: Tampon hors limites

Regardons la fonction net_hostname_get qui sera utilisée ensuite.

#if defined(CONFIG_NET_HOSTNAME_ENABLE)
const char *net_hostname_get(void);
#else
static inline const char *net_hostname_get(void)
{
  return "zephyr";
}
#endif
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Dans ce cas, lors du prétraitement, l'option liée à la branche #else a été sélectionnée . Autrement dit, dans le fichier prétraité, la fonction est implémentée comme suit:

static inline const char *net_hostname_get(void)
{
  return "zephyr";
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

La fonction renvoie un pointeur vers un tableau de 7 octets (prendre en compte le terminal zéro à la fin de la chaîne).



Regardons maintenant le code qui conduit au débordement du tableau.

static int do_net_init(void)
{
  ....
  (void)memcpy(hostname, net_hostname_get(), MAX_HOSTNAME_LEN);
  ....
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Avertissement PVS-Studio: V512 [CWE-119] Un appel de la fonction 'memcpy' conduira le tampon 'net_hostname_get ()' hors de portée. log_backend_net.c 114



Après le prétraitement, MAX_HOSTNAME_LEN se développe comme suit:

(void)memcpy(hostname, net_hostname_get(),
    sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx"));
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

En conséquence, lors de la copie de données, un dépassement du littéral de chaîne se produit. Comment cela affectera l'exécution du programme est difficile à prévoir, car cela conduit à un comportement indéfini.



Ce bogue est arrivé en haut de l'article: " Enquête sur la qualité du code du système d'exploitation Zephyr ".

Sixième place: quelque chose de très étrange

static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;

  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    ((u8_t *)mntpt)[strlen(mntpt)] = '\0';
    memcpy(cpy_mntpt, mntpt, strlen(mntpt));
  }
  return cpy_mntpt;
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Avertissement PVS-Studio: V575 [CWE-628] La fonction 'memcpy' ne copie pas toute la chaîne. Utilisez la fonction 'strcpy / strcpy_s' pour conserver la valeur null du terminal. shell.c 427



Quelqu'un a essayé de créer un analogue de la fonction strdup , mais ils ont échoué.



Commençons par l'avertissement de l'analyseur. Il dit que la fonction memcpy copie la ligne, mais elle ne copie pas le terminal zéro, ce qui est très suspect.



Il semble que ce terminal 0 soit copié ici:

((u8_t *)mntpt)[strlen(mntpt)] = '\0';
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Mais non! Voici une faute de frappe qui fait que le terminal zéro est copié sur lui-même! Notez que l'écriture va vers le tableau mntpt , pas vers cpy_mntpt . En conséquence, la fonction mntpt_prepare renvoie une chaîne nulle de terminal incomplète.



En fait, le programmeur voulait écrire comme ceci:

((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\0';
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Cependant, on ne sait toujours pas pourquoi cela a été fait si difficile! Ce code peut être simplifié comme suit:

static char *mntpt_prepare(char *mntpt)
{
  char *cpy_mntpt;

  cpy_mntpt = k_malloc(strlen(mntpt) + 1);
  if (cpy_mntpt) {
    strcpy(cpy_mntpt, mntpt);
  }
  return cpy_mntpt;
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Ce bogue est arrivé en tête de l'article susmentionné: " Examen de la qualité du code du système d'exploitation Zephyr ".

Cinquième place: protection anti-débordement incorrecte

V547 [CWE-570] L'expression «rel_wait <0» est toujours fausse. La valeur du type non signé n'est jamais <0. Os_thread_windows.c 359

static DWORD
get_rel_wait(const struct timespec *abstime)
{
  struct __timeb64 t;
  _ftime64_s(&t);
  time_t now_ms = t.time * 1000 + t.millitm;
  time_t ms = (time_t)(abstime->tv_sec * 1000 +
    abstime->tv_nsec / 1000000);

  DWORD rel_wait = (DWORD)(ms - now_ms);

  return rel_wait < 0 ? 0 : rel_wait;
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Dans ce cas, la variable rel_wait est de type DWORD non signé . Cela signifie que la comparaison rel_wait <0 n'a pas de sens, car le résultat est toujours vrai.



L'erreur elle-même n'est pas très intéressante. Mais il s'est avéré intéressant de savoir comment ils ont essayé de résoudre ce problème. Il s'est avéré que les changements n'étaient pas corrigés, mais seulement simplifié le code. Vous pouvez en savoir plus sur cette histoire dans l'article de mon collègue: " Pourquoi PVS-Studio n'offre pas de modification automatique du code ".



L'erreur est entrée en haut de l'article: " Analyse statique du code de la collection de bibliothèques PMDK d'Intel et des erreurs qui ne sont pas des erreurs ."

Quatrième place: n'écrivez pas à std, mon frère

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 std
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

L'article à partir duquel le déclencheur est tiré: " Analyse du code du projet DeepSpeech ou pourquoi vous ne devriez pas écrire dans namespace std " décrit en détail pourquoi vous ne devriez pas faire cela.

Troisième place: Scrollbar, qui a échoué

V501 . Il existe des sous-expressions identiques à gauche et à droite de l'opérateur '-': bufferHeight - bufferHeight TermControl.cpp 592

bool TermControl::_InitializeTerminal()
{
  ....
  auto bottom = _terminal->GetViewport().BottomExclusive();
  auto bufferHeight = bottom;

  ScrollBar().Maximum(bufferHeight - bufferHeight);
  ScrollBar().Minimum(0);
  ScrollBar().Value(0);
  ScrollBar().ViewportSize(bufferHeight);
  ....
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

C'est ce qu'on appelle le "déclenchement avec l'histoire". Dans ce cas, en raison d'une erreur, la barre de défilement du terminal Windows ne fonctionnait pas. Un article entier a été écrit sur la base de ce bug, dans lequel mon collègue a mené des recherches et a compris pourquoi cela s'était produit. Es tu intéressé? Le voici: "La barre de défilement qui ne pouvait pas ".

Deuxième place: rayon et hauteur confus

Et encore une fois, nous parlerons de plusieurs avertissements de l'analyseur:

• V764 Ordre incorrect possible des arguments passés à la fonction 'CreateWheel': 'height' et 'radius'. StandardJoints.cpp 791
• V764 Ordre incorrect possible des arguments passés à la fonction 'CreateWheel': 'height' et 'radius'. StandardJoints.cpp 833
• V764 Ordre incorrect possible des arguments passés à la fonction 'CreateWheel': 'height' et 'radius'. StandardJoints.cpp 884

Voici les appels de fonction:

NewtonBody* const wheel = CreateWheel (scene, origin, height, radius);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Et voici à quoi ressemble son annonce:

static NewtonBody* CreateWheel (DemoEntityManager* const scene,
  const dVector& location, dFloat radius, dFloat height)
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Les arguments ont été inversés dans les appels de fonction.



Cette erreur est arrivée en haut de l'article: " Re-contrôle de Newton Game Dynamics avec l'analyseur statique PVS-Studio ".

Première place: effacer le résultat

V519 La variable 'color_name' reçoit des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 621, 627. string.cpp 627

static bool parseNamedColorString(const std::string &value,
                                  video::SColor &color)
{
  std::string color_name;
  std::string alpha_string;

  size_t alpha_pos = value.find('#');
  if (alpha_pos != std::string::npos) {
    color_name = value.substr(0, alpha_pos);
    alpha_string = value.substr(alpha_pos + 1);
  } else {
    color_name = value;
  }

  color_name = lowercase(value); // <=

  std::map<const std::string, unsigned>::const_iterator it;
  it = named_colors.colors.find(color_name);
  if (it == named_colors.colors.end())
    return false;
  ....
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Cette fonction doit analyser le nom de la couleur avec le paramètre de transparence et renvoyer son code hexadécimal. En fonction du résultat de la vérification de la condition, le résultat de la division de ligne ou une copie de l'argument de la fonction est passé à la variable color_name .



Cependant, dans la fonction minuscule () , non pas la chaîne résultante elle-même est convertie en minuscules, mais l'argument de la fonction d'origine. En conséquence, nous perdrons simplement la couleur que parseNamedColorString () aurait dû renvoyer .

color_name = lowercase(color_name);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

Cette erreur est arrivée en haut de l'article: « PVS-Studio: analyse des pull requests dans Azure DevOps à l'aide d'agents auto-hébergés ».

Conclusion

Au cours de l'année écoulée, nous avons trouvé de nombreux bugs dans des projets open source. Il s'agissait d'erreurs courantes de copier-coller, d'erreurs constantes, de fuites de mémoire et de nombreux autres problèmes. Notre analyseur ne reste pas immobile, et en haut il y a plusieurs détections de nouveaux diagnostics écrits cette année.



J'espère que vous avez apprécié les erreurs collectées. Personnellement, je les ai trouvés assez intéressants. Mais, bien sûr, votre vision peut différer de la mienne, vous pouvez donc créer votre "Top 10 ..." en lisant les articles de notre blog ou en regardant la liste des erreurs trouvées par PVS-Studio dans les projets open source.



J'attire également votre attention sur des articles avec le top 10 des erreurs C ++ des dernières années: 2016 , 2017 , 2018 , 2019 .

Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: Vladislav Stolyarov. Top 10 des bogues trouvés dans les projets C ++ en 2020 .