Page MenuHomePhabricator

[native] Avoid pointer to other parts of class
ClosedPublic

Authored by ashoat on Oct 22 2023, 9:03 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 28, 3:47 PM
Unknown Object (File)
Sun, Oct 20, 9:58 PM
Unknown Object (File)
Sun, Oct 20, 11:14 AM
Unknown Object (File)
Sun, Oct 20, 10:25 AM
Unknown Object (File)
Sun, Oct 20, 6:11 AM
Unknown Object (File)
Sun, Oct 20, 6:10 AM
Unknown Object (File)
Sun, Oct 20, 5:59 AM
Unknown Object (File)
Sun, Oct 20, 5:44 AM
Subscribers

Details

Summary

I initially made this change hoping it would fully fix ENG-5354. Unfortunately it didn't, and the reasons are described here. That said, I still think it's a good idea to make this change, so I'm submitting it for review.

Olm library background

When calling [olm_account](https://github.com/CommE2E/olm/blob/6d0eaa993257225faf62768c898fe090fcc2dd2d/src/olm.cpp#L180) to construct an OlmAccount, you pass in the buffer (a pointer to memory) where the OlmAccount can be constructed. It's done this way because the Olm team does not want to handle memory management, and as such requires the caller to allocate the memory.

That means that the pointer returned by olm_account is exactly the same as the pointer passed to it. olm_account constructs the object at that memory location, and then returns you that same pointer cast as the specified type.

Our use

In our case, the accountBuffer is a member of CryptoModule. When we pass a pointer to accountBuffer to olm_account, we get that same buffer back, which we then assign to another member of CryptoModule, account.

The result is that if CryptoModule is constructed on the stack, account is a pointer to the stack.

This is "non-standard" behavior in C++. You generally don't want pointers to the stack. Crucially, in our case, this breaks assumptions that the default implicit copy constructor in C++ has.

The implicit copy constructor sees a pointer, assumes it points to the heap, and just copies it. This is bad for us, since we get a pointer pointing to space on the stack that quickly gets deallocated.

Solution

Instead of storing the pointer, we can just call accountBuffer->data() to get it.

Test Plan

I made sure notifs still worked in my local environment (iOS simulator running dev build, along with keyserver connected to APNs in dev mode)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 22 2023, 9:04 AM
Harbormaster failed remote builds in B23414: Diff 32295!
This revision is now accepted and ready to land.Oct 23 2023, 2:15 AM
native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
310 ↗(On Diff #32299)

For other reviewers: this line is still important since it zeros the memory held by accountBuffer.data().

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
310 ↗(On Diff #32299)

It also initializes the memory appropriately for each member of olm::Account. This calls new(buffer) olm:Account()