Page MenuHomePhabricator

Guard against possible overflow in span.subpan
AbandonedPublic

Authored by miscco on Oct 14 2019, 11:48 AM.

Details

Summary

There is a possible overflow in span.subspan as described here https://github.com/microsoft/STL/issues/159

Interestingly the other three overloads are already correct.

While we are there use element_type rather than _Tp

Diff Detail

Event Timeline

miscco created this revision.Oct 14 2019, 11:48 AM

As a side note we could also simplify the non templated subspan method

constexpr span<element_type, dynamic_extent>
_LIBCPP_INLINE_VISIBILITY
subspan(index_type __offset, index_type __count = dynamic_extent) const noexcept
{
    _LIBCPP_ASSERT(__offset <= size(), "Offset out of range in span::subspan(offset, count)");
    _LIBCPP_ASSERT(__count  <= size() || __count == dynamic_extent, "count out of range in span::subspan(offset, count)");
    if (__count == dynamic_extent)
        return {data() + __offset, size() - __offset};
    _LIBCPP_ASSERT(__offset <= size() - __count, "Offset + count out of range in span::subspan(offset, count)");
    return {data() + __offset, __count};
}

To the equivalent

constexpr span<element_type, dynamic_extent>
_LIBCPP_INLINE_VISIBILITY
subspan(index_type __offset, index_type __count = dynamic_extent) const noexcept
{
    _LIBCPP_ASSERT(__offset <= size(), "Offset out of range in span::subspan(offset, count)");
    _LIBCPP_ASSERT(__count == dynamic_extent || __offset <= size() - __count, "Offset + count out of range in span::subspan(offset, count)");
    return {data() + __offset, __count == dynamic_extent ? size() - __offset : __count };
}

If __count == dynamic_extent then the second assert is never tested. If __count != dynamic_extent then __count <= size() follows from conjunction of __offset <= size() and __offset <= size() - __count

However, I wasn't too sure whether it should go into the same revision

mclow.lists requested changes to this revision.Oct 20 2019, 8:15 PM

This looks fine to me, except that there's no test to show that it works.

This revision now requires changes to proceed.Oct 20 2019, 8:15 PM

As a side note we could also simplify the non templated subspan method

[snip]

I don't find that code to be simpler.
It's shorter, but harder to understand.

CaseyCarter requested changes to this revision.Oct 20 2019, 8:57 PM
CaseyCarter added inline comments.
include/span
449

Pre-existing bug: The return type should be span<element_type, _Count>.

452

_Offset <= size() - _Count should be _Count <= size() - _Offset per the P/R of LWG-3103. _Offset <= size() - _Count admits span("Hello").subspan<4, -size_t{2}>() and returns an enormous invalid span.

When implementing span for MS STL I locally used the libcx tests against my implementation, i found that there are some instances where libcxx is not fully conforming (due to rapidly shifting span specification). Should I prepare one big patch or go on incrementally.

[snip]

I don't find that code to be simpler.
It's shorter, but harder to understand.

What I find better about that code is that it is equivalent to the other subspan methods. I will definitely add a test against the error cases

When implementing span for MS STL I locally used the libcx tests against my implementation, i found that there are some instances where libcxx is not fully conforming (due to rapidly shifting span specification). Should I prepare one big patch or go on incrementally.

How many cases are we talking about?
In general, I would say incrementally.
Smaller patches are easier to review.

miscco updated this revision to Diff 226070.Oct 22 2019, 12:12 PM

Fixed preexisting bug and added necessary static_assert in another case

Also added (untested) Unit tests

miscco marked 2 inline comments as done.Oct 22 2019, 12:14 PM

I have to appologize, but my linux distribution needs to be reset. I havent had time for that so I only wrote some logical tests and will expand once I find time to setup linux again

CaseyCarter added inline comments.Oct 22 2019, 12:34 PM
test/std/containers/views/span.sub/subspan.fail.cpp
35

These template disambiguators should all be unnecessary since none of the object expressions have dependent type. In other words, span<int>().subspan<0, 0>() doesn't need a disambiguator since the compiler knows that span<int>() has type span<int> and it can see that subspan is a member template. This:

template<class T>
auto f() {
  return span<T>.template subspan<0, 0>();

on the other hand *does* need a disambiguator: span<T> has dependent type since T has dependent type. span<T> could be a specialization of span that the compiler hasn't even seen yet, so it can only guess what span<T>.subspan is.

CaseyCarter resigned from this revision.Oct 22 2019, 12:36 PM

(I'm not sure how to withdraw my "Request Changes" without approving - I'm not an authorized approver for libc++ - so I'll "Resign as Reviewer" and see if my red X goes away.)

(I'm not sure how to withdraw my "Request Changes" without approving - I'm not an authorized approver for libc++ - so I'll "Resign as Reviewer" and see if my red X goes away.)

Phabricator clears a request changes status when you upload a new version, so in this case you didn't need to do anything. In the general case though, I've found that the only way to clear an active request changes is to approve and then resign as reviewer. Resigning as reviewer clears your approval, but doesn't clear a request changes.

miscco abandoned this revision.Oct 28 2019, 12:10 PM

I messed with arcanist and created a new revision so I am going to close this one