PVS-Studio, Blender: series of notes on advantages of regular static analysis of code

Mar 03 2021

Author: Andrey Karpov

In our articles, we regularly repeat an important idea: a static analyzer should be used regularly. This helps detect and cheaply fix many errors at the earliest stage. It looks nice in theory. As we know, actions still speak louder than words. Let's look at some recent bugs in new code of the Blender project.

Recently, we set up a regular check of the Blender project, as my colleague described in the article "Just for Fun: PVS-Studio Team Came Up With Monitoring Quality of Some Open Source Projects". In the future, we plan to start monitoring some more interesting projects.

I must say right away that we do not set ourselves the task of finding as many errors as possible. The goal is to occasionally write small notes (such as this one), in which we will show in practice the advantages of regular code analysis. In other words, we will describe some interesting errors in new code found during a regular night PVS-Studio run, and thereby promote the right use of the static code analysis methodology.

So, let's see what we found in the latest code of the Blender project.

Fragment one: 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 warning: V1036: Potentially unsafe double-checked locking. node_ui_storage.cc 46

This is an incorrect implementation of double-checked locking. To explain the problem, I will quote a fragment from the article "C++ and the Perils of Double-Checked Locking", written by Scott Meyers and Andrei Alexandrescu back in 2004. Though this problem has been known for a long time, some developers keep shooting themselves in the foot. It's good that the PVS-Studio analyzer helps detect such problems :). A fragment from the article:

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.

If you want to learn more about writing a double-checked lock, I recommend reading the diagnostic's description and the article. Links were given above. Keep reading to find out what is of great importance for us in all this initiative.

Such errors are very insidious! They can very rarely reveal themselves. The program seems to work, pass all the tests, and so on. But from time to time, it unexpectedly crashes on users' side. It can be extremely difficult to understand the reason. Reproducing such an error can become an uphill struggle. This means once reported by a user, an error fix might cost 1000 times more compared to a code edit after code analysis by PVS-Studio or another similar tool.

Note 1. Right now the binary code might not contain an error - everything depends on the compiler and optimization keys. And though everything is working well now, it may change in the future. The error may show itself after one changes the compiler or optimization keys.

Note 2. Our readers noticed that the double-checked locking problem is obsolete. In C++17, the language does all side effects related the new T subexpression, before doing the assignment's side effects (the '=' operator). In other words, starting with C++17, you can consider this "fixed, not a bug". However, the expression is not atomic and the race condition is possible. To avoid this, declare the pointer as atomic: std::atomic<NodeTreeUIStorage *> ui_storage.

Fragment two: 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++;
}

The PVS-Studio analyzer issues two warnings here, which is correct. Indeed, we have two bugs of different types here.

First: 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

If the memory cannot be allocated, the realloc function returns NULL. The null pointer will be written to the context->read_icons variable, and its previous value will be lost. Since the previous pointer value is lost, then it is not possible to free the previously allocated memory block that this pointer addressed to. A memory leak will occur.

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

The error described above is not an actual error in the code author's opinion. There wasn't an intention to write code that would continue working if it was impossible to increase the block of allocated memory. This case is simply not considered. The author assumes that if the memory could not be allocated, the program would simply crash when dereferencing the null pointer. So the developer safely works with the pointer, without performing its preliminary check. Let's leave aside the question of how beautiful this program behavior is. In my opinion, this behavior of libraries is unacceptable.

Something else is more interesting here. In fact, the crash may not happen. A value is written not to the null pointer, but somewhere further. Theoretically, it is possible that this address is no longer in the write-protected memory page, and there will be no crash. Some random data in memory will get tainted, and the program will continue its execution. Consequences of working with corrupted data are unpredictable. For more information, see the article "Why it is important to check what the malloc function returned".

Fragment three: dereferencing a pointer before a check

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 warning: V595: The 'nldrag' pointer was utilized before it was verified against nullptr. Check lines: 1037, 1039. node_relationships.c

One of the most common error patterns (proof). First, the nldrag pointer is dereferenced. But from the following conditional statement, it becomes clear that this pointer can be null.

It's all simple and clear. Agree, it's best to fix such an error immediately when writing code, rather than deal with it after it's found by a QA specialist or a user.

By the way, there was another such error, but I don't see the fun in describing it. I will cite only the message: V595: The 'seq' pointer was utilized before it was verified against nullptr. Check lines: 373, 385. strip_add.c

Conclusion

Use static code analyzers regularly. Both developers and users benefit from this. You can download and try PVS-Studio here. Thanks for your attention!

#Cpp