[llvm-bugs] [Bug 50714] New: libc++ permits conversions between std::vector<>::iterators of distinct types

[via llvm-bugs]

https://bugs.llvm.org/show_bug.cgi?id=50714

            Bug ID: 50714
           Summary: libc++ permits conversions between
                    std::vector<>::iterators of distinct types
           Product: libc++
           Version: 12.0
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P
         Component: All Bugs
          Assignee: unassignedclangbugs at nondot.org
          Reporter: mte.zych at gmail.com
                CC: llvm-bugs at lists.llvm.org, mclow.lists at gmail.com

Hello!

Recently I've noticed that, libc++ permits conversions
between std::vector<>::iterators of two different element_types,
as long as pointers to these element_types are convertible:

    struct foo       { float f; };
    struct bar : foo { int   i; };

    auto vec = std::vector
    {
        bar { foo { 1.1f }, 7 },
        bar { foo { 0.8f }, 5 },  
    };

    typename std::vector<foo>::iterator it = vec.begin();
    //                   ^
    // note: The value_type of the iterator 'it' is foo, not bar!

    ++it;

    auto fp_value = it->f; // bug: iterator 'it' is not valid!

This is very dangerous, because the iterator 'it' expects
to iterate over a continuous block of memory, containing only foo objects:

    +-----+-----+-----
    | foo | foo | foo ...
    +-----+-----+-----
    ^     ^     ^
    it    it+1  it+2  ...

However, this is not what is actually happening in memory:

    +-----------+-----------+-------
    |    bar    |    bar    |    bar ...
    +-----------+-----------+-------
    | foo |     | foo |     | foo |  ...
    +-----------+-----------+-------
    ^     ^     ^
    it    it+1  it+2                 ...

This contradiction makes std::next(it) is not dereferenceable
and will result in undefined behavior (UB),
when an object will be accessed though the iterator 'it'.

Compiler Explorer link: https://godbolt.org/z/5zzb7dTPd


Correct me if I'm wrong,
but ideally this bug should be reported at compile-time by a C++ compiler.

BTW, this is what other C++ standard library implementations are already doing:
 - [GCC] libstdc++ -> https://godbolt.org/z/bqsfW4aMb
 - [MSVC] STL      -> https://godbolt.org/z/4TnK45jcM

Also, the ISO C++ standard only requires that std::vector<>::iterator
must be convertible to std::vector<>::const_iterator,
so other conversions could be disabled, right?

    Table 73: Container requirements [tab:container.req]
    +-------------+---------------+------------------------------------+
    |  Expression |  Return type  |                 Note               |
    +-------------+---------------+------------------------------------+
    |             | iterator type | any iterator category that meets   |
    | X::iterator | whose value   | the forward iterator requirements. |
    |             | type is T     | convertible to X::const_iterator   |
    +-------------+---------------+------------------------------------+

ISO C++20 Working Draft:
 ~ http://open-std.org/jtc1/sc22/wg21/docs/papers/2020/n4861.pdf


Why this conversion is even allowed by libc++?

>From what I've investigated, in the libc++ implementation,
the std::vector<>::iterator is an alias to the __wrap_iter<> class template:

    template <class _Tp, class _Allocator /* = allocator<_Tp> */>
    class _LIBCPP_TEMPLATE_VIS vector
    {
    ...
        typedef __wrap_iter<pointer>                     iterator;
        typedef __wrap_iter<const_pointer>               const_iterator;
    ...
    };

[GitHub] llvm-project/libcxx/include/vector:
 ~ https://github.com/llvm/llvm-project/blob/main/libcxx/include/vector#L488

Looking at the __wrap_iter<> class template revels the reason why
this dangerous conversion was even allowed by the Clang compiler:

    template <class _Iter>
    class __wrap_iter
    {
    ...
        template <class _Up> ...
        __wrap_iter(const __wrap_iter<_Up>& __u,
                    std::enable_if_t<
                    std::is_convertible_v<_Up, iterator_type>>* = nullptr)
        ... { ... }
    ...
    };

[GitHub] llvm-project/libcxx/include/iterator:
 ~ https://github.com/llvm/llvm-project/blob/main/libcxx/include/iterator#L1437

Essentially, as long as type_x* is convertible to type_y*,
std::vector<type_x>::iterator is convertible to std::vector<type_y>::iterator.


Every time I see such a weakly-constrained, non-explicit conversion
constructor,
red flags are immediately being raised in my brain, since
this could lead to quite surprising results and can potentially cause issues
...

Do you think it would be beneficial to further constrain
conversions of std::vector<>::iterator?

Would it make sense to allow only conversion
from std::vector<>::iterator to std::vector<>::const_iterator?

Thank you, Mateusz Zych

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20210615/d0b5680c/attachment.html>