GC: properly barrier the JSON stringifier cycle detection set

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 768686 [details] [diff] [review]
v0

Christian found a non-reducable testcase when fuzzing GGC that stems from lack of a post barrier on the JSON stringifier's cycle detection HashSet. The specific GGC hazard here was triggered through a minor GC through the recursion check. I don't see any reason that a normal full GC would be prevented here, so I think this is an incremental GC hazard as well, thus the sec-sensitive flag. I'll let Bill comment further on whether this is an actual issue and, if so, how severe it is.
Attachment #768686 - Flags: review?(wmccloskey)
Blocks: 676763
The pre-barrier is not needed here. Only data structures that are traced by the GC need pre-barriers, and this one never gets traced. (The stack doesn't count because it's traced entirely in the first slice.)

Why did you split the HashTable barrier code into separate pieces for HashSet and HashMap? Is that needed for this patch?

Also, I don't totally understand why this would cause a failure. As far as I can tell, we never dereference the objects in this HashSet. Was it a crash you were seeing, or a JS error? I could understand if we were incorrectly getting JSMSG_CYCLIC_VALUE. I don't see how we would get a crash, though.
Group: core-security
(Assignee)

Comment 2

5 years ago
(In reply to Bill McCloskey (:billm) [away until July 1] from comment #1)
> The pre-barrier is not needed here. Only data structures that are traced by
> the GC need pre-barriers, and this one never gets traced. (The stack doesn't
> count because it's traced entirely in the first slice.)

Ah, it was not clear to me that this structure is traced through MarkRuntime.
 
> Why did you split the HashTable barrier code into separate pieces for
> HashSet and HashMap? Is that needed for this patch?

Yes. The HashMap needs to know the map's Value type so that it can put the value on the stack while we change the key. Now that Jon has added a rekey method to HashMap, I think we can get around this. I'll get a new version up with this change.

> Also, I don't totally understand why this would cause a failure. As far as I
> can tell, we never dereference the objects in this HashSet. Was it a crash
> you were seeing, or a JS error? I could understand if we were incorrectly
> getting JSMSG_CYCLIC_VALUE. I don't see how we would get a crash, though.

We were hitting an assertion in debug builds. We move some of the items the set is referring to in the middle of the recursion. The removals do not find the objects in the table and we assert at the end because the table is not empty.
(Assignee)

Comment 3

5 years ago
Created attachment 770917 [details] [diff] [review]
v1

Sorry for the lack of context on the previous patch. The reworked patch is vastly simpler. This new patch:

  - Simplifies and makes HashTableKeyRef fully generic using the new HashTable::rekey method. This includes implementing rekey for HashSet: a direct copy of HashMap::rekey.

  - Improves the documentation on HashKeyRef and HashTablePostWriteBarrier.

  - Removes IsRelocatableHeapType. If it didn't work to catch this bug, it is totally useless.

  - Replaces the duplicate CycleDetector class in json.cpp with the common and well-tested one in jscntxt.h. Now that this uses the rooting from AutoCycleDetector, the original crash is gone without having to add any new rooting primitives.
Attachment #768686 - Attachment is obsolete: true
Attachment #768686 - Flags: review?(wmccloskey)
Attachment #770917 - Flags: review?(wmccloskey)
Attachment #770917 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/7813225999af
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

3 years ago
Depends on: 1197097
You need to log in before you can comment on or make changes to this bug.