{"action":"closed","issue":{"url":"https://api.github.com/repos/Microsoft/GSL/issues/313","repository_url":"https://api.github.com/repos/Microsoft/GSL","labels_url":"https://api.github.com/repos/Microsoft/GSL/issues/313/labels{/name}","comments_url":"https://api.github.com/repos/Microsoft/GSL/issues/313/comments","events_url":"https://api.github.com/repos/Microsoft/GSL/issues/313/events","html_url":"https://github.com/Microsoft/GSL/issues/313","id":169773290,"number":313,"title":"span<T> constructors allow unsafe conversions","user":{"login":"bogiord","id":4967002,"avatar_url":"https://avatars.githubusercontent.com/u/4967002?v=3","gravatar_id":"","url":"https://api.github.com/users/bogiord","html_url":"https://github.com/bogiord","followers_url":"https://api.github.com/users/bogiord/followers","following_url":"https://api.github.com/users/bogiord/following{/other_user}","gists_url":"https://api.github.com/users/bogiord/gists{/gist_id}","starred_url":"https://api.github.com/users/bogiord/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bogiord/subscriptions","organizations_url":"https://api.github.com/users/bogiord/orgs","repos_url":"https://api.github.com/users/bogiord/repos","events_url":"https://api.github.com/users/bogiord/events{/privacy}","received_events_url":"https://api.github.com/users/bogiord/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":7,"created_at":"2016-08-07T00:31:02Z","updated_at":"2016-09-13T19:19:39Z","closed_at":"2016-09-13T19:19:39Z","body":"`std::is_convertible<typename Container::pointer, pointer>` is the wrong test to constrain the constructor from a container. Example:\r\n\r\n```c++\r\n#include <vector>\r\n#include <iostream>\r\n#include \"gsl.h\"\r\n\r\nstruct B { int i = 7; };\r\nstruct D : B { int j = 0; };\r\n\r\nint main()\r\n{\r\n   std::vector<D> v = {{}, {}, {}};\r\n   gsl::span<B> s = v;\r\n   std::cout << s[1].i << '\\n'; // undefined behaviour, may print 0\r\n}\r\n```\r\n\r\nThe 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.)\r\n\r\nNow, conversions between `span`s: `is_allowed_element_type_conversion` is way too permissive. \r\n\r\n`is_allowed_integral_conversion` is the wrong test in this context. You can't look at an array of `int`s 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).\r\n\r\n`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.\r\n\r\nAnd on top of all this, the `reinterpret_cast<pointer>(other.data())` is the icing on the explosive cake. One example involving pointers:\r\n\r\n```c++\r\n#include <iostream>\r\n#include \"gsl.h\"\r\n\r\nstruct B1 { int i = 7; };\r\nstruct B2 { int j = 3; };\r\nstruct D : B1, B2 { int k = 0; };\r\n\r\nint main()\r\n{\r\n   D a[] = {{}, {}};\r\n   D* ap[] = {&a[0], &a[1]};\r\n   gsl::span<D*> sd = ap;\r\n   gsl::span<B2*> sb = sd;\r\n   std::cout << sb[0]->j << '\\n'; // undefined behaviour, may print 7\r\n}\r\n```\r\n\r\nOn 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).\r\n\r\nReally, 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. \r\n\r\nAlso, 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 `span`s internally.\r\n\r\nNote 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:\r\n\r\n```c++\r\n#include <iostream>\r\n#include \"gsl.h\"\r\n\r\nint f(int& i, gsl::byte& r)\r\n{\r\n   i = 7;\r\n   r <<= 1;\r\n   return i;\r\n}\r\n\r\nint main()\r\n{\r\n   int i;\r\n   std::cout << f(i, reinterpret_cast<gsl::byte&>(i)) << '\\n';\r\n}\r\n```\r\n\r\nIn MSVC it always prints `14`. In Clang and GCC it prints `7` with optimizations enabled and `14` without optimizations - classic undefined behaviour; [sample here](http://melpon.org/wandbox/permlink/hhbGtLD3y1wQB9v8).\r\n\r\nThe same code but with `using byte = unsigned char;` always prints `14` in all compilers; [sample here](http://melpon.org/wandbox/permlink/g22USSsuhVGwPPyn)."}}
{"actor":{"display_login":"neilmacintosh"}}