541790 - Too early check for rt->gcKeepAtoms in js_GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

This is a spin-off of the bug 538275 comment 34.

js_GC queries rt->gcKeepAtoms before it suspends all running requests. As such it may query the flag before another thread set it and then suspend its current requests, perhaps via calling the last-ditch GC or via calling js_InvokeOperationCallback.

In theory this could be exploited with thread-workers to circumvent the GC safety. The worker can, for example, constantly run Object.prototype.toSource or other code that relies rt->gcKeepAtoms for GC safety and that periodically calls js_InvokeOperationCallback. In the mean time the main thread can try to trigger the last-ditch GC. But it is highly non-trivial to build a reliable exploit based on that.

Comment 1

[Igor Bukanov]

Attachment: fix v1

The fix moves keepAtoms initialization after we know that all other requests are suspended. The patch also moved purge-related calls before the restart label. The purpose of the restart is to collect more garbage released during js_RemoveRoot and friends called from the finalizers, it cannot affect js_PurgeThreads or rt->builtinFunctions.

Comment 2

[Igor Bukanov]

Attachment: fix v2

There is another problem related to keepAtoms. If the last ditch GC waits for a full GC running on another thread, that full GC would collect atoms violating the assumptions about the last-ditch GC.

The new patch fixes that via calling JS_(KEEP|UNKEEP)_ATOMS around the waiting code.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 423231 [details] [diff] [review]
fix v2

Thanks for fixing this.

Comment 4

[Igor Bukanov]

https://hg.mozilla.org/tracemonkey/rev/00b8d5937a94

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like this landed on mozilla-central yesterday:
https://hg.mozilla.org/mozilla-central/rev/00b8d5937a94

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Marking this as fixed based on above comment to get it off the alpha blocker list.