fix: implement LWG-4015 to make optional conversions ADL-proof

[Daksh Goyal (daksh-goyal)]

LWG-4015 identified that LWG-3973's changes introduced ADL vulnerability in optional's converting constructors and assignment operators. The expressions *_Right and _STD move(*_Right) on optional<_Ty2> can be hijacked by ADL-found operator* overloads in the namespace of _Ty2.

Changed to directly access _Right._Value and _Right._Has_value (via friend access), avoiding any operator* call and thus any ADL.

Added a compile-only test using a deleted non-member operator* as an ADL trap to verify the fix.

fixes: #5871

[Stephan T. Lavavej (StephanTLavavej)]

Thanks for looking into this. I identified that a bunch of changes were missing, and that we really should be saying .operator*() as the Standard now depicts to be safe (see #2460 where direct use of ._Value is not acceptable), so I pushed major changes.

I will revise the test, but I need to drop everything and come back to this later. No action required.

[Stephan T. Lavavej (StephanTLavavej)]

I pushed additional test coverage for all of the product code changes, and to make the test actually effective. The templated operator*() was not sufficiently poisonous, as it wasn't causing compiler errors even with the original product code. (I believe this is because templates are less preferred during overload resolution when they are equally as good as non-templates.) We need non-templated poisonous overloads in order to actually exercise this fix. (I realized this after chasing the rabbit down the hole to LWG-3969.)

Finally, I observed that even with the maximally poisonous operator*() overloads, the member functions (such as the converting constructors/assigns and swap()) aren't actually vulnerable to ADL hijacking. Only the non-member comparisons and hash can be hijacked. This is because member lookup is stronger than ADL, see WG21-N5032 [basic.lookup.argdep]/1:

When the postfix-expression in a function call is an unqualified-id, and unqualified lookup for the name in the unqualified-id does not find any declaration of a class member, or [...] then lookup for the name also includes the result of argument-dependent lookup [...]

So we'd actually be fine saying *opt in member functions. However, because the Standard depicts .operator*() (and Do What The Standard Says is one of our cherished principles) and because it's needed in the non-members, being consistent is just simpler and worth the extra verbosity.

[Stephan T. Lavavej (StephanTLavavej)]

I'm mirroring this to the MSVC-internal repo. Please notify me if any further changes are pushed, otherwise no action is required.

[Daksh Goyal (daksh-goyal)]

Didn't realize the union member is remove_cv_t<_Ty>, and so it strips const. That would've been a regression.

I would've probably got stuck at WG21-N5032 [over.match.oper] and it would have taken me forever to realize that the explicit member function call syntax bypasses ADL.

And thanks a lot for pointing to LWG-3969, I don't how I'd have gotten there.

[Stephan T. Lavavej (StephanTLavavej)]

🎉 🐈 🐱