The patch below (1) documents the interface, including when the underlying
array is uninitialized versus set to 0; (2) adds move semantics; (3) makes
its members private; (4) adds asserts for some sanity checks in Debug
builds; (5) guards calls to memset and memcpy for NULL pointers and
0-sizes; (6) prefers initialization over assignment; and (7) switches to
numeric_limits::max() for sizes.
Regarding (3), two classes, Salsa and Sosemanuk, reached in. They are now
using SecBlock::data() to access the block. That will be a separate
proposal.
Regarding (5), its undefined behavior to call memset or memcpy (and
friends) with a NULL pointer. The current behavior of unchecked calls
lights up an assert in misc.h.
Regarding (6), "initialization over assignment" provides stronger exception
safety. Here's what a typical change looks like:
SecBlock(const T *t, size_type len)
- : m_size(len) {m_ptr = m_alloc.allocate(m_size, NULL));
memcpy(m_ptr,t, m_size*sizeof(T));}
+ : m_size(len), m_ptr(m_alloc.allocate(m_size, NULL)) { if(m_ptr &&
m_size){memcpy(m_ptr,t, m_size*sizeof(T));}}
+
Regarding (7), use of max() collided with Microsoft macros of the same
name. The fix was to define NOMINMAX in a way that did not cross pollinate
into user code through the header. For that, we needed to push and pop the
relevant macros.
Any comments or objections?
**********
$ cat secblock.h.diff
diff --git a/secblock.h b/secblock.h
index 907bca8..8c2e428
--- a/secblock.h
+++ b/secblock.h
@@ -3,7 +3,17 @@
#ifndef CRYPTOPP_SECBLOCK_H
#define CRYPTOPP_SECBLOCK_H
+// https://support.microsoft.com/en-us/kb/143208
+#if (_MSC_VER)
+# pragma push_macro("NOMINMAX")
+# pragma push_macro("min")
+# pragma push_macro("max")
+# undef min
+# undef max
+#endif
+
#include "config.h"
+#include "stdcpp.h"
#include "misc.h"
#if GCC_DIAGNOSTIC_AWARE
@@ -37,12 +47,12 @@ public:
const_pointer address(const_reference r) const {return (&r); }
void construct(pointer p, const T& val) {new (p) T(val);}
void destroy(pointer p) {p->~T();}
- size_type max_size() const {return ~size_type(0)/sizeof(T);} //
switch to std::numeric_limits<T>::max later
+ size_type max_size() const {return
std::numeric_limits<size_type>::max()/sizeof(T);}
protected:
static void CheckSize(size_t n)
{
- if (n > ~size_t(0) / sizeof(T))
+ if (n > std::numeric_limits<AllocatorBase::size_type>::max() /
sizeof(T))
throw InvalidArgument("AllocatorBase: requested size would
cause integer overflow");
}
};
@@ -71,7 +81,8 @@ typename A::pointer StandardReallocate(A& a, T *p,
typename A::size_type oldSize
if (preserve)
{
typename A::pointer newPointer = a.allocate(newSize, NULL);
- memcpy_s(newPointer, sizeof(T)*newSize, p,
sizeof(T)*STDMIN(oldSize, newSize));
+ const size_t size = sizeof(T)*STDMIN(oldSize, newSize);
+ if(p && size) {memcpy(newPointer, p, size);}
a.deallocate(p, oldSize);
return newPointer;
}
@@ -107,8 +118,10 @@ public:
}
void deallocate(void *p, size_type n)
- {
+ { CRYPTOPP_ASSERT((p && n) || (!p && !n));
SecureWipeArray((pointer)p, n);
+ // CRYPTOPP_ASSERT((n == 0) || (n > 0 && ((T*)p)[0] == 0));
+ // CRYPTOPP_ASSERT((n == 0) || (n > 0 && ((T*)p)[sizeof(T)*n-1] ==
0));
#if CRYPTOPP_BOOL_ALIGN16_ENABLED
if (T_Align16 && n*sizeof(T) >= 16)
@@ -203,6 +216,8 @@ public:
CRYPTOPP_ASSERT(m_allocated);
m_allocated = false;
SecureWipeArray((pointer)p, n);
+ // CRYPTOPP_ASSERT((n == 0) || (n > 0 && ((T*)p)[0] == 0));
+ // CRYPTOPP_ASSERT((n == 0) || (n > 0 &&
((T*)p)[sizeof(T)*n-1] == 0));
}
else
m_fallbackAllocator.deallocate(p, n);
@@ -219,7 +234,7 @@ public:
}
pointer newPointer = allocate(newSize, NULL);
- if (preserve)
+ if (preserve && newSize)
memcpy(newPointer, p, sizeof(T)*STDMIN(oldSize, newSize));
deallocate(p, oldSize);
return newPointer;
@@ -249,19 +264,31 @@ public:
typedef typename A::const_pointer const_iterator;
typedef typename A::size_type size_type;
+ //! construct a SecBlock with space for 'size' elements. To initialize
the elements to 0, create a SecBlock and then call CleanNew or CleanGrow.
explicit SecBlock(size_type size=0)
- : m_size(size) {m_ptr = m_alloc.allocate(size, NULL);}
+ : m_size(size), m_ptr(m_alloc.allocate(size, NULL)) { }
+ //! copy construct a SecBlock from another SecBlock
SecBlock(const SecBlock<T, A> &t)
- : m_size(t.m_size) {m_ptr = m_alloc.allocate(m_size, NULL);
memcpy_s(m_ptr, m_size*sizeof(T), t.m_ptr, m_size*sizeof(T));}
+ : m_size(t.m_size), m_ptr(m_alloc.allocate(m_size, NULL)) {
CRYPTOPP_ASSERT((!t.m_ptr && !m_size) || (t.m_ptr && m_size)); if(t.m_ptr
&& t.m_size){memcpy(m_ptr,t.m_ptr, m_size*sizeof(T));}}
+ //! construct a SecBlock from an array of elements
SecBlock(const T *t, size_type len)
- : m_size(len)
+ : m_size(len), m_ptr(m_alloc.allocate(m_size, NULL)) {
CRYPTOPP_ASSERT((!m_ptr && !m_size) || (m_ptr && m_size)); if(m_ptr &&
m_size){memcpy(m_ptr,t, m_size*sizeof(T));}}
+
+#if (CRYPTOPP_CXX11_RVALUES && CRYPTOPP_CXX11_MOVE)
+ SecBlock(SecBlock&& t)
+ : m_alloc(std::move(t.m_alloc)), m_size(t.m_size),
m_ptr(std::move(t.m_ptr))
{
- m_ptr = m_alloc.allocate(len, NULL);
- if (t == NULL)
- memset_z(m_ptr, 0, len*sizeof(T));
- else
- memcpy(m_ptr, t, len*sizeof(T));
+ // TODO: research the move on the Allocator, and remove it if not
needed.
+ t.m_alloc = A();
+ t.m_size = 0;
+ t.m_ptr = NULL;
}
+ SecBlock& operator=(SecBlock&& t)
+ {
+ swap(t);
+ return *this;
+ }
+#endif
~SecBlock()
{m_alloc.deallocate(m_ptr, m_size);}
@@ -281,42 +308,42 @@ public:
{return m_ptr;}
#endif
-// T *operator +(size_type offset)
-// {return m_ptr+offset;}
-
-// const T *operator +(size_type offset) const
-// {return m_ptr+offset;}
-
-// T& operator[](size_type index)
-// {CRYPTOPP_ASSERT(index >= 0 && index < m_size); return
m_ptr[index];}
-
-// const T& operator[](size_type index) const
-// {CRYPTOPP_ASSERT(index >= 0 && index < m_size); return
m_ptr[index];}
-
+ //! provide an iterator to the first element of the array
iterator begin()
{return m_ptr;}
+ //! provide a constant iterator to the first element of the array
const_iterator begin() const
{return m_ptr;}
+ //! provide an iterator set beyond the last element of the array
iterator end()
{return m_ptr+m_size;}
+ //! provide a constant iterator set beyond the last element of the
array
const_iterator end() const
{return m_ptr+m_size;}
+ //! return a pointer to the first element in the array
typename A::pointer data() {return m_ptr;}
+ //! return a constant pointer to the first element in the array
typename A::const_pointer data() const {return m_ptr;}
+ //! return the number of elements in the array
size_type size() const {return m_size;}
+ //! return the number of elements in the array
bool empty() const {return m_size == 0;}
+ //! return a byte pointer to the first element in the array
byte * BytePtr() {return (byte *)m_ptr;}
+ //! return a byte pointer to the first element in the array
const byte * BytePtr() const {return (const byte *)m_ptr;}
+ //! return the number of bytes in the array
size_type SizeInBytes() const {return m_size*sizeof(T);}
- //! set contents and size
+ //! set contents and size from an array
void Assign(const T *t, size_type len)
{
+ // New wipes the tail of the previous array if its reduced in size
New(len);
- memcpy_s(m_ptr, m_size*sizeof(T), t, len*sizeof(T));
+ if(t && len) {memcpy(m_ptr, t, m_size*sizeof(T));}
}
//! copy contents and size from another SecBlock
@@ -324,11 +351,13 @@ public:
{
if (this != &t)
{
+ // New wipes the tail of the previous array if its reduced in
size
New(t.m_size);
- memcpy_s(m_ptr, m_size*sizeof(T), t.m_ptr, m_size*sizeof(T));
+ if(t.m_ptr && t.m_size)
{memcpy(m_ptr,t.m_ptr,t.m_size*sizeof(T));}
}
}
+ //! assign contents and size from another SecBlock
SecBlock<T, A>& operator=(const SecBlock<T, A> &t)
{
// Assign guards for self-assignment
@@ -339,46 +368,54 @@ public:
// append to this object
SecBlock<T, A>& operator+=(const SecBlock<T, A> &t)
{
+ CRYPTOPP_ASSERT((!t.m_ptr && !t.m_size) || (t.m_ptr && t.m_size));
+
size_type oldSize = m_size;
Grow(m_size+t.m_size);
- memcpy_s(m_ptr+oldSize, m_size*sizeof(T), t.m_ptr,
t.m_size*sizeof(T));
+ if(t.m_ptr && t.m_size)
{memcpy(m_ptr+oldSize,t.m_ptr,t.m_size*sizeof(T));}
return *this;
}
// append operator
SecBlock<T, A> operator+(const SecBlock<T, A> &t)
{
+ CRYPTOPP_ASSERT((!m_ptr && !m_size) || (m_ptr && m_size));
+ CRYPTOPP_ASSERT((!t.m_ptr && !t.m_size) || (t.m_ptr && t.m_size));
+
SecBlock<T, A> result(m_size+t.m_size);
- memcpy_s(result.m_ptr, result.m_size*sizeof(T), m_ptr,
m_size*sizeof(T));
- memcpy_s(result.m_ptr+m_size, t.m_size*sizeof(T), t.m_ptr,
t.m_size*sizeof(T));
+ if(m_ptr && m_size) {memcpy(result.m_ptr, m_ptr,
m_size*sizeof(T));}
+ if(t.m_ptr && t.m_size) {memcpy(result.m_ptr, t.m_ptr,
t.m_size*sizeof(T));}
return result;
}
+ //! bitwise compare two SecBlocks using a constant time compare if the
arrays are equal size
bool operator==(const SecBlock<T, A> &t) const
{
return m_size == t.m_size && VerifyBufsEqual(m_ptr, t.m_ptr,
m_size*sizeof(T));
}
+ //! bitwise compare two SecBlocks using a constant time compare if the
arrays are equal size
bool operator!=(const SecBlock<T, A> &t) const
{
return !operator==(t);
}
- //! change size, without preserving contents
+ //! change size without preserving contents. new content unintialized
void New(size_type newSize)
{
+ // reallocate wipes the tail of the array if its reduced in size
m_ptr = m_alloc.reallocate(m_ptr, m_size, newSize, false);
m_size = newSize;
}
- //! change size and set contents to 0
+ //! change size without preserving contents. all content set to 0
void CleanNew(size_type newSize)
{
New(newSize);
memset_z(m_ptr, 0, m_size*sizeof(T));
}
- //! change size only if newSize > current size. contents are preserved
+ //! change size only if newSize > current size. contents are
preserved, new content unintialized
void Grow(size_type newSize)
{
if (newSize > m_size)
@@ -388,18 +425,18 @@ public:
}
}
- //! change size only if newSize > current size. contents are preserved
and additional area is set to 0
+ //! change size only if newSize > current size. contents are
preserved, new content set to 0
void CleanGrow(size_type newSize)
{
if (newSize > m_size)
{
m_ptr = m_alloc.reallocate(m_ptr, m_size, newSize, true);
- memset(m_ptr+m_size, 0, (newSize-m_size)*sizeof(T));
+ memset_z(m_ptr+m_size, 0, (newSize-m_size)*sizeof(T));
m_size = newSize;
}
}
- //! change size and preserve contents
+ //! change size and preserve contents. new content is uninitialized.
void resize(size_type newSize)
{
m_ptr = m_alloc.reallocate(m_ptr, m_size, newSize, true);
@@ -409,26 +446,33 @@ public:
//! swap contents and size with another SecBlock
void swap(SecBlock<T, A> &b)
{
+ // TODO: research the swap on the Allocator, and remove it if not
needed.
std::swap(m_alloc, b.m_alloc);
std::swap(m_size, b.m_size);
std::swap(m_ptr, b.m_ptr);
}
-//private:
+protected:
A m_alloc;
size_type m_size;
T *m_ptr;
};
-typedef SecBlock<byte> SecByteBlock;
+DOCUMENTED_TYPEDEF(SecBlock<byte>, SecByteBlock);
+DOCUMENTED_TYPEDEF(SecBlock<word>, SecWordBlock);
+// typedef SecBlock<byte> SecByteBlock;
typedef SecBlock<byte, AllocatorWithCleanup<byte, true> >
AlignedSecByteBlock;
-typedef SecBlock<word> SecWordBlock;
+// typedef SecBlock<word> SecWordBlock;
+
+// No need for move semantics on derived class *if* the class does not add
any
+// data members; see http://stackoverflow.com/q/31755703, and Rule of
{0|3|5}.
//! a SecBlock with fixed size, allocated statically
template <class T, unsigned int S, class A =
FixedSizeAllocatorWithCleanup<T, S> >
class FixedSizeSecBlock : public SecBlock<T, A>
{
public:
+ //! construct a FixedSizeSecBlock
explicit FixedSizeSecBlock() : SecBlock<T, A>(S) {}
};
@@ -442,6 +486,7 @@ template <class T, unsigned int S, class A =
FixedSizeAllocatorWithCleanup<T, S,
class SecBlockWithHint : public SecBlock<T, A>
{
public:
+ //! construct a SecBlockWithHint with a count of elements
explicit SecBlockWithHint(size_t size) : SecBlock<T, A>(size) {}
};
@@ -472,7 +517,13 @@
__stl_alloc_rebind(CryptoPP::AllocatorWithCleanup<_Tp1>& __a, const _Tp2*)
NAMESPACE_END
#if GCC_DIAGNOSTIC_AWARE
-# pragma GCC diagnostic pop
+# pragma GCC diagnostic pop
+#endif
+
+#if (_MSC_VER)
+# pragma pop_macro("min")
+# pragma pop_macro("max")
+# pragma pop_macro("NOMINMAX")
#endif
#endif
--
--
You received this message because you are subscribed to the "Crypto++ Users"
Google Group.
To unsubscribe, send an email to [email protected].
More information about Crypto++ and this group is available at
http://www.cryptopp.com.
---
You received this message because you are subscribed to the Google Groups
"Crypto++ Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
