Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Possibly missing UnmarkGray in a few places

RESOLVED FIXED in Firefox 13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(firefox13+ fixed, firefox-esr10- wontfix)

Details

(Whiteboard: [sg:low][qa-][advisory-tracking+])

Attachments

(4 attachments)

1.07 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
7.06 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
1.13 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
861 bytes, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
I'm not sure if these are correct, but it seems like we could be missing UnmarkGray calls in a few places.
(Assignee)

Comment 1

6 years ago
Created attachment 586560 [details] [diff] [review]
fix mPrototypeNoHelper

The concern here is that someone could cause get a reference to a scope's mPrototypeNoHelper. Then it could stash the reference in a DOM object or something and relinquish all other references, cause the mPrototypeNoHelper object to be marked gray. Later, they could cause an object to be created with mPrototypeNoHelper as its proto. This would lead to a black -> gray pointer (via the proto edge), which violates our invariants.
Attachment #586560 - Flags: review?(bent.mozilla)
(Assignee)

Comment 2

6 years ago
Created attachment 586568 [details] [diff] [review]
fix tearoffs

This patch is questionable. In theory, if a tear-off JS object could become gray while the XPC wrapped native's JS object was black, then the tear-off's JS object could be exposed to JS code via DefinePropertyIfFound's to->GetJSObject() call.

However, as far as I can tell from the code, once a tear-off's JS object has been created, it always has a strong reference from the wrapped native's flattened object (via property that it set during the resolve hook). However, if that's true, then I don't understand the purpose of all this tear-off sweeping code. Why don't we just keep the tear-offs alive as long as the WrappedNative is alive?
Attachment #586568 - Flags: review?(bent.mozilla)
(Assignee)

Comment 3

6 years ago
Created attachment 586570 [details] [diff] [review]
fix waiver wrappers

Same story. It seems like we could add a waiver wrapper to the map, then it could end up gray if it's stored in a DOM object, and then we could look it up in the map again and return it to JS code.

However, I have no idea what a waiver wrapper is, so I may be wrong.
Attachment #586570 - Flags: review?(bent.mozilla)
guessing at a moderate security severity due to the uncertainty indicated by "possibly" in the summary.
Whiteboard: [sg:moderate]
Attachment #586560 - Flags: review?(bent.mozilla) → review+
Comment on attachment 586568 [details] [diff] [review]
fix tearoffs

This looks good!
Attachment #586568 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 595499 [details] [diff] [review]
fix the DOM prototype cache

Another weak pointer that I just saw.
Attachment #595499 - Flags: review?(bent.mozilla)
Attachment #586570 - Flags: review?(bent.mozilla) → review+
Attachment #595499 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1300d282fd22
Target Milestone: --- → mozilla13

Comment 8

6 years ago
From philor:
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/71f5bf4df2f6 - one of the six in that push was crashing in js::gc::Mark<JSString>
Target Milestone: mozilla13 → ---
(Assignee)

Comment 9

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8246358d235f
Target Milestone: --- → mozilla13
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/mozilla-central/rev/8246358d235f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
status-firefox13: --- → fixed
Is this something we ought to take on Firefox 12 or ESR?
tracking-firefox-esr10: --- → ?
tracking-firefox13: --- → +
(Assignee)

Comment 12

5 years ago
(In reply to Daniel Veditz [:dveditz] from comment #11)
> Is this something we ought to take on Firefox 12 or ESR?

We now know that there are a lot more bugs like this. As far as we know, they'll only lead to a NULL pointer crash. However, we've never connected one of these bugs to an actual crash, so it's all pretty theoretical. I don't think there's much reason to take this on branches.
lowering the severity based on comment 12.
status-firefox-esr10: --- → wontfix
tracking-firefox-esr10: ? → -
Whiteboard: [sg:moderate] → [sg:low]
Whiteboard: [sg:low] → [sg:low][qa-]
Whiteboard: [sg:low][qa-] → [sg:low][qa-][advisory-tracking+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.