Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test C++17 byte change with dry runs from various projects #442

Closed
noloader opened this issue Jul 16, 2017 · 3 comments
Closed

Test C++17 byte change with dry runs from various projects #442

noloader opened this issue Jul 16, 2017 · 3 comments

Comments

@noloader
Copy link
Collaborator

noloader commented Jul 16, 2017

I'd like to use this issue to track some dry runs of the proposed change due to C++17, std::byte and collisions with Crypto++ byte . We've tested the change with toy programs, and we would like to solicit feedback from real projects using Crypto++.

The 3 second tour is as follows. From 10,000 feet, Crypto++ plans on moving byte from the global namespace into the Crypto++ namespace. The move creates a void in user programs, and it can be filled by (1) using CryptoPP::byte instead of byte; (2) using byte = CryptoPP::byte; (C++11 and above); or (3) typedef unsigned char byte; (C++03 and above). If possible, you should use (3) - supply your own typedef.

  1. Make this change to Crypto++ library:
$ cat byte.diff
diff --git a/config.h b/config.h
index 291b148..92a6b84 100644
--- a/config.h
+++ b/config.h
@@ -194,10 +194,12 @@ namespace CryptoPP { }
 #      define __USE_W32_SOCKETS
 #endif

-typedef unsigned char byte;
+// typedef unsigned char byte;
+#define CRYPTOPP_NO_GLOBAL_BYTE 1

 NAMESPACE_BEGIN(CryptoPP)

+typedef unsigned char byte;
 typedef unsigned short word16;
 typedef unsigned int word32;
  1. Optionally, make this change in the user program or project using Crypto++:
#include <cryptopp/config.h>
#if (CRYPTOPP_VERSION >= 600) && (__cplusplus >= 201103L)
    using byte = CryptoPP::byte;
#else
    typedef unsigned char byte;
#endif

// Alternately, use CryptoPP::byte everywhere instead of ::byte
  1. Use GCC 7
    • Fedora 26 ships with it
    • Debian Buster with apt-get install gcc-7 g++-7
  2. Use -std=c++17
    • Build Crypto++ library with -std=c++17 (will be OK)
    • Build project with -std=c++17 (report your problems)

For item (4), Build Crypto++ library with -std=c++17 , perform the following:

$ CXX=g++-7 CXXFLAGS="-DNDEBUG -g2 -O3 -std=c++17" make -j 2

CRYPTOPP_NO_GLOBAL_BYTE is a convenience item for those supporting multiple versions of Crypto++. It is really only needed for the gap between the 5.6.5 release and the 6.0 release because Master currently uses #define CRYPTOPP_VERSION 600. But signalling it through a define is painless. Related, see Release Versioning | Post-Release Increment on the Crypto++ wiki.

Please report problems and work-arounds below. If you had problems, please state if it was due to using namespace std;. Work-arounds are suggested at std::byte | Fixing Programs on the Crypto++ wiki.


Here are folks who may be able to help with the dry run. Listing their name generates the GitHub notification. The names were gathered from a GitHub search for projects with activity in 2016 and 2017. @anonimal, @swarren, @mmoss, @zooko, @huahang, @prakhs123, @pld-linux, @sechaser, @elementrem, @xItsMe, @Mowje, @NickNaso, @cawka, @wyrover, @vunguyenthe, @vster, @alphaemmeo, @smasherprog, @pschord, @psforever, @harrywye, @vcpkgs, @tpgxyz, @luofujie, @badforlabor, @smasherprog, @riebl, @DonAlonzo, @amani-lei, @xy12423, @Cyberunner23, @Xiao-Chong, @coderChenY, @familyld, @firngrod, @tcolligan-ap, @dotman14, @reganheath, @motioncity, @MarcelRaad, @DevJPM, @mouse07410, @denisbider, @gcsideal, @buildroot-auto-update, @woodsts, @snickl.

If you need to update your Crypto++ clone, then issue git pull. If you need to reset your Crypto++ clone to upstream/master, then see reset-fork.sh provided in Crypto++ Master.

The change to accommodate C++17 and std::byte will be available in Master and eventually Crypto++ 6.0. The change requires a major version bump.


For documentation purposes, see the following on the Crypto++ User mailing list:

@noloader
Copy link
Collaborator Author

noloader commented Jul 17, 2017

Here are the results from Pycryptopp. It appears Pycryptopp did not import the entire std:: or CryptoPP:: namespace, so they avoided the collisions that would have occurred in C++17. The project was in very good shape, and it only needed a few surgical fixes.

Pycryptopp needed the following due to the upcoming Crypto++ changes. The changes made to Pycryptopp are compatible with the old version (::byte) and new version (CryptoPP::byte) of the Crypto++ library.

$ cat pycryptopp.diff
diff --git a/src/pycryptopp/cipher/aesmodule.cpp b/src/pycryptopp/cipher/aesmodule.cpp
index 360827d..3d0b95a 100644
--- a/src/pycryptopp/cipher/aesmodule.cpp
+++ b/src/pycryptopp/cipher/aesmodule.cpp
@@ -20,6 +20,9 @@ typedef int Py_ssize_t;
 #include <src-cryptopp/aes.h>
 #endif

+// https://github.com/weidai11/cryptopp/issues/442
+typedef unsigned char byte;
+
 static const char*const aes___doc__ = "_aes counter mode cipher\n\
 You are advised to run aes.start_up_self_test() after importing this module.";

diff --git a/src/pycryptopp/cipher/xsalsa20module.cpp b/src/pycryptopp/cipher/xsalsa20module.cpp
index ab29787..a0b7c71 100644
--- a/src/pycryptopp/cipher/xsalsa20module.cpp
+++ b/src/pycryptopp/cipher/xsalsa20module.cpp
@@ -16,6 +16,9 @@ typedef int Py_ssize_t;
 #include <src-cryptopp/salsa.h>
 #endif

+// https://github.com/weidai11/cryptopp/issues/442
+typedef unsigned char byte;
+
 static const char* const xsalsa20__doc__ = "_xsalsa20 cipher";

 static PyObject *xsalsa20_error;
diff --git a/src/pycryptopp/hash/sha256module.cpp b/src/pycryptopp/hash/sha256module.cpp
index bf9d8e3..8272e6c 100644
--- a/src/pycryptopp/hash/sha256module.cpp
+++ b/src/pycryptopp/hash/sha256module.cpp
@@ -21,6 +21,9 @@ typedef int Py_ssize_t;
 #include <src-cryptopp/filters.h>
 #endif

+// https://github.com/weidai11/cryptopp/issues/442
+typedef unsigned char byte;
+
 static const char*const sha256___doc__ = "_sha256 hash function";

 static PyObject *sha256_error;

@anonimal
Copy link
Contributor

Hi @noloader. I've been watching this byte issue unfold and we remain unaffected. Thank you for the ping 😄

@noloader
Copy link
Collaborator Author

The change was checked in at Commit 00f9818b5d8e5aeb.

noloader referenced this issue Jul 23, 2017
…(Issue 447)

This seems to be a little cleaner than the triage at 00e1337 commit.
aryla added a commit to ultravideo/kvazaar that referenced this issue Feb 5, 2018
Changes the crypto module to use unsigned char instead of byte. The byte
typedef is no longer included in the global namespace in crypto++ 6.0.0.
See weidai11/cryptopp#442.

Fixes #184.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants