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

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\.

![0807_Blender_monitoring/image1.png](https://import.viva64.com/docx/blog/0807_Blender_monitoring/image1.png)

Recently, we set up a regular check of the [Blender](https://en.wikipedia.org/wiki/Blender_(software)) project, as my colleague described in the article "[Just for Fun: PVS\-Studio Team Came Up With Monitoring Quality of Some Open Source Projects](https://pvs-studio.com/en/blog/posts/cpp/0799/)"\. 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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf)", 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](https://pvs-studio.com/en/blog/terms/0042/) is possible\. To avoid this, declare the pointer as atomic: _std::atomic<NodeTreeUIStorage \*\> ui\_storage_\.

**Fragment two: realloc**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/cpp/0558/)"\.

**Fragment three: dereferencing a pointer before a check**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/examples/v595/)\)\. 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](https://pvs-studio.com/en/pvs-studio/download/)\. Thanks for your attention\!