[base] Constructing static spans from dynamic containers should be explicit |
||
Issue description
Currently it is possible to implicitly construct static spans from dynamic containers, e.g. like this:
void TakesStaticSpan(base::span<const uint8_t, 5> static_span) {
...
}
std::vector my_vec = ....;
TakesStaticSpan(my_vec);
This is not ideal, as this conversion can lead to CHECK failures when the dynamic container has the wrong length.
Thus the client needs to be certain about the size of the container or implement appropriate length checks. Given that the implicit conversion is invisible to the client, it is easy to forget to perform these checks. See e.g. [1] for a recent bug because of this.
I thus propose to make the dynamic container -> span constructor conditionally explicit, and provide a corresponding MakeSpan<Extent> method to aid usability.
[1] https://crbug.com/872217
,
Aug 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9cfe470d793da6e09b966d435c8fa2ba1625d5fe commit 9cfe470d793da6e09b966d435c8fa2ba1625d5fe Author: jdoerrie <jdoerrie@chromium.org> Date: Tue Aug 28 11:41:44 2018 [base] Make dynamic container to static span conversion explicit This change disallows implicit conversions from dynamic containers to static spans. This conversion can cause CHECK failures, and thus should be done carefully. Requiring explicit construction makes it more obvious when this happens. To aid usability, appropriate base::make_span<size_t> overloads are added. Bug: 877931 Change-Id: Id9f526bc57bfd30a52d14df827b0445ca087381d Reviewed-on: https://chromium-review.googlesource.com/1189985 Reviewed-by: Ryan Sleevi <rsleevi@chromium.org> Reviewed-by: Balazs Engedy <engedy@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org> Cr-Commit-Position: refs/heads/master@{#586657} [modify] https://crrev.com/9cfe470d793da6e09b966d435c8fa2ba1625d5fe/base/containers/span.h [modify] https://crrev.com/9cfe470d793da6e09b966d435c8fa2ba1625d5fe/base/containers/span_unittest.cc [modify] https://crrev.com/9cfe470d793da6e09b966d435c8fa2ba1625d5fe/base/containers/span_unittest.nc [modify] https://crrev.com/9cfe470d793da6e09b966d435c8fa2ba1625d5fe/content/browser/webauth/virtual_authenticator.cc [modify] https://crrev.com/9cfe470d793da6e09b966d435c8fa2ba1625d5fe/device/fido/cable/fido_cable_handshake_handler.cc [modify] https://crrev.com/9cfe470d793da6e09b966d435c8fa2ba1625d5fe/device/fido/ctap_response_unittest.cc [modify] https://crrev.com/9cfe470d793da6e09b966d435c8fa2ba1625d5fe/device/fido/device_response_converter.cc [modify] https://crrev.com/9cfe470d793da6e09b966d435c8fa2ba1625d5fe/device/fido/make_credential_task_unittest.cc [modify] https://crrev.com/9cfe470d793da6e09b966d435c8fa2ba1625d5fe/net/ntlm/ntlm.cc [modify] https://crrev.com/9cfe470d793da6e09b966d435c8fa2ba1625d5fe/net/ntlm/ntlm_client.cc
,
Aug 29
|
||
►
Sign in to add a comment |
||
Comment 1 by jdoerrie@chromium.org
, Aug 27