Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Description • 7 years ago

Currently, mozilla::Span can store nullptr as the pointer for a zero-length span. In fact, this what the default constructor does.

As a result, the return value of Elements() is not generally suitable for passing to Rust's std::slice::from_raw_parts() as-is. Specifically, if the pointer is null, it needs to be replaced with a bogus pointer to 0x1 instead, so that the slice wrapped in Some() can be distinguished from None.

It seems to me we should do one of:

 1) Make mozilla::Span enforce the invariant that the pointer stored in mozilla::Span shall not be nullptr. This would make a (large) subset of Span constructions in C++ to execute a null check that has no benefit for C++-only usage. Since all assertions is mozilla::Span are currently release assertions, we actually execute this branch already. (It's debatable if we should be executing the branch already in release builds.)

 2) Make Elements() do the null check when called. This would be simpler than #1 would would result in useless branches when C++ code needs to retrieve the pointer for non-FFI purposes.

 3) Add a new NonNullElements() that's like current Elements() but maps nullptr to 0x1. This would add a branch cost for FFI usage only, but would also require developers to remember to use a special accessor in the FFI case.

 4) Add a companion crate that has an inline Rust function that wraps `from_raw_parts()` and includes the null check to map null to 0x1. It seems more probable that developers will forget to use this than the method from point #3.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 1 • 7 years ago

froydnj, opinions?

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 2 • 7 years ago

(In reply to Henri Sivonen (:hsivonen) from comment #0)
> (It's debatable if we
> should be executing the branch already in release builds.)

See bug 1352734 for Span failing to gain a user for this reason.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 3 • 7 years ago

My current inclination is #1 with demotion of the associated assertions to non-release assertions. (It doesn't really make sense to assert that length isn't SIZE_MAX. The way that'd happen would involve either 1) completely bogus length when constructing from raw parts, but the length could be completely bogus without being max or 2) wrapping a buffer that consumes the entire address space. There's no way to materialize such a buffer.)

	Nathan Froyd [:froydnj]
	Comment 4 • 7 years ago

I am inclined to say that we should do #1, but keep the release assertions.  We already have release assertions all over the place (e.g. array bounds checking) for safety reasons, and this strikes me as a safety reason of equal importance, because we're attempting to ensure that blindly constructing Rust slices from Span doesn't trigger bad behavior in Rust code.  I guess badly constructed Spans would not violate the memory safety properties of Rust code, though...right?

We may want to consider moving some of the release assertions out-of-line if we can, since widespread Span usage will lead to a lot of these release assertions and corresponding code bloat.  (The actual assertion code is out-of-line for nsTArray bounds checking for just this reason.)

	Nathan Froyd [:froydnj]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8876640 [details]
Bug 1359874 - Make Span::Elements() always return a non-null pointer. .

https://reviewboard.mozilla.org/r/147986/#review152450

::: mfbt/Span.h:396
(Diff revision 1)
>   * cannot provide safety against use-after-free.
>   *
>   * (Note: Span is like Rust's slice only conceptually. Due to the lack of
>   * ABI guarantees, you should still decompose spans/slices to raw pointer
> - * and length parts when crossing the FFI.)
> + * and length parts when crossing the FFI. The Elements() and data() methods
> + * are guarenteed to return a non-null pointer even for zero-length spans,

Nit: "guaranteed to return..." (spelling)

::: mfbt/Span.h:836
(Diff revision 1)
>    public:
>      template<class OtherExtentType>
>      MOZ_SPAN_ASSERTION_CONSTEXPR storage_type(pointer elements,
>                                                OtherExtentType ext)
>        : ExtentType(ext)
> -      , data_(elements)
> +      , data_(elements ? elements : reinterpret_cast<pointer>(0x1))

Possibly worth a comment here that we are using 0x1 for explicit compatibility with Rust, or moving 0x1 into an appropriately-named constant, and using that  named constant here.

::: mfbt/tests/gtest/TestSpan.cpp:116
(Diff revision 1)
>  SPAN_TEST(default_constructor)
>  {
>    {
>      Span<int> s;
>      ASSERT_EQ(s.Length(), 0U);
> -    ASSERT_EQ(s.data(), nullptr);
> +    ASSERT_EQ(s.data(), reinterpret_cast<int*>(0x1));

Using a named constant would enable its usage here and throughout, too.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 8 • 7 years ago
mozreview-review-reply

Comment on attachment 8876640 [details]
Bug 1359874 - Make Span::Elements() always return a non-null pointer. .

https://reviewboard.mozilla.org/r/147986/#review152450

Thanks.

> Nit: "guaranteed to return..." (spelling)

Fixed.

> Possibly worth a comment here that we are using 0x1 for explicit compatibility with Rust, or moving 0x1 into an appropriately-named constant, and using that  named constant here.

Added comment. Didn't add a constant due to the type of the pointer being templatized.

> Using a named constant would enable its usage here and throughout, too.

Used constants instead.

	Pulsebot
	Comment 10 • 7 years ago

Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33ce52b7c7ea
Make Span::Elements() always return a non-null pointer. r=froydnj.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/33ce52b7c7ea