Virtual destructor warning cleanup

[Jeffrey Walton]

I think we've cleared most issues and warnings. There is one waning that was delayed while the issue was studied, and that is the "virtual destructor" warning. At minimum, the warning an annoyance. At its worse, its a security bug because of slicing *if* sensitive material is not wiped from memory. Somewhere in between is the simple memory leak due to slicing.

I believe what happened is constructors and destructors were sometimes not written because the compiler will provide them. The "sometimes" appears to be limited to stack based allocations (i.e., if 'new' was used to allocate a member variable, then a virtual dtor was written). However, the default destructor is not virtual in keeping with C/C++'s "don't pay for it if you don't need it" mantra.

Crypto++ has a mechanism to forgo vtables, but it was not applied to any of the classes below. The mechanism is CRYPTOPP_NO_VTABLE.

The patch below clears the warnings related to the library and the test suite. There could be more places a virtual dtor is needed, but I don't have test cases that highlights them.

Is anyone opposed to the patch below? Or any other comments?

**********

Index: ec2n.h
===================================================================
--- ec2n.h    (revision 562)
+++ ec2n.h    (working copy)
@@ -104,6 +104,9 @@
     void SetCurve(const EC2N &ec) {m_ec = ec;}
     const EC2N & GetCurve() const {return m_ec;}
 
+    // default dtor provided by compiler is not virtual
+    virtual ~EcPrecomputation() {}
+
 private:
     EC2N m_ec;
 };
Index: ecp.h
===================================================================
--- ecp.h    (revision 562)
+++ ecp.h    (working copy)
@@ -117,6 +117,9 @@
     }
     const ECP & GetCurve() const {return *m_ecOriginal;}
 
+    // default dtor provided by compiler is not virtual
+    virtual ~EcPrecomputation() {}
+
 private:
     value_ptr<ECP> m_ec, m_ecOriginal;
 };
Index: eprecomp.h
===================================================================
--- eprecomp.h    (revision 562)
+++ eprecomp.h    (working copy)
@@ -44,6 +44,7 @@
     typedef T Element;
 
     DL_FixedBasePrecomputationImpl() : m_windowSize(0) {}
+    virtual ~DL_FixedBasePrecomputationImpl() { }
 
     // DL_FixedBasePrecomputation
     bool IsInitialized() const
Index: luc.cpp
===================================================================
--- luc.cpp    (revision 562)
+++ luc.cpp    (working copy)
@@ -101,6 +101,10 @@
     {
         return RelativelyPrime(m_e, candidate+1) && RelativelyPrime(m_e, candidate-1);
     }
+
+    // default dtor provided by compiler is not virtual
+    virtual ~LUCPrimeSelector() {}
+
     Integer m_e;
 };
 
Index: luc.h
===================================================================
--- luc.h    (revision 562)
+++ luc.h    (working copy)
@@ -123,6 +123,9 @@
     void SetModulus(const Integer &v) {m_p = v;}
     const Integer & GetModulus() const {return m_p;}
 
+    // default dtor provided by compiler is not virtual
+    virtual ~DL_GroupPrecomputation_LUC() {}
+
 private:
     Integer m_p;
 };
@@ -142,6 +145,9 @@
     Integer CascadeExponentiate(const DL_GroupPrecomputation<Element> &group, const Integer &exponent, const DL_FixedBasePrecomputation<Integer> &pc2, const Integer &exponent2) const
         {throw NotImplemented("DL_BasePrecomputation_LUC: CascadeExponentiate not implemented");}    // shouldn't be called
 
+    // default dtor provided by compiler is not virtual
+    virtual ~DL_BasePrecomputation_LUC() {}
+
 private:
     Integer m_g;
 };
Index: modexppc.h
===================================================================
--- modexppc.h    (revision 562)
+++ modexppc.h    (working copy)
@@ -25,6 +25,9 @@
     void SetModulus(const Integer &v) {m_mr.reset(new MontgomeryRepresentation(v));}
     const Integer & GetModulus() const {return m_mr->GetModulus();}
 
+    // default dtor provided by compiler is not virtual
+    virtual ~ModExpPrecomputation() {}
+
 private:
     value_ptr<MontgomeryRepresentation> m_mr;
 };
Index: rsa.cpp
===================================================================a
--- rsa.cpp    (revision 562)
+++ rsa.cpp    (working copy)
@@ -99,6 +99,9 @@
     RSAPrimeSelector(const Integer &e) : m_e(e) {}
     bool IsAcceptable(const Integer &candidate) const {return RelativelyPrime(m_e, candidate-Integer::One());}
     Integer m_e;
+
+    // default dtor provided by compiler is not virtual
+    virtual ~RSAPrimeSelector() {}
 };
 
 void InvertibleRSAFunction::GenerateRandom(RandomNumberGenerator &rng, const NameValuePairs &alg)

[Jonathan Wakely]

On Thursday, 2 July 2015 01:39:44 UTC+1, Jeffrey Walton wrote:

I believe what happened is constructors and destructors were sometimes not written because the compiler will provide them. The "sometimes" appears to be limited to stack based allocations (i.e., if 'new' was used to allocate a member variable, then a virtual dtor was written).

That doesn't seem to be the case, because the warning means 'delete' is being used, so it's not a variable on the stack.

 

However, the default destructor is not virtual in keeping with C/C++'s "don't pay for it if you don't need it" mantra.

Crypto++ has a mechanism to forgo vtables, but it was not applied to any of the classes below. The mechanism is CRYPTOPP_NO_VTABLE.

The patch below clears the warnings related to the library and the test suite. There could be more places a virtual dtor is needed, but I don't have test cases that highlights them.

Is anyone opposed to the patch below? Or any other comments?

Another way to silence the warning is to tell the compiler that there are no derived classes, so it knows that the type being deleted is the most-derived type. That is done by marking the class as 'final' which is a C++11 feature, but GCC supports __final pre-C++11 so you could have a macro that expands to __final for GCC.

That will only work if you don't derive from those types being deleted. If that isn't always true then you definitely do want the virtual destructor.

[Jeffrey Walton]

On Monday, July 13, 2015 at 8:15:50 AM UTC-4, Jonathan Wakely wrote:

On Thursday, 2 July 2015 01:39:44 UTC+1, Jeffrey Walton wrote:

I believe what happened is constructors and destructors were sometimes not written because the compiler will provide them. The "sometimes" appears to be limited to stack based allocations (i.e., if 'new' was used to allocate a member variable, then a virtual dtor was written).

That doesn't seem to be the case, because the warning means 'delete' is being used, so it's not a variable on the stack.

My bad... I meant the class that was the target of the warning has stack-based member variables.

The class itself is clearly being created new (which kind of means its member variables cannot be on the stack :o )...

However, the default destructor is not virtual in keeping with C/C++'s "don't pay for it if you don't need it" mantra.

Crypto++ has a mechanism to forgo vtables, but it was not applied to any of the classes below. The mechanism is CRYPTOPP_NO_VTABLE.

The patch below clears the warnings related to the library and the test suite. There could be more places a virtual dtor is needed, but I don't have test cases that highlights them.

Is anyone opposed to the patch below? Or any other comments?

Another way to silence the warning is to tell the compiler that there are no derived classes, so it knows that the type being deleted is the most-derived type. That is done by marking the class as 'final' which is a C++11 feature, but GCC supports __final pre-C++11 so you could have a macro that expands to __final for GCC.

That will only work if you don't derive from those types being deleted. If that isn't always true then you definitely do want the virtual destructor.

OK, thanks.

The pain point is to ensure popular compilers provide a similar extension so the code is essentially the same everywhere. That includes some notables, like Microsoft's embedded eVC 4.0 compiler, and GCC 4.2.1 that's still alive an kickin' on OpenBSD.

As you've noted, that constraint leads to code that covers the spectrum, from clean to messy to awful :) Its times like these I wish C++ provided many of these features earlier, like at C++03.

Jeff