Status

()

defect
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

It's unexpectedly tricky to use mozilla::AlignedStorage safely, because people frequently assume that you can create a T in them, copy them around in bitwise form, and then treat the moved bytes as a T -- but that violates strict aliasing.  (Rather, you generally have to *create* an instance of T in AlignedStorage to treat the bytes that way -- and simple memcpying doesn't do that.)

JS managed to shoot itself in the foot on memcpys once, in bug 1269319, by dint of AlignedStorage's default copy constructor.  A fresh attempt was made in bug 1287006, without full awareness of the history.  I expect more will happen, if AlignedStorage sticks around for easy misuse.

It seems better, now that we can use alignas/alignof, to make people write this stuff out themselves.  That bar to entry forces *some* awareness of these strict aliasing considerations, and hopefully it will reduce these sorts of memcpy footguns.  Bug 1287006 has sort of already poached the space for Maybe, but let's fix all the other instances in this bug.  I have patches for most (but not all) of them -- will post what I have now and keep working on the incomplete ones.
This is necessary for some of the later patches.
Attachment #8835796 - Flags: review?(nfroyd)
Applying alignas to a class member forces the entire class to be aligned, unsurprisingly.  Unfortunately, alignment of parameters is an ABI detail -- so you can't safely pass these classes as parameters with MSVC.  MOZ_NON_PARAM will convert use as parameter into a static-analysis violation, which helps mitigate that.  But it's a little unfortunate that actually properly aligning these sorts of embedded instances, means the class can't be a parameter any more.  So it goes.

There are no MaybeOneOf parameters to fix.  Other classes I've had to touch have had changes in much more varied places.
Attachment #8835798 - Flags: review?(nfroyd)
The added MOZ_NON_PARAM aren't actually necessary for the static analysis to be effective -- but it seems good form to apply it for documentation/clarity purposes.

There are a surprising number of places where people were copying stuff by value, that this eliminates.  Pass by reference versus pass by copy isn't *always* a win, but I suspect the "cost" is pretty low -- especially given how many places really shouldn't have been passing by copy anyway.
Attachment #8835801 - Flags: review?(nfroyd)
Attachment #8835799 - Flags: review?(shu) → review+
Comment on attachment 8835798 [details] [diff] [review]
Use alignas/alignof, and don't use sizeof where alignof should have been used, in MaybeOneOf

Review of attachment 8835798 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/MaybeOneOf.h
@@ +30,5 @@
>   * arguments and that object will be destroyed when the owning MaybeOneOf is
>   * destroyed.
>   */
>  template<class T1, class T2>
> +class MOZ_NON_PARAM MaybeOneOf

Probably worth a comment here as to why this is MOZ_NON_PARAM.

@@ +98,5 @@
>    void construct(Args&&... aArgs)
>    {
>      MOZ_ASSERT(state == None);
>      state = Type2State<T>::result;
> +    ::new (data()) T(Forward<Args>(aArgs)...);

Can you file a followup to use our non-null-checking placement new?:

http://dxr.mozilla.org/mozilla-central/source/mfbt/OperatorNewExtensions.h#45
Attachment #8835798 - Flags: review?(nfroyd) → review+
Comment on attachment 8835801 [details] [diff] [review]
Use alignas/alignof to define Variant's internal raw storage

Review of attachment 8835801 [details] [diff] [review]:
-----------------------------------------------------------------

Conceptually, I like the idea of MOZ_NON_PARAM'ing things for better diagnostics, but I worry about that attribute going stale.  I think we should add it to Variant, and let the static analysis catch other errors.  If we need better diagnostics from the static analysis, we should handle that in a separate bug.

r=me with the changes below.

::: image/StreamingLexer.h
@@ +62,5 @@
>   * static methods on Transition, and then return them to the lexing framework
>   * for execution.
>   */
>  template <typename State>
> +class MOZ_NON_PARAM LexerTransition

Let's remove this.

::: js/src/jscntxt.h
@@ +292,5 @@
>       * Intentionally awkward signpost method that is stationed on the
>       * boundary between Result-using and non-Result-using code.
>       */
>      template <typename V, typename E>
> +    bool resultToBool(const JS::Result<V, E>& result) {

I think you meant to put the changes in this file in a separate changeset?  *shrug*

::: mfbt/Variant.h
@@ +437,5 @@
>   *       ...
>   *     };
>   */
>  template<typename... Ts>
> +class MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS MOZ_NON_PARAM Variant

Probably a good idea to explain why this is MOZ_NON_PARAM.
Attachment #8835801 - Flags: review?(nfroyd) → review+
Attachment #8835796 - Flags: review?(nfroyd) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb9b629b161a
Use alignas rather than AlignedStorage for public JS::ProfilingFrameIterator's internal storage of a private wasm or JIT profiling frame iterator.  r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8970aa3b2d6
Use alignas/alignof, and don't use sizeof where alignof should have been used, in MaybeOneOf.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb11f699091d
Make tl::Min/Max variadic.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/81b228f0a770
Use alignas/alignof to define Variant's internal raw storage.  r=froydnj
Vector and RInstructionStorage in js/ are the last two uses of this, I think.  I have patchwork for the former, but it's hitting MSVC compile errors I need to sort through.  The latter may well be incompatible with the C++ object model, in which case trickier changing may be needed.
Keywords: leave-open
This consumes a byte when there are no inline elements -- exactly as already happens now.  (It just wasn't so obvious with the current code.)  I have a followup patch that makes the quasi-minor changes necessary to eliminate that extra space.

The security/certverifier/SignedCertificateTimestamp.h change is necessary because Buffer::Init *requires* that its provided pointer be non-null, even for length zero, as it uses null/0 as internal sentinels to mark a Buffer as initialized.  It doesn't appear to me this is fixable if begin() starts returning nullptr when length is zero for no-inline-elements Vectors.  And it seems a whole lot better to return nullptr, than to return something horrific like static_cast<Vector*>(1) or something.  We could also make Vector::inlineStorage() assert, or not exist at all, if there's no inline storage -- but that would require much deeper changes to Vector's implementation and is absolutely not something I want to touch now, if ever.
Attachment #8839780 - Flags: review?(nfroyd)
Lots and lots of paint-job changes to aggregate capacity/reserved in with inline element storage, but otherwise the actual change here is limited to the CapacityAndReserved/CRAndStorage/mTail modifications.
Attachment #8839781 - Flags: review?(nfroyd)
Comment on attachment 8839780 [details] [diff] [review]
Make Vector not use AlignedStorage for its inline element storage

Review of attachment 8839780 [details] [diff] [review]:
-----------------------------------------------------------------

It is going to be fascinating to see if the nullptr change triggers crashes because people are dereferencing begin() or somesuch.

Redirecting r? to keeler for the SignedCertificateTimestamp.h change in case there's something non-obvious about how Buffer and Input work, but r=me on the rest.
Attachment #8839780 - Flags: review?(nfroyd)
Attachment #8839780 - Flags: review?(dkeeler)
Attachment #8839780 - Flags: review+
Comment on attachment 8839781 [details] [diff] [review]
Shrink Vector from (usually) four pointers in size to three when no inline storage is used

Review of attachment 8839781 [details] [diff] [review]:
-----------------------------------------------------------------

Hooray for smaller data structures!
Attachment #8839781 - Flags: review?(nfroyd) → review+
Comment on attachment 8839780 [details] [diff] [review]
Make Vector not use AlignedStorage for its inline element storage

Review of attachment 8839780 [details] [diff] [review]:
-----------------------------------------------------------------

Turns out, this is a good opportunity to add some error checking. r=me with changes outlined below. Thanks!

::: security/certverifier/SignedCertificateTimestamp.h
@@ +89,5 @@
>  
>  inline pkix::Result BufferToInput(const Buffer& buffer, pkix::Input& input)
>  {
> +  if (buffer.length() == 0) {
> +    return pkix::Success;

Actually, it would be an error to be given a zero-length buffer for almost every call site except the one at https://dxr.mozilla.org/mozilla-central/rev/a180b976c165b6cd174d24bbb77941919cdc53cb/security/certverifier/CTLogVerifier.cpp#201

So, let's do this:

* return pkix::Result::FATAL_ERROR_LIBRARY_FAILURE here
* change the above callsite to:

  // sct.extensions may be empty. If it is, sctExtensionsInput will remain in
  // its default state, which is valid but of length 0.
  Input sctExtensionsInput;
  if (sct.extensions.length() > 0) {
    rv = sctExtensionsInput.Init(sct.extensions.begin(), sct.extensions.length());
    if (rv != Success) {
      return rv;
    }
  }

Thanks!
Attachment #8839780 - Flags: review?(dkeeler) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f6b33bc6c2b
Make Vector not use AlignedStorage for its inline element storage.  r=froydnj, r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/37acaeb307f1
Shrink Vector from (usually) four pointers in size to three when no inline storage is used.  r=froydnj
Depends on: 1341951
Duplicate of this bug: 1178588
The leave-open keyword is there and there is no activity for 6 months.
:Waldo, maybe it's time to close this bug?
Flags: needinfo?(jwalden+bmo)
Yeah, AlignedStorage is gone.  AlignedStorage2 still exists, but that probably should be a new bug.
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Flags: needinfo?(jwalden+bmo)
Resolution: --- → FIXED
Bug 1343717 exists for AlignedStorage2 removal.
You need to log in before you can comment on or make changes to this bug.