Likely undefined behavior in PanickingAllocator

[jgallagher]

Apologies for the out-of-the-blue random bug report, but I've been interested in Rust OOM behavior for a while and ran across #285. I believe this introduces undefined behavior per the docs of GlobalAlloc:

The GlobalAlloc trait is an unsafe trait for a number of reasons, and implementors must ensure that they adhere to these contracts:

• It’s undefined behavior if global allocators unwind. This restriction may be lifted in the future, but currently a panic from any of these functions may lead to memory unsafety.
  ...

The last time I experimented with panicking allocators, the effect was that many (but not all!) Drop implementations did not run during the stack unwind. There is also a more general and pernicious issue that existing unsafe code (including potentially in the Rust std lib) is not exception safe across allocation attempts; even if relevant drops are run, such code may introduce memory unsafety if unwound.

[JLockerman]

@jgallagher We really appreciate any comments! Especially for tricky code like this 😊

I had (incorrectly) thought that the nounwind marker on the allocator code had been removed in anticipation of oom=panic but this is not the case. Looks like we'll have to revert, and wait until the real version is more baked.

[JLockerman]

Thank you for pointing this out!

[jgallagher]

Sure thing; thanks for the gracious followup! :) FWIW in my case I ended up using fallible-collections and changing every call site that could allocate to use checked versions of methods, but it's not something I can really recommend:

a) AFAIK there's no good way to make sure you caught all allocation points
b) you still have to do something if allocation fails; obvious options are to move the errors into the API or to panic (except panicking itself allocates memory ☹️) - neither of these seem great
c) there are some gotchas with fallible-colllections's implementation of the RFC (e.g., vcombey/fallible_collections#22)

All in all a pretty unsatisfying solution. I'm anxiously awaiting oom=panic, personally 🤷‍♂️

[JLockerman]

The oom=panic PR was recently merged.

[jfjoly]

Waiting on upstream changes in rust

[JLockerman]

The unstable flag was merged as mentioned above, it's released unstably as of 1.6.1, but is not yet stable (tracking issue) we can either wait until it hits stable, or switch to a nightly version.