Re: [Gnustep-cvs] Minor fix for strings initialised with data they don't own ... consistent [...]

[Alexander Malmberg]

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