Geekly Articles each Day
⬆️ ⬇️

Beautiful Chromium and gnarled memset

malloc 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 first part to be devoted to the memset function.



Gentlemen, programmers, with the memset function, you need to do something in C ++ programs! Rather, even immediately it is clear what to do - it must stop using. At one time I wrote an article " The most dangerous function in the world of C / C ++ ." I think it is easy to guess that the article is about memset .



However, I will not be unfounded, and again with examples I will demonstrate the danger of this function. The code of the Chromium project and the libraries used in it is of very high quality. Google developers pay a lot of attention to tests and the use of various tools for detecting defects. For example, Google has developed tools such as AddressSanitizer, ThreadSanitizer, and MemorySanitizer.



As a result, there are very few errors related to the memset functions, but it’s sad that they do exist. Very high quality project, but still they are!

')

Let's see what I noticed in the process of parsing 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. However, the defects found will be enough for us to discuss the malloc function.



Incorrect buffer size calculated



The first type of error is associated with an incorrect calculation of the buffer size. Or, in other words, the problem is that there is confusion between the size of the array in bytes and the number of elements in the array. Such errors can be classified as CWE-682 : Incorrect Calculation.



The first error example is taken directly from the Chromium project code. Note that the text and unmodified_text arrays consist of Unicode characters. 



#if defined(WIN32) typedef wchar_t WebUChar; #else typedef unsigned short WebUChar; #endif static const size_t kTextLengthCap = 4; class WebKeyboardEvent : public WebInputEvent { .... WebUChar text[kTextLengthCap]; WebUChar unmodified_text[kTextLengthCap]; .... };


As a result, only half of the elements in these arrays are filled with zeros: 



 WebKeyboardEvent* BuildCharEvent(const InputEventData& event) { WebKeyboardEvent* key_event = new WebKeyboardEvent(....); .... memset(key_event->text, 0, text_length_cap); memset(key_event->unmodified_text, 0, text_length_cap); .... }


PVS-Studio warnings:





The second error example is taken from the WebRTC library used in Chromium. The error is similar to the previous one: it is not taken into account that the array elements are of type int64_t . 



 class VCMRttFilter { .... enum { kMaxDriftJumpCount = 5 }; .... int64_t _jumpBuf[kMaxDriftJumpCount]; int64_t _driftBuf[kMaxDriftJumpCount]; .... }; void VCMRttFilter::Reset() { _gotNonZeroUpdate = false; _avgRtt = 0; _varRtt = 0; _maxRtt = 0; _filtFactCount = 1; _jumpCount = 0; _driftCount = 0; memset(_jumpBuf, 0, kMaxDriftJumpCount); memset(_driftBuf, 0, kMaxDriftJumpCount); }


Here, only the first element of the array and one byte in the second element are reset to zero.



PVS-Studio warning: V512 CWE-682A call of the 'memset' function will make it. _JumpBuf. rtt_filter.cc 52



Recommendation



The way to avoid such errors is to not use more memset . You can be very careful, but sooner or later errors will still leak into your project. This is a good picture in the Chromium project. In other projects - this is a very common problem ( proof ).



Yes, it is impossible to refuse memset in C code. However, if we are talking about C ++, then let's forget about this function. Do not use the memset function in C ++ code. Do not use and point.



What to replace the memset call?



First, you can use the std :: fill function. In this case, the array filling will look like this: 



 fill(begin(key_event->text), end(key_event->text), 0);


Secondly, it is often not necessary to use the call of special functions at all. As a rule, the memset function is used to initialize local arrays and structures. Classic: 



 HDHITTESTINFO hhti; memset(&hhti, 0, sizeof(hhti));


But you can write much easier and more reliable: 



 HDHITTESTINFO hhti = {};


If we are talking about the constructor: 



 class C { int A[100]; public: C() { memset(A, 0, sizeof(A)); } };


That can be written: 

 class C { int A[100] = {}; public: C() { } };


Wrong expectations from memset



Sometimes they forget that the second argument sets the value of a single byte, which is used to fill the buffer. It is confusing for the fact that the second argument of the memset function is of type int . As a result, errors that can be classified as CWE-628 occur: Function Call with Incorrectly Specified Arguments.



Consider an example of a similar error that I noticed in the V8 engine used in the Chromium project. 



 void i::V8::FatalProcessOutOfMemory( const char* location, bool is_heap_oom) { .... char last_few_messages[Heap::kTraceRingBufferSize + 1]; char js_stacktrace[Heap::kStacktraceBufferSize + 1]; i::HeapStats heap_stats; .... memset(last_few_messages, 0x0BADC0DE, Heap::kTraceRingBufferSize + 1); memset(js_stacktrace, 0x0BADC0DE, Heap::kStacktraceBufferSize + 1); memset(&heap_stats, 0xBADC0DE, sizeof(heap_stats)); .... }


PVS-Studio warnings:





The programmer decided to fill the memory blocks with the value 0x0BADC0DE so that when debugging it was easier to understand what was happening. However, the memory areas will be filled with a byte with the value 0xDE .



What the programmer does in code is a low-level operation, and here it is more difficult to manage without memset than in the situations described earlier. The size of the buffers is not a multiple of 4 bytes, so it’s impossible to use std :: fill as before. You have to write and use your own function. 



 void Fill_0x0BADC0DE(void *buf, const size_t size) { const unsigned char badcode[4] = { 0xDE, 0xC0, 0xAD, 0x0B }; size_t n = 0; generate_n(static_cast<char *>(buf), size, [&] { if (n == 4) n = 0; return badcode[n++]; }); }


Recommendation



There is no special recommendation here. But we were again convinced that the memset function is not really needed here, since it does not solve the task set for the programmer.



Error overwriting private data



The memset function is used to wipe private data after it is no longer needed. It is not right. If the buffer with private data is not used after the function call, the compiler has the right to remove the call to this function. This defect is classified as CWE-14 : Compiler Removal of Code to Clear Buffers.



I already foresee the objections that the compiler cannot be removed from the memset call. Can. And he does it for the purpose of optimization. To understand the topic, I suggest that you carefully study the following article " Safely clearing private data ."



Let's see how such errors look in practice. We start with the WebRTC library used by Chromium. 



 void AsyncSocksProxySocket::SendAuth() { .... char * sensitive = new char[len]; pass_.CopyTo(sensitive, true); request.WriteString(sensitive); // Password memset(sensitive, 0, len); delete [] sensitive; DirectSend(request.Data(), request.Length()); state_ = SS_AUTH; }


PVS-Studio warning : V597 CWE-14 The compiler could delete the memset function call, which is used to flush the 'sensitive' object. The RtlSecureZeroMemory () function should be used to erase the private data. socketadapters.cc 677



The memset function with a probability close to 100% will be deleted by the compiler in the Release version.



Ayayay! The password will remain hanging somewhere in the memory and, theoretically, it may be sent somewhere. I seriously, it really happens .



In the same library I met 3 more similar errors. I will not describe them, since they are of the same type. I will give only the corresponding messages of the analyzer:





Recommendation



Never use the memset function to overwrite private data!



You should use specialized memory cleaning functions that cannot be deleted by the compiler during code optimization.



Note. This applies not only to C ++ programmers, but C programmers too.



In Visual Studio, for example, you can use RtlSecureZeroMemory . Starting from C11, there is a memset_s function. If necessary, you can create your own safe function. There are many examples on the Internet how to do it. Here are some of the options.



Option N1 . 



 errno_t memset_s(void *v, rsize_t smax, int c, rsize_t n) { if (v == NULL) return EINVAL; if (smax > RSIZE_MAX) return EINVAL; if (n > smax) return EINVAL; volatile unsigned char *p = v; while (smax-- && n--) { *p++ = c; } return 0; }


Option N2 . 



 void secure_zero(void *s, size_t n) { volatile char *p = s; while (n--) *p++ = 0; }


In the case of the Chromium project, it is probably rational to use the OPENSSL_cleanse function.



Conclusion



If you are writing a program in C ++ and you want to write a call to the memset function, then stop. Most likely, you will do well without this dangerous function.





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

Source: https://habr.com/ru/post/347594/

More articles:



All Articles