Memory allocation not properly aligned on Windows platforms.

[JeremyViehland]

Assertion failed: m_allocated
cryptopp\secblock.h, line 197

The issue appears to be some problem with the alignment of the 60 byte array that is allocated.
It’s intended to be 8 byte aligned with 8 extra bytes added on to the end. The address used for
the array is the next 16 byte aligned address, which is guaranteed to end at latest at the end
of the allocated array.
However, the memory addresses I’m seeing in the debugger are not 8 byte aligned, so the
memory accessed goes past the end of the array into whatever comes next in the struct, 
which includes the m_allocated flag, which is what’s causing the assertion failure.

For whatever reason, __declspec(align(8)) isn’t working and I can’t seem to find a way around
this.  From googling, it seems like there might be an issue with the alignment of things
allocated with new / malloc.  Work around the bug by just increasing the padding on the array.

Here's the workaround.

--- cryptopp/secblock.h.orig    2013-02-20 14:30:52.000000000 -0500
+++ cryptopp/secblock.h 2015-08-12 13:46:00.088518500 -0400
@@ -227,7 +227,7 @@
    T m_array[S];
 #else
    T* GetAlignedArray() {return (CRYPTOPP_BOOL_ALIGN16_ENABLED && T_Align16) ? (T*)(((byte *)m_array) + (0-(size_t)m_array)%16) : m_array;}
-   CRYPTOPP_ALIGN_DATA(8) T m_array[(CRYPTOPP_BOOL_ALIGN16_ENABLED && T_Align16) ? S+8/sizeof(T) : S];
+   CRYPTOPP_ALIGN_DATA(8) T m_array[(CRYPTOPP_BOOL_ALIGN16_ENABLED && T_Align16) ? S+16/sizeof(T) : S];
 #endif
    A m_fallbackAllocator;
    bool m_allocated;

Here's the stack trace of the assertion

CryptoPP::FixedSizeAllocatorWithCleanup<unsigned int,60,CryptoPP::NullAllocator<unsigned int>,1>::deallocate(void * p, unsigned int n) Line 197      C++
CryptoPP::SecBlock<unsigned int,CryptoPP::FixedSizeAllocatorWithCleanup<unsigned int,60,CryptoPP::NullAllocator<unsigned int>,1> >::~SecBlock<unsigned int,CryptoPP::FixedSizeAllocatorWithCleanup<unsigned int,60,CryptoPP::NullAllocator<unsigned int>,1> >() Line 261     C++
CryptoPP::FixedSizeSecBlock<unsigned int,60,CryptoPP::FixedSizeAllocatorWithCleanup<unsigned int,60,CryptoPP::NullAllocator<unsigned int>,1> >::~FixedSizeSecBlock<unsigned int,60,CryptoPP::FixedSizeAllocatorWithCleanup<unsigned int,60,CryptoPP::NullAllocator<unsigned int>,1> >()                C++
CryptoPP::FixedSizeAlignedSecBlock<unsigned int,60,1>::~FixedSizeAlignedSecBlock<unsigned int,60,1>()          C++
CryptoPP::Rijndael::Base::~Base()           C++
CryptoPP::Rijndael::Enc::~Enc() C++
CryptoPP::ClonableImpl<CryptoPP::BlockCipherFinal<0,CryptoPP::Rijndael::Enc>,CryptoPP::Rijndael::Enc>::~ClonableImpl<CryptoPP::BlockCipherFinal<0,CryptoPP::Rijndael::Enc>,CryptoPP::Rijndael::Enc>()                C++
CryptoPP::BlockCipherFinal<0,CryptoPP::Rijndael::Enc>::~BlockCipherFinal<0,CryptoPP::Rijndael::Enc>()              C++
CryptoPP::GCM_Final<CryptoPP::Rijndael,0,0>::~GCM_Final<CryptoPP::Rijndael,0,0>()                C++

I set breakpoints on the `allocate’ function of secblock.h (line 181), and recorded the following allocations prior to ultimately hitting my breakpoint at the assertion at line 197:

Here are the return values of CryptoPP::FixedSizeAllocatorWithCleanup<unsigned int,60,CryptoPP::NullAllocator<unsigned int>,1>::GetAlignedArray:

Bytes                    Address
(De-/)Allocated          of Alloc  
60                       0x00ab88d0      
16                       0x0037ef48         
16                       0x0037efa0
-16                      0x0037efa0 
-16                      0x0037ef48 
16                       0x0037ef48
16                       0x0037efa0
-16                      0x0037efa0 
-16                      0x0037ef48 
-44                      0x00ab88d0 

Some points of interest:
• The pattern above is totally reproducible (with different address values).
• The size deallocated (-44) does not match the size allocated.
• Decryption happens just fine, it’s cleanup that aborts.

[IlyaBizyaev]

That's the problem I constantly run at on Windows! I have reported it multiple times, and I thought I am the only person who experiences it! Thank you, Mr. Viehland!

[IlyaBizyaev]

"Decryption happens just fine, it’s cleanup that aborts."
Exactly! That's why my application crashed in GUI mode and worked in console mode when the second was using exit(0)!

[JeremyViehland]

Credit to Christopher Mohr, who came up with the solution.

[IlyaBizyaev]

If the issue is fixed, I will finally manage to finish my project!

[noloader]

What version of Windows, and what version of Visual Studio? And do you have a minimal working example to demonstrate it? (I've never been able to duplicate it).

[JeremyViehland]

Windows 7 64-bit Professional, Service Pack 1
Visual Studio 2008 Professional Edition Version 9.0.30729.1 SP

And do you have a minimal working example to demonstrate it?

My `minimal' example unfortunately used proprietary code.

[IlyaBizyaev]

Same Windows version here, although the compiler is different.

[JeremyViehland]

I've asked Chris, who still has cryptopp set up for debugging. Perhaps he'll be able to respond.

[JeremyViehland]

@noloader,

Seems as though you are correct that this is not a cryptopp bug; however, I'm glad we've documented this as it is drawing attention from others who have experienced the problem.

"In VS2008, malloc is required to return 16 byte aligned memory
https://msdn.microsoft.com/en-us/library/ycsb6wwf%28v=vs.90%29.aspx,
so this doesn’t seem possible to reproduce with new in VS2008.
I did manage to reproduce it by manually allocating memory and
constructing an object offset in that memory, but I think that’s not allowed.

__alignof(CryptoPP::GCM< CryptoPP::AES>::Decryption) == 8

which means any struct containing it needs to be 8 byte aligned [...]"

[noloader]

Seems as though you are correct that this is not a cryptopp bug...

Oh, I did not say that :)

It may well be a Crypto++ bug; or it may be a platform bug that Crypto++ can help an application work around. About all I said was I'd like to see the bug in action so we can confirm it and determine a course of action.

The course of action can be anything from user education to a bug fix. It almost sounds like you need a FixedSizeAlignedAllocatorWithCleanup, which Crypto++ does not offer (I don't think it does beyond the 16-byte aligned allocator).

If you are using the MMX coprocessor for anything, then you definitely want the FixedSizeAllocatorWithCleanup<..., T_Align16 = true>. The trouble here is its not readily apparent when you need it. For example, I know of 2 or 3 bugs that exist with GCC at -O3 because of unaligned data. At -O3, GCC vectorizes, so we get a segfault in __memcmp_sse4_1(...); or GCC utilizes the vmovdqu and vmovdqa instructions, which causes a #GF because the data is not really aligned.

[noloader]

@JeremyViehland, @IlyaBizyaev - we added some documentation for the SecBlock class at secblock.h File Reference. It might help with locating the issue.

If you can reduce it to a minimum test case, then that would be immensely helpful.

[noloader]

@jeremy - Can you test this under the latest check-in? I'm curious to know if you still experience the issue.

[ChrisPMohr]

@noloader, I have been working with Jeremy on this. I tested out the latest released version (5.6.3) with our previously failing code. I still get the failing assertion.
Assertion failed m_allocated, file <...>\cryptopp\secblock.h, line 361

Sorry, still no test case that doesn't rely on proprietary code. My previous tests showed that our custom memory allocator was probably to blame, as it gave wrongly aligned blocks of memory.

[noloader]

@JeremyViehland, @ChrisPMohr - we found two issues in secblock.h. First, we found the following did not quite work as expected, and it results in an exception in most cases:

SecByteBlock b;
...
b += b;

Second, FixedSizeSecBlock would not grow properly for anything other than bytes. For word16 and above, element count, and not byte count was copied on during the grow. For example, two each word16 elements uses 4 bytes. We copied 2 bytes, not 4 bytes.

I believe I introduced the second issue when cutting over to memcpy_s from memcpy due to Issue 55. Issue 55 was purely C&A for Windows users; there was nothing technical about it.

We found the issues after adding a code coverage tool to ensure we had test cases set up. The tool identified a couple of functions that we thought were being tested but were not. Once we added the tests, we cleared the issues.

I don't think they affect you, but I wanted to pass it on so it does not become a issue for you in the future. Obviously, the fixes are in Master, and they will be cleared at large in the next release of the library.

[noloader]

@ChrisPMohr - Thanks Chris. I'm going to close this based on "My previous tests showed that our custom memory allocator was probably to blame, as it gave wrongly aligned blocks of memory...." Is that OK with you?

[IlyaBizyaev]

Not sure if you read this message, but the problem still exists on MinGW. However, I have managed to compile using TDM-GCC-64, and there everything is OK. So, that IS a Crypto++ problem.
Also, as it has already happened a couple of times, the problem may suddenly reappear. I will post a message if it happens.

[fhajji]

Same problem here:

• Visual Studio 2017 15.9.10
• CryptoPP 8.1.0, installed via vcpkg, x64, Debug
• secblock.h:253 reallocate() fails CRYPTOPP_ASSERT
• reallocate() called with arguments:
  • oldPtr == 0x...d670
  • oldSize == 0
  • newSize == 60
  • preserve == false

reallocate() is called in secblock.h:967 from

client.exe!CryptoPP::SecBlock<unsigned int,CryptoPP::AllocatorWithCleanup<unsigned int,1> >::New(unsigned __int64 newSize) Line 967	C++

Here's the code:

void New(size_type newSize)
{
  m_ptr = m_alloc.reallocate(m_ptr, m_size, newSize, false);
  m_size = newSize;
  m_mark = ELEMS_MAX;
}

In the debugger, newSize == 60.

This happens when I call CryptoPP::RandomNumberGenerator::GenerateBlock() many times (typically more than 1000 or so).

The stack trace goes like this:

client.exe!CryptoPP::AllocatorWithCleanup<unsigned int,1>::reallocate(unsigned int * oldPtr, unsigned __int64 oldSize, unsigned __int64 newSize, bool preserve) Line 253	C++

client.exe!CryptoPP::SecBlock<unsigned int,CryptoPP::AllocatorWithCleanup<unsigned int,1> >::New(unsigned __int64 newSize) Line 967	C++

client.exe!CryptoPP::Rijndael::Base::UncheckedSetKey(const unsigned char * userKey, unsigned int keyLen, const CryptoPP::NameValuePairs & __formal) Line 422	C++

client.exe!CryptoPP::SimpleKeyingInterface::SetKey(const unsigned char * key, unsigned __int64 length, const CryptoPP::NameValuePairs & params) Line 62	C++

client.exe!CryptoPP::AES_RNG::GenerateIntoBufferedTransformation(CryptoPP::BufferedTransformation & target, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & channel, unsigned __int64 size) Line 53	C++

client.exe!CryptoPP::RandomNumberGenerator::GenerateBlock(unsigned char * output, unsigned __int64 size) Line 316	C++

client.exe!PRG::operator()(unsigned char * out, unsigned __int64 outSize) Line 62	C++

client.exe!<lambda_d3d4dc71efd15cf96a403d0bc1208175>::operator()<std::array<unsigned char,20> >(std::array<unsigned char,20> & k) Line 96	C++

This is the gist of my code:

CryptoPP::SecByteBlock seed_(32 + 16);
CryptoPP::OS_GenerateRandomBlock(false, seed_, seed_.size());

CryptoPP::AES_RNG* prng_ = new CryptoPP::AES_RNG(seed_, seed_.size());

void operator()(CryptoPP::byte* out, std::size_t outSize) { prng_->GenerateBlock(out, outSize); }

I'm basically invoking operator() many times in a loop:

PRG p;

// ...

std::size_t last_key_idx = result.size() - 1;
        for (std::size_t key_idx = 0; key_idx != last_key_idx; ++key_idx) {
            std::for_each(
#if __has_include(<execution>)
              std::execution::par_unseq,
#endif
              result[key_idx].begin(),
              result[key_idx].end(),
              [&p](auto& k) {
                  p(static_cast<byte_type*>(k.data()), k.size());
              });
        }

Microsoft Visual C++ Runtime Library reports:

The Block at 0x... was allocated by aligned routines, use _aligned_free()

Output:

Assertion failed: ...\secblock.h(253): CryptoPP::AllocatorWithCleanup<unsigned int, 1>::reallocate

Assertion failed: ...\secblock.h(150): CryptoPP::StandardReallocate

[noloader]

@fhajji,

If you can reduce the problem to a single source file I can run and verify, then it would be a big help.

Please run a debug build of the library. We added asserts to help detect the condition closer to when it happens. A debug build is triggered with -DDEBUG=1.

If I recall correctly, IlyaBizyaev's issues were due to mixing/matching versions of the Crypto++ library. Can you please remove all old versions of the Crypto++ library.

Finally, please run the library's test suite and ensure all tests pass without error:

./cryptest.exe v
./cryptest.exe tv all