236595 - nsAdoptingString can adopt strings that are not "owned"

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Johnny Stenback (:jst)]

If an nsAdoptingString is assigned into another nsAdoptingString and the source
references a buffer that it doesn't own, the nsAdoptingString that's assigned
into will free the buffer on destruction and crash... Patch coming up. Daring
suggested un-inlining operator=() while I'm at it, so that'll be in the patch as
well.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Fix.

Comment 2

[Darin Fisher]

Comment on attachment 143021 [details] [diff] [review]
Fix.

>Index: xpcom/string/public/nsTString.h

>     public:
>+      explicit nsTAdoptingString_CharT() {}
>     private:
>-      explicit nsTAdoptingString_CharT() {}

what's the reasoning for this change?  it seems fine to me, but i
was just curious why you're doing it.


>Index: xpcom/string/src/nsString.cpp

>+#include <stdlib.h>

maybe no need to include this file?


>Index: xpcom/string/src/nsTString.cpp

>+nsTAdoptingString_CharT&
>+nsTAdoptingString_CharT::operator=( const self_type& str )
>+  {
>+    self_type* mutable_str = NS_CONST_CAST(self_type*, &str);

i wonder if this method should really take a non-const |self_type&|
instead.  since it is mutating the object, it seems odd to not do that.
however, GCC 3.x might not allow you to pass an anonymous instance of
nsAdoptingString to this method if the argument is not const.  hmm...

i think you document the full behavior of this method in the header
file.

also, what about MSVC linkage?	i think this function needs to be
declared NS_COM ... and you may need to do some magic in xpcom's
dlldeps.cpp.

Comment 3

[Johnny Stenback (:jst)]

(In reply to comment #2)
> (From update of attachment 143021 [details] [diff] [review])
> >Index: xpcom/string/public/nsTString.h
> 
> >     public:
> >+      explicit nsTAdoptingString_CharT() {}
> >     private:
> >-      explicit nsTAdoptingString_CharT() {}
> 
> what's the reasoning for this change?  it seems fine to me, but i
> was just curious why you're doing it.

I wanted to be able to create these guys on the stack, and w/o this change I'm
forced to pass in null, or something else...

> >+nsTAdoptingString_CharT&
> >+nsTAdoptingString_CharT::operator=( const self_type& str )
> >+  {
> >+    self_type* mutable_str = NS_CONST_CAST(self_type*, &str);
> 
> i wonder if this method should really take a non-const |self_type&|
> instead.  since it is mutating the object, it seems odd to not do that.
> however, GCC 3.x might not allow you to pass an anonymous instance of
> nsAdoptingString to this method if the argument is not const.  hmm...

I bent over backwards trying to make this work right, and I'm just not able to
do this w/o this taking const. If I make this take non-const, assignment ends up
callign one of the other operator='s, which is not what we want.

> also, what about MSVC linkage?	i think this function needs to be
> declared NS_COM ... and you may need to do some magic in xpcom's
> dlldeps.cpp.

Yes and yes.

New patch on its way...

Comment 4

[Johnny Stenback (:jst)]

Attachment: Updated diff.

Comment 5

[Darin Fisher]

Comment on attachment 143060 [details] [diff] [review]
Updated diff.

>Index: xpcom/string/public/nsTString.h

>   /**
>    * nsTAdoptString extends nsTXPIDString such that:

"nsTAdoptingString"
"nsTXPIDLString"


>+   * (1) Adopt given string on construction or assignment, i.e. take
>+   * the value of what's given, and make what's given forget its
>+   * value. Note that this class violates constnss in a few places. Be

"constness"


>+        // copy-constructor required to adopt on copy. Note that this
>+        // will violate the constness of str in the operator=().

maybe add that "|str| will be truncated as a side-effect of this function"


>+        // Adopt(), if possible, when assigning to a self_type&. Note
>+        // that this violates the constness of str, str is always
>+        // reset when this operator is called.

s/reset/truncated/ to be consistent.


r+sr=darin

Comment 6

[Darin Fisher]

it looks like this got checked in.

marking FIXED