Closed Bug 1451248 Opened 6 years ago Closed 6 years ago

Value default initialization broken by bug 1449051

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 --- fixed

People

(Reporter: jandem, Assigned: Waldo)

References

Details

(Keywords: csectype-uninitialized, regression, sec-moderate)

Attachments

(2 files)

Bug 1411415 added a constructor to Value::layout:

  layout() : asBits(JSVAL_RAW64_UNDEFINED) {}

Bug 1449051 then removed |layout|, but also removed this initialization.

This probably happened because bug 1411415 added UninitializedValue to fix the union problem here:

    // The default constructor leaves Value uninitialized. Adding a default
    // constructor prevents Value from being stored in a union.
    Value() = default;
    Value(const Value& v) = default;

But then it didn't update this comment, making the reader believe Values are still uninitialized by default. It should probably also have done the initialization in the Value constructor instead of its layout.
Flags: needinfo?(jwalden+bmo)
Bah.  This is silly.  C++11 allows fields with non-trivial (roughly, "has to do anything" like initializing or destroying fields or base class or vptr) constructors/destructors/etc. in unions.  JS::UninitializedValue actively violates the C++ memory model: if you don't actively allocate a JS::Value in a location, from the language/compiler's perspective THERE IS NO JS::Value THERE, and using that location as a JS::Value is undefined behavior.  So this is all a mess.  Let's do the right thing here.

First, restore the default constructor so it does non-default things *directly*, rather than inherited-ly (the big reason I missed this before being that I mis-deemed "= default" here to be "this doesn't do anything" -- but of course JS::UninitializedValue didn't help matters, either).

Second, remove JS::UninitializedValue.  It violates the C++ object model: unless you have an actual variable of non-trivial class type, you *must* initialize it using placement new before you use it as that type -- even to overwrite the object by assigning it!  In a C++11 world we can use JS::Value directly in unions, and for sanity's sake we should.  (Other, similar ways are possible, but we've got union code now, easiest to keep using it.)

Third, update code that used JS::UninitializedValue to use JS::Value directly.  The C++ object model requires that when you do this, accessing such JS::Value union member must occur *first* by placement-new before the JS::Value field can be used.  (Note that if the union member is something like |struct S { JS::Value v; } s;|, you *cannot* do something like |new (&u.s.v) Value();|.  I think.  You have to placement-new *the union field*, not some member within that un-constructed field.  Hence some of the new constructors added here.)

Interestingly, C++<17 under-defines how the *other* fields can be used here.  If nothing's been initialized yet, you can just write to non-trivial fields -- say for |struct S { int x, y; } s;| you can do |u.s.x = 5| and |u.s.x| will *be* an |int| with value 5 now.  (Yes, even nested madness like this is permissible -- and as current code shows, *used*.  U+1F631 FACE SCREAMING IN FEAR)  But it's really not clear until C++17 [class.union]p5 how assigning to trivial fields permissibly changes how the object model understands that location, when that location was part of a non-trivial arm of the union.  As of C++17 assigning to such actively creates a new object of that type in that location -- and even intermediate locations like |u.s| (with caveat that items along the way *not* part of the specified path are uninitialized, e.g. |u.s.y|).

So, I tried to play it as absolutely safe as possible.  In C++11 and C++14, the general suggestion is that in the presence of non-trivial union members, the approach that is safe *in general* is to placement-new everything.  So I did that here.  Every init of an arm of a union containing a non-trivial JS::Value, occurs through/after a placement-new, and every use of such *after* that placement-new (and before any other arm is placement-new'd, of course).

Anyway, this sprawls a little, but I think it's the right approach.  djvj for the Value.h changes especially and for the other js/ changes, bz for everything else.  But feel free to steal glances at the other's code to review if desired (I know I would, for this hoary hairy area).
Attachment #8965986 - Flags: review?(kvijayan)
Attachment #8965986 - Flags: review?(bzbarsky)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(jwalden+bmo)
In passing, I saw a couple unions with constructors for no obvious reason.  Turns out they had them -- and somehow evaded MSVC's warnings?  not sure how that works -- only because js::wasm::Val contains a union with a constructor -- that itself is unnecessary.  (Val's union is

    union U {
        uint32_t i32_;
        uint64_t i64_;
        float f32_;
        double f64_;
        I8x16 i8x16_;
        I16x8 i16x8_;
        I32x4 i32x4_;
        F32x4 f32x4_;
    } u;

and the first four types are obviously trivial, and the last four

typedef int8_t I8x16[16];
typedef int16_t I16x8[8];
typedef int32_t I32x4[4];
typedef float F32x4[4];

are also trivial.)  So if we remove Val::U's constructor, the rest of the chain can fall.  And that seems like a good thing, because unions with non-trivial constructors invoke all sorts of complexity best avoided as long as that's possible.
Attachment #8965987 - Flags: review?(kvijayan)
(In reply to Jeff Walden [:Waldo] from comment #2)
> and somehow evaded MSVC's warnings?  not sure how that works

The warnings are only when non-trivial members would not be initialized/destroyed, so an empty constructor in a union with no non-trivial members wouldn't trigger them.  JFYI.
Comment on attachment 8965986 [details] [diff] [review]
Readd JS::Value's default constructor, remove JS::UninitializedValue to eliminate confusion, and adjust code to deal

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

Stealing the js/ part from djvj. r=me.

::: xpcom/reflect/xptcall/xptcall.h
@@ +18,5 @@
>  
>  struct nsXPTCMiniVariant
>  {
>  // No ctors or dtors so that we can use arrays of these on the stack
>  // with no penalty.

The comment needs an adjustment now.
Attachment #8965986 - Flags: review?(kvijayan) → review+
Attachment #8965987 - Flags: review?(kvijayan) → review+
Priority: -- → P1
I'm sorry for the horrible lag here.  I will get to this first thing on Monday...
Comment on attachment 8965986 [details] [diff] [review]
Readd JS::Value's default constructor, remove JS::UninitializedValue to eliminate confusion, and adjust code to deal

OK, so a bunch of comment 1 needs to be in the commit message here.

>+++ b/dom/bindings/BindingUtils.cpp
>@@ -455,34 +458,35 @@ TErrorResult<CleanupPolicy>::operator=(T
>+    mContents.mMessage = aRHS.mContents.mMessage = nullptr;

Should this end up using InitMessage(nullptr)?  Or is direct assignment ok here for some reason?  Worth documenting the reason if the latter.

>+++ b/dom/bindings/ErrorResult.h
>+  union Contents {

So naming...

It's perfectly fine to have an ErrorResult that never touches this union.  Maybe I'd call this union Extra and the member mExtra?  I think I'd prefer it to Contents/mContents.

>+  JS::Value& InitJSException(const JS::Value& aValue) {

Both callers pass JS::UndefinedValue(), right?  How about we make this not take an arg at all.

I largely skipped over the js/src bits.

r=me.  Thank you for doing this, and I am really sorry for the horrible lag.  :(
Attachment #8965986 - Flags: review?(bzbarsky) → review+
This is sec-bad, but it also is nightly-only, so happily that means I can just land it.  Albeit with the usual don't-spill-the-beans-too-much going on, which the original patch did not do (and arguably obfuscated well, by touching lots of stuff at once).

https://hg.mozilla.org/integration/mozilla-inbound/rev/822e81707327954a2c4e7d8060c5d91ae528bbf3

I'll whip up some sort of mini-patch adding some comments as to the C++ memory model issues shortly (albeit not now, I owe efaust some reviews I promised to start/do today).
https://hg.mozilla.org/mozilla-central/rev/822e81707327
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Group: javascript-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.