Memory Leak in Status Class and Usage in leveldb

[YancyLii]

Description

There appears to be a potential memory leak issue within the Status class of the leveldb project. This issue was discovered during fuzz testing and further analysis of the source code. The memory leak is primarily associated with the dynamic memory allocation and management within the Status class' constructors and CopyState method.

Reproduce

Memory leaks were detected using a fuzzing tool, which revealed issues during operations involving error status creation and management.
extern "C" int LLVMFuzzerTestOneInput(const uint8_t data, size_t size) {
leveldb::Slice slice1(reinterpret_cast<const char>(data), size);
leveldb::Slice slice2(reinterpret_cast<const char*>(data), size);
leveldb::Status status = leveldb::Status::NotFound(slice1, slice2);
leveldb::Iterator* iterator = leveldb::NewErrorIterator(status);
return 0;
}

Analysis

1.Status::CopyState Method:
This method allocates memory for a new state string but does not have a corresponding deallocation mechanism. If the returned pointer from CopyState is not properly managed by the calling function, it can lead to memory leaks.
2.Status Constructor:
The constructor for creating an error Status object dynamically allocates memory to hold the error message and associated data. However, there is no explicit destructor in the Status class to deallocate this memory once the Status object is no longer in use.

Potential Impact

If these memory allocations are not properly managed, it can lead to memory leaks, which may affect the performance and reliability of applications using the leveldb library, especially in long-running applications where repeated status errors might occur.

Suggested Fix

Implement a destructor in the Status class that properly deallocates the state_ member if it is not nullptr.
Ensure that any usage of CopyState handles the allocated memory correctly, preferably by using smart pointers or ensuring that the memory is deleted when no longer needed.
Additional Information
The memory leak is evident from both fuzz test results and direct code analysis. This issue could potentially be resolved by revising memory management practices within the Status class implementation.

[fmorgner]

I do not see how this is an issue. The static member function Status::CopyState is private, meaning it can only ever be called from within any member function of the Status class. From what I can tell, there are only two call sites for this function:

1. The copy constructor of Status
2. The copy assignment operator of Status

Looking at the implementation of the copy constructor, Status::CopyState is invoked whenever the source object's state_ non-static member variable is not nullptr. In that case, the target object's state_ non-static member variable is initialized by the return value of Status::CopyState, which is a newly allocated array of characters, containing the same data as the source object's state_. Since this happens in the copy constructor of Status, the state_ member is not yet initialized to anything, so there is nothing to destroy.

Looking at the implementation of the copy assignment operator. The target object's state_ is deleted if it differs from the source object's state_. Afterwards, the same logic as for the copy constructor applies (with the delta of state_ having previously been initialized, but since it has also been deleted, this is not an issue).

However, there is no explicit destructor in the Status class to deallocate this memory once the Status object is no longer in use.

This is plainly incorrect. The destructor of Status is defined here. The only thing this destructor does is to delete the state_ member. This destructor has been present for approximately the past 13 years.

The memory leak is evident from both fuzz test results and direct code analysis.

The code example you show leaks memory itself, by allocating a new leveldb::Iterator, storing the returned pointer in a local variable and not freeing it.

Also, from your code, this looks strange:

leveldb::Slice slice1(reinterpret_cast<const char>(data), size);
leveldb::Slice slice2(reinterpret_cast<const char*>(data), size);

Why would you treat data as a character once and then as a pointer to a character. It seems unlikely to me, that size would be correct for both cases. Regardless, the first line would not compile, since this is not a valid application of reinterpret_cast. Looking at the formatting of the code, it appears as if the asterisk has been swallowed by GitHub, so your code may actually be valid but just rendered badly.

[omair35]

Running this code causes boost unit testing framework to report memory leak:

leveldb::DB* db = nullptr;
leveldb::Options options;
options.create_if_missing = true;
leveldb::Status status = leveldb::DB::Open(options, "testdb.db", &db);
BOOST_CHECK(status.ok());
delete db;

Run output:
*** No errors detected
Detected memory leaks!
Dumping objects ->
{6591} normal block at 0x00000153CDB57180, 16 bytes long.
Data: < _E > 80 11 5F 45 F7 7F 00 00 00 00 00 00 00 00 00 00
Object dump complete.

Compiler & Platform:
Compiled from Boost version 1.85.0 with dynamic linking to Boost.Test

• Compiler: Microsoft Visual C++ version 14.3
• Platform: Win32
• STL : Dinkumware standard library version 650