# Chromium: Memory Leaks We'd like to present the series of articles dealing with the recommendations on writing code of high quality using the examples of errors found in the Chromium project\. This is the third part, which will be focused on memory leaks\. ![0555_Chromium_3_memory_leak/image1.png](https://import.viva64.com/docx/blog/0555_Chromium_3_memory_leak/image1.png) I think the code of Chromium project and the libraries used in it is of very high quality\. Sure, in the introductory article, I wrote about [250 errors](http://cppfiles.com/chromium.txt), but actually, this is a very small number\. In view of probability laws, no doubts that in a large project many errors will be found\. However, if we talk about memory leaks, there are not few of them\. I suppose that what lets the Chromium developers down is that they give preference to dynamic code analyzers\. Of course, these tools have a number of benefits\. For instance, they don't issue false positives, once a dynamic analyzer has detected an error, we know for sure that an errors is really presented\. On the other hand, dynamic analysis also has weaknesses\. If a code is not executed, an error will not be detected\. But any developer realizes that it is very difficult to cover 100% of code with tests, or rather, this is impossible in practice\. As a result, the number of errors remains in code and they are waiting for a favorable set of circumstances to reveal themselves\. Here static code analysis might come to aid\. Yes, this is a hint for Google developers, that we will be glad, if they become our clients\. Moreover, we are ready to get the additional work done on PVS\-Studio adaptation and configuring for the specifications of the Chromium project\. Our team is also ready to take on the correction of errors found\. We already had similar experience \([example](https://www.unrealengine.com/en-us/tech-blog/static-analysis-as-part-of-the-process)\)\. But let's get back to memory leaks\. As you will see, they hide in code that is rarely controlled\. Basically, these are different error handlers\. Static analyzers, unlike dynamic, are not always able to monitor the "future of a pointer" on the allocated memory and don't detect a lot of memory leaks\. On the other hand, static analyzers check all code, regardless of the likelihood of its execution and notice errors\. Thus, dynamic and static analyzers are complementary\. Let's see what I've noticed during studying the report issued by PVS\-Studio\. As I wrote in the [introductory article](https://pvs-studio.com/en/blog/posts/cpp/0552/), I skimmed through the report quite fluently, so there may be other, unnoticed errors\. I would also like to note that memory leaks are extremely unpleasant for such a project as Chromium, so it will be exciting to talk about them\. Such errors can be classified as [CWE\-401](https://cwe.mitre.org/data/definitions/401.html)\. ## Part 1: one forgot to free memory before exiting the function Let's look at the error in the Chromium code\. First I'll show you the _BnNew_ helper function, which allocates and returns a nullified memory buffer: ```cpp 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 that can lead to a memory leak: ```cpp 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 executed, then the function exit occurs without freeing the buffer, a pointer at which is stored in the _n_ variable\. The analyzer points at this defect by issuing the warning: [V773](https://pvs-studio.com/en/docs/warnings/v773/) CWE\-401 The function was exited without releasing the 'n' pointer\. A memory leak is possible\. android\_rsa\.cc 248 By the way, at this point, memory leaks related to Chromium itself, ended\. Anyway, many of them are presented in the used libraries\. Users don't care whether there will be memory leaks in Chromium libraries or Chromium itself\. That's why the errors in libraries are no less important\. The following bugs relate to the WebKit engine\. We'll begin again with the helper function: ```cpp static CSSValueList* CreateSpaceSeparated() { return new CSSValueList(kSpaceSeparator); } ``` Now here is the code containing the error: ```cpp 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 a pointer _component_ turns out to be null, the function will finish its work, and in doing so, a memory leak will occur\. PVS\-Studio analyzer issues 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 another error related to WebKit\. ```cpp Request* Request::CreateRequestWithRequestOrString(....) { .... BodyStreamBuffer* temporary_body = ....; .... temporary_body = new BodyStreamBuffer(script_state, std::move(init.GetBody())); .... if (exception_state.HadException()) return nullptr; .... } ``` If the function _HadException\(\)_ returns true, then the function will prematurely exit\. Whereas no one will call the _delete _operator for a pointer, stored in the variable _temporary\_body_\. PVS\-Studio warning: V773 CWE\-401 The function was exited without releasing the 'temporary\_body' pointer\. A memory leak is possible\. request\.cpp 381 Other errors that I noticed in WebKit, are no different from those described, so I don't see any reason to consider them in the article and I shall confine myself to list the analyzer warnings: * V773 CWE\-401 The function was exited without releasing the 'image\_set' pointer\. A memory leak is possible\. csspropertyparserhelpers\.cpp 1507 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. csspropertyparserhelpers\.cpp 1619 * V773 CWE\-401 The function was exited without releasing the 'shape' pointer\. A memory leak is possible\. cssparsingutils\.cpp 248 * V773 CWE\-401 The function was exited without releasing the 'shape' pointer\. A memory leak is possible\. cssparsingutils\.cpp 272 * V773 CWE\-401 The function was exited without releasing the 'shape' pointer\. A memory leak is possible\. cssparsingutils\.cpp 289 * V773 CWE\-401 The function was exited without releasing the 'shape' pointer\. A memory leak is possible\. cssparsingutils\.cpp 315 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. cssparsingutils\.cpp 1359 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. cssparsingutils\.cpp 1406 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. cssparsingutils\.cpp 1359 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. cssparsingutils\.cpp 1406 * V773 CWE\-401 The function was exited without releasing the 'values' pointer\. A memory leak is possible\. cssparsingutils\.cpp 1985 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. cssparsingutils\.cpp 2474 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. cssparsingutils\.cpp 2494 * V773 CWE\-401 The function was exited without releasing the 'values' pointer\. A memory leak is possible\. atruledescriptorparser\.cpp 30 * V773 CWE\-401 The function was exited without releasing the 'values' pointer\. A memory leak is possible\. atruledescriptorparser\.cpp 57 * V773 CWE\-401 The function was exited without releasing the 'values' pointer\. A memory leak is possible\. atruledescriptorparser\.cpp 128 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. csssyntaxdescriptor\.cpp 193 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. computedstylecssvaluemapping\.cpp 1232 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. computedstylecssvaluemapping\.cpp 1678 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. computedstylecssvaluemapping\.cpp 1727 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. computedstylecssvaluemapping\.cpp 2036 * V773 CWE\-401 The function was exited without releasing the 'size\_and\_line\_height' pointer\. A memory leak is possible\. computedstylecssvaluemapping\.cpp 2070 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. computedstylecssvaluemapping\.cpp 2070 * V773 CWE\-401 The function was exited without releasing the 'file\_list' pointer\. A memory leak is possible\. v8scriptvaluedeserializer\.cpp 249 * V773 CWE\-401 The function was exited without releasing the 'file\_list' pointer\. A memory leak is possible\. v8scriptvaluedeserializer\.cpp 264 * V773 CWE\-401 The function was exited without releasing the 'computed\_style\_info' pointer\. A memory leak is possible\. inspectordomsnapshotagent\.cpp 367 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. cursor\.cpp 42 * V773 CWE\-401 The function was exited without releasing the 'values' pointer\. A memory leak is possible\. content\.cpp 103 * V773 CWE\-401 The function was exited without releasing the 'variation\_settings' pointer\. A memory leak is possible\. fontvariationsettings\.cpp 56 * V773 CWE\-401 Visibility scope of the 'font\_variation\_value' pointer was exited without releasing the memory\. A memory leak is possible\. fontvariationsettings\.cpp 58 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. rotate\.cpp 32 * V773 CWE\-401 The function was exited without releasing the 'values' pointer\. A memory leak is possible\. quotes\.cpp 25 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. textindent\.cpp 52 * V773 CWE\-401 The function was exited without releasing the 'list' pointer\. A memory leak is possible\. shapeoutside\.cpp 35 * V773 CWE\-401 The function was exited without releasing the 'port\_array' pointer\. A memory leak is possible\. v8messageeventcustom\.cpp 127 A lot? Yeap, a lot\. I had enough energy only for the warnings that I highlighted\. I quickly got bored and just skimmed through the warnings\. Most likely, with closer analysis of errors there would be much more errors found in WebKit\. What does it mean? This means that the WebKit project has problems with memory leaks, so accept my "congratulations"\. Now let's move on to the ICU project and consider a bug found in it\. ```cpp 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 an error of the _UVector _type occurs when object initializing, this will have the impact on the status, which is placed in the _ec _variable\. For example, the constructor will return a status _U\_MEMORY\_ALLOCATION\_ERROR_ if it cannot allocate the memory buffer to store the desired number of elements\. However, regardless whether it is possible to allocate memory for storing elements or not, an object of the _UVector_ type will be created itself and a pointer to that object will be placed in the _rules_ variable\. If the constructor returns a status _U\_MEMORY\_ALLOCATION\_ERROR_, then there will be an exit from the function\. The object of the _UVector_ type will not be removed 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 Other errors from the ICU library will be also listed: * V773 CWE\-401 The function was exited without releasing the 'tmpSet' pointer\. A memory leak is possible\. uspoof\_impl\.cpp 184 * V773 CWE\-401 The function was exited without releasing the 'result' pointer\. A memory leak is possible\. stsearch\.cpp 301 * V773 CWE\-401 The function was exited without releasing the 'values' pointer\. A memory leak is possible\. tznames\_impl\.cpp 154 * V773 CWE\-401 The function was exited without releasing the 'filter' pointer\. A memory leak is possible\. tridpars\.cpp 298 * V773 CWE\-401 The function was exited without releasing the 'targets' pointer\. A memory leak is possible\. transreg\.cpp 984 * V773 CWE\-401 The function was exited without releasing the 'instance' pointer\. A memory leak is possible\. tzgnames\.cpp 1216 * V773 CWE\-401 The function was exited without releasing the 'uset' pointer\. A memory leak is possible\. rbbiscan\.cpp 1276 What else did I notice? Libwebm Library: * V773 CWE\-401 The function was exited without releasing the 'new\_frame' pointer\. A memory leak is possible\. mkvmuxer\.cc 3513 * V773 CWE\-401 The function was exited without releasing the 'new\_frame' pointer\. A memory leak is possible\. mkvmuxer\.cc 3539 SwiftShader library: * V773 CWE\-401 The function was exited without releasing the 'node' pointer\. A memory leak is possible\. intermediate\.cpp 405 * V773 CWE\-401 The function was exited without releasing the 'node' pointer\. A memory leak is possible\. intermediate\.cpp 443 * V773 CWE\-401 The function was exited without releasing the 'node' pointer\. A memory leak is possible\. intermediate\.cpp 514 * V773 CWE\-401 The function was exited without releasing the 'rightUnionArray' pointer\. A memory leak is possible\. intermediate\.cpp 1457 * V773 CWE\-401 The function was exited without releasing the 'unionArray' pointer\. A memory leak is possible\. intermediate\.cpp 1457 * V773 CWE\-401 The function was exited without releasing the 'aggregateArguments' pointer\. A memory leak is possible\. parsehelper\.cpp 2109 Probably, these are not all errors, but they are enough for me to demonstrate the abilities of PVS\-Studio and write this article\. ## Part 1: Recommendation What unites all of the above cases? The errors became possible due to manual memory management\! Friends, we are already using C\+\+17\. Stop calling the _new_ operator, placing the result in the ordinary pointer, and then forgetting to free it\! So embarrassing\! No more ordinary pointers and subsequent manual management of allocated resources\! Let's always use smart pointers\. The modern C\+\+ standard offers such [smart pointers](https://en.wikipedia.org/wiki/Smart_pointer) as _unique\_ptr_, _shared\_ptr_ and _weak\_ptr_\. In most cases, just _unique\_ptr_ will be enough\. For example, let's return to this incorrect code: ```cpp 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, we need to change the type of the pointer\. Secondly, in the end we need to call the _release_ function to return the pointer to a controlled object and do not control it any more\. Correct code: ```cpp 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(); } ``` In this article, I'm not going to teach how to use smart pointers\. This topic is widely discussed in articles and book sections\. I just wanted to show that the code has not become more difficult and cumbersome due to changes\. But now it will be much harder to make a mistake\. Don't think that you can handle the _new/delete _or _malloc/free_ and not slip up\. Chromium developers make such mistakes\. Other developers [do](https://pvs-studio.com/en/blog/examples/v773/)\. You are doing and will do such errors\. There is no need to indulge in illusions that your team is oh\-so\-special :\)\. I'd like to take this opportunity to ask managers to read now this [info](https://pvs-studio.com/en/blog/posts/0498/)\. Use smart pointers\. ## Part 2: Realloc According to my own observations, programmers sometimes incorrectly use the function [_realloc_](https://en.cppreference.com/w/c/memory/realloc)\. Here is a classic pattern of errors, associated with using this function: ```cpp p = realloc(p, n); if (!p) return ERROR; ``` Let's pay attention to the following function property: If there is not enough memory, the old memory block is not freed and null pointer is returned\. As _NULL _will be written in the _p_ variable, which stored a pointer to a buffer, then you lose the opportunity to release this buffer\. A memory leak occurs\. The right thing to do is to rewrite the code as follows: ```cpp void *old_p = p; p = realloc(p, n); if (!p) { free(old_p); return ERROR; } ``` It has not been without such errors in libraries used in the Chromium project\. For example, let's consider the following code fragment in the FLAC codec\. ```cpp 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 prematurely finishes and returns the _false_ value\. With this, the previous value of the pointer is lost, and a memory leak will occur\. Here PVS\-Studio analyzer reports two relevant warnings: * [V701](https://pvs-studio.com/en/docs/warnings/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 Similar shortcomings in the WebRTC project: * V701 CWE\-401 realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'self\-\>binary\_far\_history' is lost\. Consider assigning realloc\(\) to a temporary pointer\. delay\_estimator\.cc 303 * V701 CWE\-401 realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'self\-\>far\_bit\_counts' is lost\. Consider assigning realloc\(\) to a temporary pointer\. delay\_estimator\.cc 306 * V701 CWE\-401 realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'self\-\>mean\_bit\_counts' is lost\. Consider assigning realloc\(\) to a temporary pointer\. delay\_estimator\.cc 453 * V701 CWE\-401 realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'self\-\>bit\_counts' is lost\. Consider assigning realloc\(\) to a temporary pointer\. delay\_estimator\.cc 456 * V701 CWE\-401 realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'self\-\>histogram' is lost\. Consider assigning realloc\(\) to a temporary pointer\. delay\_estimator\.cc 458 Fortunately, there are few errors of this type in Chromium\. At least, a lot less than I usually come across in other projects\. ## Part 2: Recommendation It is not always possible to give up using the _realloc_ function, as it allows writing efficient code when you need to frequently change the size of a buffer\. So we will not get ahead of ourselves to recommend avoiding it completely\. Sometimes it would be unreasonable\. I just ask to be careful with this function and not to forget the error pattern I've described above\. However, very often in C\+\+ it is quite possible to do without this function and use containers like _std::vector_ or _std::string_\. The effectiveness of the containers has significantly increased in recent years\. For example, I was pleasantly surprised when I saw that in the core of PVS\-Studio there is no more difference in performance between a self\-made string class and_ std::string_\. Nevertheless, many years ago a self\-made string class gave about 10% of productivity gains to the analyzer\. There is no such effect any more, so it became possible to remove your own class\. Now the class _std::string_ is not the same as it was 10 years ago\. Efficiency has improved significantly thanks to modern compilers and optimization abilities and such language innovations as, for example, a move constructor\. Anyway, no rush to roll up your sleeves and manage memory manually by using the functions _malloc_, _realloc,_ _free_\. Almost certainly, _std::vector _will prove to be no less effective for your needs\. In addition, it's much simpler to use _std::vector\._ It will become more difficult to make an error\. It makes sense to return to low\-level functions only when the profiler shows that it is really one of the bottlenecks in the program work\. Thank you for your attention\. I invite you all to [download](https://pvs-studio.com/en/pvs-studio/) and try PVS\-Studio\.