How to avoid memory leak with string pointer in virtual class

Question

I'm writing a base class that holds a pointer to a string, and now I'm trying to write a derived class with an assignment operator that replaces it with the new value. For this particular project, I'm required to store the string as a pointer.

At first, I tried to use new, but that failed to compile.

kolo& operator=(const kolo& ref){
  if(this!=&ref){
    delete kolor;
    kolor=new string(ref.kolor);
    r=ref.r;
  }
  return *this;
}

I've changed it to use the assignment operator, which successfully compiles. However, valgrind is now reporting a memory leak, among other problems.

kolo& operator=(const kolo& ref){
  if(this!=&ref){
    delete kolor;
    *kolor=*ref.kolor;
    r=ref.r;
  }
  return *this;
}

Why is valgrind reporting a memory leak from my assignment operator?

My complete code (which is in polish):

class figura {
  protected:
    string *kolor;
  public:
      figura() : kolor(new string("nic")) {}

      figura(const string& a1) : kolor(new string(a1)) {}

      virtual double pole() const = 0;

      virtual void wypisz(ostream& out) const {
        out<<*kolor;
      }

      friend ostream& operator<<(ostream& out,const figura& r);

      virtual ~figura() { delete kolor; }
    };

class kolo: public figura {
  protected:
    unsigned r;

  public:
    kolo(): figura(), r(0) {}

    kolo(const string& a1, const unsigned a2)  :figura(a1), r(a2) {}

    kolo(const kolo& ref) : figura(ref), r(ref.r) {}

    kolo& operator=(const kolo& ref) {
      if(this!=&ref) {
        delete kolor;
        *kolor=*ref.kolor;
        r=ref.r;
      }
      return *this;
    }

    double pole()const{
      return 3.14*r*r;
    }

    void wypisz(ostream& out)const{
      out<<"Kolor: "<<*kolor<<" "<<"Skladowe: "<<r<<endl;
    }

    friend ostream& operator<<(ostream& out,const kolo& r);
};

And the valgrind output:

==1774== Invalid read of size 8
==1774==    at 0x49ABE34: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
==1774==    by 0x10AB7F: figura::~figura() (test.cpp:14)
==1774==    by 0x10B155: kolo::~kolo() (test.cpp:20)
==1774==    by 0x10A7B4: main (test.cpp:84)
==1774==  Address 0x4dc1c80 is 0 bytes inside a block of size 32 free'd
==1774==    at 0x483C1CF: operator delete(void*, unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1774==    by 0x10AB8C: figura::~figura() (test.cpp:14)
==1774==    by 0x10B155: kolo::~kolo() (test.cpp:20)
==1774==    by 0x10B175: kolo::~kolo() (test.cpp:20)
==1774==    by 0x10A78B: main (test.cpp:102)
==1774==  Block was alloc'd at
==1774==    at 0x483AE63: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1774==    by 0x10AAD8: figura::figura(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (test.cpp:8)
==1774==    by 0x10AC27: kolo::kolo(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int) (test.cpp:25)
==1774==    by 0x10A467: main (test.cpp:84)
==1774== 
==1774== Invalid free() / delete / delete[] / realloc()
==1774==    at 0x483C1CF: operator delete(void*, unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1774==    by 0x10AB8C: figura::~figura() (test.cpp:14)
==1774==    by 0x10B155: kolo::~kolo() (test.cpp:20)
==1774==    by 0x10A7B4: main (test.cpp:84)
==1774==  Address 0x4dc1c80 is 0 bytes inside a block of size 32 free'd
==1774==    at 0x483C1CF: operator delete(void*, unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1774==    by 0x10AB8C: figura::~figura() (test.cpp:14)
==1774==    by 0x10B155: kolo::~kolo() (test.cpp:20)
==1774==    by 0x10B175: kolo::~kolo() (test.cpp:20)
==1774==    by 0x10A78B: main (test.cpp:102)
==1774==  Block was alloc'd at
==1774==    at 0x483AE63: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1774==    by 0x10AAD8: figura::figura(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (test.cpp:8)
==1774==    by 0x10AC27: kolo::kolo(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int) (test.cpp:25)
==1774==    by 0x10A467: main (test.cpp:84)
==1774== 
==1774== 
==1774== HEAP SUMMARY:
==1774==     in use at exit: 32 bytes in 1 blocks
==1774==   total heap usage: 14 allocs, 14 frees, 74,088 bytes allocated
==1774== 
==1774== 32 bytes in 1 blocks are definitely lost in loss record 1 of 1
==1774==    at 0x483AE63: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1774==    by 0x10AA29: figura::figura() (test.cpp:7)
==1774==    by 0x10ABE1: kolo::kolo() (test.cpp:24)
==1774==    by 0x10A561: main (test.cpp:88)
==1774== 
==1774== LEAK SUMMARY:
==1774==    definitely lost: 32 bytes in 1 blocks
==1774==    indirectly lost: 0 bytes in 0 blocks
==1774==      possibly lost: 0 bytes in 0 blocks
==1774==    still reachable: 0 bytes in 0 blocks
==1774==         suppressed: 0 bytes in 0 blocks
==1774== 
==1774== For lists of detected and suppressed errors, rerun with: -s
==1774== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

`new string`? Do you really need pointer of string instead of just `std::string`? — Jarod42, Jan 16 '20 at 20:52
[Edit] the question to include the error(s) you get in the first version of your function. I suspect, however, that the problem is that you do not dereference `ref.kolor` when trying to make a copy of it: `new string(*ref.kolor);`. The second version results in Undefined Behavior since you access a pointer you've deleted. — 1201ProgramAlarm, Jan 16 '20 at 21:04
@Spectra In the first implementation of the operator you should use kolor=new string( *ref.kolor ); — Vlad from Moscow, Jan 16 '20 at 21:07
@Spectra You need correctly to write also the copy constructor. And the memory leak can be produced by some other code. — Vlad from Moscow, Jan 16 '20 at 21:16

score 1 · Answer 1 · answered Jan 16 '20 at 20:59

Get rid of pointer, and so have Rule of 0 :)

class figura
{
protected:
    string kolor;
public:
    figura() : kolor("nic") {}
    /*explicit*/ figura(const string& s) : kolor(s){}
    virtual ~figura() = default;

    virtual double pole()const = 0;
    virtual void wypisz(ostream& out)const { out<< kolor; }

    friend ostream& operator<<(ostream& out,const figura& r);
};

class kolo:public figura
{
protected:
    unsigned r = 0;
public:
    kolo() : figura(), r(0){}
    kolo(const string& s,const unsigned r):figura(s), r(r){}
    kolo(const kolo& ref) = default;
    kolo& operator=(const kolo& ref) = default;
    double pole()const override { return 3.14*r*r; }
    void wypisz(ostream& out)const override {
        std::out << "Kolor: " << kolor << " " << "Skladowe: " << r << std::endl;
    }
    friend ostream& operator<<(ostream& out,const kolo& r);
};

Korosia · Accepted Answer · 2020-01-17T08:02:01.530

The rule of three says that if you implement any of destructor, copy-assignment, or copy-constructor, you need to implement all three. You've done this for kolo, but not for figurea

Somewhere in your code, kolo is being copy-constructed (e.g. inside a vector), and using the default version of figureas copy-contructor. This will simply copy the pointer. Now, the original string has been lost, which is causing the leak. The new string is being pointed to by two different objects, so will be destroyed twice. The second destroy is what is causing the invalid read and delete.

There are various ways to implement both copy-assignment and copy-constructor without having to repeat code. Try this question, for example.

I would also expect that, since figurea is the one that actually has kolor on it, the copy assignment for figurea should be responsible for it, not the one for kolor. That way, if you derive another class from figurea, you don't need to reimplement the kolor copying.

It sounds like you're required to use raw pointers, but generally the best way to write memory-management is to not write memory management. Just using string directly, and therefore the built-in memory management of string, would make things a lot easier. Alternatively, using smart pointers, like shared_ptr allow you to do the copying and deletion automatically.

Thank you a lot, that is working( i add copy constructor ) in class figura! — Spectra, Jan 17 '20 at 12:54

How to avoid memory leak with string pointer in virtual class

2 Answers2