1127246 - baseShapes table is not updated after generational GC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

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.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: check-base-shape-table

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.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: post-barrier-base-shapes-table

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.

Comment 3

[Jon Coppeard (:jonco)]

This issue has been present since we enabled GGC in bug 619558.

Comment 4

[Andrew McCreight [:mccr8]]

This sounds like it could result in dead pointers, though it also sounds difficult to exploit.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8556481 [details] [diff] [review]
post-barrier-base-shapes-table

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

Nice!

::: 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.

Ditto.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1127246-post-barrier-base-shapes-table

Combined patch with review comments addressed.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8557038 [details] [diff] [review]
bug1127246-post-barrier-base-shapes-table

[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?

No.

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.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Jon, could you land and fill the uplift requests so that we have it in beta asap? Thanks

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a00fff536eef

Comment 11

[Jon Coppeard (:jonco)]

Attachment: baseShapes-patch-aurora

Comment 12

[Jon Coppeard (:jonco)]

Attachment: baseShapes-patch-beta

Rebased patch for mozilla-beta, carrying r+.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a00fff536eef

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Jon, sorry to bother you again but we would also need the uplift request, thanks!

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 8557906 [details] [diff] [review]
baseShapes-patch-aurora

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.

Comment 16

[Jon Coppeard (:jonco)]

Comment on attachment 8557907 [details] [diff] [review]
baseShapes-patch-beta

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.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8557907 [details] [diff] [review]
baseShapes-patch-beta

Thanks

Comment 18

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b4e61581f650
https://hg.mozilla.org/releases/mozilla-beta/rev/e0a36c5bdf4c

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b4e61581f650