Double Free at Shutdown: Rule of Three in a Packet Queue
A message bus library defines:
class Packet {
public:
explicit Packet(const char* src, std::size_t n)
: data_(new char[n]), size_(n) { std::memcpy(data_, src, n); }
~Packet() { delete[] data_; }
const char* data() const { return data_; }
std::size_t size() const { return size_; }
private:
char* data_;
std::size_t size_;
};
Applications build a std::vector<Packet> outbox, push temporary packets, and occasionally pass a Packet by value to logPacket(Packet p). A service that handles about 500 packets per minute has run for months. After a refactor that adds outbox.reserve(1024) and starts copying packets into a std::map<int, Packet>, the process aborts at shutdown with free(): double free detected in tcache 2, sometimes much earlier during a push_back. AddressSanitizer reports a heap-use-after-free in memcpy called from the constructor. The team's first fix is to add outbox.reserve(2048) and avoid logPacket.
1. Identify which special member functions the compiler still generates for `Packet`, and trace how at least three distinct copy sites lead to the double free. Explain why `reserve` does not fix it.
Read the worked answer
Because Packet has a user-declared destructor, the compiler does not implicitly declare move operations, but it still implicitly declares a copy constructor and copy assignment operator (deprecated, but generated). Both are memberwise: they copy the char* and the size. After a copy, two Packet objects own the same new char[] buffer. The first destructor calls delete[]; the second calls delete[] on the same pointer, producing the tcache double-free report. There are several copy sites: push_back of a temporary copies it, and the temporary is destroyed at the end of the full expression, leaving the vector element dangling; vector reallocation copies elements to new storage and destroys the old ones; insertion into std::map<int, Packet> copies; logPacket(Packet p) copies by value. reserve(1024) and reserve(2048) only reduce reallocation, so they delay one trigger but do not stop map insertion or by-value calls. The ASan heap-use-after-free in memcpy called from the constructor is the signature of copying from a buffer that the source's destructor already freed.
2. Write the copy constructor and copy assignment operator that make `Packet` safe, and explain the exception-safety and self-assignment properties of your implementation.
Read the worked answer
Implement the rule of three and use copy-and-swap:
Packet(const Packet& o)
: data_(new char[o.size_]), size_(o.size_) {
std::memcpy(data_, o.data_, size_);
}
Packet& operator=(const Packet& o) {
if (this != &o) {
Packet tmp(o);
swap(tmp);
}
return *this;
}
void swap(Packet& o) noexcept {
std::swap(data_, o.data_);
std::swap(size_, o.size_);
}
~Packet() { delete[] data_; }
The copy constructor allocates a new buffer before copying, so ownership is unique. The assignment operator constructs the copy first; if allocation throws, *this is unchanged, which gives the strong exception guarantee. Swapping with the temporary releases the old buffer when tmp is destroyed. The self-assignment check prevents copying from a buffer that would be freed. new char[0] is valid and returns a non-null pointer; decide whether to special-case zero for clarity. Do not forget that size_ must be copied before the memcpy uses it.
3. The team considers instead deleting copying and adding move operations. Explain how that changes the design, why `noexcept` matters for `std::vector`, and why a standard-library member is an even simpler fix.
Read the worked answer
Copying is not always the right semantic. If Packet is a handle to a buffer, either give it value semantics as above or make it move-only: Packet(Packet&&) noexcept, Packet& operator=(Packet&&) noexcept, and Packet(const Packet&) = delete;. The move operations steal the pointer and set the source to nullptr, so destruction is safe. Marking them noexcept matters because std::vector uses move_if_noexcept during reallocation: if the move constructor is not noexcept and the type is copy-constructible, the vector may copy instead, bringing back the double free. The best structural fix is to remove manual ownership: hold std::vector<char> data_ or std::string data_ as a member and let the standard library implement the rule of five. Then Packet needs no destructor, copy constructor, or assignment operator, and std::map<int, Packet> and by-value calls become safe. Add a delete on copying only if duplicate packets would be a logic error, and document the choice.