Remove dangerous public destructor of nsJSArgArray

RESOLVED FIXED in mozilla33

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bjacob, Assigned: Shashank VRSN Sabniveesu, Mentored)

Tracking

Other Branch
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 5 obsolete attachments)

In bug 1028588 we removed dangerous public destructors of XPCOM-refcounted classes outside of a finite whitelist, see HasDangerousPublicDestructor. Now we are going over the entries in this whitelist.

One of them is: nsJSArgArray
Component: JavaScript Engine → DOM
I think that all that is needed for this is:
1. Delete the whole "struct HasDangerousPublicDestructor<nsJSArgArray>"
  http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#3089
2. Turn |ret| into an nsRefPtr<nsJSArgArray> in NS_CreateJSArgv and remove the |delete ret|.
While you are there, delete the null check on ret, because new is infallible now.
Mentor: continuation@gmail.com
Whiteboard: [lang=c++]
(Assignee)

Comment 2

4 years ago
Created attachment 8452821 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray r=mccr8
Attachment #8452821 - Flags: review?(continuation)
Comment on attachment 8452821 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray r=mccr8

I think you forgot to qref or something, as the patch is empty.
Attachment #8452821 - Flags: review?(continuation)
Flags: needinfo?(shashank)
(Assignee)

Comment 4

4 years ago
Created attachment 8453926 [details] [diff] [review]
Fixed part-2 as part-1 seems to be fixed already

Funny that I submitted an empty one! I feel bad for the extra work I caused
Attachment #8452821 - Attachment is obsolete: true
Attachment #8453926 - Flags: review?(continuation)
Flags: needinfo?(shashank)
Comment on attachment 8453926 [details] [diff] [review]
Fixed part-2 as part-1 seems to be fixed already

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

What you have here looks fine aside from the indentation, but you still need to:
- Delete HasDangerousPublicDestructor<nsJSArgArray> (and the forward declaration of nsJSArgArray right before it)
- Make the destructor for nsJSArgArray protected
and of course, make sure it still compiles. :)

::: dom/base/nsJSEnvironment.cpp
@@ +3237,1 @@
>                                         static_cast<JS::Value *>(argv), &rv);

You should fix the indentation here on this line with static_cast.
Attachment #8453926 - Flags: review?(continuation) → feedback+
> I feel bad for the extra work I caused
Don't feel bad, it only took me a few seconds to realize it was empty and then needinfo you. :)
Assignee: nobody → shashank
(Assignee)

Comment 7

4 years ago
Created attachment 8454032 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray r=mccr8

(In reply to Andrew McCreight [:mccr8] from comment #5)
> - Delete HasDangerousPublicDestructor<nsJSArgArray> (and the forward
> declaration of nsJSArgArray right before it)
This is what happens when one forgets to update one's codebase to the latest. Apparently I haven't updated my codebase since Jun 23, when this code was added.

You could have commented on my '(old)Patch Description' itself!

> - Make the destructor for nsJSArgArray protected
> and of course, make sure it still compiles. :)
Done. It compiles!

> ::: dom/base/nsJSEnvironment.cpp
> @@ +3237,1 @@
> >                                         static_cast<JS::Value *>(argv), &rv);
> 
> You should fix the indentation here on this line with static_cast.
I moved the 'static_cast..' part to align with arguments in its previous line
Attachment #8453926 - Attachment is obsolete: true
Attachment #8454032 - Flags: review?(continuation)
Comment on attachment 8454032 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray r=mccr8

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

Looks good to me!  I'll pass this over to Olli to review.  Thanks for the patch.
Attachment #8454032 - Flags: review?(continuation)
Attachment #8454032 - Flags: review?(bugs)
Attachment #8454032 - Flags: feedback+

Comment 9

4 years ago
Comment on attachment 8454032 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray r=mccr8

ret.swap(aArray); or similar would be nice (and then drop ret->QueryInterface), but this is fine too. Not performance critical at all.
Attachment #8454032 - Flags: review?(bugs) → review+
(Assignee)

Comment 10

4 years ago
(In reply to Andrew McCreight [:mccr8] from comment #8)
> Looks good to me!  I'll pass this over to Olli to review.  Thanks for the
> patch.

Thanks for the opportunity & support. I have a doubt. While I was halfway through implementing this and testing [intentionally done :-)], I received this error:

../../dist/include/nsISupportsImpl.h:91:3: error: static assertion failed: Reference-counted class nsJSArgArray should not have a public destructor. Try to make this class's destructor non-public. If that is really not possible, you can whitelist this class by providing a HasDangerousPublicDestructor specialization for it.

What are Reference-counted classes and why can't they have public destructors?
[I'm reading https://en.wikipedia.org/wiki/Reference_counting but would like to know from you; If this is not the place to ask, warn me to use IRC :) ]
Flags: needinfo?(continuation)
(Assignee)

Comment 11

4 years ago
(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8454032 [details] [diff] [review]
> Bug 1034910 - Remove dangerous public destructor of nsJSArgArray r=mccr8
> 
> ret.swap(aArray); or similar would be nice (and then drop
> ret->QueryInterface), but this is fine too. Not performance critical at all.

Do you mean to change - 
'return ret->QueryInterface(NS_GET_IID(nsIArray), (void **)aArray); '
to

'return ret.swap((void **)aArray);' ?
If yes, I'd do that now!
Flags: needinfo?(bugs)
(void**) shouldn't be needed.
Flags: needinfo?(bugs)
> What are Reference-counted classes and why can't they have public destructors?

A reference counted class does not really use the normal fully manually memory management of C++.

Normally in C++, you'd say something like
Foo* x = new Foo();
// use x
// later, you know for sure you are done with x, so you destroy it.
delete x;

With a reference counted object, you aren't not as sure when it will go away, so you just have various places claiming that they hold the object alive.  They do this by calling an Addref() method (often hidden inside the nsRefPtr<> class) when they want to hold the object alive (this increases the reference count), and calling Release() method when they are done with it (this decreases the reference count).  If the reference count becomes 0 in Release(), then we delete the object.

The problem is that if the destructor is public, then any code can call delete on the object, even if the refcount is greater than 0.  This will cause any pointers to the object to end up pointing to dead objects, which can cause security problems when another object is allocated in the same space.

The purpose of this bug, and the other bugs blocking bug 1028132, is to make it less likely that some random code will delete an object with a nonzero reference count, by making the destructor protected.  There is some fancy C++ template stuff I don't know much about that enforces that, and that HasDangerousPublicDestructor<nsJSArgArray> class you deleted tells it to not produce an error for that.
Flags: needinfo?(continuation)
(Assignee)

Comment 14

4 years ago
Created attachment 8454070 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray; simplify the return statement of NS_CreateJSArgv r=smaug

Simplified the return statement of NS_CreateJSArgv
Attachment #8454032 - Attachment is obsolete: true
Attachment #8454070 - Flags: review?(bugs)
(Assignee)

Comment 15

4 years ago
(In reply to Andrew McCreight [:mccr8] from comment #13)
> > What are Reference-counted classes and why can't they have public destructors?
> 
> A reference counted class does not really use the normal fully manually
> memory management of C++.
> 
> Normally in C++, you'd say something like
> ...

Woww! Well explained. That is very helpful! Now that I get an idea of what's happening with my changes, I'd look at bugs in 'Depends on' of BUG 1028132! Thank you very much mccr8!!
Comment on attachment 8454070 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray; simplify the return statement of NS_CreateJSArgv r=smaug

Did you compile this?
swap() doesn't return anything, so I don't quite see
how return ret.swap(aArray) could work.
I guess you're just missing return NS_OK;

And use nsCOMPtr for an interface, not nsRefPtr.
Attachment #8454070 - Flags: review?(bugs)
(Assignee)

Comment 17

4 years ago
Created attachment 8454093 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray; change type of 'ret' r=smaug
Attachment #8454070 - Attachment is obsolete: true
Attachment #8454093 - Flags: review?(bugs)
Comment on attachment 8454093 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray; change type of 'ret' r=smaug

Thanks
Attachment #8454093 - Flags: review?(bugs) → review+
(In reply to Olli Pettay from comment #9)
> ret.swap(aArray); or similar would be nice

Please only use swap for true swaps! For setting outparams, ret.forget(aArray) is better (and also allows the outparam to be a base class of the smart pointer, although that's not necessary in this case).
I'll change it to .forget() and do a try run today.
Flags: needinfo?(continuation)
Blocks: 1037510
No longer blocks: 1037510
try run: https://tbpl.mozilla.org/?tree=Try&rev=f136c8fe434e
Flags: needinfo?(continuation)
Created attachment 8454649 [details] [diff] [review]
Make the dtor of nsJSArgArray protected.

I changed the swap to a forget. Carrying forward smaug's r+.
Attachment #8454093 - Attachment is obsolete: true
Attachment #8454649 - Flags: review+
Keywords: checkin-needed

Updated

4 years ago
Blocks: 1037772
No longer blocks: 1037772
https://hg.mozilla.org/mozilla-central/rev/7c366c305105
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.