fix: implement LWG-4015 to make `optional` conversions ADL-proof by daksh-goyal · Pull Request #6102 · microsoft/STL · GitHub
Skip to content

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

Merged
StephanTLavavej merged 12 commits into
microsoft:mainfrom
daksh-goyal:adl-proof-optional-conversion
Mar 10, 2026
Merged

fix: implement LWG-4015 to make optional conversions ADL-proof#6102
StephanTLavavej merged 12 commits into
microsoft:mainfrom
daksh-goyal:adl-proof-optional-conversion

Conversation

@daksh-goyal

Copy link
Copy Markdown
Contributor

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

Use _Right._Value instead of *_Right in converting constructors and assignments to avoid ADL-found operator* in the namespace of the source type. Also use _Right._Has_value instead of if(_Right).
@daksh-goyal daksh-goyal requested a review from a team as a code owner February 22, 2026 21:55
@github-project-automation github-project-automation Bot moved this to Initial Review in STL Code Reviews Feb 22, 2026
@StephanTLavavej StephanTLavavej added the LWG Library Working Group issue label Feb 23, 2026
@StephanTLavavej StephanTLavavej changed the title fix: implement LWG-4015 to make optional conversions ADL-proof fix: implement LWG-4015 to make optional conversions ADL-proof Feb 26, 2026
Comment thread stl/inc/optional
@StephanTLavavej StephanTLavavej self-assigned this Mar 3, 2026
@StephanTLavavej StephanTLavavej moved this from Initial Review to Work In Progress in STL Code Reviews Mar 3, 2026
@StephanTLavavej

Copy link
Copy Markdown
Member

@StephanTLavavej

Copy link
Copy Markdown
Member

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.

@StephanTLavavej StephanTLavavej removed their assignment Mar 4, 2026
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Ready To Merge in STL Code Reviews Mar 4, 2026
@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Merging in STL Code Reviews Mar 9, 2026
@StephanTLavavej

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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.

@StephanTLavavej StephanTLavavej merged commit 51277b4 into microsoft:main Mar 10, 2026
49 checks passed
@github-project-automation github-project-automation Bot moved this from Merging to Done in STL Code Reviews Mar 10, 2026
@StephanTLavavej

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LWG Library Working Group issue

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

LWG-4015 LWG-3973 broke const overloads of std::optional monadic operations

2 participants