CRYPTOPP_ASSERT

154 views
Skip to first unread message

Jeffrey Walton

unread,
Jul 15, 2015, 4:27:04 PM7/15/15
to cryptop...@googlegroups.com
I'm catching an assert when running with -DDEBUG (i.e., not -DNDEBUG):

    cryptest.exe: misc.h:703: T CryptoPP::rotlMod(T, unsigned int)
        [with T = unsigned int]: Assertion `y <= 255' failed.

The assert is fine. I adore asserts, because they create "self debugging code". When the code tells me where the problem is, I don't have to spend time under a debugger. In code under my purview in real life, we make debug instrumentation a security gate because it so helpful. If code does not have the instrumentation, it cannot be checked in.

The issue I have is Posix's default behavior of "lets crash the program while the developer is debugging it". Its completely useless behavior.

I want to cut-in a CRYPTOPP_ASSERT. The CRYPTOPP_ASSERT will do everything assert does, but it will raise a SIG_TRAP rather than SIG_ABRT. SIG_TRAP will snap the debugger if present. If not present, you sometimes get a message on the console. In both cases, the program continues.

You can see an example of the improved assert at https://code.google.com/p/owasp-esapi-cplusplus/source/browse/trunk/esapi/EsapiCommon.h (line 183).

Are there any comments or objections to cutting in a CRYPTOPP_ASSERT?

Jean-Pierre Münch

unread,
Jul 15, 2015, 4:36:56 PM7/15/15
to cryptop...@googlegroups.com
I'm fine with this as long as it doesn't change behavior on Windows / Visual Studio, which normally catches asserts and uncatched exceptions by default :)

BR

JPM
--
--
You received this message because you are subscribed to the "Crypto++ Users" Google Group.
To unsubscribe, send an email to cryptopp-user...@googlegroups.com.
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 cryptopp-user...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Jeffrey Walton

unread,
Jul 15, 2015, 6:48:17 PM7/15/15
to cryptop...@googlegroups.com


On Wednesday, July 15, 2015 at 4:36:56 PM UTC-4, jean-pierre.muench wrote:
I'm fine with this as long as it doesn't change behavior on Windows / Visual Studio, which normally catches asserts and uncatched exceptions by default :)

Yes, it won't muck with Windows. Command line programs running outside a debugger will still crash, IIRC. (Microsoft recognized the problem, and fixed it under the IDE a long time ago).

Jeff
 

Jeffrey Walton

unread,
Jul 20, 2015, 12:06:14 PM7/20/15
to cryptop...@googlegroups.com
Attached is the meat and potatoes of the proposed patch. It omits all the assert -> CRYPTOPP_ASSERT changes.

It works great outside and under the debugger. The program does not crash with a SIGABRT at critical times during the debug cycle.


On Wednesday, July 15, 2015 at 4:27:04 PM UTC-4, Jeffrey Walton wrote:
I'm catching an assert when running with -DDEBUG (i.e., not -DNDEBUG):

    cryptest.exe: misc.h:703: T CryptoPP::rotlMod(T, unsigned int)
        [with T = unsigned int]: Assertion `y <= 255' failed.

The assert is fine. I adore asserts, because they create "self debugging code". When the code tells me where the problem is, I don't have to spend time under a debugger. In code under my purview in real life, we make debug instrumentation a security gate because it so helpful. If code does not have the instrumentation, it cannot be checked in.

The issue I have is Posix's default behavior of "lets crash the program while the developer is debugging it". Its completely useless behavior.

diff --git a/config.h b/config.h
index e359729..9c4fd9e 100644
--- a/config.h
+++ b/config.h
@@ -53,6 +53,11 @@
 #define PREFER_BERKELEY_STYLE_SOCKETS
 // #define PREFER_WINDOWS_STYLE_SOCKETS
 
+// Undefine this in Debug builds if you don't want a SIGTRAP handler installed.
+// The cryptest program will install the handler for CRYPTOPP_ASSERT which
+// raises SIGTRAP. Its does not affect Release builds when NDEBUG is defined.
+#define CRYPTOPP_INSTALL_SIGTRAP_HANDLER
+
 // set the name of Rijndael cipher, was "Rijndael" before version 5.3
 #define CRYPTOPP_RIJNDAEL_NAME "AES"
 
diff --git a/test.cpp b/test.cpp
index a3250a8..0dd5937 100644
--- a/test.cpp
+++ b/test.cpp
@@ -56,6 +56,12 @@ USING_NAMESPACE(std)
 
 const int MAX_PHRASE_LENGTH=250;
 
+#if defined(CRYPTOPP_INSTALL_SIGTRAP_HANDLER) && !defined(NDEBUG)
+#if defined(CRYPTOPP_UNIX_AVAILABLE)
+# include <signal.h>  // SIGTRAP handler
+#endif
+#endif
+
 void RegisterFactories();
 
 void GenerateRSAKey(unsigned int keyLength, const char *privFilename, const char *pubFilename, const char *seed);
@@ -238,7 +244,7 @@ int CRYPTOPP_API main(int argc, char *argv[])
 
             // compute MAC
             member_ptr<MessageAuthenticationCode> pMac(NewIntegrityCheckingMAC());
-            assert(pMac->DigestSize() == sizeof(mac));
+            CRYPTOPP_ASSERT(pMac->DigestSize() == sizeof(mac));
             MeterFilter f(new HashFilter(*pMac, new ArraySink(mac, sizeof(mac))));
             f.AddRangeToSkip(0, checksumPos, 4);
             f.AddRangeToSkip(0, certificateTableDirectoryPos, 8);
@@ -369,6 +375,55 @@ int CRYPTOPP_API main(int argc, char *argv[])
     }
 }
 
+#if defined(CRYPTOPP_INSTALL_SIGTRAP_HANDLER) && !defined(NDEBUG)
+#if defined(CRYPTOPP_UNIX_AVAILABLE)
+// Add a SIGTRAP handler for *nix.
+struct DebugTrapHandler
+{
+    DebugTrapHandler()
+    {
+        // http://pubs.opengroup.org/onlinepubs/007908799/xsh/sigaction.html
+        struct sigaction old_handler, new_handler;
+        memset(&old_handler, 0x00, sizeof(old_handler));
+        memset(&new_handler, 0x00, sizeof(new_handler));
+
+        do
+        {
+            int ret = 0;
+
+            ret = sigaction (SIGTRAP, NULL, &old_handler);
+            if (ret != 0) break; // Failed
+
+            // Don't step on another's handler
+            if (old_handler.sa_handler != NULL) break;
+
+            // Set up the structure to specify the null action.
+            new_handler.sa_handler = &DebugTrapHandler::NullHandler;
+            new_handler.sa_flags = 0;
+
+            ret = sigemptyset (&new_handler.sa_mask);
+            if (ret != 0) break; // Failed
+
+            // Install it
+            ret = sigaction (SIGTRAP, &new_handler, NULL);
+            if (ret != 0) break; // Failed
+
+        } while(0);
+    }
+
+    static void NullHandler(int /*unused*/) { }
+};
+
+#if __GNUC__
+// Specify a relatively low priority to make sure we run before other CTORs
+// http://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Attributes.html#C_002b_002b-Attributes
+static const DebugTrapHandler g_dummyHandler __attribute__ ((init_priority (110)));
+#else
+static const DebugTrapHandler g_dummyHandler;
+#endif // __GNUC__
+#endif // CRYPTOPP_UNIX_AVAILABLE
+#endif // CRYPTOPP_INSTALL_SIGTRAP_HANDLER and not NDEBUG
+
 void FIPS140_GenerateRandomFiles()
 {
 #ifdef OS_RNG_AVAILABLE
 
***** BEGIN cat of trap.h *****

// trap.h - written and placed in public domain by Jeffrey Walton.
//          Copyright assigned to Crypto++ project

#ifndef CRYPTOPP_TRAP_H
#define CRYPTOPP_TRAP_H

#ifndef NDEBUG
#ifdef CRYPTOPP_UNIX_AVAILABLE
# include <iostream>
# include <sstream>
# include <signal.h>
#endif // CRYPTOPP_UNIX_AVAILABLE
#endif // NDEBUG

#include <cassert>

// ************** run-time assertion ***************

// Linux and Unix
#if !defined(NDEBUG) && defined(CRYPTOPP_UNIX_AVAILABLE)
#  define CRYPTOPP_ASSERT(exp) {                                  \
    if(!(exp)) {                                                  \
      std::ostringstream oss;                                     \
      oss << "Assertion failed: " << (char*)(__FILE__) << "("     \
          << (int)(__LINE__) << "): " << (char*)(__func__)        \
          << std::endl;                                           \
      std::cerr << oss.str();                                     \
      raise(SIGTRAP);                                             \
    }                                                             \
  }
// Fallback to original behavior (including for NDEBUG)
#else
#  define CRYPTOPP_ASSERT(exp) assert(exp)
#endif // NDEBUG

#endif // CRYPTOPP_TRAP_H

***** END cat of trap.h *****
 
assert.diff

Jeffrey Walton

unread,
Jul 20, 2015, 2:11:20 PM7/20/15
to cryptop...@googlegroups.com


On Monday, July 20, 2015 at 12:06:14 PM UTC-4, Jeffrey Walton wrote:
Attached is the meat and potatoes of the proposed patch. It omits all the assert -> CRYPTOPP_ASSERT changes.

It works great outside and under the debugger. The program does not crash with a SIGABRT at critical times during the debug cycle.
It looks like the changes to the Visual Studio projects will be limited to adding a header to the appropriate projects (*.vcproj). Can anyone confirm this looks sane?

Jeff

**********

$ grep hex.h * | grep -v include
cryptdll.dsp:SOURCE=.\hex.h
cryptdll.vcproj:                RelativePath="hex.h"
cryptlib.dsp:SOURCE=.\hex.h
cryptlib.vcproj:                RelativePath="hex.h"

$ grep cryptlib.h * | grep -v include
cryptdll.dsp:SOURCE=.\cryptlib.h
cryptdll.vcproj:                RelativePath="cryptlib.h"
cryptlib.dsp:SOURCE=.\cryptlib.h
cryptlib.vcproj:                RelativePath="cryptlib.h"

$ grep config.h * | grep -v include
cryptdll.dsp:SOURCE=.\config.h
cryptdll.vcproj:                RelativePath="config.h"
cryptlib.dsp:SOURCE=.\config.h
cryptlib.vcproj:                RelativePath="config.h"

$ grep misc.h * | grep -v include
cryptdll.dsp:SOURCE=.\misc.h
cryptdll.vcproj:                RelativePath="misc.h"
cryptlib.dsp:SOURCE=.\misc.h
cryptlib.vcproj:                RelativePath="misc.h"

 

Jean-Pierre Münch

unread,
Jul 20, 2015, 3:30:13 PM7/20/15
to cryptop...@googlegroups.com
You may want to add that #define CRYPTOPP_INSTALL_SIGTRAP_HANDLER (in the comment) only affects GCC / Unix.
I'm not sure if everybody knows this is unix-only behavior.

I think you may missed one CRYPTOPP_ASSERT...

BR

JPM

Jeffrey Walton

unread,
Jul 21, 2015, 6:35:23 AM7/21/15
to cryptop...@googlegroups.com


On Monday, July 20, 2015 at 3:30:13 PM UTC-4, jean-pierre.muench wrote:
You may want to add that #define CRYPTOPP_INSTALL_SIGTRAP_HANDLER (in the comment) only affects GCC / Unix.
I'm not sure if everybody knows this is unix-only behavior.

I'm beginning to wonder if the config setting is even needed. The signal handler is part of cryptest, and not cryptlib, so there's nothing to configure for the library. I think we should always install it.

What *might* be more useful is to allow someone to select the signal - SIGTRAP, SIGABRT or some other. Posix uses SIGABRT, and its useless behavior in Debug. SIGTRAP is much more useful in Debug.

In Release mode, asserts should not be in effect, so it does not matter what its set to.

Jeff

Ilya Bizyaev

unread,
Jul 23, 2015, 6:08:54 AM7/23/15
to Crypto++ Users, nolo...@gmail.com
Could you please fix annoying warnings caused by CRYPTOPP_ASSERT declaration in MinGW?
Here they are:

C:\cryptopp\misc.h|58|warning: unused variable 'cryptopp_assert_26' [-Wunused-variable]|
C:\cryptopp\misc.h|61|note: in definition of macro 'CRYPTOPP_DO_ASSERT_JOIN'|
C:\cryptopp\misc.h|58|note: in expansion of macro 'CRYPTOPP_ASSERT_JOIN'|
C:\cryptopp\misc.h|54|note: in expansion of macro 'CRYPTOPP_COMPILE_ASSERT_INSTANCE'|
C:\cryptopp\algparam.h|26|note: in expansion of macro 'CRYPTOPP_COMPILE_ASSERT'|

Thanks in advance!

Jeffrey Walton

unread,
Jul 23, 2015, 6:16:48 AM7/23/15
to Ilya Bizyaev, Crypto++ Users
> C:\cryptopp\misc.h|58|warning: unused variable 'cryptopp_assert_26'
> [-Wunused-variable]|
> C:\cryptopp\misc.h|61|note: in definition of macro
> 'CRYPTOPP_DO_ASSERT_JOIN'|
> C:\cryptopp\misc.h|58|note: in expansion of macro 'CRYPTOPP_ASSERT_JOIN'|
> C:\cryptopp\misc.h|54|note: in expansion of macro
> 'CRYPTOPP_COMPILE_ASSERT_INSTANCE'|
> C:\cryptopp\algparam.h|26|note: in expansion of macro
> 'CRYPTOPP_COMPILE_ASSERT'|

CRYPTOPP_ASSERT is different than CRYPTOPP_COMPILE_ASSERT. But yeah,
give me a minute to fire up MinGW's VM and see what can be done.

What warnings are you using? Is this at -Wall and higher?

Note that you might be hitting this GCC bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431. There's nothing we
can do about that. From comment 13
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c13):

This issued caused Crypto++ to remove -Wall (and above)
under GCC. Crypto++ is C++ with lots of interfaces, and it
performs a fair amount of intermediate calculations used in
an assert. It really needed the following to work as expected:

// GCC diagnostcs available after GCC 4.2
(https://gcc.gnu.org/ml/gcc-help/2015-07/msg00063.html)
#define GCC_DIAGNOSTIC_AWARE ((__GNUC__ > 4 || (__GNUC__ == 4 &&
__GNUC_MINOR__ >= 2)) || defined(__clang__))

#if GCC_DIAGNOSTIC_AWARE
# pragma GCC diagnostic push
# pragma GCC diagnostic ignored "-Wunused-value"
# pragma GCC diagnostic ignored "-Wunused-variable"
#endif

I offered to pay one of the GCC devs a bounty to fix it. No takers yet.

Jeff

Ilya Bizyaev

unread,
Jul 23, 2015, 6:54:57 AM7/23/15
to Crypto++ Users
I use -Wall, GCC 4.8.1
By the way, there are lots of other warnings, not with asserts.
I can't say for sure right now, but there were unused StringNarrow method and unused *p pointer. I'll send more details tomorrow.

Jeffrey Walton

unread,
Jul 23, 2015, 9:12:18 AM7/23/15
to Crypto++ Users, bizyae...@gmail.com


I use -Wall, GCC 4.8.1
By the way, there are lots of other warnings, not with asserts.

Yeah, you'll need to send something that gives us something to latch onto to begin investigating. An output from a compile would be nice.
 
I can't say for sure right now, but there were unused StringNarrow method and unused *p pointer. I'll send more details tomorrow.

I haven't seen those in a while.

I thought StringNarrow (from misc.h) was cleared at https://github.com/weidai11/cryptopp/commit/a0390f1fd725e37a201cc47ba47204182ebc1ccf. I thought the unused ptr (from argparams.h) was cleared at https://github.com/weidai11/cryptopp/commit/5f25c7363534656ce6e1ecac63ca694e6d4ba7d6.

Are you using the latest sources from either SVN or GitHub?

Jeff

Ilya Bizyaev

unread,
Jul 23, 2015, 10:00:03 AM7/23/15
to Crypto++ Users
Well... Actually, no; I use Crypto++ 5.6.2 from cryptopp.com. Am I wrong?

Ilya Bizyaev

unread,
Jul 23, 2015, 10:03:12 AM7/23/15
to Crypto++ Users
Oh, I see I am! The thing that was fixed in bdb7ea3 commit also caused a warning in my case! I'll upgrade my Crypto++ tomorrow!..

Jeffrey Walton

unread,
Jul 23, 2015, 10:09:58 AM7/23/15
to Crypto++ Users, bizyae...@gmail.com


On Thursday, July 23, 2015 at 10:00:03 AM UTC-4, Ilya Bizyaev wrote:
Well... Actually, no; I use Crypto++ 5.6.2 from cryptopp.com. Am I wrong?

We've cleared a lot of issues in the last month or two. There is no updated ZIP file.

In fact, the 5.6.2 ZIP file was stale as soon as Wei pushed the first update after building it. That was a couple of years ago.

You might consider switching to GitHub. Moving forward, that's the delivery platform of choice.

Its good that Zooko got us to move to GiHub. SourceForge is still down. Its been 8 days and counting. https://twitter.com/sfnet_ops.

Jeff
Message has been deleted

Jeffrey Walton

unread,
Jul 25, 2015, 5:08:59 AM7/25/15
to Ilya Bizyaev, Crypto++ Users
On Sat, Jul 25, 2015 at 3:47 AM, Ilya Bizyaev <bizyae...@gmail.com> wrote:
> Sorry to disappoint you, Jeffrey, but the MinGW build of the latest Crypto++
> from GitHub fails on integer.cpp...
> I attach the compiler output:
> https://yadi.sk/i/N_BePZlei5e3C

Damn, sorry about that. Firing up the Windows VM is still on the short
list of things TODO tonight.

Give me about 10 minutes and I'll have it backed out.

Jeff

Jeffrey Walton

unread,
Jul 25, 2015, 6:07:05 AM7/25/15
to Ilya Bizyaev, Crypto++ Users
On Sat, Jul 25, 2015 at 3:47 AM, Ilya Bizyaev <bizyae...@gmail.com> wrote:
> Sorry to disappoint you, Jeffrey, but the MinGW build of the latest Crypto++
> from GitHub fails on integer.cpp...
> I attach the compiler output:
> https://yadi.sk/i/N_BePZlei5e3C

OK, that's been backed out:
https://github.com/weidai11/cryptopp/commit/e75cb8dd8b0fe25e0766be2de0105a72625ec862.

Thanks again.

Ilya Bizyaev

unread,
Jul 26, 2015, 5:46:14 AM7/26/15
to Crypto++ Users, nolo...@gmail.com
No errors now, but there is a warning:

All tests passed, but I have not yet tried to recompile my app.


Ilya Bizyaev

unread,
Jul 26, 2015, 8:12:18 AM7/26/15
to Crypto++ Users, nolo...@gmail.com, bizyae...@gmail.com
After the library update, this code causes a segmentation fault:

unsigned int result;
GenerateBlock((byte*)&result, sizeof(unsigned int));

Here is the call stack:


Илья Бизяев

unread,
Jul 28, 2015, 2:43:03 AM7/28/15
to nolo...@gmail.com, Crypto++ Users

OK, I'll try to recompile it either this evening or tomorrow.


сб, 25 июля 2015, 13:07, Jeffrey Walton <nolo...@gmail.com>:

Ilya Bizyaev

unread,
Jul 28, 2015, 2:10:17 PM7/28/15
to Crypto++ Users
The problem I've reported on the 26th of July is still actual.

Jeffrey Walton

unread,
Jul 29, 2015, 12:48:29 AM7/29/15
to Crypto++ Users, bizyae...@gmail.com


On Tuesday, July 28, 2015 at 2:10:17 PM UTC-4, Ilya Bizyaev wrote:
The problem I've reported on the 26th of July is still actual.

What problem is that?

Looking at the two threads dated July 26, they are "Fwd: AlgorithmParameters and noexcept patch" and "INSTALL added". The first issue was cleared, the secondwtopic was informational.

(Its hard to follow some of your threads because you start a new topic on an unrelated thread on occasion rather than starting a new thread).

Ilya Bizyaev

unread,
Jul 29, 2015, 2:20:13 AM7/29/15
to Crypto++ Users
In this very thread ;)
I am messaging from a mobile device, thus I cannot make a quote. Scroll this topic up until you see a call stack screenshot.
Reply all
Reply to author
Forward
0 new messages