[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Gnustep-cvs] Minor fix for strings initialised with data they don't
From: |
Alexander Malmberg |
Subject: |
Re: [Gnustep-cvs] Minor fix for strings initialised with data they don't own ... consistent [...] |
Date: |
Mon, 03 Nov 2003 12:33:39 +0100 |
Richard Frith-Macdonald wrote:
> On Sunday, November 2, 2003, at 01:44 AM, Alexander Malmberg wrote:
> >> + Sat Nov 01 11:10:00 2003 Richard Frith-Macdonald <address@hidden>
> >> +
> >> + * Source/NSString.m: ([-copyWithZone:]) always do true copy
> >> + * Source/GSString.m: Where the string data pointed to is not
> >> + owned by the string instance, have copy operations create new
> >> + instances rather than retaining the receiver.
> >
> > This broke -gui pretty badly.
>
> How did that happen? Changing copy operations to be true copies
> in some additional cases would not appear to have potential to
> break anything. Certainly I tried running quite a few gui apps without
> trouble before committing.
You added a -copy implementation to GSCString and GSUnicodeString. The
relevant part is:
obj = (GSCString*)NSCopyObject(self, 0, NSDefaultMallocZone());
if (_contents.c != 0)
{
unsigned char *tmp;
tmp = NSZoneMalloc(NSDefaultMallocZone(), _count);
memcpy(tmp, _contents.c, _count);
obj->_contents.c = tmp;
}
Since the concrete classes for inline and substring inherit this -copy,
and since 'free' is always 0 for them, this is what happens when you
copy such a string.
In the inline string case, it leaks memory (the buffer is copied, but
the inline string's -dealloc doesn't deallocate it). In the substring
case, it causes a crash (the _parent pointer value is copied but not
retained, so you have two substring instances sharing a single retain
and get a double-release when the last substring is released).
[snip]
> > Since the 'structure' parts' interest in the memory management can be
> > summed up in a single simple "is my buffer guaranteed to be valid at
> > least until I am released"-flag (equivalent to the current 'free'-flag
> > for the external buffer case), this will work out nicely and should fix
> > this and any other bugs caused by leaking non-owned buffers.
>
> That's not what the 'free' flag means. The flag means 'should I free
> my buffer' when I'm deallocated.
True, but this doesn't really have anything to do with whether it's safe
to retain-instead-of-copy or not. Inline strings and substrings don't
have any buffers to free, but it's always safe for them (trivially for
inline strings; for substrings, if it wasn't safe, we shouldn't have
made a substring of it in the first place).
Thus, the structure layer needs a "do I own my buffer (directly or
indirectly, eg. by retaining someone who does)"-flag. If, as the
documentation implies, this always true, we don't need an actual flag,
but making the optimizations conditional on the free flag would still be
wrong.
> The data is *always* assumed to be
> "guaranteed to be valid at least until I am released"
Indeed, the OpenStep docs do say this.
> The problem is that the existing code assumes that the buffer will be
> valid
> until the instance is deallocated (which is true within the gnustep
> libraries,
> but might not be true for code ported from MacOS-X).
> In additions to changing copy behavior to cope with this, we also need
> to
> change creation of substrings so that we don't use the substring classes
> for substrings of classes which don't own their buffers.
The cocoa docs seem equivalent OpenStep. This does simplify things a
bit. However, given this, I don't see what you're trying to fix. If the
buffer is always owned by the string, I don't see how the "retain
instead of copy"- or substring-optimizations could ever be unsafe for
correct code. Do you have an example of (correct) code for which this
isn't safe?
- Alexander Malmberg