search

timescale / timescaledb-toolkit

Extension for more hyperfunctions, fully compatible with TimescaleDB and PostgreSQL 📈
https://www.timescale.com
Other
372 stars 46 forks source link

Likely undefined behavior in PanickingAllocator #294

Open jgallagher opened 2 years ago

jgallagher commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

Thank you for pointing this out!

jgallagher commented 2 years ago

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., https://github.com/vcombey/fallible_collections/issues/22)

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

JLockerman commented 2 years ago

The oom=panic PR was recently merged.

jfjoly commented 2 years ago

Waiting on upstream changes in rust

JLockerman commented 2 years ago

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.