Is extending a base class with non-virtual destructor dangerous?
Asked Answered
S

6

12

In the following code:

class A {
};
class B : public A {
};
class C : public A {
   int x;
};

int main (int argc, char** argv) {
   A* b = new B();
   A* c = new C();

   //in both cases, only ~A() is called, not ~B() or ~C()
   delete b; //is this ok?
   delete c; //does this line leak memory?

   return 0;
}

When calling delete on a class with a non-virtual destructor with member functions (like class C), can the memory allocator tell what the proper size of the object is? If not, is memory leaked?

Secondly, if the class has no member functions, and no explicit destructor behaviour (like class B), is everything ok?

I ask this because I wanted to create a class to extend std::string, (which I know is not recommended, but for the sake of the discussion just bear with it), and overload the +=, + operator. -Weffc++ gives me a warning because std::string has a non virtual destructor, but does it matter if the sub-class has no members and does not need to do anything in its destructor?

FYI the += overload was to do proper file path formatting, so the path class could be used like:

class path : public std::string {
    //... overload, +=, +
    //... add last_path_component, remove_path_component, ext, etc...
};

path foo = "/some/file/path";
foo = foo + "filename.txt";
std::string s = foo; //easy assignment to std::string
some_function_taking_std_string (foo); //easy implicit conversion
//and so on...

I just wanted to make sure someone doing this:

path* foo = new path();
std::string* bar = foo;
delete bar;

would not cause any problems with memory allocation?

Scenario answered 8/4, 2010 at 2:10 Comment(0)
O
5

So everyone has been saying you cant do it - it leads to undefined behaviour. However there are some cases where it is safe. If you are never creating instances of your class dynamically then you should be OK. (i.e. no new calls)

That said, it is generally considered a bad thing to do as someone might try to create one polymorphically at some later date. ( You may be able to protect against this by having a private unimplemented operator new, but I'm not sure. )

I have two examples where I don't hate deriving from classes with non-virtual destructors. The first is creating syntactic sugar using temporaries ... here's a contrived example.

class MyList : public std::vector<int>
{
   public:
     MyList operator<<(int i) const
     {
       MyList retval(*this);
       retval.push_back(i);
       return retval;
     }
   private: 
     // Prevent heap allocation
     void * operator new   (size_t);
     void * operator new[] (size_t);
     void   operator delete   (void *);
     void   operator delete[] (void*);
};

void do_somthing_with_a_vec( std::vector<int> v );
void do_somthing_with_a_const_vec_ref( const std::vector<int> &v );

int main()
{
   // I think this slices correctly .. 
   // if it doesn't compile you might need to add a 
   // conversion operator to MyList
   std::vector<int> v = MyList()<<1<<2<<3<<4;

  // This will slice to a vector correctly.
   do_something_with_a_vec( MyList()<<1<<2<<3<<4 );

  // This will pass a const ref - which will be OK too.
   do_something_with_a_const_vec_ref( MyList()<<1<<2<<3<<4 );

  //This will not compile as MyList::operator new is private
  MyList * ptr = new MyList();
}

The other valid usage I can think of comes from the lack of template typedefs in C++. Here's how you might use it.

// Assume this is in code we cant control
template<typename T1, typename T2 >
class ComplicatedClass
{
  ...
};

// Now in our code we want TrivialClass = ComplicatedClass<int,int>
// Normal typedef is OK
typedef ComplicatedClass<int,int> TrivialClass;

// Next we want to be able to do SimpleClass<T> = ComplicatedClass<T,T> 
// But this doesn't compile
template<typename T>
typedef CompilicatedClass<T,T> SimpleClass;

// So instead we can do this - 
// so long as it is not used polymorphically if 
// ComplicatedClass doesn't have a virtual destructor we are OK.
template<typename T>
class SimpleClass : public ComplicatedClass<T,T>
{
  // Need to add the constructors we want here :(
  // ...
   private: 
     // Prevent heap allocation
     void * operator new   (size_t);
     void * operator new[] (size_t);
     void   operator delete   (void *);
     void   operator delete[] (void*);
}

Heres a more concrete example of this. You want to use std::map with a custom allocator for many different types, but you dont want the unmaintainable

std::map<K,V, std::less<K>, MyAlloc<K,V> >

littered through your code.

template<typename K, typename V>
class CustomAllocMap : public std::map< K,V, std::less<K>, MyAlloc<K,V> >
{
  ...
   private: 
     // Prevent heap allocation
     void * operator new   (size_t);
     void * operator new[] (size_t);
     void   operator delete   (void *);
     void   operator delete[] (void*);
}; 

MyCustomAllocMap<K,V> map;
Oddfellow answered 8/4, 2010 at 3:26 Comment(6)
Both examples don't protect anyone from deleting through the base pointer, getting undefined behavior. For the first one, it's better to have a private container and a conversion operator. For the second, a struct with a typedef within servers the same purpose.Paratyphoid
@GMan I agree that deletion would be bad, and specifically say you should not use these objects with allocations through new. If your're deleting something that was not newed you run into UB no matter what. I also suggest how to prevent the calling of new .. but I've not tried that out myself.Oddfellow
@GMan also, I find "CustomAllocMap<K,V>::type map_" a little less intuitive to read than "CustomAllocMap<K,V> map_" ... but the typedef in a templated struct is a good option.Oddfellow
@Michael: I agree, but I always typedef that stuff anyway, so you'd never know the difference: typedef CustomAllocMap<K, V>::type map_type; for map_type map; // how did map_type come about? who knowsParatyphoid
So I've verified that privating operator new prevents heap allocation via new, see #125356 so I've updated my examples to use it too.Oddfellow
Even though I've been convinced by GMan et al. that this should not be done and is in general bad design. I wanted to bump this answer for anyone who needs to do it and wants to do it safely.Scenario
P
13

No, it's not safe to publically inherit from classes without virtual destructors, because if you delete the derived through a base you enter undefined behavior. The definition of the derived class is irrelevant (data members or not, etc.):

§5.3.5/3: In the first alternative (delete object), if the static type of the operand is different from its dynamic type, the static type shall be a base class of the operand’s dynamic type and the static type shall have a virtual destructor or the behavior is undefined. (Emphasis mine.)

Both of those examples in your code lead to undefined behavior. You can inherit non-publicly, but that obviously defeats the purpose of using that class then extending it. (Since it's not longer possible to delete it through a base pointer.)

This is (one reason*) why you shouldn't inherit from standard library classes. The best solution is to extend it with free-functions. In fact, even if you could you should prefer free-functions anyway.


*Another being: Do you really want to replace all your string usage with a new string class, just to get some functionality? That's a lot of unnecessary work.

Paratyphoid answered 8/4, 2010 at 2:13 Comment(9)
ok, thanks, so the behavior is undefined. I'll take note of that.Scenario
I was hoping that since the sub-class destructor was effectivly a no-op, it would be ok that it was not called.Scenario
@Akusete: That doesn't make a difference: §5.3.5/3: In the first alternative (delete object), if the static type of the operand is different from its dynamic type, the static type shall be a base class of the operand’s dynamic type and the static type shall have a virtual destructor or the behavior is undefined."Paratyphoid
Quoting 5.3.5 without qualification is too strong: if he really wants to subclass a type without a virtual destructor, he can safely do so using private inheritance, since the static type of the operand will never be the static type with no virtual destructor.Nonconcurrence
@jemfinch: Since he's extending the class, nonpublic inheritance isn't an option. I agree that my text, without that assumption, reads poorly. Fixed.Paratyphoid
As long as you don't delete the object via the base class pointer, however, you're totally safe. E.g. extending std::string via inheritance and then only using that extended class on the stack or with my_string pointers will not cause undefined behavior. (But I'd still prefer aggregation to inheritance in pretty much every case, if needs were more complicated than just adding a few free functions.)Legwork
IIRC, in C++ the pattern to use for extending non-extendable classes is the has-a with a conversion operator that returns the reference to the underlying member of the extended type. Thus class Foo { std::string m_value; public: operator std:string&()() { return m_value; } operator const std::string&()() const { return m_value; } ... };Ninebark
@GManNickG, could you expand on or link to an expansion on the specific idea in your comment that nonpublic inheritance isn't an option when extending a class? I'm not challenging the idea, I just want to be clear on what you meant or the implications of mistakenly trying to do extension with non-public inheritance. Thanks.Francescafrancesco
@JeremyW.Murphy: Old answer. :) IIRC, OP was trying to define a type X which is a Y, so that anywhere something asked for a Y he could provide an X. The reason for X was for some addition convenience functions. If X were a Y, but only privately, then X's couldn't be used where Y's were called for. If X was a Y and didn't need to act like a Y, then nonpublic inheritance would be fine.Paratyphoid
O
5

So everyone has been saying you cant do it - it leads to undefined behaviour. However there are some cases where it is safe. If you are never creating instances of your class dynamically then you should be OK. (i.e. no new calls)

That said, it is generally considered a bad thing to do as someone might try to create one polymorphically at some later date. ( You may be able to protect against this by having a private unimplemented operator new, but I'm not sure. )

I have two examples where I don't hate deriving from classes with non-virtual destructors. The first is creating syntactic sugar using temporaries ... here's a contrived example.

class MyList : public std::vector<int>
{
   public:
     MyList operator<<(int i) const
     {
       MyList retval(*this);
       retval.push_back(i);
       return retval;
     }
   private: 
     // Prevent heap allocation
     void * operator new   (size_t);
     void * operator new[] (size_t);
     void   operator delete   (void *);
     void   operator delete[] (void*);
};

void do_somthing_with_a_vec( std::vector<int> v );
void do_somthing_with_a_const_vec_ref( const std::vector<int> &v );

int main()
{
   // I think this slices correctly .. 
   // if it doesn't compile you might need to add a 
   // conversion operator to MyList
   std::vector<int> v = MyList()<<1<<2<<3<<4;

  // This will slice to a vector correctly.
   do_something_with_a_vec( MyList()<<1<<2<<3<<4 );

  // This will pass a const ref - which will be OK too.
   do_something_with_a_const_vec_ref( MyList()<<1<<2<<3<<4 );

  //This will not compile as MyList::operator new is private
  MyList * ptr = new MyList();
}

The other valid usage I can think of comes from the lack of template typedefs in C++. Here's how you might use it.

// Assume this is in code we cant control
template<typename T1, typename T2 >
class ComplicatedClass
{
  ...
};

// Now in our code we want TrivialClass = ComplicatedClass<int,int>
// Normal typedef is OK
typedef ComplicatedClass<int,int> TrivialClass;

// Next we want to be able to do SimpleClass<T> = ComplicatedClass<T,T> 
// But this doesn't compile
template<typename T>
typedef CompilicatedClass<T,T> SimpleClass;

// So instead we can do this - 
// so long as it is not used polymorphically if 
// ComplicatedClass doesn't have a virtual destructor we are OK.
template<typename T>
class SimpleClass : public ComplicatedClass<T,T>
{
  // Need to add the constructors we want here :(
  // ...
   private: 
     // Prevent heap allocation
     void * operator new   (size_t);
     void * operator new[] (size_t);
     void   operator delete   (void *);
     void   operator delete[] (void*);
}

Heres a more concrete example of this. You want to use std::map with a custom allocator for many different types, but you dont want the unmaintainable

std::map<K,V, std::less<K>, MyAlloc<K,V> >

littered through your code.

template<typename K, typename V>
class CustomAllocMap : public std::map< K,V, std::less<K>, MyAlloc<K,V> >
{
  ...
   private: 
     // Prevent heap allocation
     void * operator new   (size_t);
     void * operator new[] (size_t);
     void   operator delete   (void *);
     void   operator delete[] (void*);
}; 

MyCustomAllocMap<K,V> map;
Oddfellow answered 8/4, 2010 at 3:26 Comment(6)
Both examples don't protect anyone from deleting through the base pointer, getting undefined behavior. For the first one, it's better to have a private container and a conversion operator. For the second, a struct with a typedef within servers the same purpose.Paratyphoid
@GMan I agree that deletion would be bad, and specifically say you should not use these objects with allocations through new. If your're deleting something that was not newed you run into UB no matter what. I also suggest how to prevent the calling of new .. but I've not tried that out myself.Oddfellow
@GMan also, I find "CustomAllocMap<K,V>::type map_" a little less intuitive to read than "CustomAllocMap<K,V> map_" ... but the typedef in a templated struct is a good option.Oddfellow
@Michael: I agree, but I always typedef that stuff anyway, so you'd never know the difference: typedef CustomAllocMap<K, V>::type map_type; for map_type map; // how did map_type come about? who knowsParatyphoid
So I've verified that privating operator new prevents heap allocation via new, see #125356 so I've updated my examples to use it too.Oddfellow
Even though I've been convinced by GMan et al. that this should not be done and is in general bad design. I wanted to bump this answer for anyone who needs to do it and wants to do it safely.Scenario
M
2

Problem can occur if you store the memory address of a derived type inside the base type and then call delete on the base type:

B* b = new C();
delete b;

If B had a virtual destructor then C's destructor would be called and then B's. But without a virtual destructor you have undefined behavior.

the following 2 deletes cause no problem:

B* b = new B();
delete b;
C* c = new C()
delete c;
Men answered 8/4, 2010 at 2:11 Comment(4)
class B has no member variables, and no necessary destructor behavior... so if I don't care if ~B() is called, is there a problem? I guess I'm asking, if delete needs to do any extra work for the subclassScenario
@Akusete: You can just be VERY careful not to store derived types inside the base type pointers and call delete on the base types.Men
If there is no way to make it semantically impossible I could bet it would happen eventually :) ... a nice way to do path formatting is not worth even one extra bug.Scenario
@Akusete: It doesn't matter what the definition of class B is, if the base has no virtual destructor and you delete a derived class through the base, it's undefined behavior. See my answer for quote.Paratyphoid
A
2

This is not an answer to your question, but to the problem you were trying to solve (path formatting). Take a look at boost::filesystem, which has a better way to concatenate paths:

boost::filesystem::path p = "/some/file/path";
p /= "filename.txt";

You can then retrieve the path as a string in both platform-neutral and platform-specific formats.

The best part is that it has been accepted into TR2, which means it will become part of the C++ standard in the future.

Ammoniac answered 8/4, 2010 at 5:18 Comment(0)
N
1

It is only safe to privately inherit from base classes without a virtual destructor. Public inheritance would make it possible for a derived class to be deleted through a base class pointer, which is undefined behavior in C++.

This is one of the only reasonable uses of private inheritance in C++.

Nonconcurrence answered 8/4, 2010 at 2:14 Comment(1)
Definitely... not. Using composition shall be preferred to using private inheritance. The only reasonable use of private inheritance I know is to trigger Empty Base Optimization... and it's more about performance than actual design.Kneehole
K
1

To add to what's been said.

  • You are actually considering adding methods to a class that's already considered bloated.
  • You want your path class to be considered as a string even though a number of operations would make little sense

I would propose that you use Composition over Inheritance. This way you will only reimplement (forward) those operations which really make sense for your class (I don't think you really need the subscript operator, for example).

Also, you might consider using a std::wstring instead, or perhaps a ICU string, to be able to handle more than ASCII characters (I am a nitpick, but then I am French and lower ASCII is insufficient for the French language).

It is, really, a matter of encapsulation. If you decide to handle UTF-8 characters properly one day and change your base class... chances are your clients will hang you by the feet. On the other hand, if you have used composition, they'll never have any issue as long as you tread carefully with the interface.

Finally, as has been suggested, free functions shall lead the way. Once again because they provide better encapsulation (as long as they are not friend...).

Kneehole answered 8/4, 2010 at 6:56 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.