Closed Bug 1155500 Opened 9 years ago Closed 9 years ago

Potentially dangerous allowed use of nsAutoPtr (would lead to crash)

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jya, Assigned: jya)

Details

Attachments

(1 file, 2 obsolete files)

Example:

class U { };
class T : public U { };

nsAutoPtr<U> foo = new U();
nsAutoPtr<T> bar = new T();

foo = bar;

This will lead to foo taking ownership of T*, yet nsAutoPtr<U> destructor will be deleted it; leading foo to keep a dangling, now deleted object.
This prevents use of nsAutoPtr which in its current form would lead to crashes
Attachment #8593759 - Flags: review?(nfroyd)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
What part1 prevents is in fact perfectly acceptabe (and wanted) for solving the issue describe in this bug. type I may be related to T, and as such should be able to use nsRefPtr<T> to store the pointer, or in my actual use case receive the result of a returned function. If T and I aren't related, we *will* get a compilation error as you wouldn't be able to store I* into mRawPtr of type T*
Attachment #8593765 - Flags: review?(nfroyd)
Comment on attachment 8593759 [details] [diff] [review]
Part1. Avoid misuse of AutoPtr when copying related objects

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

I guess I approve of making nsAutoPtr slightly more robust.  But why aren't you using mozilla::UniquePtr instead?

::: xpcom/base/nsAutoPtr.h
@@ +90,5 @@
>  
>    // This constructor shouldn't exist; we should just use the &&
>    // constructor.
> +  template <typename I>
> +  nsAutoPtr(nsAutoPtr<I>& aSmartPtr)

Making this a template doesn't do the right thing, because there's now no copy constructor for nsAutoPtr, which means the compiler auto-generates one, which leads to even more problems than nsAutoPtr already has.  You need to add a templated constructor here in addition to the copy constructor that already exists.  (This rule is [class.copy]p2 or thereabouts if you have a copy of the standard at hand; the copy constructor cannot be a template function.)

@@ +95,4 @@
>      : mRawPtr(aSmartPtr.forget())
>      // Construct by transferring ownership from another smart pointer.
>    {
> +    static_assert(mozilla::IsSame<T, I>::value, "This is bad");

I don't see a lot of point to adding these asserts in part 1 only to remove them in part 2.

@@ +99,4 @@
>    }
>  
> +  template <typename I>
> +  nsAutoPtr(nsAutoPtr<I>&& aSmartPtr)

Same comment from the copy constructor case applies here.  (This is [class.copy]p3.)

@@ +116,5 @@
>      return *this;
>    }
>  
> +  template <typename I>
> +  nsAutoPtr<T>& operator=(nsAutoPtr<I>& aRhs)

Same comment from the copy constructor case applies here.  (This is [class.copy]p17.)

@@ +125,5 @@
>      return *this;
>    }
>  
> +  template <typename I>
> +  nsAutoPtr<T>& operator=(nsAutoPtr<I>&& aRhs)

Same comment from the copy constructor case applies here.  (This is [class.copy]p19.)
Attachment #8593759 - Flags: review?(nfroyd)
Comment on attachment 8593765 [details] [diff] [review]
Part2. Avoid misuse of AutoPtr when copying related objects

I think this should just be folded into part 1.
Attachment #8593765 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3)
> Comment on attachment 8593759 [details] [diff] [review]
> Part1. Avoid misuse of AutoPtr when copying related objects
> 
> Review of attachment 8593759 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess I approve of making nsAutoPtr slightly more robust.  But why aren't
> you using mozilla::UniquePtr instead?

Because I didn't know about it :)

The idea is that nsAutoPtr should have the same behaviour than nsRefPtr in their copy constructor: transfer ownership.

> 
> ::: xpcom/base/nsAutoPtr.h
> @@ +90,5 @@
> >  
> >    // This constructor shouldn't exist; we should just use the &&
> >    // constructor.
> > +  template <typename I>
> > +  nsAutoPtr(nsAutoPtr<I>& aSmartPtr)
> 
> Making this a template doesn't do the right thing, because there's now no
> copy constructor for nsAutoPtr, which means the compiler auto-generates one,
> which leads to even more problems than nsAutoPtr already has.  You need to
> add a templated constructor here in addition to the copy constructor that
> already exists.  (This rule is [class.copy]p2 or thereabouts if you have a
> copy of the standard at hand; the copy constructor cannot be a template
> function.)

?? Not sure I follow.
There's no change to copy constructor here, as you have I == T.


I did experience a particular behaviour that I couldn't explain. Originally I added the extra templated copy constructor, with a static_assert(false). My aim at first was just to see if any of those existed in the code, and cause a compilation error.

This caused an error for *every* use of nsAutoPtr<T> blah = nsAutoPtr<T> foo!
Hence why I made it all with the extra I template.

No one around me could explain that behaviour .

> 
> @@ +95,4 @@
> >      : mRawPtr(aSmartPtr.forget())
> >      // Construct by transferring ownership from another smart pointer.
> >    {
> > +    static_assert(mozilla::IsSame<T, I>::value, "This is bad");
> 
> I don't see a lot of point to adding these asserts in part 1 only to remove
> them in part 2.

For the case where you thought that we shouldn't allow transfer from one nsAutoPtr to another.
Better have a compilation failure than a crash (which I had when I attempted to do what I mentioned in this bug description).

So part2 was a nice to have only...

> 
> @@ +99,4 @@
> >    }
> >  
> > +  template <typename I>
> > +  nsAutoPtr(nsAutoPtr<I>&& aSmartPtr)
> 
> Same comment from the copy constructor case applies here.  (This is
> [class.copy]p3.)

Not sure what you mean by that, but I'll investigate.

BTW what I did here is the same as what nsRefPtr does... So maybe we should address your concerns there too.
(In reply to Jean-Yves Avenard [:jya] from comment #5)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3)
> > I guess I approve of making nsAutoPtr slightly more robust.  But why aren't
> > you using mozilla::UniquePtr instead?
> 
> Because I didn't know about it :)

Ah!  Please use that instead. :)

> The idea is that nsAutoPtr should have the same behaviour than nsRefPtr in
> their copy constructor: transfer ownership.

I don't follow this statement; nsRefPtr's copy constructor doesn't transfer ownership, it shares ownership.  The newly-constructed nsRefPtr AddRefs() the underlying pointer, but the original nsRefPtr doesn't Release() the underlying pointer.

> > ::: xpcom/base/nsAutoPtr.h
> > @@ +90,5 @@
> > >  
> > >    // This constructor shouldn't exist; we should just use the &&
> > >    // constructor.
> > > +  template <typename I>
> > > +  nsAutoPtr(nsAutoPtr<I>& aSmartPtr)
> > 
> > Making this a template doesn't do the right thing, because there's now no
> > copy constructor for nsAutoPtr, which means the compiler auto-generates one,
> > which leads to even more problems than nsAutoPtr already has.  You need to
> > add a templated constructor here in addition to the copy constructor that
> > already exists.  (This rule is [class.copy]p2 or thereabouts if you have a
> > copy of the standard at hand; the copy constructor cannot be a template
> > function.)
> 
> ?? Not sure I follow.
> There's no change to copy constructor here, as you have I == T.

There is.  The compiler won't instantiate this template for I == T, it will look for a non-template copy constructor.  And since there no non-template copy constructor, it will use the default one provided by the compiler, which doesn't do what you want.

> I did experience a particular behaviour that I couldn't explain. Originally
> I added the extra templated copy constructor, with a static_assert(false).
> My aim at first was just to see if any of those existed in the code, and
> cause a compilation error.
> 
> This caused an error for *every* use of nsAutoPtr<T> blah = nsAutoPtr<T> foo!
> Hence why I made it all with the extra I template.
> 
> No one around me could explain that behaviour .

That does seem really odd...the static_assert shouldn't fire until the template is instatiated, and the template shouldn't be instantiated for the ordinary T = T case...I guess I'd have to see the code that you used.

> > @@ +99,4 @@
> > >    }
> > >  
> > > +  template <typename I>
> > > +  nsAutoPtr(nsAutoPtr<I>&& aSmartPtr)
> > 
> > Same comment from the copy constructor case applies here.  (This is
> > [class.copy]p3.)
> 
> Not sure what you mean by that, but I'll investigate.

I meant that you need both non-template and template versions:

  nsAutoPtr(nsAutoPtr&&);
  template<typename I> nsAutoPtr(nsAutoPtr<I>&&);

and so on for the operator= overloads too.

> BTW what I did here is the same as what nsRefPtr does... So maybe we should
> address your concerns there too.

We already did address the concerns in nsRefPtr when similar changes were made there; that's how I learned about this issue. :)  nsRefPtr has a non-template copy constructor:

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsRefPtr.h#72

and a templated copy constructor:

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsRefPtr.h#111

and I guarantee that if you remove the former, thinking the latter is sufficient, things will not work.
Attachment #8593765 - Attachment is obsolete: true
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3)

> Making this a template doesn't do the right thing, because there's now no
> copy constructor for nsAutoPtr, which means the compiler auto-generates one,
> which leads to even more problems than nsAutoPtr already has.  You need to
> add a templated constructor here in addition to the copy constructor that
> already exists.  (This rule is [class.copy]p2 or thereabouts if you have a
> copy of the standard at hand; the copy constructor cannot be a template
> function.)

allright, I tested this with clang (version 602.0.49) ; and this is not the behaviour I'm seeing when I debug the code.

With only the templated constructor (as in my first patch), doing:
nsAutoPtr<T> foo;
nsAutoPtr<I> blah;
// I inherit from T.
foo = blah;

The ownership is transferred from blah to foo (as auto_ptr is doing).

Now that could be a unique behaviour of clang++, I don't know enough on the matter to be sure.

Am I misinterpreting anything ?

As I believe nsAutoPtr should be behaving in a similar fashion to auto_ptr, transfer of ownership is what would be expected. After reading about UniquePtr it won't help to do that.

However, I will certainly use it in place of nsAutoPtr in a few places !
Attachment #8593759 - Attachment is obsolete: true
Implementing requested changes. This makes nsAutoPtr have similar behaviour to auto_ptr
Attachment #8594294 - Flags: review?(nfroyd)
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> allright, I tested this with clang (version 602.0.49) ; and this is not the
> behaviour I'm seeing when I debug the code.
> 
> With only the templated constructor (as in my first patch), doing:
> nsAutoPtr<T> foo;
> nsAutoPtr<I> blah;
> // I inherit from T.
> foo = blah;
> 
> The ownership is transferred from blah to foo (as auto_ptr is doing).
>
> Now that could be a unique behaviour of clang++, I don't know enough on the
> matter to be sure.
> 
> Am I misinterpreting anything ?

Hm, I would think that would be OK, because you're supposed to invoke the templated constructor (or operator=, depending) in that case.  The case that should not work is:

nsAutoPtr<T> foo;
nsAutoPtr<T> blah;
...
foo = blah;

But test code here suggests that this code transfers ownership without the explicit (non-templated) copy constructor.  So I'm seeing the same thing you are.

Ah, I see what the issue is: even though the compiler will implicitly declare a copy constructor, that copy constructor has the signature:

  nsAutoPtr(const nsAutoPtr&);

and that constructor loses out during overload resolution to the non-const templated "copy" constructor that comes from the template you have declared.  At least, that's what I'm going to assume; I don't really want to trace through all the overload resolution rules to figure out if this is really the case or not.  (This case didn't come up with nsRefPtr because the copy constructor and the templated "copy" constructor both had |const nsRefPtr&| parameters, so the non-templated copy constructor could win in this case.)

So, my apologies for insisting that a particular behavior was the case when that wasn't really what was happening!  It does seem like a templated constructor/operator= is sufficient; I think you might still need the non-templated version for the move constructor/operator= case, since the implicitly-declared versions by the compiler would win out during overload resolution...

I was curious about what auto_ptr did here, and I see that auto_ptr has the setup I was insisting on: one non-template copy constructor and one templated copy constructor.  I am willing to believe that there's a good reason for requiring this, even with the behavior observed above, but I'm not entirely sure what it is.  So, in the absence of other reasoning, let's cargo-cult auto_ptr's setup (which is what your patch does) and call it a day.  Sound reasonable?

> As I believe nsAutoPtr should be behaving in a similar fashion to auto_ptr,
> transfer of ownership is what would be expected. After reading about
> UniquePtr it won't help to do that.
> 
> However, I will certainly use it in place of nsAutoPtr in a few places !

Excellent!
Attachment #8594294 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/e572ad1b9e39
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.