We would like to suggest reading the series of articles dedicated to the recommendations on writing code of high quality using the examples of errors found in the Chromium project. This is the first part which will be devoted to the memset function.
We must do something about the memset function in C++ programs! Rather, it is clear what we must do at once - we have to stop using it. I wrote the article "The most dangerous function in the C/C++ world" at the time. I think it is easy to guess that this article will be exactly about memset.
However, I will not waste words, so I am going to demonstrate the danger of this function once again with the examples. The code of the Chromium project and the libraries used in it are of very high quality. Google developers pay much attention to the tests and the use of various tools for detecting defects. For instance, Google has developed such tools as AddressSanitizer, ThreadSanitizer and MemorySanitizer.
As a result, there are few errors related to memset function, but sadly, that they are still presented. Despite the errors, it is a very qualitative project!
Let's see what I noticed during studying the report issued by the PVS-Studio. As I wrote in the introductory article, I looked through the report quite fluently, so there may be other, unnoticed errors. However, the found defects will be enough for us to discuss the malloc function.
The first type of errors is related to the 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 may be classified as CWE-682: Incorrect Calculation.
The first example of the error is taken directly from the Chromium project code. Note that the arrays text and unmodified_text 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 is 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 example of the error is taken from the WebRTC library used in Chromium. The error is similar to the previous bug: it is not taken into account that the elements in the array are of int64_t type.
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 is set to null, and one byte in the second element.
PVS-Studio warning: V512 CWE-682 A call of the 'memset' function will lead to underflow of the buffer '_jumpBuf'. rtt_filter.cc 52
Recommendation
To avoid such errors do not use memset any more. You may be really careful, but sooner or later errors will get passed around in your project anyway. In Chromium the situation is quite favorable. Nevertheless, in other projects it is a very common problem (proof).
Yes, it is impossible to avoid the use of memset in C code. However, if we are talking about C++, let's forget about this function. Do not use memset function in C++ code. Do not use, end of story.
How to replace the memset call?
Firstly, you can use the std: fill function. In this case, a filling of an array will look like this:
fill(begin(key_event->text), end(key_event->text), 0);Secondly, you should not often use a call of special functions. Typically, memset function serves to initialize local arrays and structures. Classic example:
HDHITTESTINFO hhti;
memset(&hhti, 0, sizeof(hhti));But you can write much easier and safer:
HDHITTESTINFO hhti = {};If we are talking about the constructor:
class C
{
int A[100];
public:
C() { memset(A, 0, sizeof(A)); }
};It is possible to write as follows:
class C
{
int A[100] = {};
public:
C() { }
};Developers sometimes forget that the second argument sets the value of a single byte that is used to fill the buffer. What is confusing is that the second argument of the memset function is of int type. As a result, such errors appear, which can be classified as CWE-628: Function Call with Incorrectly Specified Arguments.
Let's look at the example of such an 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:
A developer decided to fill the memory blocks with the 0x0BADC0DE value, so that it was easier to understand the situation when debugging. However, the memory space will be filled with the byte with the 0xDE value.
What a programmer does in code is a low-level operation and here it is harder to do without memset than in the situations described earlier. The buffers' size is not multiple to 4 bytes, so a usage of std::fill will not work as earlier. A programmer will have to write and use his 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 any special recommendation. Once again, we have seen that memset function is actually not needed here, as it does not solve the programmer task.
Memset function is used for clearing of private data after it is no longer needed. This is wrong. If a buffer with private data is not used in any way after the call of memset, the compiler may remove the call to this function. This defect is classified as CWE-14: Compiler Removal of Code to Clear Buffers.
I already anticipate the objection that a compiler cannot remove a memset calling. It can. It does it in terms of optimization. To understand the topic, I would like to suggest to carefully study the following article "Safe Clearing of Private Data".
Let's see how these errors look like in practice. We will start the WebRTC library used in 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 'sensitive' object. The RtlSecureZeroMemory() function should be used to erase the private data. socketadapters.cc 677
Memset function will be removed by a compiler in a Release version with a probability close to 100%.
Ow ow ow! The password will remain hanging out somewhere in memory and, theoretically, can be sent somewhere. I am serious, this really happens.
In the same library I came across 3 more similar errors. I will not describe them because they are similar. I will just cite only the appropriate analyzer messages:
Recommendation
Never use the memset function for clearing private data!
You should use special memory clearing functions that the compiler is not allowed to remove for its optimization purposes.
Note. This concerns not only C++ programmers, but also C programmers as well.
Visual Studio, for instance, offers the RtlSecureZeroMemory function. Starting with C11, you can use the memset_s function. If necessary, you can create your own secure function. There are a lot of examples in the internet, how to write 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 Chromium, probably, it is reasonable to use the function OPENSSL_cleanse.
If you are writing a C++ program and you want to write a function call to memset, then stop. Most likely, you will do great without this dangerous function.