fix: Refactor executor

[adamspofford-dfinity]

The current executor is unsound in that it requires a pointer to be unique for safety, but shares it between the polled future and the context passed to it. Fixing this requires overhauling the executor to no longer manually manage memory lifetimes; Rc is used instead. This fixes the bug exhibited in https://github.com/hpeebles/ic-error .

[roelstorms]

Do you have any tests covering this code? For example tests that track the strong_counter to see it remains correct.

The bug is about having a double free which got resolved by using RC. Do we have tests to ensure we don't have memory leaks by calling increase_strong_count on the RC?

[roelstorms]

Why is CLEANUP an AtomicBool if we are working in a single threaded environment?

[roelstorms]

@robin-kunzler and I had a quick review. We noticed that the pull request significantly decreases the usage of unsafe and makes for much clearer code.

• For added clarify, would it be possible to add a reasoning for each unsafe code block. I believe it's because of the RawWaker API that requires raw pointers but maybe you could add this somewhere as comment. This is a requirement from the DFINITY rust coding guidelines.
• Have you investigated alternatives to this Waker which requires the use of raw pointers to avoid unsafe code in general?
• You added a single e2e test to verify this bug. We advise to add additional unit tests to more granularly test the behavior of this code given it contains unsafe blocks.
• It's advised to have unsafe code peer reviewed for example by @roman-kashitsyn.

As you know, unsafe code bears the risk of memory corruptions and is hard to review.

@adamspofford-dfinity tagging you since you are the author.

[adamspofford-dfinity]

Why is CLEANUP an AtomicBool if we are working in a single threaded environment?

Because it's shorter, at least until feature(local_key_cell_methods) stabilizes. There is no actual difference.

Have you investigated alternatives to this Waker which requires the use of raw pointers to avoid unsafe code in general?

That's Future's API. You have to implement RawWaker in order to construct Waker in order to construct Context in order to call Future::poll. There is no other way of implementing async/await.

[adamspofford-dfinity]

There's no way that I have found to automate a test of this kind. Only the executor knows when a value has been dropped. Manual testing however has proved to my satisfaction that all spawned tasks are dropped, including in multiple-ownership scenarios like join_all.

[adamspofford-dfinity]

Looking closer, there does appear to be a less fundamentally unsafe internal architecture, and I may need to fully redesign it to take it. Any reasonably implemented future should not cause a memory leak, but it's not resilient against unreasonably implemented ones.

[roelstorms]

Our remarks are mainly about justification and documentation (as per the DFINITY rust guidelines). However, since this pull request definitely improves the situation, I believe it should be put in production. If a complete redesign can further improve the situation, can we do that separately so that we at already mitigate the bug found by Hamish.

@robin-kunzler WDYT?

[adamspofford-dfinity]

If you say so. My concerns with the design have been mostly alleviated anyway - the lingering potential for memory leaks (a future holding onto Waker after returning Ready) is one already present in tokio, so any futures library already knows how to avoid it.

[roelstorms]

Since we only review the changes for security purposes we won't approve the entire PR. Please go through standard peer review for the required approval.

From a security perspective we saw that the unsafe code which was changed is an improvement and doesn't contain memory issues. The unsafe code which is part of this change is very simple as it only containers transformations from raw pointer to the original type and manipulating the strong count of an RC.