Add checks for invalid iterator when _ITERATOR_DEBUG_LEVEL is non-zero

[rogerorr]

fixes #3281

[fsb4000]

I don't believe I have the permissions required to link these "properly"

You can link properly, just change your message to

Fixes #3821

Github docs: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

[rogerorr]

I don't believe I have the permissions required to link these "properly"

You can link properly, just change your message to

Fixes #3821

Github docs: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

Hmm, I've tried changing the message to both Fixes #3821 and fixes #3821 but still don't see a linking item in the right hand pane. What am I doing wrong?

[fsb4000]

Really strange. Look at my message: #3276 (comment)
I don't know why
Fixes #3281
doesn't work for you...

[fsb4000]

Oh, my bad.
I wrote a wrong number for you.
you need write 3281, not 3821.
Sorry.

[rogerorr]

Oh, my bad. I wrote a wrong number for you. you need write 3281, not 3821. Sorry.

Ach, and I didn't notice either. Now changed and working properly. Thank you!

[strega-nil-ms]

Doesn't this subsume the previous line? Can we just check for _STL_VERIFY(_Mycont and not check for _Ptr as well?

[rogerorr]

Well, we could but I think there is a useful distinction to retain between attempting to dereference a value initialized vector iterator and attempting to dereference an invalidated iterator.

[rogerorr]

Ah, and additionally I believe _Mycont is only cleared at _ITERATOR_DEBUG_LEVEL of 2, so the check would be lost at level 1.

[strega-nil-ms]

I don't know enough about iterator debugging checks to say, but this does seem extremely reasonable. I would like someone who's been on the project longer than me to take a look, though.

[CaseyCarter]

In the past we've simply allowed these cases to crash by dereferencing a null _Mycont, but reporting specific error messages is probably much more digestible for non-expert users. Thanks Roger!

[StephanTLavavej]

As @CaseyCarter noted, we previously removed a lot of our "we're about to dereference null" debugging checks because null dereferences are fairly easy to understand. I agree that it makes sense to add null checks here to help users, because these null dereferences are special - they would occur in our debugging machinery itself. I also checked, and in other containers we always test _Mycont before dereferencing it, so this is making vector consistent with them.

I noticed that there's one remaining case:

STL/stl/inc/vector

Lines 101 to 115 in 4483e87

	_CONSTEXPR20 void _Verify_offset(const difference_type _Off) const noexcept {
	#if _ITERATOR_DEBUG_LEVEL == 0
	(void) _Off;
	#else // ^^^ _ITERATOR_DEBUG_LEVEL == 0 / _ITERATOR_DEBUG_LEVEL != 0 vvv
	const auto _Mycont = static_cast<const _Myvec*>(this->_Getcont());
	_STL_VERIFY(_Off == 0 || _Ptr, "cannot seek value-initialized vector iterator");
	if (_Off < 0) {
	_STL_VERIFY(_Off >= _Mycont->_Myfirst - _Ptr, "cannot seek vector iterator before begin");
	}
	if (_Off > 0) {
	_STL_VERIFY(_Off <= _Mycont->_Mylast - _Ptr, "cannot seek vector iterator after end");
	}
	#endif // _ITERATOR_DEBUG_LEVEL == 0
	}

This is used by operator+= et al. and it'll dereference null _Mycont if it's called on an invalidated vector iterator that ever pointed to an element (i.e. it had a non-null _Ptr) and it's being asked to seek non-zero:

D:\GitHub\STL\out\x64>type meow.cpp

#include <cstdio>
#include <vector>
using namespace std;

int main() {
    vector<int>::iterator it;

    {
        vector<int> vec{11, 22, 33};
        it = vec.begin();
    }

    puts("Running it += 0;");
    it += 0;
    puts("... done.");

    puts("Running it += 1;");
    it += 1;
    puts("... done. (UH OH)");

    puts("Running it += -1;");
    it += -1;
    puts("... done. (ALSO UH OH)");
}

D:\GitHub\STL\out\x64>cl /EHsc /nologo /W4 /MTd meow.cpp && meow
meow.cpp
Running it += 0;
... done.
Running it += 1;
[***NULL DEREF***]

(Seeking an invalidated iterator by zero is technically bogus, but it's debatable whether we should complain here. Seeking by nonzero is super bogus.)

Ordinarily I would simply push changes to handle this case, but that might cause our Contributor License Agreement bot to ask you to agree to our CLA (it appears to have a de minimis threshold for tiny changes like a 5-line PR, but it'll ask for larger PRs). While our CLA is totally cool, on rare occasions it makes someone nervous, so I prefer to avoid pushing small PRs from new contributors over the limit if we can avoid it.

As this PR is a strict improvement, I think we can merge it as-is, and I've recorded a todo to update _Verify_offset later.

[rogerorr]

Ordinarily I would simply push changes to handle this case, but that might cause our Contributor License Agreement bot to ask you to agree to our CLA (it appears to have a de minimis threshold for tiny changes like a 5-line PR, but it'll ask for larger PRs). While our CLA is totally cool, on rare occasions it makes someone nervous, so I prefer to avoid pushing small PRs from new contributors over the limit if we can avoid it.

I'd be fine with signing it; Iwas expecting it to trigger when I raised the P/R and wondered whether it was suppressed by the prior NDA I'd signed with MS

As this PR is a strict improvement, I think we can merge it as-is, and I've recorded a todo to update _Verify_offset later.

Agree. Dereferencing is also I suspect more common than invoking operator+=

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[StephanTLavavej]

Thanks for improving these runtime diagnostics, and congratulations on your first microsoft/STL commit! 🎉 🚀 😸

This will be available in VS 2022 17.6 Preview 1.

[pbdot]

I don't have a minimal reproduction case yet, though this trips when using Clang in debug builds, where MSVC does not

[spectre-ns]

I don't have a minimal reproduction case yet, though this trips when using Clang in debug builds, where MSVC does not

@pbdot We have encountered this issue on clang 16 in the xtensor project as well.