[native] Disable CryptoModule copy construction
ClosedPublic
Actions

Authored by ashoat on Oct 22 2023, 9:07 AM.

Details

Reviewers

marcin
tomek
bartek
atul

Commits

rCOMMd3271a251de4: [native] Disable CryptoModule copy construction

Summary

See explanation here for why it's unsafe to copy-construct CryptoModule. Basically, the accountBuffer contains memory that represents an olm::Account, which includes members that are olm::Lists. olm::Lists contains a member that is a pointer to another member, which makes it unsafe to copy the accountBuffer wholesale.

Depends on D9561

Test Plan

I confirmed that after I saw compilation errors if I reverted the preceding diff (D9561)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.Oct 22 2023, 9:07 AM

Herald added a subscriber: will. · View Herald TranscriptOct 22 2023, 9:07 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 22 2023, 9:07 AM

Harbormaster failed remote builds in B23417: Diff 32298!

Rebase

Harbormaster completed remote builds in B23421: Diff 32302.Oct 22 2023, 9:34 AM

ashoat requested review of this revision.Oct 22 2023, 9:34 AM

After reading this doc: https://en.cppreference.com/w/cpp/language/move_constructor
I am wondering if CryptoModule class has trivial move constructor that works like a default copy constructor. If that is the case we should consider deleting move constructor as well to entirely disable copying.

This revision is now accepted and ready to land.Oct 23 2023, 2:35 AM

I'm not sure if move construction is unsafe. I would actually assume it is safe. The question is whether std::vector (what we use for OlmBuffer) has a move constructor that allocates new space on the heap for the vector's data. I would assume it does not – the point of a move constructor is generally to avoid having to do that.

Closed by commit rCOMMd3271a251de4: [native] Disable CryptoModule copy construction (authored by ashoat). · Explain WhyOct 23 2023, 5:13 AM

This revision was automatically updated to reflect the committed changes.

ashoat added a commit: rCOMMd3271a251de4: [native] Disable CryptoModule copy construction.