Cleanups relating to CurveGFp #4436
Conversation
randombit
force-pushed
the
jack/cleanup-curvegfp
branch
3 times, most recently
from
54629dd to
3de01c7
Compare
| /** | ||
| * Note this constructor should *only* be called by EC_Group_Data::create. | ||
| * | ||
| * It is only public to allow use of std::make_shared | ||
| */ | ||
| EC_Group_Data(const BigInt& p, | ||
| const BigInt& a, | ||
| const BigInt& b, | ||
| const BigInt& g_x, | ||
| const BigInt& g_y, | ||
| const BigInt& order, | ||
| const BigInt& cofactor, | ||
| const OID& oid, | ||
| EC_Group_Source source); |
There was a problem hiding this comment.
This bugs me so often... Typically, I default to using std::shared_ptr(new SomeType(...)) for such cases. IIRC, EC_Group_Data is internal, so no hard opinion. Otherwise, I'd say that hiding the constructor should probably take precedence over the avoidance of new.
There was a problem hiding this comment.
That got me thinking: CppCoreGuidelines has this covered, they suggest making the constructor public and letting it take a private Token type that cannot be created/obtained by anyone outside the class. Quite smart, but also a little sneaky, I find:
class Foo {
private:
class Token{};
public:
Foo(Token) {};
static std::shared_ptr<Foo> create() {
return std::make_shared<Foo>(Token{});
}
};There was a problem hiding this comment.
I would tend to agree on using new rather than exposing a constructor we don't want called, but in this case the type is entirely internal, so a comment seemed sufficient.
| * This EC_Group_Data is not owned because instead the EC_Group_Data | ||
| * owns this CurveGFp, so we can always access it safely. If it was | ||
| * a shared_ptr this would cause a reference cycle. |
There was a problem hiding this comment.
It would work but it would complicate things because we'd have to pay the weak->shared upgrade cost each time the group is used (eg for each point addition/doubling!). Not a huge fan of raw pointers, for the obvious reasons, but here the potential for misuse is relatively low; the only way a CurveGFp can be constructed at all is via a private/friend only constructor, it is only constructed in one place, and that code is careful to maintain the liveness invariant.
If this was something we were stuck with long term I'd probably look for a cleaner solution but this seems to me good enough to work until such time as the whole class can just be deleted.
This removes CurveGFp_Repr and its subclasses. Now the Montgomery logic previously implemented there is delegated to the already existing Montgomery_Params. This also guts CurveGFp; at this point it could easily be removed but unfortunately this type is used in a couple of interfaces which, while already deprecated, are still part of the public API. So complete removal will have to wait for Botan4.
randombit
force-pushed
the
jack/cleanup-curvegfp
branch
from
3de01c7 to
ff5656e
Compare
This removes CurveGFp_Repr and its subclasses. Now the Montgomery logic previously implemented there is delegated to the already existing Montgomery_Params.
This also guts CurveGFp; at this point it could easily be removed but unfortunately this type is used in a couple of interfaces which, while already deprecated, are still part of the public API. So complete removal will have to wait for Botan4.
#4027