PVS-Studio, Blender: un cycle de notes sur les bénéfices d'une utilisation régulière de l'analyse statique

Dans nos articles, nous répétons régulièrement une pensée importante: un analyseur statique doit être utilisé régulièrement. Dans ce cas, de nombreuses erreurs sont détectées au stade le plus précoce et leur correction est la moins chère possible. Cependant, la théorie est une chose, mais il est préférable de sauvegarder les mots avec des exemples pratiques. Jetons un coup d'œil à quelques bogues récents apparus dans le nouveau code du projet Blender.

Récemment, nous avons mis en place un contrôle régulier du projet Blender , dont mon collègue a parlé dans l'article " Juste pour le plaisir: l'équipe PVS-Studio a eu l'idée de surveiller la qualité de certains projets open source ". À l'avenir, nous prévoyons de commencer à surveiller certains projets plus intéressants.

, . ( ), . , , PVS-Studio, .

, , Blender.

: double-checked locking

typedef struct bNodeTree {
  ....
  struct NodeTreeUIStorage *ui_storage;
} bNodeTree;

static void ui_storage_ensure(bNodeTree &ntree)
{
  /* As an optimization, only acquire a lock if the UI storage doesn't exist,
   * because it only needs to be allocated once for every node tree. */
  if (ntree.ui_storage == nullptr) {
    std::lock_guard<std::mutex> lock(global_ui_storage_mutex);
    /* Check again-- another thread may have allocated the storage
       while this one waited. */
    if (ntree.ui_storage == nullptr) {
      ntree.ui_storage = new NodeTreeUIStorage();
    }
  }
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

PVS-Studio. V1036: Potentially unsafe double-checked locking. node_ui_storage.cc 46

. "C++ and the Perils of Double-Checked Locking", Scott Meyers Andrei Alexandrescu 2004 . , , , . , PVS-Studio :). , :

Consider again the line that initializes pInstance: pInstance = newSingleton;



This statement causes three things to happen:



Step 1: Allocate memory to hold a Singleton object.



Step 2: Construct a Singleton object in the allocated memory.



Step 3: Make pInstance point to the allocated memory.



Of critical importance is the observation that compilers are not constrained to perform these steps in this order! In particular, compilers are sometimes allowed to swap steps 2 and 3. Why they might want to do that is a question we'll address in a moment. For now, let's focus on what happens if they do.



Consider the following code, where we've expanded pInstance's initialization line into the three constituent tasks we mentioned above and where we've merged steps 1 (memory allocation) and 3 ( pInstance assignment) into a single statement that precedes step 2 ( Singleton construction). The idea is not that a human would write this code. Rather, it's that a compiler might generate code equivalent to this in response to the conventional DCLP source code (shown earlier) that a human would write.

, , . .

! . , . , . . , 1000 , , PVS-Studio .

1. , , . , , , . / .

2. , . C++17 , new T (operator =). , C++17, " , ". , , . , : std::atomic<NodeTreeUIStorage *> ui_storage\.

: realloc

static void icon_merge_context_register_icon(struct IconMergeContext *context,
                                             const char *file_name,
                                             struct IconHead *icon_head)
{
  context->read_icons = realloc(context->read_icons,
    sizeof(struct IconInfo) * (context->num_read_icons + 1));
  struct IconInfo *icon_info = &context->read_icons[context->num_read_icons];
  icon_info->head = *icon_head;
  icon_info->file_name = strdup(path_basename(file_name));
  context->num_read_icons++;
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

PVS-Studio , . .

: V701: realloc() possible leak: when realloc() fails in allocating memory, original pointer 'context->read_icons' is lost. Consider assigning realloc() to a temporary pointer. datatoc_icon.c 252

, realloc NULL. context->read_icons, . , , . .

: V522: There might be dereferencing of a potential null pointer 'context->read_icons'. Check lines: 255, 252. datatoc_icon.c

– - . , . . , , , . , . , . , .

. - . , - . , , , . - , . . " , malloc".

:

static int node_link_invoke(bContext *C, wmOperator *op, const wmEvent *event)
{
  ....
  bNodeLinkDrag *nldrag = node_link_init(bmain, snode, cursor, detach);
  nldrag->last_picked_multi_input_socket_link = NULL;
  if (nldrag) {
    op->customdata = nldrag;
  ....
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

PVS-Studio: V595: The 'nldrag' pointer was utilized before it was verified against nullptr. Check lines: 1037, 1039. node_relationships.c

(proof). nldrag . , .

. , , , , , .

, - , . : V595: The 'seq' pointer was utilized before it was verified against nullptr. Check lines: 373, 385. strip_add.c

. , . PVS-Studio . .

, : Andrey Karpov. PVS-Studio, Blender: Series of Notes on Advantages of Regular Static Analysis of Code.