Make heap-poisoned Values more likely to crash when accessed

RESOLVED FIXED in Firefox 41

Status

()

Core
JavaScript: GC
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox41 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8591002 [details] [diff] [review]
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.
Attachment #8591002 - Flags: review?(sphink)
Attachment #8591002 - Flags: feedback?(efaustbmo)

Comment 1

3 years ago
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!
Attachment #8591002 - Flags: feedback?(efaustbmo) → feedback+
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?
Attachment #8591002 - Flags: review?(sphink) → review+
(Assignee)

Comment 3

3 years ago
(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.
(Assignee)

Comment 4

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83e6c7fde463
https://hg.mozilla.org/integration/mozilla-inbound/rev/42ed856b37d9
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.
Flags: needinfo?(terrence)
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
(Assignee)

Comment 7

3 years ago
(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.
Flags: needinfo?(terrence)

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/012638ffaacc
(Assignee)

Comment 9

2 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/012638ffaacc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 11

2 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1171430
Depends on: 1171904
(Assignee)

Comment 12

a year ago
This got done. I'm not sure why the bug isn't closed.
Status: REOPENED → RESOLVED
Last Resolved: 2 years agoa year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.