Skip to content

Commit

Permalink
Prevent clobbering of data in Result's tail padding internally and ex…
Browse files Browse the repository at this point in the history
…ternally

llvm/llvm-project#70494 points out that `[[no_unique_address]]` on a
union member can cause its placement-new construction to clobber data in
its tail padding, as ctors can and do write their entire `sizeof()`
size, not just their data-size.

So we do some wild things to prevent this from occurring:

The `T` and `E` union are grouped with the state into a struct, which we
call `Inner`. This structure is always constructed together as a group
when changing the state to `Ok` or `Err`. This ensures that we construct
`T` or `E` and then set the state (which follows in the struct) thus
avoiding clobbering of the state.

Second, the `Inner` struct needs to be protected from clobbering other
things in its tail padding. So it is wrapped in another struct that
conditionally applies `[[no_unique_address]]` to it, which we call
`MaybeNoUniqueAddress`.

We mark `Inner` as `[[no_unique_address]]` only when there is no tail
padding in `T` or `E`. Then the tail padding of `Inner` which is
introduced by the state flag can be used by things external to the
Result if `T` are without tail padding.

All of this is put into the outermost structure `StorageVoid` or
`StorageNonVoid` for a void `T` or not, respectively. Everything is
marked `[[no_unique_address]]` except `Inner` which is conditionally
marked as mentioned above.

In order to do this, we've rewritten the entire implementation of
`Result`'s storage. In the process, copy/move operations and destructors
are all trivial if the same for both `T` and `E` are trivial, which
fixes chromium#403.
  • Loading branch information
danakj committed Nov 25, 2023
1 parent 7147b8f commit 91361cf
Show file tree
Hide file tree
Showing 8 changed files with 821 additions and 677 deletions.
6 changes: 6 additions & 0 deletions sus/mem/copy.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,10 @@ concept TrivialCopy =
template <class T>
concept CopyOrRef = Copy<T> || std::is_reference_v<T>;

/// Matches types which are [`CopyOrRef`]($sus::mem::CopyOrRef) or are `void`.
///
/// A helper for genertic types which can hold void as a type.
template <class T>
concept CopyOrRefOrVoid = CopyOrRef<T> || std::is_void_v<T>;

} // namespace sus::mem
6 changes: 6 additions & 0 deletions sus/mem/move.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ concept Move =
template <class T>
concept MoveOrRef = Move<T> || std::is_reference_v<T>;

/// Matches types which are [`MoveOrRef`]($sus::mem::MoveOrRef) or are `void`.
///
/// A helper for genertic types which can hold void as a type.
template <class T>
concept MoveOrRefOrVoid = MoveOrRef<T> || std::is_void_v<T>;

/// Cast `t` to an r-value reference so that it can be used to construct or be
/// assigned to a (non-reference) object of type `T`.
///
Expand Down
5 changes: 5 additions & 0 deletions sus/option/__private/storage.h
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,11 @@ struct Storage<T, false> final {

private:
union {
// TODO: We can make this T [[no_unique_address]], however then
// we can not construct_at on it. Instead, we would need to only
// construct_at the Storage class, and have the ctor set them both.
//
// See also https://github.com/llvm/llvm-project/issues/70494
T val_;
};
State state_ = None;
Expand Down
4 changes: 4 additions & 0 deletions sus/option/option.h
Original file line number Diff line number Diff line change
Expand Up @@ -1991,6 +1991,10 @@ class Option final {
using StorageType =
std::conditional_t<std::is_reference_v<U>, Storage<StoragePointer<U>>,
Storage<U>>;
// TODO: We can make this no_unique_address, however... if StorageType<T> has
// tail padding and Option was marked with no_unique_address, then
// constructing T may clobber stuff OUTSIDE the Option. So this can only be
// no_unique_address when StorageType<T> has no tail padding.
StorageType<T> t_;

sus_class_trivially_relocatable_if_types(::sus::marker::unsafe_fn,
Expand Down
Loading

0 comments on commit 91361cf

Please sign in to comment.