1034910 - Remove dangerous public destructor of nsJSArgArray

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Shashank VRSN Sabniveesu]

Attachment: Bug 1034910 - Remove dangerous public destructor of nsJSArgArray r=mccr8

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Shashank VRSN Sabniveesu]

Attachment: 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

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Andrew McCreight [:mccr8]]

> 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. :)

Comment 7

[Shashank VRSN Sabniveesu]

Attachment: 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

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Shashank VRSN Sabniveesu]

(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 :) ]

Comment 11

[Shashank VRSN Sabniveesu]

(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!

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(void**) shouldn't be needed.

Comment 13

[Andrew McCreight [:mccr8]]

> 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.

Comment 14

[Shashank VRSN Sabniveesu]

Attachment: Bug 1034910 - Remove dangerous public destructor of nsJSArgArray; simplify the return statement of NS_CreateJSArgv r=smaug

Simplified the return statement of NS_CreateJSArgv

Comment 15

[Shashank VRSN Sabniveesu]

(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 16

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 17

[Shashank VRSN Sabniveesu]

Attachment: Bug 1034910 - Remove dangerous public destructor of nsJSArgArray; change type of 'ret' r=smaug

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8454093 [details] [diff] [review]
Bug 1034910 - Remove dangerous public destructor of nsJSArgArray; change type of 'ret' r=smaug

Thanks

Comment 19

[[e-mail]]

(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).

Comment 20

[Andrew McCreight [:mccr8]]

I'll change it to .forget() and do a try run today.

Comment 21

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=f136c8fe434e

Comment 22

[Andrew McCreight [:mccr8]]

Attachment: Make the dtor of nsJSArgArray protected.

I changed the swap to a forget. Carrying forward smaug's r+.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c366c305105

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c366c305105