span<T> constructors allow unsafe conversions

[bogiord]

std::is_convertible<typename Container::pointer, pointer> is the wrong test to constrain the constructor from a container. Example:

#include <vector>
#include <iostream>
#include "gsl.h"

struct B { int i = 7; };
struct D : B { int j = 0; };

int main()
{
   std::vector<D> v = {{}, {}, {}};
   gsl::span<B> s = v;
   std::cout << s[1].i << '\n'; // undefined behaviour, may print 0
}

The same problem is present if we construct the span from std::array<D, 3>, because the constructor from std::array<ArrayElementType, N>& is not constrained and then storage_type uses pointer to store the pointer to the first element, allowing derived-to-base pointer conversions. (By the way, the default template argument for ArrayElementType is pointless, it will always be deduced.)

Now, conversions between spans: is_allowed_element_type_conversion is way too permissive.

is_allowed_integral_conversion is the wrong test in this context. You can't look at an array of ints through a span<long>, even if int and long have the same internal representation; this is breaking aliasing rules and is undefined behaviour according to [basic.lval]/8 in the Standard (N4606, the current draft).

is_allowed_pointer_conversion is also wrong. Just because a pointer type is convertible to another it doesn't mean that an array of the former is convertible to an array of the latter.

And on top of all this, the reinterpret_cast<pointer>(other.data()) is the icing on the explosive cake. One example involving pointers:

#include <iostream>
#include "gsl.h"

struct B1 { int i = 7; };
struct B2 { int j = 3; };
struct D : B1, B2 { int k = 0; };

int main()
{
   D a[] = {{}, {}};
   D* ap[] = {&a[0], &a[1]};
   gsl::span<D*> sd = ap;
   gsl::span<B2*> sb = sd;
   std::cout << sb[0]->j << '\n'; // undefined behaviour, may print 7
}

On the other hand, std::is_same<From, std::remove_cv_t<To>> disallows conversions like volatile T to const volatile T, which are safe (it also disallows identical cv-qualified types, like const T to const T, but those cases are handled by the copy and move constructors, so don't use this test).

Really, the only conversions that should be allowed on span elements are qualification conversions as defined in [conv.qual] in the Standard. The previous version of the convertibility tests, which used pointers to arrays of unknown bound of the element types, were perfect for this purpose.

Also, I'm not sure why is_allowed_element_type_conversion allows conversions to byte. span's constructors don't seem to be able to handle such conversions properly (adjusting the extent); as far as I can tell, they're supposed to be done through as_bytes or as_writeable_bytes, which don't use conversions between spans internally.

Note that the current definition of gsl::byte doesn't allow viewing an array of T through a span<byte> according to the Standard. This would be allowed by the aliasing rules if byte were a typedef for char or unsigned char. Hopefully the proposal for std::byte will be accepted, but until then enum class byte is just asking for trouble in a library that aims to work safely across compilers. Example:

#include <iostream>
#include "gsl.h"

int f(int& i, gsl::byte& r)
{
   i = 7;
   r <<= 1;
   return i;
}

int main()
{
   int i;
   std::cout << f(i, reinterpret_cast<gsl::byte&>(i)) << '\n';
}

In MSVC it always prints 14. In Clang and GCC it prints 7 with optimizations enabled and 14 without optimizations - classic undefined behaviour; sample here.

The same code but with using byte = unsigned char; always prints 14 in all compilers; sample here.

[MikeGitb]

Just wanted to say that I also consider both points very serious bugs that need to be addressed as soon as possible (in particular before someone starts relying on that part of the gsl in production code).

[neilmacintosh]

@MikeGitb regarding the byte issue you have mentioned...you could offer a PR that changes the declaration of byte to be an alias for unsigned char for compilers that do not support the desired behavior yet. This would resolve the issue of the undefined behavior, correct? There would be some issues around specialization of templates that wanted to key off byte...but that's pretty much unavoidable on those platforms until the changes to the language standard finish making their way through the committee.

[MikeGitb]

@neilmacintosh: I believe so and that is exactly what I've done in the past. I was just in the process of migrating a smaller project from my own ArrayView / ByteType to the gsl::span and gsl::byte when I was made aware of this problem.

If you think that this is the right way to go I can offer a PR. The frustrating thing is that the enum version is no doubt the far superior one and even with gcc and clang it probably works 99.9% of the time (e.g. I didn't find a realistic example involving as_bytes where this was a problem). So I'm a bit reluctant to take this away - I'll at least allow the possibility to override the default.

Another option would probably be to pass the no-strict-aliasing flag that should prevent such "optimizations". However, I can't gauge if that is a) always sufficient and b) acceptable for other projects that want to use the gsl.

[neilmacintosh]

@MikeGitb I think the right fix here is probably to use a using byte = unsigned char for the compilers that are likely to enforce pre-C++17 strict aliasing behavior (or you could use a struct as @martinmoene has in gsl-lite). Given you can't know what compiler flags people want to use, that seems safest. I'd be happy to look at a PR.

[bogiord]

@neilmacintosh Any thoughts about the constructor-related issues?

[neilmacintosh]

@bogiord Yep, I started taking a look on Friday. I'll update the issue when I next get a chance.

[neilmacintosh]

@bogiord thanks for identifying the issues with conversions here. I have to say this thread has made me laugh a little, though...there's been talk of explosions! I usually think of my job in software as rather boring. I had no idea things could get so dramatic with some overly permissive SFINAE!

I've corrected the logic inside is_allowed_element_type_conversion and removed the conversions it allowed to byte...they were the leftovers of an experiment...apologies for the confusion there.

I've just committed some changes that mean this should now be fixed.

[bogiord]

Oh, no! You... laughed? I was hoping you'd be terrified by the prospect of undefined behaviour making your computer explode. Damn! I guess I should have written something else than "explosive cake".

:-D

The patch looks good, but it doesn't address the first point - construction from Container and std::array: they still use the wrong constraint.

And looking at the code a second time I've just realized that the constructors from (pointer, index_type) and (pointer, pointer) have the same problem: they allow conversions from pointer to derived to pointer to base.

On the other hand, the constructor from built-in array doesn't allow qualification conversions between element types (there's a related active Standard issue - CWG2018).

I think the restrictions should be uniform across all these constructors.

Some more nitpicking:

• std::is_same<From, std::remove_cv_t<To>> is now redundant as far as I can tell. It doesn't handle any case that's not handled by the pointer-to-array test, and it misses some, as explained above.
• static_cast<pointer> in the initialization of storage_ is also redundant if the SFINAE condition enabled the constructor. The initialization should work without any explicit conversion.

[neilmacintosh]

@bogiord ;-D

Yes, I consciously separated out the fix for the first point and left it for the next round of change, because I want to change some of the other aspects of construction from Container first. As Vicente has pointed out, there are better ways available (at least on some platforms) to identify contiguous sequence containers. After that, I will revise the Container, array and pointer-based conversion ctors to all add consistent conversion restrictions in one pass.

Good catch of the redundant static_cast, I left that behind by accident. I suspect you're right about the std::is_same test, but I'll double-check to be sure, then remove it if possible. Sometimes SFINAE can be surprising!

[MikeGitb]

@neilmacintosh: Sorry for not coming back to this earlier. I was traveling and didn't have access to a proper development environment.
Originally, I also wanted to propose to use @martinmoene's solution, but when I tried to port it to the MS-gsl it didn't get rid of the error as it did in gsl-lite's unit tests. I'm not really surprised, because - as far as I can tell - it also breaks the strict aliasing rule, but I couldn't figure out, why it worked for gsl-lite, but not for my adaptation. So maybe its just a bug in my code and not a general problem.

Anyway, I created a PR that implements both solutions and maybe someone with more experience can have a closer look at this issue.