To get a trial key
fill out the form below
Team License (a basic version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

Free PVS-Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
PVS-Studio, Blender: series of notes on…

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

Mar 03 2021
Author:

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

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!

Popular related articles
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…

Comments (0)

Next comments
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept