Closed Bug 1411415 Opened 2 years ago Closed 2 years ago

Initialize Values to undefined by default

Categories

(Core :: JavaScript Engine, defect, P1)

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 60+ fixed
firefox56 --- wontfix
firefox57 - wontfix
firefox58 - wontfix
firefox59 + wontfix
firefox60 + fixed

People

(Reporter: djvj, Assigned: djvj)

References

Details

(Keywords: csectype-uninitialized, sec-moderate, Whiteboard: [adv-main60+][adv-esr52.8+][post-critsmash-triage])

Attachments

(2 files, 6 obsolete files)

Currently our default Value constructor has a default constructor.  It should initialize to Undefined for consistent behaviour across the engine.
This seems to be a reasonably high security vulunerability to my eyes, so I'd like confirmation.  The core issue is the interaction between two behaviours:

1. The 'Value()' constructor is specified as the default PoD constructor, which leaves the Value bits uninitialized [http://searchfox.org/mozilla-central/source/js/public/Value.h#309]

2. In GCVector, if we trace the implementation of 'growBy':

  http://searchfox.org/mozilla-central/source/js/public/GCVector.h#73
  http://searchfox.org/mozilla-central/source/mfbt/Vector.h#1148
  http://searchfox.org/mozilla-central/source/mfbt/Vector.h#177
  http://searchfox.org/mozilla-central/source/mfbt/Vector.h#165

In VectorImpl::new_, called from VectorImpl::initialize, we have parameter type T=Value, Args=() for:

  T temp(Forward<Args>(aArgs)...);
  *aDst = temp;

Which should desugar to:

  Value temp;
  *aDst = temp;

Value's PoD default constructor should imply that temp's bits are uninitialized and arbitrary.  Now, most optimizing compilers will notice that |temp|s bits are all undefined and reduce the store to a constant (e.g. zero).  A particularly clever compiler _might_ decide that since |temp| is undefined, the store itself is a no-op under optimized conditions, and elide the store entirely (and since our mozilla::Vector stores small-size vectors directly on the stack, this elided store would then give raw access to a stack location via a Value).

Similarly, a naive compiler that actually constructs |temp| on stack will pick up any bits left behind by previous call stacks that used that stack region.


Is there something I'm missing here?  This seems like a hole.  If it's a hole, it seems serious.

I wrote a rough patch for this that builds and seems to be passing initial tests.  It changes Value to have a default constructor that initiliazes to JSVAL_UNDEFINED.  It adds a UnionizableValue (nice and annoying to type to make sure anyone who uses it thinks a little bit) that has a PoD constructor and convenience methods to convert back and forth from Value, which can be used in unions.
Attachment #8921664 - Flags: feedback?
Flags: needinfo?(jorendorff)
Attachment #8921664 - Flags: feedback?(jorendorff)
Attachment #8921664 - Flags: feedback?(jcoppeard)
Attachment #8921664 - Flags: feedback?
Comment on attachment 8921664 [details] [diff] [review]
initialize-value-on-construct.patch

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

I'm not sure how serious this is, but it does sounds prudent, so f+.

::: js/public/Value.h
@@ +955,5 @@
> +/**
> + * This is a null-constructible structure that can convert to and from
> + * a Value, allowing UnionizableValue to be stored in unions.
> + */
> +struct MOZ_NON_PARAM alignas(8) UnionizableValue

Bikeshedding, but UninitializedValue might be clearer.

@@ +985,2 @@
>  static_assert(sizeof(Value) == 8, "Value size must leave three tag bits, be a binary power, and is ubiquitously depended upon everywhere");
> +static_assert(sizeof(UnionizableValue) == sizeof(Value), "Value and UnionizableValue must be the same size");

We should probably add a test somewhere that |Value().isUndefined()|.
Attachment #8921664 - Flags: feedback?(jcoppeard) → feedback+
Group: core-security → javascript-core-security
Anything we could do here to make this issue visible to fuzzing? Like adding assertions or a testing function if necessary. I'm trying to make sure that if we have a security vulnerability here, we would also find it (or similar ones in the future) by JS fuzzing. Thanks!
Flags: needinfo?(kvijayan)
Comment on attachment 8921664 [details] [diff] [review]
initialize-value-on-construct.patch

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

::: js/public/Value.h
@@ +970,5 @@
> +        return bits;
> +    }
> +
> +    inline Value& asValueRef() {
> +        return reinterpret_cast<Value&>(bits);

This looks a bit strange to me; I think we usually cast |this| to Value* or something.

::: js/src/jit/CacheIRCompiler.cpp
@@ +925,4 @@
>              *reinterpret_cast<uint64_t*>(destWords) = field.asInt64();
>              break;
>            case StubField::Type::Value:
> +            InitGCPtr<JS::UnionizableValue>(destWords, field.asInt64());

Does this one have to be a UnionizableValue? If not we don't need the Barrier stuff probably.
decoder:  Currently it's just the gczeal that's exposing it.

On further review, I don't think this code actually allows the potentially bad values to escape to anything other than the GC.  The usage of growBy() in the crashing testcase was followed immediately by VMcode that fills the buffer.

I originally discovered the issue when I was fixing tests with my NaNboxing patch, and noticed null-JSObject-pointer values being generated from this location, but the test it triggered on was using a gczeal parameter that triggered a gc between the call to |growBy()| and subsequent filling of the vector by the caller.

So I think the trigger steps for this reduce the threat level quite a bit.

If somehow we trigger GC between |growBy()| and vector-population (not sure this is even possible in release or if requires gczeal to trigger), then we risk the GC scanning roots and finding GCVector instances which contain boxedvals that may be invalid.

I think the hooks for this in fuzzing are already there: the gczeal is what triggered the failure.  A builtin function that recurses moderately and does a lot of things-that-require-GCVectors might be good for surfacing the rarer rooting issues.
Flags: needinfo?(kvijayan)
sec-high based on comment 1, but maybe comment 5 is saying it's sec-moderate or sec-low?
Comment on attachment 8921664 [details] [diff] [review]
initialize-value-on-construct.patch

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

::: js/public/Value.h
@@ +970,5 @@
> +        return bits;
> +    }
> +
> +    inline Value& asValueRef() {
> +        return reinterpret_cast<Value&>(bits);

I agree with Jan -- cast `this` or `&bits`, then unary *.
Attachment #8921664 - Flags: feedback?(jorendorff) → feedback+
Flags: needinfo?(jorendorff)
Priority: -- → P1
Is this something you're aiming to uplift for 57? Can you request sec approval if so? Time is pretty tight, so depending on how bad of a vulnerability we think this is vs. the risk for last minute uplifts, we may need to hold off till 58.
Flags: needinfo?(kvijayan)
(In reply to Kannan Vijayan [:djvj] from comment #5)
> I originally discovered the issue when I was fixing tests with my NaNboxing
> patch, and noticed null-JSObject-pointer values being generated from this
> location, but the test it triggered on was using a gczeal parameter that
> triggered a gc between the call to |growBy()| and subsequent filling of the
> vector by the caller.

Oh, this does sound serious then.  The GC should not observe uninitialised values.  Any GC triggered by gczeal could potentially happen in real use.  Do you have a testcase for this?
(In reply to Jon Coppeard (:jonco) from comment #9)
> 
> Oh, this does sound serious then.  The GC should not observe uninitialised
> values.  Any GC triggered by gczeal could potentially happen in real use. 
> Do you have a testcase for this?

I would also be interested in a testcase here, to make sure that we can hit this in fuzzing. It seems important to me that we can detect such situations in our security testing.
I don't have a testcase on the existing code base.  It turns out in practice the actual initialized value is 0, which is a valid value in our cases, so escaped values show up as doubles.  The error showed up as one of the test failures caused by changing the boxing format so that 0 was no longer a valid uint64_t Value.

I think it'll be extremely hard to get a test case up for this, because it depends on the usage of uninitialized Values producing invalid pointer-like Value bitpatterns.
Flags: needinfo?(kvijayan)
(In reply to Jan de Mooij [:jandem] from comment #4)
> +            InitGCPtr<JS::UnionizableValue>(destWords, field.asInt64());
> 
> Does this one have to be a UnionizableValue? If not we don't need the
> Barrier stuff probably.

InitGCPtr uses mozilla::BitwiseCast, so I can't use it directly.  It's just a helper function though so I can specialize it directly in the case statement.
Attached patch always-initialize-value.patch (obsolete) — Splinter Review
Updated patch with comments addressed.  `UnionizableValue` is now `UninitializedValue`, and the GC stuff around UninitializedValue has gone away.
Attachment #8921664 - Attachment is obsolete: true
Attachment #8925665 - Flags: review?(jorendorff)
Attachment #8925665 - Flags: review?(jdemooij)
Attachment #8925665 - Flags: review?(jorendorff) → review+
Just realized I forgot to make that style change for how we do bitcasting typically.  I'll address that now.
Ok, so I'll need to put an updated patch for review.  Turns out there are bits outside SM that use Value in unions, and the general firefox build was failing (https://treeherder.mozilla.org/#/jobs?repo=try&revision=8af7542ba558768feaa13a13534c9a7c9821df37&selectedJob=142773599).

Fixing that gave me a working build (https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc983ebd6ae4248bc06663cdfe9f25a68bd4f7f4&selectedJob=142789052), but the test failures in Asan indicated a write overflow.

Guessing the no-op Value() constructor was hiding some writes to memory that are now triggering buffer overflows in ASAN.  Seems to be happening in the frame rematerialization code from JIT => Interp, when we're filling slots in the frame.

Stack looks like:

(gdb) bt
#0  ReportGenericError ()
    at /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_report.cc:
1063  
#1  0x0000000000502f9e in __asan_report_store8 ()
    at /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_rtl.cc:137
#2  0x000000000053f1c8 in JS::Value::layout::layout (this=0x53f1c8)
    at /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Value.h:850
#3  0x0000000000e2a973 in js::jit::RematerializedFrame::RematerializedFrame (this=0x608000005ea0,
    cx=0x62000000f080, top=0x0, numActualArgs=0, iter=..., fallback=...)
    at /builds/worker/workspace/build/src/js/src/jit/RematerializedFrame.cpp:34
#4  0x0000000000e2b279 in js::jit::RematerializedFrame::New (cx=0x62000000f080,
    top=0x7fffffffb470 "\257\255\302=\250+", iter=..., fallback=...)
    at /builds/worker/workspace/build/src/js/src/jit/RematerializedFrame.cpp:71
#5  0x0000000000e2b6a4 in js::jit::RematerializedFrame::RematerializeInlineFrames (cx=0x53f1c8,
    top=0x7fffffffb470 "\257\255\302=\250+", iter=..., fallback=..., frames=...)
    at /builds/worker/workspace/build/src/js/src/jit/RematerializedFrame.cpp:86
#6  0x00000000016d6f1a in js::jit::JitActivation::getRematerializedFrame (this=<optimized out>,
    cx=0x53f1c8, iter=..., inlineDepth=0)
    at /builds/worker/workspace/build/src/js/src/vm/Stack.cpp:1613
#7  0x00000000016d6b04 in js::FrameIter::ensureHasRematerializedFrame (this=0x7fffffff5420,
    cx=0x62000000f080) at /builds/worker/workspace/build/src/js/src/vm/Stack.cpp:1053
#8  0x000000000142c0c8 in js::DebuggerFrame::getOlder (cx=<optimized out>, frame=..., result=...)
    at /builds/worker/workspace/build/src/js/src/vm/Debugger.cpp:7692
#9  0x0000000001432af9 in js::DebuggerFrame::olderGetter (cx=0x62000000f080, argc=<optimized out>,
    vp=<optimized out>) at /builds/worker/workspace/build/src/js/src/vm/Debugger.cpp:8382
#10 0x00000000007ae442 in js::CallJSNative (cx=0x62000000f080,
    native=0x1432890 <js::DebuggerFrame::olderGetter(JSContext*, unsigned int, JS::Value*)>,
    args=...) at /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291
#11 0x00000000007ae01b in js::InternalCallOrConstruct (cx=0x53f1c8, args=...,
    construct=js::NO_CONSTRUCT) at /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:472
#12 0x00000000007af0c6 in InternalCall (cx=0x62000000f080, args=...)
    at /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:521
#13 0x00000000007af2dd in js::Call (cx=0x62000000f080, fval=..., thisv=..., args=..., rval=...)
    at /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:540
#14 0x00000000007b05be in js::CallGetter (cx=<optimized out>, thisv=..., getter=..., rval=...)
    at /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:655
#15 0x00000000015d80f1 in CallGetter (cx=0x62000000f080, obj=..., receiver=..., shape=..., vp=...)
    at /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2119
#16 0x000000000159bcd8 in GetExistingProperty<(js::AllowGC)1> (cx=0x62000000f080, receiver=...,
    obj=..., shape=..., vp=...)
    at /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2172
#17 0x000000000159c66c in NativeGetPropertyInline<(js::AllowGC)1> (cx=0x62000000f080, obj=...,
    receiver=..., id=..., nameLookup=NotNameLookup, vp=...)
    at /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2375
#18 0x00000000005afbe5 in js::GetProperty (cx=0x62000000f080, obj=..., receiver=..., id=...,
    vp=...) at /builds/worker/workspace/build/src/js/src/vm/NativeObject.h:1604
Attached patch always-initialize-value.patch (obsolete) — Splinter Review
Updated patch which actually lets you build a browser.  Try is here:

(https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc983ebd6ae4248bc06663cdfe9f25a68bd4f7f4&selectedJob=142789052)

I guess I'll roll the ASAN overflow fix in as a second patch to this on the same bug.
Attachment #8925665 - Attachment is obsolete: true
Attachment #8925665 - Flags: review?(jdemooij)
Did you forget to put a review request on this patch? Or is this an unready WIP?
Flags: needinfo?(kvijayan)
It's not a complete patch yet.  Still a few crashes on try.  I'm starting work on it again.
Flags: needinfo?(kvijayan)
Last try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=013ab620a2749d3c79a2994d1a721f7a0488789b

The ASAN Jit crashes here are caused by a particular behaviour of RematerializedFrame.  This is a variable-length structure which is expected to be allocated with an array of values after the main object payload.  This is represented as a 1-element Value[] array at the end of the struct.

When allocating the struct, we calculate the number of needed, subtract 1 (to account for the 1-element slot declared in the struct), and add space for that many values to the size of the struct.

However, if the number of slots is zero, the adjusted (post-subtract) number can be negative, and the memory allocated actually smaller than sizeof(RematerializedFrame).  This was not a problem previously because the |Value()| constructor was default and thus a no-op.  Now that it's no-longer a no-op, this causes a write beyond the end of the allocated space when a RematerializedFrame is constructed and the implicit member initialization occurs.

Fix is to always make sure we allocate at least sizeof(RematerializedFrame) bytes.
Attached patch always-initialize-value.patch (obsolete) — Splinter Review
Updated patch with the oranges fixed.  Try looks a lot better with this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7904df04c1c5a27c0d2995157062d5e8819ce5d1

The windows build errors don't seem to be mine, and the other oranges seem spurious.  I suspect this is the final patch.  Will check the oranges more closely, but it should be good for review.
Attachment #8926102 - Attachment is obsolete: true
Attachment #8931813 - Flags: review?(jorendorff)
Attachment #8931813 - Flags: review?(jcoppeard)
Comment on attachment 8931813 [details] [diff] [review]
always-initialize-value.patch

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

::: js/src/jit/RematerializedFrame.cpp
@@ +69,5 @@
> +    if (extraSlots > 0)
> +        extraSlots -= 1;
> +
> +    size_t numBytes = sizeof(RematerializedFrame) + (extraSlots * sizeof(Value));
> +    MOZ_ASSERT(numBytes >= sizeof(RematerializedFrame));

Since only the first slot will be initialised by the constructor, would it be simpler to make them UninitialisedValues?
Attachment #8931813 - Flags: review?(jcoppeard) → review+
Attached patch always-iinitialize-value.patch (obsolete) — Splinter Review
I need just the code in dom/bindings reviewed (see js/public/Value.h changes for reference though).  Rest of it is r+-ed already.  This is fixing a security issue where the empty constructor for Value() was default (i.e. uninitialized).

This security issue is one I discovered while trying to change the value boxing format, and triggered a few latent bugs in the code.  I'm fixing it by making the empty-constructor for Value always initialize, and adding a new UninitializedValue to be used in unions.

This patch is just changes around the codebase to accommodate that change.  ErrorResult uses JS::Value in a union, so the dom/bindings code is slightly affected.
Attachment #8931813 - Attachment is obsolete: true
Attachment #8931813 - Flags: review?(jorendorff)
Attachment #8933866 - Flags: review?(bzbarsky)
Comment on attachment 8933866 [details] [diff] [review]
always-iinitialize-value.patch

I assume there's no good way to static_assert that Value and UninitializedValue have the same alignment requirements?

We should probably make the various default ctors layout constexpr, right?

r=me
Attachment #8933866 - Flags: review?(bzbarsky) → review+
We have things like [1] that try to static_assert alignment.

[1] https://searchfox.org/mozilla-central/source/js/src/jsscript.cpp#2628-2633
Yeah I'm asserting the size but not the alignment.  Thanks for the pointer Ted, will use that.

In the meantime, seems like there are still failures in the rust bindgen integration due to the new struct, but that should be easy enough to fix and review separately.
Attached patch bindgen-fix.patch (obsolete) — Splinter Review
Delta patch to address bindgen integration with rust.  Just need a review on those bits where I'm adding InitializedValue to lists-of-things-rust-should-know-about.

Try with all of that looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68fb7ee05e22420d075edd15350bf9f226574392

Should be landable after this r?
Attachment #8935436 - Flags: review?(nfitzgerald)
Comment on attachment 8935436 [details] [diff] [review]
bindgen-fix.patch

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

::: js/rust/build.rs
@@ +233,5 @@
>      "JS::SymbolCode",
>      "JS::TraceKind",
>      "JS::TransferableOwnership",
>      "JS::Value",
> +    "JS::UninitializedValue",

Why is this whitelisted for the js crate but hidden for stylo? If it needs to be hidden to avoid the `alignas` bug in stylo, I'd expect it also needs to be hidden for the js crate.
Attachment #8935436 - Flags: review?(nfitzgerald)
Are we still planning to land this? Thanks
Per IRC discussion with Kannan, this isn't likely to land until closer to the end of Q1.
Comment on attachment 8935436 [details] [diff] [review]
bindgen-fix.patch

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

I didn't realize the stylo bindings were already hiding JS::Value, so it makes sense to also hide JS::UninitializedValue
Attachment #8935436 - Flags: review+
These two patches basically apply as-is from before, and are green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c71cce996e649d6c39e54b5201f98365e2ffec0e

Landing.
As a sec-high this shouldn't land without sec-approval.
Flags: needinfo?(kvijayan)
(In reply to Julien Cristau [:jcristau] from comment #32)
> As a sec-high this shouldn't land without sec-approval.

Apologies.  This was an error on my part, caused mostly by distraction.  Finally found some time to fix up and land this patch after months of being pulled off on other stuff, and I did not fully consider the right course of action.  Where do we go from here?  This patch can be uplifted to beta and release immediately - it should apply clean AFAIK.

Should I ask for sec-approval retroactively for uplift?
Flags: needinfo?(kvijayan) → needinfo?(jcristau)
(In reply to Kannan Vijayan [:djvj] from comment #34)
> 
> Should I ask for sec-approval retroactively for uplift?

Yes.

Now that we're exposed, we should take this on affected branches: Beta, Release (now that we've merged) and ESR52.
Flags: needinfo?(kvijayan)
Yes, please ask for sec-approval so we can consider what branches we need to port this fix to.

Never got clarification on comment 6 so if you could comment on that, too, that would be great. (Is comment 9 saying sec-high was right after all?)
There's uncertainty as to whether this mainfests in the engine directly.  It's been sitting idle in the engine for a very long time but never exposed by fuzzbugs or any testing infrastructure or others.  It only came up because of a related patch where I changed the value format and ran across a default-initialization.

Exploitability is an unknown unknown, but it seems hard, but if it's possible then it's a serious issue.  I'll issue sec-review now.  Let's keep it sec-high.
Flags: needinfo?(kvijayan)
If we really need to land this on release for 59 we still can, but it should be today if possible.
[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I cannot think of an easy way to construct an exploit.  It's been lingering in our codebase for a very long time, and never hit any tests or crashes or fuzzbugs.  But I won't bet on it not being exploitable.


Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Comments don't, but it's pretty easy to figure out what flaw is bing fixed.


Which older supported branches are affected by this flaw?

Every branch that is relevant.  It's an old flaw.


If not all supported branches, which bug introduced the flaw?

I'd have to dig this up, but it would take some tracking.  It's old enough (like early days of the engine old), that it might not really be worth tracking.


Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The patch should apply clean on release and beta.


How likely is this patch to cause regressions; how much testing does it need?

Very unlikely to cause regressions.  Security fix that tightens things up.
Attachment #8933866 - Attachment is obsolete: true
Attachment #8935436 - Attachment is obsolete: true
Attachment #8956583 - Flags: sec-approval?
I'd prefer not to add more risk to delaying the release, but I'll wait for the sec-approval request and risk assessment...
I think you've made a good case that this is really a "sec-moderate" flaw -- we don't have any evidence web content can trigger this at this point. It's a lurking latent issue. Given the immanent release it would be better to let this slide into 60 and not try to push it to beta and release.
Flags: needinfo?(jcristau)
Keywords: sec-highsec-moderate
Comment on attachment 8956583 [details] [diff] [review]
always-initialize-values.patch

retroactive sec-approval for the nightly landing. Based on the information we have we don't need the churn of stuffing it into release at this point (if it were ready last week that'd be another story). We'll still want an ESR version of the patch for next cycle.
Attachment #8956583 - Flags: sec-approval? → sec-approval+
Thank you all for bringing my mistake to my attention so quickly and helping push it upstream.  I'm very disappointed in myself for this slip.  I'll make sure it doesn't happen again.
Group: javascript-core-security → core-security-release
Kannan, it's not super urgent, but when you've got time, we're going to need a rebased patch for ESR52 per comment 42.
Flags: needinfo?(kvijayan)
This is the rebased patch for esr52.  I did a try run earlier, but there seems to be some infrastructure-related failure that's obscuring the results.  Linux looks clean but the other platforms didn't really build at all:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b129d90ab2834e8f19b1af85f2b8d9c9f80b1a9a

Trying again on tip:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e83cee18e93fcf2c08dc397abbb0525d9232a784
Flags: needinfo?(kvijayan)
Thanks to :RyanVM for informing me of the correct way to run try builds against ESR52:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaaeb2e0a4866df476dbb9f760c2dcdc31576265
Can you also request uplift to esr?  (For esr52) Thanks!
Flags: needinfo?(kvijayan)
Comment on attachment 8967467 [details] [diff] [review]
rebase-value-init-for-esr52.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Is a hard-to-exploit but potentially more risky sec-high bug.

User impact if declined: 

Security vulnerability.

Fix Landed on Version:

61

Risk to taking this patch (and alternatives if risky):

Very low.  Passes try on green, and is a pretty minor change in the way we use Javascript Values.
 
String or UUID changes made by this patch: 

N/A
Flags: needinfo?(kvijayan)
Attachment #8967467 - Flags: approval-mozilla-esr52?
Comment on attachment 8967467 [details] [diff] [review]
rebase-value-init-for-esr52.patch

Since we're fixing this in 60 and it may be sec-high in some cases, let's uplift to esr52.
Attachment #8967467 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main60+][adv-esr52.8+]
Flags: qe-verify-
Whiteboard: [adv-main60+][adv-esr52.8+] → [adv-main60+][adv-esr52.8+][post-critsmash-triage]
Depends on: 1463741
Adding an initializing default constructor to JS::Value means JS::Value is now non-trivial, ergo non-POD.  This means making MSVC (and Solaris) can't return JS::Value in a register.

We previously had considered that a somewhat-constraint on how we could implement JS::Value.  Even to this day we have (obsolete) #ifdefs to make the fields of JS::Value public for those compilers, ostensibly to preserve the passed-as-register ABI detail.

It's mildly unfortunate that we made this change without ever specifically considering this side effect, but oh well.  We'd almost certainly have acted exactly the same if we had -- except I'd have something to point at specifically in response to a complaint about the ABI change.  https://mozilla.logbot.info/jsapi/20180622#c14918658

I'll probably remove the #ifdef and comment in a driveby at some point, but past that I very much doubt there's any change we *can* make to return to the old ABI setup.  But if anyone has some flash of inspiration, please file the bug.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.