Destructor bodies can be treated as empty

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript: GC
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

(Blocks: 1 bug, {sec-audit})

unspecified
mozilla48
sec-audit
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main48-])

Attachments

(2 attachments, 1 obsolete attachment)

Sadly, when I "implemented" MUST_NOT_THROW_EXPR, I thought it was just an ignorable annotation. It turns out that it's a *wrapper* around the body of a method, and at least in gcc 4.9 it's used for destructors (presumably destructors that cannot throw?)

Which means that all such destructors are treated as having empty bodies, and therefore will never GC.

This is obviously not correct.

I don't think there should be too many hazards exposed by this, because I think this should only have been in effect since upgrading gcc, but I'm going to mark this s-s for now.
Created attachment 8734927 [details] [diff] [review]
Upgrade sixgill to fix destructor handling

Update to version of sixgill that pays attention to the body.
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Created attachment 8734929 [details] [diff] [review]
Handle MUST_NOT_THROW_EXPR correctly (it is just a wrapper around the real expr)
Created attachment 8734930 [details] [diff] [review]
Handle MUST_NOT_THROW_EXPR correctly (it is just a wrapper around the real expr)
Attachment #8734929 - Attachment is obsolete: true
Comment on attachment 8734930 [details] [diff] [review]
Handle MUST_NOT_THROW_EXPR correctly (it is just a wrapper around the real expr)

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

Argh. Typed |hg bzexport| when I intended |hg export|. And yet, it was better than what I was trying to do. Oddly, bugzilla barfed on a direct bzexport, which was why I was doing the 2-step:

error uploading attachment: REST error on POST to https://bugzilla.mozilla.org/rest/bug/1259843/attachment: You asked Terrence Cole [:terrence] <terrence@mozilla.com> for review on bug 1259843, attachment 8734928, but that bug has been restricted to users in certain groups, and the user you asked isn't in all the groups to which the bug has been restricted. Please choose someone else to ask, or make the bug accessible to users on its CC: list and add that user to the list.
Attachment #8734930 - Flags: review?(terrence)
Huh. Same error with the web interface. Adding :terrence to CC list "fixed" it.

I tried switching it from general s-s to JS s-s, but it won't let me remove general s-s.
Group: core-security → javascript-core-security
Attachment #8734930 - Flags: review?(terrence) → review+
Keywords: sec-audit
https://hg.mozilla.org/mozilla-central/rev/719ded1e521d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Group: javascript-core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main48-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.