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

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

[Richard Frith-Macdonald]

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.
 

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

[John Carter]

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.
 
 

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

[Richard Frith-Macdonald]

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

From: Richard Frith-Macdonald <richard@brainstorm.co.uk>
To: gcc-prs@gcc.gnu.org, john.carter@tait.co.nz, gcc-bugs@gcc.gnu.org,
   gcc-gnats@gcc.gnu.org, nobody@gcc.gnu.org
Cc:  
Subject: Re: libobjc/9751: malloc of strlen, not strlen+1
Date: Mon, 12 May 2003 09:51:54 +0100

 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.
 

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

[john.carter]

>Number:         9751
>Category:       libobjc
>Synopsis:       malloc of strlen, not strlen+1
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    unassigned
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Feb 19 03:46:00 UTC 2003
>Closed-Date:
>Last-Modified:
>Originator:     john.carter@tait.co.nz
>Release:        gcc-3.2.2
>Organization:
>Environment:
all
>Description:
In file /gcc-3.2.1/libobjc/gc.c line 415 there is the following (possibly buggy?) code...


	  /* 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;

Probably that should be malloc(strlen()+1) for the null. Also strncpy is an fugly beast that almost always doesn't do what you want. ie. Will there always be a null termination in the right place for the following strcat()?
>How-To-Repeat:
Found using my collection of perl regex's for finding standard "gotcha's"
>Fix:

>Release-Note:
>Audit-Trail:
>Unformatted: