Closed Bug 1153382 Opened 5 years ago Closed 4 years ago

Make heap-poisoned Values more likely to crash when accessed

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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 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+
(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.
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
(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)
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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: 1171904
This got done. I'm not sure why the bug isn't closed.
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.