Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<deque>: Use _Next_iter and _Prev_iter #1161

Merged
merged 2 commits into from
Aug 12, 2020

Conversation

CaseyCarter
Copy link
Member

@CaseyCarter CaseyCarter commented Aug 7, 2020

Fixes DevCom-1134328 / AB#1166791.
@CaseyCarter CaseyCarter added the bug Something isn't working label Aug 7, 2020
@CaseyCarter CaseyCarter requested a review from a team as a code owner August 7, 2020 17:26
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have any sympathy for this in the same way we didn't have sympathy for that user who reported a 'vector does ADL things in its dtor requiring more things to be complete than are otherwise needed' before.

But the mitigation is cheap so why not.

@AlexGuteniev AlexGuteniev mentioned this pull request Aug 9, 2020
58 tasks
@StephanTLavavej
Copy link
Member

I wonder if we should have a more comprehensive test (like our Evil class with overloaded operator, and operator&), given that this code seems to be conformant and there could be issues elsewhere in the STL especially during future refactoring, but certainly this is a strict improvement over the status quo. Perhaps we should file a test issue to further enhance coverage.

@BillyONeal
Copy link
Member

@StephanTLavavej The difference is that unlike the &/, situation, there's nothing we can really do to guard against this kind of ADL-injected-ill-formed problem in the general case. For example, allowing this in general would prohibit calling any standard library algorithms from any of the containers, because the algorithms have expressions like iter+42 inside which are vulnerable in the same way this is vulnerable. Even if the spec doesn't require this right now I think we should fix the spec to prohibit and behave as if the spec does prohibit such ADL hijacking of operators when neither of the operands are a program-defined type (here, _Deque_iterator and int).

@CaseyCarter
Copy link
Member Author

Formally iter+42 is allowed only if iter is a random-access iterator whose difference type is int. We guard against this in the general case by only adding or subtracting values of an iterator's difference type (ignoring cv-qualifiers) to an iterator. We've been very careful about the types of variables that we add and subtract from iterators, but not so much with literals.

@BillyONeal
Copy link
Member

Formally iter+42 is allowed only if iter is a random-access iterator whose difference type is int.

Would that have protected against this particular case?

@CaseyCarter
Copy link
Member Author

Formally iter+42 is allowed only if iter is a random-access iterator whose difference type is int.

Would that have protected against this particular case?

Yes. The syndrome here is that we have two functions:

struct S {
  template <class T> S(T&&); // Everything converts to S
};

void f(S, int);              // Unintentional ADL-hijacking function
void f(iterator, ptrdiff_t); // Actual function we want to call

and we're evaluating f(iterator{}, 1) with an iterator whose difference type is ptrdiff_t. This works as we intend in 32-bit compiles - where ptrdiff_t is int - but not in 64-bit compiles when it's long long. Our f has a better ICS (implicit conversion sequence) for the first argument - exact match - but the user f has a better ICS for the second argument - again exact match. Overload resolution is ambiguous. f(iterator{}, ptrdiff_t{1}) would unambiguously call the correct function.

stl/inc/deque Outdated Show resolved Hide resolved
stl/inc/deque Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Aug 12, 2020
@StephanTLavavej StephanTLavavej merged commit ae1068e into microsoft:master Aug 12, 2020
@CaseyCarter CaseyCarter deleted the deque branch August 12, 2020 19:52
@StephanTLavavej
Copy link
Member

Thanks for investigating and fixing this surprising bug! I never would have thought that an STL container couldn't say iter + 1 for its own iterators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants