Add checks for invalid iterator when _ITERATOR_DEBUG_LEVEL is non-zero by rogerorr · Pull Request #3282 · microsoft/STL

rogerorr · 2022-12-10T22:46:08Z

fsb4000 · 2022-12-11T04:03:54Z

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 · 2022-12-11T09:24:18Z

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 · 2022-12-11T11:15:32Z

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

fsb4000 · 2022-12-11T11:19:08Z

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

rogerorr · 2022-12-11T14:11:08Z

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 · 2022-12-12T18:44:48Z

stl/inc/vector

 #if _ITERATOR_DEBUG_LEVEL != 0
        const auto _Mycont = static_cast<const _Myvec*>(this->_Getcont());
        _STL_VERIFY(_Ptr, "can't dereference value-initialized vector iterator");
+        _STL_VERIFY(_Mycont, "can't dereference invalidated vector iterator");


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

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.

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 · 2022-12-13T01:04:33Z

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 · 2022-12-13T10:41:48Z

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 · 2022-12-15T06:29:20Z

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

StephanTLavavej · 2022-12-15T23:13:07Z

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 · 2023-08-18T11:20:51Z

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

spectre-ns · 2023-11-09T12:09:44Z

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.

Add checks for invalid iterator when _ITERATOR_DEBUG_LEVEL is non-zero

cab95f5

rogerorr requested a review from a team as a code owner December 10, 2022 22:46

rogerorr mentioned this pull request Dec 10, 2022

<vector>: iterator debugging misses a check before dereferencing #3281

Closed

Add checks for invalid iterator when _ITERATOR_DEBUG_LEVEL is non-zero

8bfdbd8

strega-nil-ms reviewed Dec 12, 2022

View reviewed changes

strega-nil-ms approved these changes Dec 12, 2022

View reviewed changes

strega-nil-ms added the bug Something isn't working label Dec 12, 2022

CaseyCarter approved these changes Dec 12, 2022

View reviewed changes

StephanTLavavej approved these changes Dec 13, 2022

View reviewed changes

StephanTLavavej added enhancement Something can be improved and removed bug Something isn't working labels Dec 14, 2022

StephanTLavavej self-assigned this Dec 15, 2022

StephanTLavavej merged commit 435250b into microsoft:main Dec 15, 2022

spectre-ns mentioned this pull request Nov 8, 2023

[CI] Multiple failures in CI on Clang 16.0.6 MSVC xtensor-stack/xtensor#2743

Closed

Conversation

rogerorr commented Dec 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fsb4000 commented Dec 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rogerorr commented Dec 11, 2022

Uh oh!

fsb4000 commented Dec 11, 2022

Uh oh!

fsb4000 commented Dec 11, 2022

Uh oh!

rogerorr commented Dec 11, 2022

Uh oh!

strega-nil-ms Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

rogerorr Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogerorr Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

strega-nil-ms left a comment

Choose a reason for hiding this comment

Uh oh!

CaseyCarter left a comment

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej commented Dec 13, 2022

Uh oh!

rogerorr commented Dec 13, 2022

Uh oh!

StephanTLavavej commented Dec 15, 2022

Uh oh!

StephanTLavavej commented Dec 15, 2022

Uh oh!

pbdot commented Aug 18, 2023

Uh oh!

spectre-ns commented Nov 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Comments

rogerorr commented Dec 10, 2022 •

edited

Loading

fsb4000 commented Dec 11, 2022 •

edited

Loading

rogerorr Dec 12, 2022 •

edited

Loading