Closed Bug 236595 Opened 20 years ago Closed 20 years ago

nsAdoptingString can adopt strings that are not "owned"

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jst, Assigned: jst)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Fix. (obsolete) — Splinter Review
Attachment #143021 - Flags: superreview?(darin)
Attachment #143021 - Flags: review?(darin)
Status: NEW → ASSIGNED
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.
Attachment #143021 - Flags: superreview?(darin)
Attachment #143021 - Flags: superreview-
Attachment #143021 - Flags: review?(darin)
Attachment #143021 - Flags: review-
(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...
Attached patch Updated diff.Splinter Review
Attachment #143021 - Attachment is obsolete: true
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
Attachment #143060 - Flags: superreview+
Attachment #143060 - Flags: review+
it looks like this got checked in.

marking FIXED
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: