secblock.h: Error inside assert()

[AlanStokes]

Line 569 and 589 of secblock.h contain this:

assert((!t.m_ptr && !t.m_size) || (t.m_ptr && t.m_ptr.m_size));

m_ptr.m_size doesn't work, since m_ptr is a pointer. I believe what is meant is

assert((!t.m_ptr && !t.m_size) || (t.m_ptr && t.m_size));

This prevents use of operators +=and+` in a debug build (i.e. with assertions not disabled).

[AlanStokes]

(via http://stackoverflow.com/a/34480590/212870)

[noloader]

Thanks Alan.

m_ptr.m_size doesn't work, since m_ptr is a pointer. I believe what is meant is:

assert((!t.m_ptr && !t.m_size) || (t.m_ptr && t.m_size));

Yeah, you're right. I'm not sure why that was not caught with cryptest.sh (or one of the other Debug build tests).

Would you mind providing a Pull Request? If you provide the pull request, we can ensure you receive credit at Contributors. If not, I can check-in a fix.

Thanks again.

[AlanStokes]

Sorry, I don't really feel I can do that; it would be potentially complex. I'd be grateful if you would go ahead and fix it.

[noloader]

Sorry, I don't really feel I can do that; it would be potentially complex. I'd be grateful if you would go ahead and fix it.

No problem.

By the way... I checked the original code before I changed it. You can see it at secblock.h from 5.6.2.

Ignoring the assert typos (which are valid findings...), the old and existing code performs the following:

  335         size_type oldSize = m_size;
  336         Grow(m_size+t.m_size);
  337         memcpy_s(m_ptr+oldSize, m_size*sizeof(T), t.m_ptr, t.m_size*sizeof(T));
  338         return *this;

It would work fine under the following condition:

SecBlock a, t;
...
a += t;
...

_If_ we perform:

SecBlock t;
...
t += t;
...

Then the code breaks because the memcpy_s uses the _new_ size; i.e., t.m_size*sizeof(T). The issue is t is grown when this is grown because they are the same object.

We need to rework this code and get test cases in place to catch problems like it in the future.

We may need to release 5.6.4 because of it, too. We cannot guarantee clients are not doing the t += t; and memory errors make me nervous. The mitigating circumstance is the memcpy_s, but its handled differently on Windows and non-Windows.

Good find. Thanks for bringing it up.

[noloader]

Here's the test case I used while examining the Stack Overflow question.

#include "ripemd.h"
#include "files.h"
#include "hex.h"
using namespace CryptoPP;

#include <string>
using namespace std;

int main(int argc, char* argv[])
{
  SecByteBlock digest(RIPEMD160::DIGESTSIZE);
  RIPEMD160 hash;

  std::string message("now is the time for all good men to come to the aide of their country");
  HexEncoder hexer(new FileSink(cout));

  // RIPEMD-160
  hash.Update((const byte*)message.data(), message.size());
  hash.TruncatedFinal(digest, digest.size());

  cout << "RIPEMD-160: ";
  hexer.Put(digest, digest.size());
  cout << endl;

  // Make it RIPEMD-320
  digest += digest;

  cout << "RIPEMD-320: ";
  hexer.Put(digest, digest.size());
  cout << endl;

  return 0;
}

[noloader]

And here's the patch I'm looking at:

$ cat git.diff 
diff --git a/secblock.h b/secblock.h
index 8b31810..4cd8da4 100644
--- a/secblock.h
+++ b/secblock.h
@@ -566,15 +566,23 @@ public:
     //! \details Internally, this SecBlock calls Grow and then appends t.
     SecBlock<T, A>& operator+=(const SecBlock<T, A> &t)
     {
-        assert((!t.m_ptr && !t.m_size) || (t.m_ptr && t.m_ptr.m_size));
+        assert((!t.m_ptr && !t.m_size) || (t.m_ptr && t.m_size));

-        if(t.size)
+        if(t.m_size)
         {
-            size_type oldSize = m_size;
-            Grow(m_size+t.m_size);
-        
-            if (m_ptr && t.m_ptr)
-                {memcpy_s(m_ptr+oldSize, (m_size-oldSize)*sizeof(T), t.m_ptr, t.m_size*sizeof(T));}
+            if(this != &t)    // s += t
+            {
+                const size_type oldSize = m_size;
+                Grow(m_size+t.m_size);
+                memcpy_s(m_ptr+oldSize, (m_size-oldSize)*sizeof(T), t.m_ptr, t.m_size*sizeof(T));
+            }
+            else    // t += t
+            {
+                SecBlock temp(m_size+t.m_size);
+                if(m_size) {memcpy_s(temp.m_ptr, temp.m_size*sizeof(T), m_ptr, m_size*sizeof(T));}
+                memcpy_s(temp.m_ptr+m_size, (temp.m_size-m_size)*sizeof(T), t.m_ptr, t.m_size*sizeof(T));
+                swap(temp);
+            }
         }
         return *this;
     }
@@ -586,7 +594,7 @@ public:
     SecBlock<T, A> operator+(const SecBlock<T, A> &t)
     {
         assert((!m_ptr && !m_size) || (m_ptr && m_size));
-        assert((!t.m_ptr && !t.m_size) || (t.m_ptr && t.m_ptr.m_size));
+        assert((!t.m_ptr && !t.m_size) || (t.m_ptr && t.m_size));
         if(!t.size) return SecBlock(*this);

         SecBlock<T, A> result(m_size+t.m_size);

[noloader]

Fixed at commit 605744d

[noloader]

Now related: Need a Code Coverage tool added to the Release Process.