Cryptopp: AppVeyor failures after fixing FixedSizeSecBlock

[noloader]

@dwmcrobb discovered a latent bug in FixedSizeSecBlock. It was reported at GH #988. In the 988 bug we fixed the pointer problem and cleaned up the class.

Since the FixedSizeSecBlock overhaul we started seeing failures on AppVeyor, which tests the Windows gear. For example, see Noloader | Crypto++ | Win32. (Win64 is OK).

The problem appears to be too many asserts. I've seen this before on AppVeyor. A few intermittent asserts are OK, but a stream of them causes a failure. The asserts that are firing are alignment related in ByteReverse and IteratedHash:

// misc.h
template <class T>
void ByteReverse(T *out, const T *in, size_t byteCount)
{
    // Alignment check due to Issues 690
    CRYPTOPP_ASSERT(byteCount % sizeof(T) == 0);
    CRYPTOPP_ASSERT(IsAligned<T>(in));
    CRYPTOPP_ASSERT(IsAligned<T>(out));

    size_t count = byteCount/sizeof(T);
    for (size_t i=0; i<count; i++)
        out[i] = ByteReverse(in[i]);
}

And:

// iterhash.h
inline void CorrectEndianess(HashWordType *out, const HashWordType *in, size_t byteCount)
{
    CRYPTOPP_ASSERT(in != NULLPTR);
    CRYPTOPP_ASSERT(out != NULLPTR);
    CRYPTOPP_ASSERT(IsAligned<T_HashWordType>(in));
    CRYPTOPP_ASSERT(IsAligned<T_HashWordType>(out));

    ConditionalByteReverse(T_Endianness::ToEnum(), out, in, byteCount);
}

Tracing things back, it looks like GetAlignmentOf<T> is going a bit sideways. For example, for word64 array, the compiler lays out an array aligned on 4 bytes (recall this is a 32-bit problem). However, GetAlignmentOf<T> is returning 8 because of __alignof(T) from misc.h:

// misc.h
template <class T>
inline unsigned int GetAlignmentOf()
{
#if defined(CRYPTOPP_CXX11_ALIGNOF)
    return alignof(T);
#elif (_MSC_VER >= 1300)
    return __alignof(T);
#elif defined(__GNUC__)
    return __alignof__(T);
#elif defined(__SUNPRO_CC)
    return __alignof__(T);
#elif defined(__IBM_ALIGNOF__)
    return __alignof__(T);
#elif CRYPTOPP_BOOL_SLOW_WORD64
    return UnsignedMin(4U, sizeof(T));
#else
    return sizeof(T);
#endif
}

GetAlignmentOf<word64> returns 8. I think this is actually just a symptom. I _think_ the real problem is IsAligned<T> from misc.h:

// misc.h
template <class T>
inline bool IsAligned(const void *ptr)
{
    return IsAlignedOn(ptr, GetAlignmentOf<T>());
}

Notice the function calls accepts a pointer and calls GetAlignmentOf<T>, and not the pointer-to-T GetAlignmentOf<T*>. Switching to a pointer-to-T with GetAlignmentOf<T*> clears the asserts. This makes sense since a pointer only needs to be aligned to 32-bits on a 32-bit platform. And on 64-bits, pointers are 64-bits which coincides with the sizeof(word64), so everything is OK.

I also noticed the same behavior on 32-bit Linux. Those same asserts are firing, so it is not a Windows-specific problem.

My question is, why are we using GetAlignmentOf<T> instead of the pointer-to-T GetAlignmentOf<T*>? What insight did Wei have, or what was Wei trying to achieve by increasing alignment requirements for 64-bit words?

I _think_ we should change it to pointer-to-T GetAlignmentOf<T*> for this test.

@mouse07410, any thoughts?

[mouse07410]

I'm inclined to change it, and observe is any problem surfaces.

Haven't looked at our CI fur a while. We do test on platforms different "enough" (e.g., 32 bits, 64 bits, LE, BE)? And with various levels of compiler optimization, from -O0 to -Ofast?

[noloader]

Thanks @mouse07410,

We do test on platforms different "enough" (e.g., 32 bits, 64 bits, LE, BE)? And with various levels of compiler optimization, from -O0 to -Ofast?

Yes, or I believe so. I still regularly run cryptest.sh on different platforms: cryptest.sh Configurations.

---

When I test this under Visual Studio 32-bit:

std::cout << "__alignof(word64): " << __alignof(word64) << std::endl;
std::cout << "__alignof(word64*): " << __alignof(word64*) << std::endl;

It results in:

__alignof(word64): 8
__alignof(word64*): 4

I _think_ we will be OK with the change.

[noloader]

OK, let's give it a whirl and see what breaks... Commit e235ac57e85b:

$ git diff c534c833..e235ac57
diff --git a/misc.h b/misc.h
index 5f9e6eed..83bf49ec 100644
--- a/misc.h
+++ b/misc.h
@@ -1212,14 +1212,15 @@ inline bool IsAlignedOn(const void *ptr, unsigned int alignment)
 /// \brief Determines whether ptr is minimally aligned
 /// \tparam T class or type
 /// \param ptr the pointer to check for alignment
-/// \return true if <tt>ptr</tt> is aligned to at least <tt>T</tt>
+/// \return true if <tt>ptr</tt> is aligned to at least a pointer to <tt>T</tt>
 ///  boundary, false otherwise
 /// \details Internally the function calls IsAlignedOn with a second
-///  parameter of GetAlignmentOf<T>.
+///  parameter of GetAlignmentOf<T*>.
 template <class T>
 inline bool IsAligned(const void *ptr)
 {
-       return IsAlignedOn(ptr, GetAlignmentOf<T>());
+       // Switch to T* due to https://github.com/weidai11/cryptopp/issues/992
+       return IsAlignedOn(ptr, GetAlignmentOf<T*>());
 }

 #if (CRYPTOPP_LITTLE_ENDIAN)
(END)

[noloader]

Windows tested OK with the change: AppVeyor tests.

Linux and OS X tested OK with the change: Travis tests.

I'm going to close the issue for now.

[dwmcrobb]

I believe this is incorrect.

A pointer's alignment is a pointer's alignment. Usually it's going to be the size of a pointer. alignof(T) and friends are type-based. alignof(T*) will return the same thing for any T on most platforms (ignoring legacy near/far/huge pointers). I don't think it's what you want.

I generally don't care at all about the default alignment of a pointer type; I care about the alignment of what it points to! In nearly all cases, the reason I care about alignment in Crypto++ is due to needing a specific alignment for SIMD instructions and the like, right? If I want to know the alignment of my array of doubles, I can't just ask for the alignment of the pointer to double type. It will invariably be incorrect on platforms with 32-bit pointers but 64-bit doubles, as you saw.

  std::cout << "GetAlignmentOf<double>(): "
            << GetAlignmentOf<double>() << '\n'
            << "GetAlignmentOf<double[]>(): "
            << GetAlignmentOf<double[]>() << '\n'
            << "GetAlignmentOf<double *>(): "
            << GetAlignmentOf<double *>() << '\n';

Output on a platform with 32-bit pointers and 64-bit doubles, using C++11 alignof():

GetAlignmentOf<double>(): 8
GetAlignmentOf<double[]>(): 8
GetAlignmentOf<double *>(): 4

From section 8.3.6 of the standard:

When alignof is applied to an array type, the result is the alignment of the element type.

It's also worth noting that alignof(T) and sizeof(T) are orthogonal. Hence the fall-through in GetAlignmentOf()may or may not yield a correct answer. It also won't compile if you wind up there with GetAlignmentOf<T[]>() since sizeof() doesn't work on an 'incomplete type'. Consider what happens if you do this and wind up with the sizeof(T) fall-through:

GetAlignmentOf(char[32])

[noloader]

I'm reopening until we get to the bottom of this issue...

[dwmcrobb]

Note that all you did was make the assertions pass because now you're giving them a weaker alignment to check. IsAligned() will now return an unexpected answer in many cases (it'll return true in cases where it would be expected to return false). If you want the alignment of a pointer type (though I don't see the use case here), you could call the original IsAligned() with your pointer type as the template parameter.

I'm not seeing how ByteReverse(T *out, const T *in, size_t byteCount) needs alignment. Is it a legacy requirement or am I just not seeing the call tree from there that needs it? The values used in its inner loop are passed by value to the scalar ByteReverse(). Ditto for IteratedHash() (looks to me like the assertion there is also redundant).

I think it's the assertions that are wrong here since I don't see any code in the paths mentioned that require alignment. Legacy?

[noloader]

OK, so here's what I am seeing under GDB on a Ubuntu 32-bit machine.

Tiger, a 64-bit hash, triggers the library's assert. The library's assert raises a SIGTRAP, not a SIGABRT. SIGTRAP snaps the debugger for us.

(gdb) r v 27
...
Tiger validation suite running...

Assertion failed: iterhash.h(158): CorrectEndianess

Program received signal SIGTRAP, Trace/breakpoint trap.
0xb7fd7aa5 in __kernel_vsyscall ()
(gdb) bt
#0  0xb7fd7aa5 in __kernel_vsyscall ()
#1  0xb7d084d2 in __libc_signal_restore_set (set=0xbfffdf1c)
    at ../sysdeps/unix/sysv/linux/nptl-signals.h:80
#2  raise (sig=5) at ../sysdeps/unix/sysv/linux/raise.c:48
#3  0x009244f5 in CryptoPP::IteratedHash<unsigned long long, CryptoPP::EnumToType<CryptoPP::ByteOrder, 0>, 64u, CryptoPP::HashTransformation>::CorrectEndianess
    (this=0xbfffe428, out=0xbfffe43c, in=0xbfffe43c, byteCount=56)
    at iterhash.h:158
#4  0x00921820 in CryptoPP::Tiger::TruncatedFinal (this=0xbfffe428,
    digest=0xc2bc60 "", digestSize=24) at tiger.cpp:43
#5  0x004ce59a in CryptoPP::HashTransformation::Final (this=0xbfffe428,
    digest=0xc2bc60 "") at cryptlib.h:1143
#6  0x005d1e58 in CryptoPP::Test::HashModuleTest (md=..., testSet=0xbfffe4dc,
    testSetSize=12) at validat5.cpp:83
#7  0x005d42fe in CryptoPP::Test::ValidateTiger () at validat5.cpp:392
#8  0x004ccaed in CryptoPP::Test::Validate (alg=27, thorough=false)
    at test.cpp:975
#9  0x004c592b in CryptoPP::Test::scoped_main (argc=3, argv=0xbffff644)
    at test.cpp:405
#10 0x004cd124 in main (argc=3, argv=0xbffff644) at test.cpp:1096
(gdb) f 3
#3  0x009244f5 in CryptoPP::IteratedHash<unsigned long long, CryptoPP::EnumToType<CryptoPP::ByteOrder, 0>, 64u, CryptoPP::HashTransformation>::CorrectEndianess
    (this=0xbfffe428, out=0xbfffe43c, in=0xbfffe43c, byteCount=56)
    at iterhash.h:158
158                     CRYPTOPP_ASSERT(IsAligned<T_HashWordType>(in));
(gdb) p m_data
$1 = {<CryptoPP::SecBlock<unsigned long long, CryptoPP::FixedSizeAllocatorWithCleanup<unsigned long long, 8, CryptoPP::NullAllocator<unsigned long long>, false> >> = {
    m_alloc = {<CryptoPP::AllocatorBase<unsigned long long>> = {<No data fields>}, m_array = {1, 0, 0, 0, 0, 0, 0, 54720838301280336},
      m_fallbackAllocator = {<CryptoPP::AllocatorBase<unsigned long long>> = {<No data fields>}, <No data fields>}, m_allocated = true}, m_mark = 536870911,
    m_size = 8, m_ptr = 0xbfffe43c}, <No data fields>}
(gdb) p m_data.m_alloc.m_array
$2 = {1, 0, 0, 0, 0, 0, 0, 54720838301280336}
(gdb) p &m_data
$3 = (CryptoPP::FixedSizeSecBlock<unsigned long long, 8, CryptoPP::FixedSizeAllocatorWithCleanup<unsigned long long, 8, CryptoPP::NullAllocator<unsigned long long>, false> > *) 0xbfffe43c

Notice the array is not aligned for word64 (unsigned long long): p &m_data results in 0xbfffe43c, which is a 4-byte aligned address for the array.

Why is the compiler laying out the array with a 4-byte boundary, and not a 8 byte boundary?

[noloader]

@dwmcrobb,

Is it a legacy requirement or am I just not seeing the call tree from there that needs it?

We are trying to avoid undefined behavior that happens to work under some compilers.

SunCC on Sparc is a real bastard. For 64-bit hashes like Tiger or SHA-512, they must be 8-byte aligned or else we trigger a SIGBUS.

---

And I ran the test on this 32-bit machine:

std::cout << "alignof(word64): " << alignof(word64) << std::endl;
std::cout << "alignof(word64*): " << alignof(word64*) << std::endl;
std::cout << "alignof(word64[]): " << alignof(word64[]) << std::endl;

It returned:

alignof(word64): 8
alignof(word64*): 4
alignof(word64[]): 8

That array of 8 word64 is misaligned. I don't know where it is coming from... Sigh...

[dwmcrobb]

OK, so here's what I am seeing under GDB on a Ubuntu 32-bit machine.

Tiger, a 64-bit hash, triggers the library's assert. The library's assert raises a SIGTRAP, not a SIGABRT. SIGTRAP snaps the debugger for us.

(gdb) r v 27
...
Tiger validation suite running...

Assertion failed: iterhash.h(158): CorrectEndianess

Program received signal SIGTRAP, Trace/breakpoint trap.
0xb7fd7aa5 in __kernel_vsyscall ()
(gdb) bt
#0  0xb7fd7aa5 in __kernel_vsyscall ()
#1  0xb7d084d2 in __libc_signal_restore_set (set=0xbfffdf1c)
    at ../sysdeps/unix/sysv/linux/nptl-signals.h:80
#2  raise (sig=5) at ../sysdeps/unix/sysv/linux/raise.c:48
#3  0x009244f5 in CryptoPP::IteratedHash<unsigned long long, CryptoPP::EnumToType<CryptoPP::ByteOrder, 0>, 64u, CryptoPP::HashTransformation>::CorrectEndianess
    (this=0xbfffe428, out=0xbfffe43c, in=0xbfffe43c, byteCount=56)
    at iterhash.h:158
#4  0x00921820 in CryptoPP::Tiger::TruncatedFinal (this=0xbfffe428,
    digest=0xc2bc60 "", digestSize=24) at tiger.cpp:43
#5  0x004ce59a in CryptoPP::HashTransformation::Final (this=0xbfffe428,
    digest=0xc2bc60 "") at cryptlib.h:1143
#6  0x005d1e58 in CryptoPP::Test::HashModuleTest (md=..., testSet=0xbfffe4dc,
    testSetSize=12) at validat5.cpp:83
#7  0x005d42fe in CryptoPP::Test::ValidateTiger () at validat5.cpp:392
#8  0x004ccaed in CryptoPP::Test::Validate (alg=27, thorough=false)
    at test.cpp:975
#9  0x004c592b in CryptoPP::Test::scoped_main (argc=3, argv=0xbffff644)
    at test.cpp:405
#10 0x004cd124 in main (argc=3, argv=0xbffff644) at test.cpp:1096
(gdb) f 3
#3  0x009244f5 in CryptoPP::IteratedHash<unsigned long long, CryptoPP::EnumToType<CryptoPP::ByteOrder, 0>, 64u, CryptoPP::HashTransformation>::CorrectEndianess
    (this=0xbfffe428, out=0xbfffe43c, in=0xbfffe43c, byteCount=56)
    at iterhash.h:158
158                     CRYPTOPP_ASSERT(IsAligned<T_HashWordType>(in));
(gdb) p m_data
$1 = {<CryptoPP::SecBlock<unsigned long long, CryptoPP::FixedSizeAllocatorWithCleanup<unsigned long long, 8, CryptoPP::NullAllocator<unsigned long long>, false> >> = {
    m_alloc = {<CryptoPP::AllocatorBase<unsigned long long>> = {<No data fields>}, m_array = {1, 0, 0, 0, 0, 0, 0, 54720838301280336},
      m_fallbackAllocator = {<CryptoPP::AllocatorBase<unsigned long long>> = {<No data fields>}, <No data fields>}, m_allocated = true}, m_mark = 536870911,
    m_size = 8, m_ptr = 0xbfffe43c}, <No data fields>}
(gdb) p m_data.m_alloc.m_array
$2 = {1, 0, 0, 0, 0, 0, 0, 54720838301280336}
(gdb) p &m_data
$3 = (CryptoPP::FixedSizeSecBlock<unsigned long long, 8, CryptoPP::FixedSizeAllocatorWithCleanup<unsigned long long, 8, CryptoPP::NullAllocator<unsigned long long>, false> > *) 0xbfffe43c

Notice the array is not aligned for word64 (unsigned long long): p &m_data results in 0xbfffe43c, which is a 4-byte aligned address for the array.

Why is the compiler laying out the array with a 4-byte boundary, and not a 8 byte boundary?

Because it's instantiating FixedSizeAllocatorWithCleanup with T_Align16 = false?

[noloader]

Because it's instantiating FixedSizeAllocatorWithCleanup with T_Align16 = false?

T_Align16 is false, so the array is not 16-byte aligned.

The type T is word64, which is an unsigned long long on 32-bit machines (or more correctly, the ILP32 data model). According to the test, the word64 array requires 8-byte alignments.

I wonder if the linker is changing the layout.

---

Just in case, here's a search for a declaration that could be causing this problem (8-bytes -> 4-bytes):

$ grep 'CRYPTOPP_ALIGN_DATA(4)' *.h
camellia.h:             CRYPTOPP_ALIGN_DATA(4) static const byte s1[256];
secblock.h:             // If we needed to use CRYPTOPP_ALIGN_DATA(4) due to toolchain
$ grep 'CRYPTOPP_ALIGN_DATA(4)' *.cpp
camellia.cpp:CRYPTOPP_ALIGN_DATA(4)
sm4.cpp:CRYPTOPP_ALIGN_DATA(4)

So we are not inadvertently reducing the alignment. Something else has to be doing it.

[noloader]

Testing on a 32-bit ARM device does not witness the problem. Testing on 32-bit PowerPC does witness the problem.

I bet the toolchain is the cause of this. Either the compiler or linker is doing some optimization that breaks things.

[dwmcrobb]

Because it's instantiating FixedSizeAllocatorWithCleanup with T_Align16 = false?

T_Align16 is false, so the array is not 16-byte aligned.

The type T is word64, which is an unsigned long long. According to the test, the word64 array requires 8-byte alignments.

I wonder if the linker is changing the layout.

That's what I was wondering, only because you had mentioned it for other platforms. Note that CRYPTOPP_ALIGN_DATA(x), when defined as alignas(), can't be trusted on some platforms. I don't have a 32-bit Ubuntu to check, but it doesn't work correctly on Raspbian 10 with g++ 8.3.0. But the compiler emits warnings, and only for sizes above 8. clang++ 9.0.1 works correctly there with respect to alignas().

Hmm, wait... secblock.h. I'm confused what the intent of m_allocated is supposed to be. If someone calls allocate twice, the second time they're going to get a pointer from the fallback allocator, which may or may not align data.

Part of the issue here, to me... FixedSizeAllocatorWithCleanup isn't what it seems by name. It can allocate more memory just by asking it to do so. There was another issue I was going to open... if you call allocate() a second time before deallocate(), m_array gets abandoned and never gets wiped. Which kind of defeats the other part of its name. So it doesn't promise fixed size and potentially misses a cleanup. Shouldn't allocate() wipe when it abandons m_array?

I know it's a lot of work, but it feels like secblock.h needs a decent amount of refactoring (despite how much it trickles up :-(). It'd be nice if FixedSizeAllocatorWithCleanup actually yielded only m_array (sized by the template parameter) and couldn't use a fallback allocator that potentially break the expected contract. If you put a hard abort() or a CRYPTOPP_ASSERT(1 == 0) in FixedSizeAllocatorWithCleanup::allocate(size_type) where it calls the fallback allocator, how many code paths hit it?

For what it's worth, I did some playing around with generating code from test programs to get proper alignment on my platforms, from 8 to 512 by powers of 2. It ends up generating templates based on the runtime behavior; if alignas() is available and works, use it. Else fall back to std::align() (more widely available, implementation is trivial and not that much different than what's in secblock.h right now). On platforms where alignas() always works, I get a pure alignas() set of templates. On others I get a mix of alignas() and std::align().

I basically had this thought that the configure script could try compiling a new test_cxx11_alignas.cxx. If it fails, we don't have enough of C++11 to use alignas(). If it compiles, it can be run and will generate the correct templates based on the runtime behavior of alignas() and fall back to std::align(). If it doesn't compile, a test_cxx11_std_align.cxx can be compiled and run to generate a pure std::align() solution. Then if we don't have std::align() in our standard library, we use our own version.

[noloader]

That's what I was wondering, only because you had mentioned it for other platforms. Note that CRYPTOPP_ALIGN_DATA(x), when defined as alignas(), can't be trusted on some platforms...

Yeah, I think we need to add CRYPTOPP_ALIGN_DATA(8) back to the class. I removed it after we cleaned it up earlier. It looks like that was a mistake in practice.

FixedSizeAllocatorWithCleanup isn't what it seems by name. It can allocate more memory just by asking it to do so. There was another issue I was going to open... if you call allocate() a second time before deallocate(), m_array gets abandoned and never gets wiped. Which kind of defeats the other part of its name. So it doesn't promise fixed size and potentially misses a cleanup. Shouldn't allocate() wipe when it abandons m_array?

I'm not really worried about this. Names and such cannot change after 27 years or so.

I image the class methods, like allocate and deallocate, are present so you can swap-in different SecBlocks.

[dwmcrobb]

I don't think this fixes it. It's possible for CRYPTOPP_ALIGN_DATA(x) to expand to nothing. And as you've seen, some of the methods it utilizes aren't reliable across the board (alignas() for example) due to toolchain issues.

Remember, m_array's alignment no longer matters if the patched code is being used; it's private and the only way to get an address in there from the outside is via the public methods. Which call GetAlignedArray() to get an address inside m_array, which should be guaranteed to be aligned to 16, at runtime.

The way you'd get an unaligned address is by falling back on a fallback allocator that provides no alignment guarantee. Note that GetAlignedArray() isn't in your backtrace (debug has assertions). Use the wrong fallback allocator and expect it to be aligned... you get what you asked for. Ditto if you fall through to natural alignment.

As is in the repo, line 518 of secblock.h shouldn't exist (CRYPTOPP_ASSERT(IsAlignedOn(m_array, 8));). The declaration of m_array on line 546 should not include CRYPTOPP_ALIGN_DATA(8); we don't care about its alignment anymore and we know it's potentially meaningless/misleading.

There should be no fall-through based on CRYPTOPP_BOOL_ALIGN16 anymore, correct? It's just a code path to heartache at this point.

[dwmcrobb]

That's what I was wondering, only because you had mentioned it for other platforms. Note that CRYPTOPP_ALIGN_DATA(x), when defined as alignas(), can't be trusted on some platforms...

Yeah, I think we need to add CRYPTOPP_ALIGN_DATA(8) back to the class. I removed it after we cleaned it up earlier. It looks like that was a mistake in practice.

FixedSizeAllocatorWithCleanup isn't what it seems by name. It can allocate more memory just by asking it to do so. There was another issue I was going to open... if you call allocate() a second time before deallocate(), m_array gets abandoned and never gets wiped. Which kind of defeats the other part of its name. So it doesn't promise fixed size and potentially misses a cleanup. Shouldn't allocate() wipe when it abandons m_array?

I'm not really worried about this. Names and such cannot change after 27 years or so.

I image the class methods, like allocate and deallocate, are present so you can swap-in different SecBlocks.

Except allocate() doesn't do that. If it's been called before and deallocate() hasn't been called in between, it doesn't allow you to swap in anything at all. It gives you a new buffer allocated by the fallback allocator. Call it a third time and you presumably get a leak?

You're not even in that code path. Look at your backtrace. You've got FixedSizeAllocatorWithCleanup<T, S, A, false>.

[dwmcrobb]

Note the Tiger class inherits from IteratedHashWithStaticTransform. i.e. it doesn't ask for alignment, it gets the default alignment value for the IteratedHashWithStaticTransform template, which is false. Which is then used to instantiate a FixedSizeAlignedSecBlock with T_Align16 set to false. Which instantiates a FixedSizeAllocatorWithCleanup with T_Align16 set to false.

[noloader]

Note the Tiger class inherits from IteratedHashWithStaticTransform. i.e. it doesn't ask for alignment, it gets the default alignment value for the IteratedHashWithStaticTransform template, which is false. Which is then used to instantiate a FixedSizeAlignedSecBlock with T_Align16 set to false. Which instantiates a FixedSizeAllocatorWithCleanup with T_Align16 set to false.

Right. And the array is word64 but it is 4-byte aligned on 32-bit machines, not 8-byte aligned.

I think that means one of two things. First, this is correct and the toolchain is incorrect.

alignof(word64): 8
alignof(word64*): 4
alignof(word64[]): 8

Or, the toolchain is correct and alignof is incorrect.

Either way, someone else's problem has become our problem.

[dwmcrobb]

Given what we know, I think it's our problem to fix.

I don't see why void ByteReverse(T *out, const T *in, size_t byteCount) would want to assert alignment. Even when the scalar ByteReverse() it calls is using specific assembly instructions, the values to be reversed are passed by value. Meaning the alignment of the array isn't in play; the location of the value inside the scalar ByteReverse() is up to the calling conventions of the platform.

If void ByteReverse(T *out, const T *in, size_t byteCount) requires alignment, it should say so in the documentation, loudly. But I don't see how it requires alignment. So to me, the problem is that it's asserting something that it shouldn't assert because it doesn't (and shouldn't) care. Yes, there could be performance implications for unaligned loads, but that shouldn't be the decision of void ByteReverse(T *out, const T *in, size_t byteCount).

[noloader]

If void ByteReverse(T *out, const T *in, size_t byteCount) requires alignment, it should say so in the documentation, loudly. But I don't see how it requires alignment.

It is a C/C++ requirement. If alignment is not respected, then it is undefined behavior.

[dwmcrobb]

Keep in mind... the template instantiation explicitly said, "I don't care about alignment.". Unbeknownst to them, something it uses forces alignment in debug builds, despite that something not actually requiring alignment.

I don't think the answer is to try to force alignment on everything that calls ByteReverse(). My assumption would be, "The caller may or may not care about alignment. I don't care, so I don't assert it."

[mouse07410]

@noloader are you sure that C++ requires word (or greater) alignment for this operation? I didn't dig into the docs deep enough, but I don't recall ever seeing that.

[dwmcrobb]

If void ByteReverse(T *out, const T *in, size_t byteCount) requires alignment, it should say so in the documentation, loudly. But I don't see how it requires alignment.

It is a C/C++ requirement. If alignment is not respected, then it is undefined behavior.

Not quite. C++ doesn't have a specific rule for alignment, it's instead covered by other rules like strict aliasing.

We're talking about a template here, which accepts "pointer to T". It is perfectly valid for ByteReverse() to assume that the alignment of what that pointer points to is aligned in a manner that works on the given platform for T. Which as you've seen, might be smaller than the alignment of T in some cases. Not chosen by the user but by the toolchain based on what it knows about the platform (zero-cost misaligned load/stores, for example).

This should _never_ cause an assert, for any foo of interest here; it is not undefined behavior:

foo  src[20] = { /*whatever */},    dst[20];                                                          
ByteReverse(src, dst, sizeof(dst));                                             

That's different than this, which is the user asking for undefined behavior:

char  src[20] = { /* whatever */ }, dst[20];
uint32_t  *src_uint = (uint32_t *)src, *dst_uint = (uint32_t *)dst;
ByteReverse(src_uint, dst_uint, 20);                                             

Put another way, if you had code that asked for undefined behavior as above, and a platform that didn't have hardware support or trap code to deal with unaligned access, you'd see crashes in release builds. And you'd fix the caller. In my own experience, toolchains that align things in unexpected ways are targeting hardware that has hardware support for misaligned loads and stores and a motivation for reducing the alignment.

There are almost always ways for us to shoot ourselves in the foot, of course! :-) Take for example passing nullptr to ByteReverse() (which it doesn't assert). And of course platform and toolchain oddities (smaller-than-alignment-of-T) or breakage (alignof() or similar gives the wrong answer). If the platform has low-cost unaligned loads and stores, the toolchain is free to produce them. If not, it's free to work around it (traps and more instructions to handle them, potentially very slow) or crash. I assume you have all 3 scenarios in your compile farm. You haven't seen any crashes in release builds simply because no caller of ByteReverse() is asking for undefined behavior.

[noloader]

@mouse,

are you sure that C++ requires word (or greater) alignment for this operation?

Yes. Here's the Linux i386 ABI. It says long long alignment is 8 bytes. System V Application Binary Interface
AMD64 Architecture Processor Supplement (With LP64 and ILP32 Programming Models).

And here is Microsoft's page on it. It says long long alignment is 8 bytes. Alignment.

---

@dwmcrobb,

This should never cause an assert, for any foo of interest

This may cause an assert to fire. It will cause an assert to fire when T = word64 and either (or both) in or out are not aligned to 8-bytes. This is the assert we are talking about.

template <class T>
void ByteReverse(T *out, const T *in, size_t byteCount)
{
    // Alignment check due to Issue 690
    CRYPTOPP_ASSERT(byteCount % sizeof(T) == 0);
    CRYPTOPP_ASSERT(IsAligned<T>(in));
    CRYPTOPP_ASSERT(IsAligned<T>(out));

    size_t count = byteCount/sizeof(T);
    for (size_t i=0; i<count; i++)
        out[i] = ByteReverse(in[i]);
}

in[i] and out[i] perform a dereference. They are the same as *(in+i) and *(out+i).

Also see Issue #690, Solaris 11 and Sparc crash in Tiger . It was a bigger problem than Tiger. It affected just about all 64-bit hashes.

[dwmcrobb]

Yes, because your assert is bogus. It unnecessarily penalizes those who did not ask for undefined behavior. It shouldn't be there unless ByteReverse() requires a particular alignment, which it doesn't.

[dwmcrobb]

Put another way, if the toolchain can use a smaller alignment for the target arch and obey all of the language rules (in this case, pointer arithmetic et. al.), it is free to do that.

Or another way... think of the huge mounds of trivial code that wouldn't work (despite the user obeying all the language rules) on such a platform. This code is just looping over an array; it doesn't get much simpler. The toolchain makes sure it will work if you're obeying the language rules (if it didn't, most any non-trivial code would not work; you couldn't have any function that looped over an array of uint64_t). You've come along and told the user "I don't accept the way this toolchain does things" despite the fact that it breaks no rules, the user broke no rules, and the toolchain produces correct machine code for the target.

[dwmcrobb]

And note that my comments are apart from a toolchain that produces misaligned loads and stores on its own for a target that doesn't handle them. That'd be a fundamental toolchain bug.

[dwmcrobb]

@Mouse,

are you sure that C++ requires word (or greater) alignment for this operation?

Yes. Here's the Linux i386 ABI. It says long long alignment is 8 bytes. System V Application Binary Interface AMD64 Architecture Processor Supplement (With LP64 and ILP32 Programming Models).

That's the wrong ABI for 32-bit architectures. You want the IA32 ABI if we're talking about an IA32. Which is 4-byte alignments.

https://refspecs.linuxfoundation.org/LSB_3.1.0/LSB-Core-IA32/LSB-Core-IA32.pdf

[noloader]

I think we can close this now.