FreeBSD/Clang++38: AlgorithmParameters/MakeParameters throws when used

[anonimal]

State the operating system and version (Ubutnu 17 x86_64, Windows 7 Professional x64, etc)

FreeBSD freebsd 10.3-RELEASE-p11 FreeBSD 10.3-RELEASE-p11 #0: Mon Oct 24 18:49:24 UTC 2016 root@amd64-builder.daemonology.net:/usr/obj/usr/src/sys/GENERIC amd64

State the version of the Crypto++ library (Crypto++ 5.6.5, Master, etc)

Tested against f1a80e6 and 591d70f

State how you built the library (Makefile, Cmake, distro, etc)
Show a typical command line (the output of the compiler for cryptlib.cpp)
Show the link command (the output of the linker for libcryptopp.so or cryptest.exe)

cd deps/cryptopp/ && gmake CXXFLAGS="-march=native -DCRYPTOPP_NO_CPU_FEATURE_PROBES=1" static as noted here. Note: also reproducible with clang++39.

Show the exact error message you are receiving (copy and paste it); or
Clearly state the undesired behavior (and state the expected behavior)

• Kovri: new base32/64 impl / radix interface / unit-test monero-project/kovri#788 (comment)
• Expected behavior with pre-processor patch noted in the above comment.

throwIfNotUsed is ignored if using a compiler that does not support std::uncaught_exception(), such as MSVC 7.0 and earlier.

I don't think this applies but noting in case I've overlooked another caveat.

[noloader]

Thanks @anonimal and @coneiric.

I've seen this before on OS X while watching cryptest.exe under a debugger. If I recall correctly, on OS X cryptest.exe would crash on exit in _fini because that's where the global destructors were. Because the program was exiting, OS X did not report the error or uncaught exception to the user. I thought it was unusual but I did not see a library problem. I wrote it off as one of those bad interactions that was mostly benign.

In Korvi's case I think it is a similar situation with two or three items contributing to it. Give me some time to sort them out so we can remediate them in turn.

I don't think we have a self test for swapping in an new alphabet. I should probably get one added.

[anonimal]

Are you still seeing the issue with the latest release?

I responded this Saturday but apparently never pressed the Comment button! Sorry (it was a long week)!

Surprisingly, no, I'm not seeing this issue with the latest release. Which commit was the fix?

[noloader]

Surprisingly, no, I'm not seeing this issue with the latest release. Which commit was the fix?

In 6.0 we moved to unrolling the decode table. Previously it was built on the fly but it suffered a race in multi-threaded environments. The unrolling also removed the code to build the decode table, and I believe some code of that code showed up in _init and _fini. The runtime decoding table also had some troubles in early Microsoft environments, but I don't believe that applied here.

I think the commit of interest is/was 0dc97f1d3a801c33.

[anonimal]

@noloader I'm sorry, I was a very tired buffoon when I tested this at the end of last week and now realize that I was working in the wrong branch. I can confirmed that the issue is not resolved. https://build.getmonero.org/builders/kovri-all-freebsd64/builds/778/steps/test/logs/stdio

Sorry about that; I've been overworked ☹️

[noloader]

@anonimal and @coneiric,

I got to look at this a little more. My FreeBSD 11 VM did not experience the crash, and neither did my new FreeBSD 10.3 VM. However I am seeing a failure in aligned allocations with CXXFLAGS="-DDEBUG -march=native -DCRYPTOPP_NO_CPU_FEATURE_PROBES=1. -DDEBUG enables asserts and they are firing quite frequently.

This is the assert from misc.cpp : 309:

void * AlignedAllocate(size_t size)
{
    ...

    while ((p = (byte *)memalign(16, size)) == NULLPTR)
        CallNewHandler();  // free/compact memory

    CRYPTOPP_ASSERT(IsAlignedOn(p, 16));
    return p;
}

I checked the FreeBSD malloc(3) man page and it says the function returns a block aligned for any type of object. If memory is not aligned then we probably wandered into undefined behavior.

I'm trying to locate a function that really provides aligned memory. In the meantime would you try Commit c4392c40e052b1bb.

[anonimal]

In the meantime would you try Commit c4392c4.

No observable differences, and backtrace looks the same.

[noloader]

@anonimal,

I cloned your GitHub. I can't duplicate this with :

cd deps/cryptopp/ && gmake CXXFLAGS="-march=native -DCRYPTOPP_NO_CPU_FEATURE_PROBES=1" static

I'm testing on FreeBSD 10.3 x86_64 (unpatched, original ISO). I'm also testing on FreeBSD 11 x86_64 (fully patched). I'm using both c++ compiler and clang++38 compiler. I've also tried debug and release builds, Once the test suite builds I then run:

./cryptest.exe v

Is there anything I seem to be missing here?

[anonimal]

I cloned your GitHub. I can't duplicate this with :

Which compiler and which branch/revision?

[noloader]

@anonimal,

Which compiler?

Both the default c++ compiler and clang++38 compiler.

[anonimal]

Which branch/revision? We have the patch still in master.

[noloader]

@anonimal,

Which branch/revision? We have the patch still in master.

Yeah, I am on master.

Let me ask you... when you run make ... static, only the static library is built. What are you running it against? I have been running against the cryptest.exe program by building it too.

If it is another program, then do you have instructions for building it?

[anonimal]

What are you running it against?

I hope I'm answering your question: https://github.com/monero-project/kovri/blob/master/tests/unit_tests/CMakeLists.txt#L41. We simply link the unit-test executable to libcryptopp.a.

[anonimal]

I'm also testing on FreeBSD 11 x86_64 (fully patched)

I'll go ahead and pull in a fresh image locally. If I can't reproduce there then I'm assuming there's a problem with our build machine.

[noloader]

@anonimal,

I'm running different tests than you. I'm running the cryptest.exe program. You are running something from your test suite. I think they are apples and oranges.

According to:

#0  0x000000080349339a in thr_kill () from /lib/libc.so.7
#1  0x0000000803493386 in raise () from /lib/libc.so.7
#2  0x0000000803493309 in abort () from /lib/libc.so.7
#3  0x000000000047a78f in __clang_call_terminate () at ios:507
#4  0x000000000059651e in CryptoPP::member_ptr<CryptoPP::AlgorithmParametersBase>::~member_ptr (this=0x7fffffffca80) at smartptr.h:71
#5  0x0000000000595481 in CryptoPP::AlgorithmParameters::~AlgorithmParameters (this=0x7fffffffca78) at algparam.h:433
#6  0x0000000000594e5d in kovri::core::Base32::Decode (in=0x805032a99 "", len=0) at /usr/home/anonimal/kovri/src/core/crypto/impl/cryptopp/radix.cc:76
#7  0x00000000007a2183 in kovri::core::Tag<32>::FromBase32 (this=0x7fffffffd1c0, encoded=@0x805032a98) at identity.h:124
#8  0x000000000092bf90 in kovri::client::ParseACL (list=
      {<std::__1::__basic_string_common<true>> = {<No data fields>}, __r_ = {<std::__1::__libcpp_compressed_pair_imp<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__rep, std::__1::allocator<char>, 2>> = {<std::__1::allocator<char>> = {<No data fields>}, __first_ = {{__l = {__cap_ = 177, __size_ = 164, __data_ = 0x805045300 "7ooubi6turxmjmtrpx2sq2mfpmjaywr3aqzs6skii4lwiwipcmoa,,,,65m4tm6zwzn3wkj2c56abig7vjrxnucszbsdpwrog3cocpxru2hq,,,,3pz774osmoosan7opdjyzld2knlyizwnggfounbzmd6m5tz5brla"}, __s = {{__size_ = 177 '', __lx = -79 ''}, __data_ = 0x7fffffffd531 ""}, __r = {__words = 0x7fffffffd530}}}}, <No data fields>}, static npos = <optimized out>}) at /usr/home/anonimal/kovri/src/client/util/parse.cc:56
#9  0x00000000004de97c in ClientParsing::ParseACL::test_method (this=0x7fffffffddb0) at /usr/home/anonimal/kovri/tests/unit_tests/client/util/parse.cc:77

Base32::Decode is the problem. The code for that is:

  template <typename Base_N>
  static std::vector<std::uint8_t> Decode(
      const CryptoPP::AlgorithmParameters& params,
      const char* in,
      const std::uint64_t len)
  {
    if (!in || !len)
      throw std::runtime_error("Decoder: null arg(s)");

    Base_N decoder;
    decoder.IsolatedInitialize(params);
    decoder.Put(reinterpret_cast<const CryptoPP::byte*>(in), len);
    decoder.MessageEnd();

    const CryptoPP::word64 size = decoder.MaxRetrievable();
    if (!size || size > SIZE_MAX)
      throw std::runtime_error("Radix: invalid decoded size");

    std::vector<std::uint8_t> out(size);
    decoder.Get(reinterpret_cast<CryptoPP::byte*>(&out[0]), out.size());

    return out;
  }

I'm having trouble determining where const CryptoPP::AlgorithmParameters& params is coming from. I see this from identity.h, but I don't see the name/value pairs:

  void FromBase32(const std::string& encoded)
  {
    std::vector<std::uint8_t> const decoded =
        core::Base32::Decode(encoded.c_str(), encoded.length());

    if (decoded.size() > Size)
      throw std::length_error("Tag: decoded base32 size too large");

    std::memcpy(m_Buf, decoded.data(), decoded.size());
  }

What I am wondering is, is the compiler making a copy of const CryptoPP::AlgorithmParameters& params under the AS-IF rule, and then forgetting to perform the accesses that marks unused=false.

[anonimal]

I'm having trouble determining where const CryptoPP::AlgorithmParameters& params is coming from.

We use a form of static CRTP:

https://github.com/monero-project/kovri/blob/master/src/core/crypto/impl/cryptopp/radix.cc#L63-L85:

std::vector<std::uint8_t> Base32::Decode(
    const char* in,
    const std::uint64_t len)
{
  // Prepare decoder
  int lookup[256];
  CryptoPP::Base32Decoder::InitializeDecodingLookupArray(
      lookup,
      reinterpret_cast<const CryptoPP::byte*>(GetAlphabet().c_str()),
      GetAlphabet().size(),
      true);

  CryptoPP::AlgorithmParameters const params(CryptoPP::MakeParameters(
      CryptoPP::Name::DecodingLookupArray(),
#if defined(__FreeBSD__)  // See #788
      reinterpret_cast<const int*>(lookup), false));
#else
      reinterpret_cast<const int*>(lookup)));
#endif

  // Decode
  return Radix::Decode<CryptoPP::Base32Decoder>(params, in, len);
}

What I am wondering is, is the compiler making a copy of const CryptoPP::AlgorithmParameters& params under the AS-IF rule, and then forgetting to perform the accesses that marks unused=false.

Good question. I don't have an answer now but can look further this week.

[anonimal]

We use a form of static CRTP:

Behind a pimpl pattern no-less 😏 monero-project/kovri#785 (the CRTP I like, the pimpl I don't).

[noloader]

OK, so this looks like a small issue. From argnames.h:

CRYPTOPP_DEFINE_NAME_STRING(DecodingLookupArray) ///< const byte *

The comments say DecodingLookupArray is a const byte *. So I would not use an const int* cast:

  CryptoPP::AlgorithmParameters const params(CryptoPP::MakeParameters(
      CryptoPP::Name::DecodingLookupArray(),
#if defined(__FreeBSD__)  // See #788
      reinterpret_cast<const int*>(lookup), false));
#else
      reinterpret_cast<const int*>(lookup)));
#endif

Instead I would use something like this. It may need a cast.

  CryptoPP::AlgorithmParameters const params(CryptoPP::MakeParameters(
      CryptoPP::Name::DecodingLookupArray(), ConstByteArrayParameter (lookup));

And if the compiler is doing clever things and causing an exception, then I would use this on all code paths:

  CryptoPP::AlgorithmParameters const params(CryptoPP::MakeParameters(
      CryptoPP::Name::DecodingLookupArray(), ConstByteArrayParameter (lookup), false);

[noloader]

Here is where I am at with things. I am kind of stuck.

I installed cpp-netlib, boost-libs and boost-all. Then I configured out-of-tree with cmake -DCMAKE_CC_COMPILER=clang38 -DCMAKE_CXX_COMPILER=clang++38. Compiling results in:

[anonimal]

The comments say DecodingLookupArray is a const byte *. So I would not use an const int* cast:
Instead I would use something like this. It may need a cast.
And if the compiler is doing clever things and causing an exception, then I would use this on all code paths:

Thanks, I will make the changes though I'm having build issues with a few of them right now. I'll need to look at the lib.

I installed cpp-netlib, boost-libs and boost-all

Is this for building kovri? We use a cpp-netlib submodule, so no need to install separately as long as you git clone --recursive (they have yet to release 0.13-release which includes related fixes).

FreeBSD build instructions here (see to the end of doc for the recursive clone note, that should probably be moved to the beginning), also noted in Quickstart.

[noloader]

Is this for building kovri?

Yes, I cloned your Github recursively.

[anonimal]

I'm also testing on FreeBSD 11 x86_64 (fully patched)

I'll go ahead and pull in a fresh image locally.

JFTR: I can reproduce this issue (#562) on a fresh FreeBSD 11.1-RELEASE image against master without any patches (old or new). I have yet to test the new patches.

[noloader]

@anonimal and @coneiric,

... though I'm having build issues with a few of them right now ...

I was looking at the Kovri's failed OpenBSD build. The Build 323 failure is due to Crypto++. It was tracked under Issue 579 and this should fix it: Commit 0de445b56a5d.

Confirmed fixed when building Crypto++ with eg++. Build 324 shows a failure, but it is due to undefined references for Boost. It looks like this Boost issue.

[anonimal]

The comments say DecodingLookupArray is a const byte *. So I would not use an const int* cast:

Doing so produces failed kovri test cases where they weren't failing before. On linux, gcc 7.2.1:

diff --git a/src/core/crypto/impl/cryptopp/radix.cc b/src/core/crypto/impl/cryptopp/radix.cc
index 50fdfd73..bbfba68e 100644
--- a/src/core/crypto/impl/cryptopp/radix.cc
+++ b/src/core/crypto/impl/cryptopp/radix.cc
@@ -74,11 +74,7 @@ std::vector<std::uint8_t> Base32::Decode(
 
   CryptoPP::AlgorithmParameters const params(CryptoPP::MakeParameters(
       CryptoPP::Name::DecodingLookupArray(),
-#if defined(__FreeBSD__)  // See #788
-      reinterpret_cast<const int*>(lookup), false));
-#else
-      reinterpret_cast<const int*>(lookup)));
-#endif
+      reinterpret_cast<const CryptoPP::byte *>(lookup)));
 
   // Decode
   return Radix::Decode<CryptoPP::Base32Decoder>(params, in, len);

Error:

unknown location(0): fatal error: in "Radix/Base32DestHash": CryptoPP::NameValuePairs::ValueTypeMismatch: NameValuePairs: type mismatch for 'DecodingLookupArray', stored 'PKh', trying to retrieve 'PKi'

diff --git a/src/core/crypto/impl/cryptopp/radix.cc b/src/core/crypto/impl/cryptopp/radix.cc
index 50fdfd73..bc2ed9ad 100644
--- a/src/core/crypto/impl/cryptopp/radix.cc
+++ b/src/core/crypto/impl/cryptopp/radix.cc
@@ -65,20 +65,16 @@ std::vector<std::uint8_t> Base32::Decode(
     const std::uint64_t len)
 {
   // Prepare decoder
-  int lookup[256];
+  CryptoPP::byte lookup[256];
   CryptoPP::Base32Decoder::InitializeDecodingLookupArray(
-      lookup,
+      reinterpret_cast<int*>(lookup),
       reinterpret_cast<const CryptoPP::byte*>(GetAlphabet().c_str()),
       GetAlphabet().size(),
       true);
 
   CryptoPP::AlgorithmParameters const params(CryptoPP::MakeParameters(
       CryptoPP::Name::DecodingLookupArray(),
-#if defined(__FreeBSD__)  // See #788
-      reinterpret_cast<const int*>(lookup), false));
-#else
-      reinterpret_cast<const int*>(lookup)));
-#endif
+      reinterpret_cast<const CryptoPP::byte*>(lookup)));
 
   // Decode
   return Radix::Decode<CryptoPP::Base32Decoder>(params, in, len);

Error:

unknown location(0): fatal error: in "ClientParsing/ParseACL": memory access violation at address: 0xa00000009: no mapping at fault address

Instead I would use something like this. It may need a cast.

Then how would you advise to cast? Or re-implement without an int array?

diff --git a/src/core/crypto/impl/cryptopp/radix.cc b/src/core/crypto/impl/cryptopp/radix.cc
index 50fdfd73..23f0c690 100644
--- a/src/core/crypto/impl/cryptopp/radix.cc
+++ b/src/core/crypto/impl/cryptopp/radix.cc
@@ -74,11 +74,7 @@ std::vector<std::uint8_t> Base32::Decode(
 
   CryptoPP::AlgorithmParameters const params(CryptoPP::MakeParameters(
       CryptoPP::Name::DecodingLookupArray(),
-#if defined(__FreeBSD__)  // See #788
-      reinterpret_cast<const int*>(lookup), false));
-#else
-      reinterpret_cast<const int*>(lookup)));
-#endif
+      CryptoPP::ConstByteArrayParameter(lookup)));
 
   // Decode
   return Radix::Decode<CryptoPP::Base32Decoder>(params, in, len);

[  0%] Building CXX object src/core/CMakeFiles/kovri-core.dir/crypto/impl/cryptopp/radix.cc.o
In file included from /home/anonimal/kovri/deps/cryptopp/../cryptopp/simple.h:17:0,
                 from /home/anonimal/kovri/deps/cryptopp/../cryptopp/filters.h:17,
                 from /home/anonimal/kovri/deps/cryptopp/../cryptopp/basecode.h:10,
                 from /home/anonimal/kovri/deps/cryptopp/../cryptopp/base32.h:11,
                 from /home/anonimal/kovri/src/core/../core/crypto/radix.h:38,
                 from /home/anonimal/kovri/src/core/crypto/impl/cryptopp/radix.cc:31:
/home/anonimal/kovri/deps/cryptopp/../cryptopp/algparam.h: In instantiation of ‘CryptoPP::ConstByteArrayParameter::ConstByteArrayParameter(const T&, bool) [with T = int [256]]’:
/home/anonimal/kovri/src/core/crypto/impl/cryptopp/radix.cc:77:47:   required from here
/home/anonimal/kovri/deps/cryptopp/../cryptopp/algparam.h:56:33: error: ‘int [256]’ is not a class, struct, or union type
   CRYPTOPP_COMPILE_ASSERT(sizeof(typename T::value_type) == 1);
                                 ^
/home/anonimal/kovri/deps/cryptopp/../cryptopp/misc.h:158:25: note: in definition of macro ‘CRYPTOPP_COMPILE_ASSERT_INSTANCE’
   static CompileAssert<(assertion)> \
                         ^~~~~~~~~
/home/anonimal/kovri/deps/cryptopp/../cryptopp/algparam.h:56:3: note: in expansion of macro ‘CRYPTOPP_COMPILE_ASSERT’
   CRYPTOPP_COMPILE_ASSERT(sizeof(typename T::value_type) == 1);
   ^~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/anonimal/kovri/deps/cryptopp/../cryptopp/filters.h:22:0,
                 from /home/anonimal/kovri/deps/cryptopp/../cryptopp/basecode.h:10,
                 from /home/anonimal/kovri/deps/cryptopp/../cryptopp/base32.h:11,
                 from /home/anonimal/kovri/src/core/../core/crypto/radix.h:38,
                 from /home/anonimal/kovri/src/core/crypto/impl/cryptopp/radix.cc:31:
/home/anonimal/kovri/deps/cryptopp/../cryptopp/algparam.h:57:31: error: request for member ‘data’ in ‘string’, which is of non-class type ‘const int [256]’
   Assign((const byte *)string.data(), string.size(), deepCopy);
                        ~~~~~~~^~~~
/home/anonimal/kovri/deps/cryptopp/../cryptopp/algparam.h:57:46: error: request for member ‘size’ in ‘string’, which is of non-class type ‘const int [256]’
   Assign((const byte *)string.data(), string.size(), deepCopy);
                                       ~~~~~~~^~~~
make[3]: *** [src/core/CMakeFiles/kovri-core.dir/build.make:903: src/core/CMakeFiles/kovri-core.dir/crypto/impl/cryptopp/radix.cc.o] Error 1

[noloader]

@anonimal and @coneiric,

I went ahead and made a PR showing how I would handle it. I hope you don't mind. Sorry it took three tries. I don't have a Kovri test machine at the moment. Also see Kovri | PR 804.

The ConstByteArrayParameter looked like more trouble than it was worth so I stayed with the int*. The int* is how we test it so we should know if that's a bad idea. Also see validat1.cpp : 3175 or so, MyEncoder and MyDecoder test classes.

[anonimal]

Thank you very much @noloader.

Referencing monero-project/kovri#804 (comment) so we don't get lost.

I've got two theories on it. First, I think the optimizer may be getting a bit overzealous. In this case the optimizer deduces m_used is never written so it schedules the throw. Second, the AlgorithmParameters const params is being allocated in an unexpected way, like the BSS section with initialized and read-only memory. In this case static and const play a role in the problem.

IMHO, that actually sounds very plausible. Would you treat that as a possible clang bug or something that crypto++ should redesign?

[noloader]

@anonimal and @coneiric,

Would you treat that as a possible clang bug or something that crypto++ should redesign?

Yes, I think it is a bug we should work around. I don't think we need a redesign in this area (and I would not want to be the guy to have to do it :)

I think this issue is a FreeBSD-ism. Consider, you are seeing it on FreeBSD, and the other platform I witnessed it on was OS X. The OS X Darwin kernel is XNU, and XNU borrows a lot of code from FreeBSD.

This is one of those low priority bugs for me. It happens infrequently, and when it surfaces we know how to contain it. The pain point for me is, reproducing it under a debugger. Right now I can't reproduce it even on OS X.

---

Based on your observation that it happens with a 0-byte array I am thinking it is closer to the first case - aggressive optimizations. The compiler sees there is nothing to process so it starts short circuiting computation, even the ones with side effects like setting m_used = true.

I don't recall the circumstances when I witnessed the issue, so I can't really say more about it on OS X.

[noloader]

@anonimal ,

I'm closing this out for the moment. If I duplicate the issue in the future we will definitely cite this issue.

[anonimal]

Ok, thanks for the collaboration.