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

[Richard Frith-Macdonald <richard at brainstorm dot co dot uk>]

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

From: Richard Frith-Macdonald <richard@brainstorm.co.uk>
To: John Carter <john.carter@tait.co.nz>
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 06:02:20 +0100

 On Monday, May 12, 2003, at 10:56  pm, John Carter wrote:
 
 > 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);
 
 So the size of the memory allocated is correct, but the use of
 the strcat() is wrong... should have been strcpy().
 
 I'd agree with your rewriting ... except for the typo in the argument
 to strlen() and the improper to use of whitespace (as far as gnu
 coding standards are concerned) of course :-)
 
 There is no functional difference between strncpy() and memcpy()
 in this case, but the memcpy() should be marginally faster.