Allow the pubkey operations wrapper types to move

[randombit]

GH #3641

[randombit]

MSVC having problems here that I don't understand

[reneme]

For some reason MSVC seems to instantiate the destructor of m_op (holding a std::unique_ptr<> of the internal pubkey operation). This is illegal in pubkey.h, as we only have forward declarations of the pubkey operations. Defaulting the move c'tors and assignment operators in pubkey.cpp (similarly to the d'tors) fixes this (pubkey.cpp includes pk_ops.h).

I don't know why MSVC needs to call the destructor of m_op when implementing the default move c'tor/assignment. In particular, this seems to be needed for the shared library builds only.

@randombit: I replaced your attempted fix with the above-described workaround on this branch.

[reneme]

Yay! That'll make certain things easier for sure.

[reneme]

I don't know why MSVC needs to call the destructor of m_op when implementing the default move c'tor/assignment. In particular, this seems to be needed for the shared library builds only.

Thinking about this a bit more, it's actually fairly obvious: The d'tor of m_op is simply needed to implement the move c'tor/assignment. The error just didn't show up in any other build as nowhere any objects of the classes in pubkey.h are actually moved, yet. I guess the MSVC shared library build is the only one that actually attempted to generate code for those methods despite them being unused in the code base.

For instance, when applying the following patch to the initial PR (without the = default moved to pubkey.cpp), GCC produces a similar error. With my additional commit, it works as expected.

diff --git a/src/tests/test_kyber.cpp b/src/tests/test_kyber.cpp
index 2e85d156a..948efa158 100644
--- a/src/tests/test_kyber.cpp
+++ b/src/tests/test_kyber.cpp
@@ -29,6 +29,10 @@ namespace Botan_Tests {

 #if defined(BOTAN_HAS_KYBER) || defined(BOTAN_HAS_KYBER_90S)

+inline decltype(auto) encrypt_wrapper(Botan::PK_KEM_Encryptor enc) {
+   return enc.encrypt(Test::rng());
+}
+
 class KYBER_Tests final : public Test {
    public:
       static Test::Result run_kyber_test(const char* test_name, Botan::KyberMode mode, size_t strength) {
@@ -50,7 +54,7 @@ class KYBER_Tests final : public Test {
          // Bob (reading from serialized public key)
          Botan::Kyber_PublicKey alice_pub_key(pub_key_bits, mode);
          auto enc = Botan::PK_KEM_Encryptor(alice_pub_key, "Raw", "base");
-         const auto kem_result = enc.encrypt(Test::rng());
+         const auto kem_result = encrypt_wrapper(std::move(enc));

          // Alice (reading from serialized private key)
          Botan::Kyber_PrivateKey alice_priv_key(priv_key_bits, mode);

/usr/include/c++/11/bits/unique_ptr.h: In instantiation of ‘void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = Botan::PK_Ops::KEM_Encryption]’:
/usr/include/c++/11/bits/unique_ptr.h:361:17:   required from ‘std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = Botan::PK_Ops::KEM_Encryption; _Dp = std::default_delete<Botan::PK_Ops::KEM_Encryption>]’
build/include/botan/pubkey.h:616:7:   required from here
/usr/include/c++/11/bits/unique_ptr.h:83:23: error: invalid application of ‘sizeof’ to incomplete type ‘Botan::PK_Ops::KEM_Encryption’
   83 |         static_assert(sizeof(_Tp)>0,
      |                       ^~~~~~~~~~~
make: *** [Makefile:1636: build/obj/test/test_kyber.o] Error 1

[randombit]

Have to be honest my intuition on how move works does not explain why the destructor would have to be visible but ¯_(ツ)_/¯

[randombit]

Hm I guess the issue is that in C++ move isn't really the right word to describe what is happening. You start with one object, after the move you have a new object, along with the original moved-from object which is in a potentially invalid state. So you still need to destroy the original.

Versus Rust where a move really is just a move.

[reneme]

You start with one object, after the move you have a new object, along with the original moved-from object which is in a potentially invalid state. So you still need to destroy the original.

That's also my intuition of what is happening.

Versus Rust where a move really is just a move.

Yeah, the situation you describe above is the price we pay for C++'s legacy. When do we start rewriting Botan in Rust? 😄

[randombit]

When do we start rewriting Botan in Rust?

LOL I would love to tbh; I sincerely do think "Rust implementation of X designed to provide a C API" is an underexplored territory.

That said I think people would be unhappy since Rust's story for weird architectures, baremetal, accessing CPU instructions, inline asm, etc, while certainly improving in the last few years, is still way behind C++.

I have on the todo list writing a new Rust ASN.1 library (the two major ones that currently exist are both pretty terrible, so this is a personal pain point for me) that would also expose it to C. Something like this would give a pretty good indicator if it was viable or not.