Can't push_back a class containing an ofstream pointer to a vector

Question

I've come across a weird segfault while trying to push a class containing a pointer of an ofstream to a vector. I have narrowed the problem down to the simplest test case possible:

Test.h

#ifndef __TEST_
#define __TEST_

#include <fstream>
#include <string>

class Test {
public:
    Test(std::string path);
    ~Test();
private:
    ofstream* ofstr;
}

#endif

Test.cpp

#include "Test.h"

Test::Test(std::string path) {
    ofstr = new ofstream(path, std::ios::app);
}

Test::~Test() {
    delete ofstr;
}

main.cpp

#include <vector>
#include "Test.h"

int main() {
    Test test("hello.txt");
    std::vector<Test> vec;
    vec.push_back(test); // segfaults
}

I think that the segfault has to do with the destructor for Test, but I'm not sure why. The segfault occurs when I use emplace_back as well.

@πάνταῥεῖ I'm changing the dupe target as it is a rule of three violation since the OP has a pointer to the stream — NathanOliver, Jul 27 '18 at 21:11
You need to learn [the rules of three, five and zero](http://en.cppreference.com/w/cpp/language/rule_of_three). — Some programmer dude, Jul 27 '18 at 21:11
You have a double delete of your pointer due to violation of the rule of three. That is undefined behaviour. — john, Jul 27 '18 at 21:12
@NathanOliver, unfortunately, the OP's problem is deeper than what one is advised to do to follow The Rule of Three. They can't use `ofstr = new ofstream(*copy.ofstr);` in the copy constructor or the copy assignment operator. — R Sahu, Jul 27 '18 at 21:16
Your code segfaults because both variable `test` in main and the vector element have a pointer that point to the same element, and when they are destructed, you are destructing the ofstream element twice. If you want multiple objects of class `Test` to share a pointer to ofstream, you should use a `std::shared_pointer`, you can read more about it here https://en.cppreference.com/w/cpp/memory/shared_ptr — Kaldrr, Jul 27 '18 at 21:16
@RSahu Want me to reopen then? suggest they use a `std::shared_pointer`? — NathanOliver, Jul 27 '18 at 21:19
Notice: I removed the dupe as none of the suggestions in the rule of three dupe will work with a `ofstream *`. — NathanOliver, Jul 27 '18 at 21:29
Totally unrelated side note: Leading underscores and double underscores anywhere have special meaning. More on that in [What are the rules about using an underscore in a C++ identifier?](https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier) It's rare that breaking this rule bites, but when it does the results can be inscrutable. — user4581301, Jul 27 '18 at 21:41
Don’t put two underscores in front of your include guards, it’s forbidden by the standard (these identifiers are reserved for the compiler implementation). — Konrad Rudolph, Jul 27 '18 at 21:43

score 2 · Accepted Answer · answered Jul 27 '18 at 21:38

The first problem that your code suffers from is that you are not following The Rule of Three.

However, your problem is deeper than what is suggested to follow The Rule of Three.

Say your class had a different member variable than std::ofstream*.

class Test {
  public:
    Test(int in) : ptr(new int(in)) {}
    ~Test();
private:
    int* ptr;
}

You can update that class to follow the rule of three by making the sure that you do the right thing in the copy constructor and the copy assignment operator. In both of those, you'll have to use something along the lines of:

ptr = new int(*copy.ptr);

That works for most types. However, that does not work for std::ofstream since std::ofstream does not have a copy constructor or a virtual function that can return a pointer by cloning the object.

In your case, neither of the following is an option.

ofstr = new ofstream(*copy.ofstr);
ofstr = copy.ofstr->clone();

To get around that problem, you can use std::shared_ptr<std::ofstream>.

class Test {
  public:
    Test(std::string path);
    ~Test();
  private:
    std::shared_ptr<std::ofstream> ofstr;
}

When you do that, not only do you fix your problem but also, you can let the compiler generated destructor, copy constructor, and copy assignment operator do the right thing. Your class definition can be simplified to:

class Test {
  public:
    Test(std::string path);
  private:
    std::shared_ptr<std::ofstream> ofstr;
}

Can't push_back a class containing an ofstream pointer to a vector

1 Answers1