microsoft / wil

Windows Implementation Library
MIT License
2.57k stars 234 forks source link

Add the notion of "checked iterators" to WIL #422

Closed dunhor closed 8 months ago

dunhor commented 8 months ago

Background: a couple months or so ago someone reported a number of "gotchas" when using the wil::vector_iterator_nothrow type, most of which stem from the fact that the iterator shares state with its "range" object. This means that some operations such as maintaining multiple copies of an iterator (or, similarly, calling begin() multiple times) range from risky to guaranteed unexpected behavior. For example, if you increment one iterator and then dereference another, there's a good chance that the result of the dereference is not what you are expecting (since the contents will be for the iterator that you incremented).

This adds the notion of "checked" or "debug" iterators to WIL, which closely follows MSVC's debug iterator support. Similar to MSVC's _ITERATOR_DEBUG_LEVEL define, this introduces the WIL_ITERATOR_DEBUG_LEVEL define which can take on the values 0, 1, or 2 - similar to MSVC's counterpart:

For a more in-depth example, consider the scenario that was originally reported - trying to use vector_range_nothrow with std::lower_bound. There are a couple issues here (e.g. std::distance is both slow and destructive to the shared state), but let's focus on the more obvious issue - there are lots of operations on copies of iterators. Consider the vector contents [0, 1, 2]. If we're looking for the value 2 things will probably work out okay since we advance to index 1 for the first mid-point check, see that the value is less than 2, advance one more, see that the value is not less than 2, and then the search is done. If we're instead looking for the value 1 or 2, then things will probably blow up in one way or another. In both cases, we'll see that 1 is not less than either number and will instead look at the earlier elements. This involves advancing the begin iterator by zero elements - something that will be a no-op - meaning that the algorithm will end up seeing the value 1 when dereferencing the iterator instead of 0. This will end up returning the wrong iterator that when dereferenced returns the correct value when looking for 1, and the correct iterator that when dereferenced returns the incorrect value when looking for 0. With WIL_ITERATOR_DEBUG_LEVEL set to 1, the code will not fail the assertion when looking for 2, but will fail the assertion when looking for 0 or 1. With WIL_ITERATOR_DEBUG_LEVEL set to 2, the code will always fail the assertion.

For now, I've only touched the nothrow iterators in winrt.h as these have shared state and it's comparatively easy to find yourself in a position where an operation is unsafe. Skimming around, there are some other iterators that would benefit from having similar checking (looking at you com_iterator), however no simple mechanism exists for adding these checks without also adding a breaking change (e.g. adding a new requirement that com_iterator not outlive the result from make_range)