Clever Geek Handbook

PVS-Studio, Blender: a cycle of notes on the benefits of regular use of static analysis

PVS-Studio monitors Blender code







In our articles, we regularly repeat an important thought: a static analyzer should be used regularly. In this case, many errors are detected at the earliest stage, and their correction is as cheap as possible. However, theory is one thing, but it is much better to back up words with practical examples. Let's take a look at some recent bugs in the new code of the Blender project.







Recently we set up a regular check of the Blender project , which my colleague told about in the article " Just for fun: the PVS-Studio team came up with the idea of ​​monitoring the quality of some open source projects ". In the future, we plan to start monitoring some more interesting projects.







, . ( ), . , , 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.







More articles:


All Articles