Re: libobjc/9751: malloc of strlen, not strlen+1

[John Carter <john.carter@tait.co.nz>]

The following reply was made to PR libobjc/9751; it has been noted by GNATS.

From: John Carter <john.carter@tait.co.nz>
To: Richard Frith-Macdonald <richard@brainstorm.co.uk>
Cc: gcc-prs@gcc.gnu.org, gcc-bugs@gcc.gnu.org, gcc-gnats@gcc.gnu.org
Subject: Re: libobjc/9751: malloc of strlen, not strlen+1
Date: Tue, 13 May 2003 09:56:14 +1200

 Hmm, looking at it again I still don't like it.
 
 If strncpy terminates due to having copied its "n" characters, it
 _doesn't_ copy in the null. (Yip, check the libc info page, as I say,
 the strncpy semantics are plain fugly and almost always doesn't do what
 you want...)
 
 The very next line uses strcat, which _expects_ a properly null
 terminated string! I can't believe this bit of code is reliable.
 
 In fact I will state a categorical principle any...
   strncpy( blah, bloo, fishpaste);
 Followed by immediately by...
   strwhateverlibcthing( blah,....);
 Can only work by accident!
 
 This is the code from gcc-3.2.3...
 	  /* The variable is gc_invisible and we have to reverse it */
 	  new_type = objc_atomic_malloc (strlen (ivar->ivar_type));
 	  strncpy (new_type, ivar->ivar_type,
 		   (size_t)(type - ivar->ivar_type));
 	  strcat (new_type, type + 1);
 	  ivar->ivar_type = new_type;
 
 I would rewrite that as...
   size_t len = type - ivar->ivar_type;
   new_type=objc_atomic_malloc(strlen(ivar-ivar_type));
   memcpy( new_type, ivar->ivar_type, len);
   strcpy( new_type+len, type+1);
 
 
 On Mon, 2003-05-12 at 20:51, Richard Frith-Macdonald wrote:
 > http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=9751
 > 
 > I was just looking at this ... and I don't think this is a bug.
 > If I understand the code correctly, it is removing a single byte (the 
 > garbage collecting invisibility marker) from the type string.  So the 
 > length of the new string is one byte less than that of the original.
 > So allocating strlen(ivar->ivar_type) bytes is correct.
 > It might perhaps be worth adding a comment to thiks effect in the source 
 > though.