1153382 - Make heap-poisoned Values more likely to crash when accessed

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: make_poison_crash_when_used_as_a_value-v0.diff

Our current poisoning scheme does make Values invalid, however, we frequently only access this is Value::isMarkable, particularly when marking. Unfortunately, Value::isMarking happens to be false, at least with our Nursery poisoning pattern, so we will silently skip over poisoned Slots in this case. Instead, we should poison at the value level by ensuring Value::isObject is true, but with an invalid pointer, so that subsequent code will crash.

Comment 1

[Eric Faust [:efaust]]

Comment on attachment 8591002 [details] [diff] [review]
make_poison_crash_when_used_as_a_value-v0.diff

I can confirm that this crashes immediately while tracing the uninitialized slots that were previously seen as doubles. Thanks so much for doing this!

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8591002 [details] [diff] [review]
make_poison_crash_when_used_as_a_value-v0.diff

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

::: js/src/jsutil.h
@@ +304,5 @@
>          char* env = getenv("JSGC_DISABLE_POISONING");
>          if (env)
>              poison = false;
>          inited = true;
>      }

I wonder if this whole thing could be replaced with

  static bool poison = getenv("JSGC_DISABLE_POISONING");

Ah, who cares.

@@ +309,5 @@
>  
> +    if (poison) {
> +        // Without a valid Value tag, a poisoned Value may look like a valid
> +        // floating point number. To ensure that we crash more readily when
> +        // observing a poised Value, we make the poison an invalid ObjectValue.

*poisoned

@@ +315,5 @@
> +        memset(&obj, value, sizeof(obj));
> +#if defined(JS_PUNBOX64)
> +        obj >>= JSVAL_TAG_SHIFT;
> +#endif
> +        jsval_layout layout = OBJECT_TO_JSVAL_IMPL((JSObject*)obj);

reinterpret_cast<>

ObjectOrNullValue(reinterpret_cast<JSObject*>(obj)) doesn't work?

Comment 3

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink, :s:] from comment #2)
> Comment on attachment 8591002 [details] [diff] [review]
> make_poison_crash_when_used_as_a_value-v0.diff
> 
> Review of attachment 8591002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsutil.h
> @@ +304,5 @@
> >          char* env = getenv("JSGC_DISABLE_POISONING");
> >          if (env)
> >              poison = false;
> >          inited = true;
> >      }
> 
> I wonder if this whole thing could be replaced with
> 
>   static bool poison = getenv("JSGC_DISABLE_POISONING");
> 
> Ah, who cares.

Yeah, I'll clean this up while I'm in the area.

> @@ +309,5 @@
> >  
> > +    if (poison) {
> > +        // Without a valid Value tag, a poisoned Value may look like a valid
> > +        // floating point number. To ensure that we crash more readily when
> > +        // observing a poised Value, we make the poison an invalid ObjectValue.
> 
> *poisoned

That may explain why I didn't see any crashes. :-(

> @@ +315,5 @@
> > +        memset(&obj, value, sizeof(obj));
> > +#if defined(JS_PUNBOX64)
> > +        obj >>= JSVAL_TAG_SHIFT;
> > +#endif
> > +        jsval_layout layout = OBJECT_TO_JSVAL_IMPL((JSObject*)obj);
> 
> reinterpret_cast<>
> 
> ObjectOrNullValue(reinterpret_cast<JSObject*>(obj)) doesn't work?

No, this just dispatches to ObjectValue |if (obj)|, so does *obj.

Comment 4

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83e6c7fde463
https://hg.mozilla.org/integration/mozilla-inbound/rev/42ed856b37d9

Comment 5

[Andrew McCreight [:mccr8]]

dmajor pointed out in IRC that it looks like your environment checking logic is reversed, eg it only enables poisoning when DISABLE_POISONING is set.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ef1b55f785 to see if it possibly fixes a suddenly-near-permafailing mochitest-e10s-bc1 orange that popped up around the time this landed.

https://treeherder.mozilla.org/logviewer.html#?job_id=8910719&repo=mozilla-inbound

[16:44]	terrence	KWierso: I need to push a fixup anyway, so feel free to backout if you want to check directly

Comment 7

[Terrence Cole [:terrence]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> dmajor pointed out in IRC that it looks like your environment checking logic
> is reversed, eg it only enables poisoning when DISABLE_POISONING is set.

Yup, missed the negation.

"Luckily", disabling poisoning results in a reliable bc1 timeout, so there is that.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/012638ffaacc

Comment 9

[Terrence Cole [:terrence]]

I ran ~10 bc1 with and without poisoning and did not see a significant difference in their (startlingly large) timeout rate. So, relanding, I guess.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/012638ffaacc

Comment 11

[Terrence Cole [:terrence]]

This appears to have regressed builds with js-diagnostics enabled. I think I'd like to let this sit a week and see if this makes any crashes reliable, then mask the new code as #ifdef DEBUG so that diagnostics builds get the prior behavior and only debug builds get the better assertions.

Comment 12

[Terrence Cole [:terrence]]

This got done. I'm not sure why the bug isn't closed.