Fix assignment operator of optional_t for non-trivial types#5633
Fix assignment operator of optional_t for non-trivial types#5633asimonov1 wants to merge 1 commit into
Conversation
|
@uxlfoundation/onednn-arch As std::optional is a native part of C++17, maintaining compatibility layers like this seems low value. Can we consider updating the minimum supported C++ version? Given that it C++17 was approved nearly 9 years ago, this doesn't seems like it would be a particularly stringent change. |
Sure, we can consider it. It would require an RFC to document and discuss all the implications and a deprecation period. If there's an impact on ABI compatibility of existing packages it would require a major version change as well. |
b0e8f1e to
fd05208
Compare
Copy/move assignment ran T::operator= on storage holding no live object, which is UB for non-trivial T; use placement-new instead. Also add the missing return in operator=(nullopt_t) and set has_value_ only after a successful construct (basic exception guarantee).
fd05208 to
3133955
Compare
|
make test |
|
Results for the previous "make test": https://ecmd.jf.intel.com/commander/link/jobDetails/jobs/f184d458-da23-f126-b256-d4f5ef20c6a0 |
|
make test |
Summary
optional_tinsrc/common/optional.hppis a fallbackoptionalimplementation for builds with C++ < 17 (it aliases
std::optionalwhen C++17is available). Its assignment operator had several defects — undefined behavior
for non-trivial
T, a missing return, and no exception guarantee — detailedbelow.
Problem
value_.~T()and thenvalue_ = other.value_(i.e.T::operator=). Afterthe explicit destructor — or when
*thiswas empty andvalue_was neverconstructed —
value_is not a live object, so runningT::operator=on itis UB. (The constructors already do the right thing via placement-new.)
operator=(nullopt_t)never returned. It fell off the end of avalue-returning function, which is UB.
has_value_setto
truebefore the new value was constructed. IfT's copy/moveconstructor threw, the object was left claiming to hold a value that was never
constructed — no longer destructible (the destructor would run
value_.~T()on non-constructed storage).
For a trivial/POD
Tthese are benign, which is why the bug went unnoticed. Fora resource-owning
T(e.g.dnnl::memory::desc, which holds astd::shared_ptr) it corrupts memory / crashes.The bug is currently latent: no call site assigns onto a non-trivial
optional_t. The riskiest instantiation isoptional_t<memory::desc>in thegraph DNNL backend (
src/graph/backend/dnnl/layout_id_mgr.cpp), which isconstruct-and-read only today. This fix removes the landmine before an
assignment is ever added there.
Fix
value_instead ofrunning
T::operator=on non-living storage.operator=(nullopt_t)now returns*this.has_value_is set totrueonly after the placement-new succeeds, so on athrowing
Tcopy/move constructor the optional is left validly empty. Thisgives the basic exception guarantee (not strong — the previous value is
not preserved).
Open questions
A couple of
std::optionaldivergences I noticed but kept out of scope — bothinvolve currently-unused API:
value_oris unused and takesT&&, so it rejects lvalue arguments (unlikestd::optional::value_or, which takesconst T&). Leave it, fix thesignature, or remove it?
operator boolis unused and non-explicit, unlikestd::optional's. Anon-explicit conversion allows surprising implicit
boolconversions. Make itexplicit, remove it, or leave it?