Chromium: memory leaks

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

I believe that the code of the Chromium project 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, a huge project will surely find a lot of mistakes.

Nevertheless, if we talk about memory leaks, they are not so few. I think the developers of Chromium are failing 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 has detected an error, then it really is.

On the other hand, dynamic analysis 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 with tests 100% of the code, or rather, in practice it is simply impossible. As a result, a number of errors remain in the code and are waiting for a favorable set of circumstances in order to prove themselves.
')
Here a static code analyzer can help. Yes, this is a hint to Google developers that we will be happy if they become our customers. Moreover, we are ready for additional work on the adaptation and configuration of PVS-Studio for the features of the Chromium project. Our team is also ready to take on editing the errors found. We already had a similar experience ( example ).

But back to memory leaks. As you will see, they are hiding in code that rarely gets control. Basically, these are different error handlers. Static analyzers, unlike dynamic ones, are not always able to track the “fate of the pointer” on the allocated memory and will not detect many memory leaks. On the other hand, static analyzers check the entire 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 during the analysis of the report issued by PVS-Studio. As I wrote in the introductory article , I looked at the report rather briefly, so there may be other errors that I did not notice. 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 CWE, these errors can be classified as CWE-401 .

Part 1: forgot to free the memory before exiting the function

Consider an error in the Chromium code. First, I’ll show the auxiliary function BnNew , which allocates and returns a zeroed memory buffer:

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

And now let's look at the code that could 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 is satisfied (pkey.n0inv == 0) , then the function is exited without releasing the buffer, the pointer to which is stored in the variable n .

The analyzer points to this defect by issuing a warning: V773 CWE-401. A memory leak is possible. android_rsa.cc 248

By the way, this is where memory leaks related to Chromium itself end. But there are a lot of them in used libraries. But users do not care, memory will flow in the libraries of Chromium or in the 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 zero, the function will terminate, and a memory leak will occur.

The PVS-Studio analyzer issues a warning: V773 CWE-401 The transform_css_value pointer. A memory leak is possible. csstransformvalue.cpp 73

Let's look at some more 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, the function will complete its work ahead of schedule. In this case, no one will call the operator delete for the pointer stored in the variable temporary_body .

PVS-Studio warning: V773 CWE-401 The function was extinguished. A memory leak is possible. request.cpp 381


Lot? Lot. In this case, here are written only those messages for which I had the strength. I quickly got bored and looked through such warnings very superficially. Most likely, with a more careful analysis of the report, there will be much more errors found in WebKit.

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

We now turn to the ICU project and consider 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 during the initialization of an object of the UVector type some error occurs, this will affect the status that is placed in the variable ec . For example, the constructor will return the status of U_MEMORY_ALLOCATION_ERROR if it is not possible to allocate a memory buffer to store 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 variable rules .

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 exhibited. A memory leak is possible. rbtz.cpp 668


What else did I notice?



Probably, these are not 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 above cases? The fact that errors became possible due to manual memory management!

Friends, we already use C ++ 17. Stop calling the new operator, put the result in an ordinary pointer, and then forget to release it. It's a shame.

No more ordinary pointers and subsequent manual control of the selected 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, we first 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.

Correct code:

 const CSSValue* CSSTransformValue::ToCSSValue(....) const { unique_ptr<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.release(); } 

I do not plan to teach in this article how to use smart pointers. This topic is devoted many good articles and sections in the books. I just wanted to show that the code did not become more complicated and cumbersome due to the 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 malloc / free and make no mistakes. Chromium developers make such mistakes. Other developers do . You make and will make such mistakes. And do not unnecessary dreams that your team is special :). Taking this opportunity, I ask the managers to read now this note here .

Use smart pointers.

Part 2: realloc

According to my observations, programmers sometimes incorrectly use the realloc function. Here is the classic error pattern associated with using this function:

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

Let's pay attention to the following property of the function:

Since NULL will be written to the variable p , which kept the pointer to the buffer, it is no longer possible to free this buffer. 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 project Chromium.

For example, consider the code snippet 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 early and returns false . This will lose the previous value of the pointer, and a memory leak will occur.

The PVS-Studio analyzer issues two corresponding 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

Errors of this type in Chromium, fortunately, are very few. At least, much less than I usually meet in other projects.

Part 2: recommendation

It is not always possible to abandon 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 abandon 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 . The efficiency of containers has increased significantly in recent years. For example, I was pleasantly surprised when I saw that there is no longer any difference in performance in the PVS-Studio core between the self-made string class and std :: string . But many years ago, the self-made class of the line gave about 10% increase in performance to the analyzer. More such effect is not observed and it became possible to remove its own class. Now the class std :: string is not at all what it was 10 years ago. Efficiency has significantly increased due to modern compiler optimization capabilities and thanks to such language innovations as, for example, the displacement constructor.

In general, do not rush to roll up your sleeves and manually manage memory using functions malloc , realloc , free . Almost certainly std :: vector will be no less effective for your tasks. At the same time, using std :: vector is much easier. Making a mistake will also become more difficult. It makes sense to return to the low-level functions only when the profiler shows that this is indeed 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 .