My own Smart pointer, double free or corruption (fasttop)

Question

I want to write my simple smart-pointer but I got a problem. This is only part of the code but I spotted the problem here:

#include <iostream>

using namespace std;

class Number{
  public:
    Number(){}
    Number(float value):_value(value){cout << "Creating Object\n";}
    ~Number(){cout << "Destroying Object\n";}
  private:
    float _value;
  }; 

  class Smart_ptr {
  public:
   Smart_ptr(){}
   Smart_ptr(Number* other):Ptr(other){}
   Smart_ptr(const Smart_ptr &other):Ptr(other.Ptr){} // PROBLEM !!!
   Smart_ptr& operator=(Smart_ptr& other){
     if(this!=&other)
     {
       if(Ptr!=NULL)delete Ptr; // is that properly writen ?
       Ptr=other.Ptr;
       other.Ptr=NULL;
     }
     return *this;
   }
   ~Smart_ptr(){if(Ptr!=NULL) delete Ptr;}
  private:
    Number* Ptr;
 };

 int main(){

  Smart_ptr number5 = new Number (5); // <--- Creating Object
  {
    Smart_ptr number6 = number5;
  }                             // <--- Destroying Object

  std::cout<<"END"<<std::endl;  
 }

I should get output like this:

Creating Object
Destroying Object
END

but instead I got double free or corruption (fasttop) and memory map error. I know that the problem is double deleting number5 (it should be deleted in the braces in main()) but I don't know how to deal with it.

This seems to be a shared_ptr<T> like smart pointer. You need to count how many references there are and check in your destructor before calling delete. You only want to call delete when there is one reference. — bku_drytt
It depends on what kind of smart pointer implementation you want to write. You are missing a counter that counts how many references point to your initial reference. Once the second instance is deleted your reference count goes down to zero and the object can be deleted. — HelloWorld
I think you'll find that when constructing number6 you are invoking the copy constructor, not the assignment operator. — quamrana
What are the desired semantics of your smart pointer? Is it supposed to ensure that only a single smart pointer instance owns an object? Or is it supposed to support shared ownership of a single object through multiple smart pointers? It seems like your current implementation does neither, which makes it a pretty useless smart pointer. — David Schwartz
@Handsomeguy123 Well, the first thing you need to do before you code something is decide what it is you want the code to do. It's not clear what this smart pointer is supposed to be for, so there's no way to make it do the right thing. You can use a smart pointer to support shared ownership. You can use a smart pointer to enforce single ownership. You can't do both at the same time. You can't split the difference either. — David Schwartz

David Schwartz David Schwartz · Accepted Answer · 2015-09-28T19:21:22

A single-ownership smart pointer can't really have a constructor that takes a const reference. Since only one smart pointer can own the object, either the original smart pointer or the new smart pointer must own the object. Since the original smart pointer currently owns the object and is const, it must own the object afterwards, leaving nothing for the new smart pointer to own.

You probably want to change:

   Smart_ptr(const Smart_ptr &other):Ptr(other.Ptr){} // PROBLEM !!!

to

   Smart_ptr(Smart_ptr &other):Ptr(other.Ptr){ other.Ptr = NULL; }

But, I beg you, please just don't do this. Just stop. Every possible mistake you can make with smart pointers has probably already been made. If you want to make your own, please start by studying auto_ptr for what not to do and unique_ptr/shared_ptr for what to do. Otherwise, you're going to have to make every mistake and, quite likely, not realize that most of them are mistakes.

My own Smart pointer, double free or corruption (fasttop)

1 Answers