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.

We recently set up a regular project review Blender, 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.

I must say right away that we do not set ourselves the task of finding as many errors as possible. The goal is to periodically write small notes (such as this one) in which we will show in practice the merits of regular code review. In other words, we will sometimes describe some interesting errors in the new code found during the next night PVS-Studio launch, and thereby popularize correct using the methodology of static code analysis.

So let’s take a look at what’s found in the fresh code of the Blender project.

Snippet 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 clarify the problem, I will quote a fragment of the article “C ++ and the Perils of Double-Checked Locking“written by Scott Meyers and Andrei Alexandrescu back in 2004. As you can see, the problem has been known for a long time, but this does not protect developers from stepping on the same rake. It’s good that the PVS-Studio analyzer helps to identify such problems 🙂 So, a snippet 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 constrainedto 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 understand in more detail how to write a double-checked lock, then I refer you to the description of the diagnostics and the article, links to which were given above. Another thing is important for us.

Such mistakes are very insidious! They can very rarely show themselves. The program seems to work, passes all tests, and so on. But from time to time, it unexpectedly drops among users, and it can be extremely problematic to understand the reason. This error can be very difficult to reproduce. In other words, fixing this error based on a complaint from users can cost 1000 times more than editing a code based on the result of analyzing the code using PVS-Studio or a similar tool.

Note. The error may not exist in the binary code now, and it all depends on the compiler and optimization keys. However, even if everything is working well now, this does not mean that it will always be so. The error can manifest itself when changing the compiler / optimization keys.

Snippet 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, and this is correct. Here, indeed, two mistakes of a different plan were made at once.

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 function realloc will return the value NULL… The null pointer will be written to the variable context-> read_icons, and its previous value will be lost. Once the previous value of the pointer is lost, then it is impossible to free the previously allocated block of memory, which was referred to by this pointer. 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 in a sense and not an error from the point of view of the author of the code. He was not going to write code that would continue to work if it was impossible to increase the block of allocated memory. Such a case is simply not considered. The author assumes that if memory fails, the program will simply crash when a null pointer is dereferenced. And therefore, it boldly works with a pointer without performing a preliminary check on it. Let’s leave aside the question of how beautiful this program behavior is. In my opinion, this behavior of libraries is unacceptable.

Another thing is more interesting. In fact, the fall may not happen. Writing is done not by a null pointer, but somewhere further. In theory, a situation is possible when this address is no longer in a write-protected memory page, and there will be no fall. Some random data in memory will be corrupted, and the program will continue its execution. The consequences of working with corrupted data are unpredictable. All this is discussed in more detail in the article “Why is it important to check what the malloc function returned”.

Snippet three: dereferencing a pointer for validation

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). At the beginning a pointer nldrag is dereferenced. But from the next conditional operator it becomes clear that in fact this pointer can be null.

Everything is simple and straightforward. But, you see, it is much better to fix such an error right away, even at the stage of writing the code, and not after the tester or user stumbles upon it.

By the way, there was one more error of the same kind, but it is not interesting to describe it. Here is just 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 will benefit from this. You can download and try PVS-Studio here… Thanks for attention.

If you want to share this article with an English-speaking audience, please use the translation link: Andrey Karpov. PVS-Studio, Blender: Series of Notes on Advantages of Regular Static Analysis of Code.

Similar Posts

Leave a Reply

Your email address will not be published. Required fields are marked *