Chromium: memory leaks / PVS-Studio Blog

We bring to your attention a series of articles devoted to recommendations for writing high-quality code on the example of errors found in the Chromium project. This is the third part that will be devoted to memory leaks.

I believe that the Chromium project code and the libraries used in it are of very high quality. Yes, in the introductory article I wrote about 250 errors , but in fact - this is a very small number. By virtue of the laws of probability in a huge project there are bound to be many mistakes.

Nevertheless, if we talk about memory leaks, there are not so few of them. I think the developers of Chromium are letting down that they prefer dynamic code analyzers. Of course, these tools have a number of advantages. For example, they do not give false positives, because once the dynamic analyzer detected an error, then it really is.

Dynamic analysis, on the other hand, has weaknesses. If some code is not executed, then the error will not be detected. And any programmer understands that it is extremely difficult to cover 100% of the code with tests, or rather, in practice this is simply impossible. As a result, a number of errors remain in the code and wait for a favorable set of circumstances to prove itself.

A static code analyzer can come in handy here. Yes, this is a hint to developers from Google that we will be happy if they become our customers. Moreover, we are ready for additional work on adapting and setting up PVS-Studio for the features of the Chromium project. Our team is also ready to take care of correcting the errors found. We already had a similar experience ( example ).

But back to the memory leaks. As you will see, they hide in code that rarely gets control. These are mainly various error handlers. Static analyzers, unlike dynamic ones, are not always able to track the "fate of the pointer" to the allocated memory and will not detect many memory leaks. On the other hand, static analyzers check all the code, regardless of the probability of its execution, and notice errors. Thus, static and dynamic analyzers complement each other.

Let's see what memory leaks I noticed while parsing the report issued by PVS-Studio. As I wrote in the introductory article, I looked at the report quite fluently, so there may be other errors that I have not noticed. I also note that memory leaks are extremely unpleasant for a project like Chromium, so it will be interesting to talk about them. According to the CWE, these errors can be classified as CWE-401 .

Part 1: forgot to free memory before exiting function

Consider the error in the Chromium code. To start, I will show the BnNew helper function , which allocates and returns a nullified memory buffer:

uint32_t* BnNew() {
  uint32_t* result = new uint32_t[kBigIntSize];
  memset(result, 0, kBigIntSize * sizeof(uint32_t));
  return result;
}

Now let's look at the code, which can lead to a memory leak:

std::string AndroidRSAPublicKey(crypto::RSAPrivateKey* key) {
  ....
  uint32_t* n = BnNew();
  ....
  RSAPublicKey pkey;
  pkey.len = kRSANumWords;
  pkey.exponent = 65537; // Fixed public exponent
  pkey.n0inv = 0 - ModInverse(n0, 0x100000000LL);
  if (pkey.n0inv == 0)
    return kDummyRSAPublicKey;
  ....
}

If the condition (pkey.n0inv == 0) is fulfilled , then the function exits without freeing the buffer, the pointer to which is stored in the variable n .

The analyzer indicates this defect by issuing a warning: V773 CWE-401 The function was exited without releasing the 'n' pointer. A memory leak is possible. android_rsa.cc 248

By the way, the memory leaks related to Chromium itself end there. But there are a lot of them in the libraries used. And users do not care if memory flows in the Chromium libraries or in Chromium itself. Therefore, errors in libraries are no less important.

The following errors are related to the WebKit engine. We will have to start again with an auxiliary function:

static CSSValueList* CreateSpaceSeparated() {
  return new CSSValueList(kSpaceSeparator);
}

Now the code containing the error:

const CSSValue* CSSTransformValue::ToCSSValue(....) const {
  CSSValueList* transform_css_value =
    CSSValueList::CreateSpaceSeparated();
  for (size_t i = 0; i < transform_components_.size(); i++) {
    const CSSValue* component =
        transform_components_[i]->ToCSSValue(secure_context_mode);
    if (!component)
      return nullptr;                              // <=
    transform_css_value->Append(*component);
  }
  return transform_css_value;
}

If the component pointer turns out to be null, then the function will exit, and a memory leak will occur.

The PVS-Studio analyzer generates a warning: V773 CWE-401 The function was exited without releasing the 'transform_css_value' pointer. A memory leak is possible. csstransformvalue.cpp 73

Let's see some other error related to WebKit.

Request* Request::CreateRequestWithRequestOrString(....)
{
  ....
  BodyStreamBuffer* temporary_body = ....;
  ....
  temporary_body =
   new BodyStreamBuffer(script_state, std::move(init.GetBody()));
  ....
  if (exception_state.HadException())
    return nullptr;
  .... 
}

If the HadException () function returns true, then the function will finish its work ahead of schedule. In this case, no one will call the delete operator for the pointer stored in the temporary_body variable .

PVS-Studio Warning: V773 CWE-401 The function was exited without releasing the 'temporary_body' pointer. A memory leak is possible. request.cpp 381


Many? Many. At the same time, only those messages for which I had the strength were written out here. I quickly got bored and looked at such warnings very superficially. Most likely, with a more careful analysis of the report, more errors will be found in WebKit.

What does it mean? This means that the WebKit project has problems with memory leaks, for which I congratulate this project.

Now let's move on to the ICU project and look at the error found in it.

UVector*
RuleBasedTimeZone::copyRules(UVector* source) {
    if (source == NULL) {
        return NULL;
    }
    UErrorCode ec = U_ZERO_ERROR;
    int32_t size = source->size();
    UVector *rules = new UVector(size, ec);
    if (U_FAILURE(ec)) {
        return NULL;
    }
  ....
}

If a certain error occurs during the initialization of an object of type UVector , this will affect the status that is placed in the ec variable . For example, the constructor will return the status U_MEMORY_ALLOCATION_ERROR if it is not possible to allocate a memory buffer for storing the required number of elements. However, regardless of whether it is possible to allocate memory for storing elements or not, an object of type UVector will be created and a pointer to this object will be placed in the rules variable .

If the constructor returns the status U_MEMORY_ALLOCATION_ERROR , then the function will exit. In this case, an object of type UVector will not be deleted, and a memory leak will occur.

PVS-Studio Warning: V773 CWE-401 The function was exited without releasing the 'rules' pointer. A memory leak is possible. rbtz.cpp 668


What else did I notice?



Probably, these are far from all errors, but they are enough for me to demonstrate the capabilities of PVS-Studio and write this article.

Part 1: recommendation

What unites all the cases considered above? That errors became possible due to manual memory management!

Friends, we are already using C ++ 17. Stop calling the new operator , put the result in an ordinary pointer, and then forget to free it. I am ashamed.

No more ordinary pointers and subsequent manual control of the allocated resource! Let's always use smart pointers.

The modern C ++ standard offers smart pointers such as unique_ptr , shared_ptr, and weak_ptr . In most cases, one unique_ptr will suffice .

Let us return, for example, to this incorrect code:

const CSSValue* CSSTransformValue::ToCSSValue(....) const {
  CSSValueList* transform_css_value =
    CSSValueList::CreateSpaceSeparated();
  for (size_t i = 0; i < transform_components_.size(); i++) {
    const CSSValue* component =
        transform_components_[i]->ToCSSValue(secure_context_mode);
    if (!component)
      return nullptr;
    transform_css_value->Append(*component);
  }
  return transform_css_value;
}

Let's rewrite it using unique_ptr . To do this, first of all, we need to change the type of the pointer itself. Secondly, at the very end, you need to call the release function to return a pointer to the managed object and no longer control it.

The correct code is:

const CSSValue* CSSTransformValue::ToCSSValue(....) const {
  unique_ptr transform_css_value(
    CSSValueList::CreateSpaceSeparated());
  for (size_t i = 0; i < transform_components_.size(); i++) {
    const CSSValue* component =
        transform_components_[i]->ToCSSValue(secure_context_mode);
    if (!component)
      return nullptr;
    transform_css_value->Append(*component);
  }
  return transform_css_value.release();
}

I do not plan to teach how to use smart pointers in this article. There are many good articles and sections in books devoted to this topic. I just wanted to show that the code did not become more complex and cumbersome due to changes. But now it will be much more difficult to make a mistake.

Do not think that it is you who will cope with new / delete or with malloc / free and will not make mistakes. Chromium developers make such mistakes. Other developers do . You make and will make such mistakes. And do not have unnecessary dreams that your team is special :). Taking this opportunity, I ask managers to read this note now .

Use smart pointers.

Part 2: realloc

In my experience, programmers sometimes misuse the realloc function . Here's what the classic error pattern associated with using this function looks like:

p = realloc(p, n);
if (!p)
  return ERROR;

Note the following property of the function: If there is not enough memory, the old memory block is not freed and null pointer is returned.

Since NULL will be written to the variable p , which stored the pointer to the buffer, the ability to free this buffer is lost. There is a memory leak.

The correct option would be to rewrite the code, for example, like this:

void *old_p = p;
p = realloc(p, n);
if (!p)
{
  free(old_p);
  return ERROR;
}

Not without such errors in the libraries used in the Chromium project.

For example, consider a piece of code in the FLAC codec.

FLAC__bool FLAC__format_entropy_codi.....ce_contents_ensure_size(
  FLAC__EntropyCodingMethod_PartitionedRiceContents *object,
  unsigned max_partition_order)
{
  ....
  if(object->capacity_by_order < max_partition_order) {
    if(0 == (object->parameters =
               realloc(object->parameters, ....)))
      return false;
    if(0 == (object->raw_bits = realloc(object->raw_bits, ....)))
      return false;
    ....
}

The function increases the size of two buffers:

• object-> parameters
• object-> raw_bits

If a memory allocation error occurs, the function terminates ahead of schedule and returns false . In this case, the previous value of the pointer is lost, and a memory leak occurs.

The PVS-Studio analyzer generates two relevant warnings here:

• V701 CWE-401 realloc () possible leak: when realloc () fails in allocating memory, original pointer 'object-> parameters' is lost. Consider assigning realloc () to a temporary pointer. format.c 576
• V701 CWE-401 realloc () possible leak: when realloc () fails in allocating memory, original pointer 'object-> raw_bits' is lost. Consider assigning realloc () to a temporary pointer. format.c 578

Fortunately, there are very few errors of this type in Chromium. At least much less than what I usually see in other projects.

Part 2: recommendation

It is not always possible to refuse the realloc function , since it allows you to write efficient code when you often need to change the buffer size.

Therefore, I will not rush to recommend completely abandoning it. Sometimes it will be unjustified. I ask only to be careful with this function and not to forget the error pattern that I described above.

However, very often in C ++ it is quite possible to do without this function and use containers such as std :: vector or std :: string . Container efficiency has grown significantly in recent years. For example, I was pleasantly surprised when I saw that there is no longer a performance difference in the PVS-Studio core between the self-made string class and std :: string. But many years ago, a home-made string class yielded about 10% of the performance gain to the analyzer. More such an effect is not observed and it became possible to remove your own class. Now the std :: string class is not at all what it was 10 years ago. Efficiency has improved significantly thanks to modern compiler optimization capabilities and thanks to such innovations of the language as, for example, the move constructor.

In general, do not rush to roll up your sleeves and manually manage memory using the malloc , realloc , free functions . Almost certainly, std :: vector will be no less effective for your tasks. In doing so, use std :: vectormuch easier. Making a mistake will also become more difficult. It makes sense to return to low-level functions only when the profiler shows that this is really one of the bottlenecks in the program.

Thank you all for your attention. I invite you to download and try the PVS-Studio analyzer .

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. Chromium: Memory Leaks .