Closed Bug 1127246 Opened 5 years ago Closed 5 years ago

baseShapes table is not updated after generational GC


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox35 --- wontfix
firefox36 + fixed
firefox37 + fixed
firefox38 + fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- fixed
b2g-master --- fixed


(Reporter: jonco, Assigned: jonco)



(Keywords: sec-high, Whiteboard: [adv-main36+])


(3 files, 2 obsolete files)

The baseShapes table is keyed based on parent and metadata objects pointers, both of which may be in the nursery.  It needs a postbarrier to rekey entries if these objects are moved on generational GC.

Forking this issue from bug 1124563.
Attached patch check-base-shape-table (obsolete) — Splinter Review
Add a check to see if the contents of the baseShapes table are valid.  This causes our tests to fail if we enable the checks with zeal mode 13.
Attachment #8556479 - Flags: review?(terrence)
Attached patch post-barrier-base-shapes-table (obsolete) — Splinter Review
I had an idea to mark generic barriers first to get rid of the problem where hashtable barriers see an inconsistent view of, where the table's keys have are based on original object locations but their values contain object pointers that have already been updated by other barriers.

Sadly this doesn't work, because moving the entry in the generic barrier means that the barriers for any pointers in the value now have the wrong address.

I've just fixed this in the same way we use for other barriers, by giving the lookup structure the ability to match an entry for which the key hash and key value contents are based on prior and updated object pointers respectively.
Attachment #8556481 - Flags: review?(terrence)
This issue has been present since we enabled GGC in bug 619558.
Keywords: sec-high
This sounds like it could result in dead pointers, though it also sounds difficult to exploit.
Attachment #8556479 - Flags: review?(terrence) → review+
Comment on attachment 8556481 [details] [diff] [review]

Review of attachment 8556481 [details] [diff] [review]:


::: js/public/HashTable.h
@@ +252,5 @@
>          if (old_key != new_key)
>              rekeyAs(old_key, new_key, new_key);
>      }
>      // Infallibly rekey one entry, if present.

Please extend the comment to explain the bool being returned.

@@ +473,5 @@
>          if (old_value != new_value)
>              rekeyAs(old_value, new_value, new_value);
>      }
>      // Infallibly rekey one entry, if present.

Attachment #8556481 - Flags: review?(terrence) → review+
Combined patch with review comments addressed.
Attachment #8556479 - Attachment is obsolete: true
Attachment #8556481 - Attachment is obsolete: true
Attachment #8557038 - Flags: review+
Comment on attachment 8557038 [details] [diff] [review]

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Very hard.

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


Which older supported branches are affected by this flaw?

Everything back to FF32.

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

Bug 619558.

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

This patch should apply directly or with minimal changes.

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

Unlikely to cause regressions.
Attachment #8557038 - Flags: sec-approval?
sec-approval+ for trunk. I'd like to see Aurora and Beta patches nominated and then checked in as well to get it in the next beta.
Attachment #8557038 - Flags: sec-approval? → sec-approval+
Jon, could you land and fill the uplift requests so that we have it in beta asap? Thanks
Flags: needinfo?(jcoppeard)
Attachment #8557906 - Flags: review+
Rebased patch for mozilla-beta, carrying r+.
Attachment #8557907 - Flags: review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Jon, sorry to bother you again but we would also need the uplift request, thanks!
Flags: needinfo?(jcoppeard)
Comment on attachment 8557906 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: Bug 619558.
[User impact if declined]: Possible stability/security issue.
[Describe test coverage new/current, TreeHerder]: On m-c for 24 hours.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8557906 - Flags: approval-mozilla-aurora?
Comment on attachment 8557907 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: Bug 619558.
[User impact if declined]: Possible stability/security issue.
[Describe test coverage new/current, TreeHerder]: On m-c for 24 hours.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.
Attachment #8557907 - Flags: approval-mozilla-beta?
Comment on attachment 8557907 [details] [diff] [review]

Attachment #8557907 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8557906 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [adv-main36+]
Group: 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.