944173 - mfbt/Scoped.h's mozilla::Scoped fails to prevent copies, so Vector<Scoped...> crashes in malloc

Status: RESOLVED WONTFIX

(Core :: MFBT, defect)

Description

[Jim Blandy :jimb]

Trying to use an instance of mozilla::Vector<mozilla::ScopedFreePtr<char> > (or a Vector of any other mozilla::Scoped-derived type) crashes the first time the vector tries to resize its buffer.

The problem is that mozilla::Scoped defines both a constructor accepting a const Resource &, and a conversion operator, operator const Resource &(). We delete the copy constructor to prevent code like this:

   ScopedFreePtr<char> a;
   a = strdup("Hi!");
   ScopedFreePtr<char> b(a);

but unfortunately, code like this still compiles without error:

   ScopedFreePtr<char> a;
   a = strdup("Hi!");
   ScopedFreePtr<char> b(mozilla::Move(a));

because the result of the move is not a 'Resource &'. The compiler decides to convert a to a 'const char *', and then invoke b's 'const Resource *&' constructor.

mozilla::Vector<ScopedFreePtr<char>>::growTo contains the equivalent:

        new(dst) T(Move(*src));

As a result, declaring a variable of type Vector<ScopedFreePtr<char>> compiles fine, but produces code that crashes the first time the vector needs to grow its buffer.

Comment 1

[Jim Blandy :jimb]

Luke's sense is that conversion operators, like Scoped<Traits>::operator const Resource &() are asking for trouble, in the presence of constructors accepting the same type. So perhaps that operator should be deleted.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Removing operators is often a good idea, so I'm all for it.

Comment 3

[Jeff Walden [:Waldo]]

We should implement C++11's std::unique_ptr class as mozilla::UniquePtr; that class forces you to move it around by dint of having no copy constructor.  Ask in #developers any normal workday (i.e. next week :-) ) and a bunch of people will speak up and ask for it.  Using Scoped for unique-pointer sorts of things is just broken, unless you're really careful.  (See, for example, the sadface comments in bug 724768's patch, which it didn't seem to make sense to hold up for UniquePtr.)

Comment 4

[Jim Blandy :jimb]

I just did a little bit of reading, and from what I can tell, our ScopedPtr is what unique_ptr is. We have no copy constructor nor copy assignment operator. Bug 944176 would give ScopedPtr move support, making it even more like unique_ptr.

So I think Waldo and I are in agreement, modulo names...

Comment 5

[Jim Blandy :jimb]

Waldo, this is resolved fixed now, right?

Comment 6

[Jeff Walden [:Waldo]]

Erm, I'm not sure how this is fixed now.  In the process of *being* fixed by UniquePtr in bug 953296, perhaps, but it ain't fixed yet.  If this stuff is needed sooner rather than later, I can certainly push on that work to clear it out quicker, but right now other things seem to beckon harder.

Comment 7

[Jeff Walden [:Waldo]]

People should use Vector<UniquePtr> instead of Scoped for this, now that UniquePtr has landed.