Re: Using strlcpy if target OS allows it

[Magnus Fromreide <magfr at lysator dot liu dot se>]

On Thu, 21 Apr 2005, it was written:

> Date: Thu, 21 Apr 2005 13:40:29 -0700
> From: Joe Buck <Joe dot Buck at synopsys dot COM>
> To: "J dot T dot  Conklin" <jtc at acorntoolworks dot com>
> Cc: libstdc++ at gcc dot gnu dot org
> Subject: Re: Using strlcpy if target OS allows it
>
> On Thu, Apr 21, 2005 at 11:55:34AM -0700, J.T. Conklin wrote:
> > Joe Buck <Joe.Buck@synopsys.COM> writes:
> > > There appear to be only four calls of strcpy in the library itself
> > > (as opposed to the testsuite).  All four take this exact form:
> > >
> > >       char* __sav = new char[std::strlen(__old) + 1];
> > >       std::strcpy(__sav, __old);
> > >
> > > Perhaps the V3 maintainers could be persuaded to accept a patch that
> > > replaces all four with a call to
> > >
> > > inline char* __cstring_duplicate(const char* __old) {
> > >   unsigned __size = std::strlen(__old) + 1;
> > >   char* __sav = new char[__size];
> > >   return std::strcpy(__sav, __old);
> > > }
> > >
> > > This would have maintainance advantages, as four identical code sequences
> > > become one.
> >
> > If a change is made, why not replace the strcpy() with a memcpy()?  As
> > it is, strcpy() is copying until it reaches the terminating null char,
> > even though we already know how many bytes to copy.  memcpy() is often
> > more efficent than strcpy as well, even those implementations that go
> > to Herculean efforts to do only word-aligned read/writes.
>
> Good point.  That would be a small (probably negligle) performance
> improvement and would get rid of the need to patch OpenBSD (other than the
> test suite issue).  Perhaps some interested party would like to make a patch?
>

I think the folowing is the patch you were asking for, but see below for
further comments.

Index: libstdc++-v3/config/locale/generic/c_locale.h
===================================================================
RCS file: /cvsroot/gcc/gcc/libstdc++-v3/config/locale/generic/c_locale.h,v
retrieving revision 1.14
diff -u -r1.14 c_locale.h
--- libstdc++-v3/config/locale/generic/c_locale.h	30 Jan 2005 14:09:58 -0000	1.14
+++ libstdc++-v3/config/locale/generic/c_locale.h	2 May 2005 17:08:53 -0000
@@ -63,8 +63,9 @@
       char* __sav = NULL;
       if (std::strcmp(__old, "C"))
 	{
-	  __sav = new char[std::strlen(__old) + 1];
-	  std::strcpy(__sav, __old);
+	  std::size_t strsize = std::strlen(__old) + 1;
+	  __sav = new char[strsize];
+	  std::memcpy(__sav, __old, strsize);
 	  std::setlocale(LC_NUMERIC, "C");
 	}

Index: libstdc++-v3/config/locale/generic/time_members.h
===================================================================
RCS file: /cvsroot/gcc/gcc/libstdc++-v3/config/locale/generic/time_members.h,v
retrieving revision 1.4
diff -u -r1.4 time_members.h
--- libstdc++-v3/config/locale/generic/time_members.h	2 Oct 2003 23:06:12 -0000	1.4
+++ libstdc++-v3/config/locale/generic/time_members.h	2 May 2005 17:08:53 -0000
@@ -54,9 +54,10 @@
     __timepunct<_CharT>::__timepunct(__c_locale __cloc, const char* __s,
 				     size_t __refs)
     : facet(__refs), _M_data(NULL)
-    {
-      char* __tmp = new char[std::strlen(__s) + 1];
-      std::strcpy(__tmp, __s);
+    {
+      std::size_t strsize = std::strlen(__s) + 1;
+      char* __tmp = new char[strsize];
+      std::memcpy(__tmp, __s, strsize);
       _M_name_timepunct = __tmp;
       _M_initialize_timepunct(__cloc);
     }
Index: libstdc++-v3/config/locale/gnu/c_locale.h
===================================================================
RCS file: /cvsroot/gcc/gcc/libstdc++-v3/config/locale/gnu/c_locale.h,v
retrieving revision 1.13
diff -u -r1.13 c_locale.h
--- libstdc++-v3/config/locale/gnu/c_locale.h	24 Mar 2005 05:45:43 -0000	1.13
+++ libstdc++-v3/config/locale/gnu/c_locale.h	2 May 2005 17:08:53 -0000
@@ -77,8 +77,9 @@
 		     _Tv __v, const __c_locale&, int __prec)
     {
       char* __old = std::setlocale(LC_ALL, NULL);
-      char* __sav = new char[std::strlen(__old) + 1];
-      std::strcpy(__sav, __old);
+      std::size_t strsize = std::strlen(__old) + 1;
+      char* __sav = new char[strsize];
+      std::memcpy(__sav, __old, strsize);
       std::setlocale(LC_ALL, "C");
 #endif

Index: libstdc++-v3/config/locale/gnu/messages_members.h
===================================================================
RCS file: /cvsroot/gcc/gcc/libstdc++-v3/config/locale/gnu/messages_members.h,v
retrieving revision 1.13
diff -u -r1.13 messages_members.h
--- libstdc++-v3/config/locale/gnu/messages_members.h	22 May 2004 23:46:31 -0000	1.13
+++ libstdc++-v3/config/locale/gnu/messages_members.h	2 May 2005 17:08:53 -0000
@@ -46,8 +46,9 @@
      : facet(__refs), _M_c_locale_messages(_S_clone_c_locale(__cloc)),
      _M_name_messages(__s)
      {
-       char* __tmp = new char[std::strlen(__s) + 1];
-       std::strcpy(__tmp, __s);
+       std::size_t strsize = std::strlen(__s) + 1;
+       char* __tmp = new char[strsize];
+       std::memcpy(__tmp, __s, strsize);
        _M_name_messages = __tmp;
      }

Index: libstdc++-v3/config/locale/gnu/time_members.h
===================================================================
RCS file: /cvsroot/gcc/gcc/libstdc++-v3/config/locale/gnu/time_members.h,v
retrieving revision 1.5
diff -u -r1.5 time_members.h
--- libstdc++-v3/config/locale/gnu/time_members.h	22 May 2004 23:46:31 -0000	1.5
+++ libstdc++-v3/config/locale/gnu/time_members.h	2 May 2005 17:08:54 -0000
@@ -51,9 +51,10 @@
 				     size_t __refs)
     : facet(__refs), _M_data(NULL), _M_c_locale_timepunct(NULL),
     _M_name_timepunct(__s)
-    {
-      char* __tmp = new char[std::strlen(__s) + 1];
-      std::strcpy(__tmp, __s);
+    {
+      std::size_t strsize = std::strlen(__s) + 1;
+      char* __tmp = new char[strsize];
+      std::memcpy(__tmp, __s, strsize);
       _M_name_timepunct = __tmp;
       _M_initialize_timepunct(__cloc);
     }


Here follows the questionable part.
I am not at all certain about which of the uses of strcpy in the testsuite
that were intentional.

Index: libstdc++-v3/testsuite/22_locale/codecvt/unshift/char/1.cc
===================================================================
RCS file: /cvsroot/gcc/gcc/libstdc++-v3/testsuite/22_locale/codecvt/unshift/char/1.cc,v
retrieving revision 1.2
diff -u -r1.2 1.cc
--- libstdc++-v3/testsuite/22_locale/codecvt/unshift/char/1.cc	23 Sep 2003 20:02:16 -0000	1.2
+++ libstdc++-v3/testsuite/22_locale/codecvt/unshift/char/1.cc	2 May 2005 17:08:55 -0000
@@ -67,7 +67,7 @@
   VERIFY( to_next == c_arr );

   // unshift
-  strcpy(c_arr, c_lit);
+  memcpy(c_arr, c_lit, strlen(c_lit) + 1);
   result r3 = cvt->unshift(state, c_arr, c_arr + size, to_next);
   VERIFY( r3 == codecvt_base::noconv );
   VERIFY( !strcmp(c_arr, c_lit) );
Index: libstdc++-v3/testsuite/22_locale/codecvt/unshift/wchar_t/1.cc
===================================================================
RCS file: /cvsroot/gcc/gcc/libstdc++-v3/testsuite/22_locale/codecvt/unshift/wchar_t/1.cc,v
retrieving revision 1.3
diff -u -r1.3 1.cc
--- libstdc++-v3/testsuite/22_locale/codecvt/unshift/wchar_t/1.cc	23 Sep 2003 20:02:16 -0000	1.3
+++ libstdc++-v3/testsuite/22_locale/codecvt/unshift/wchar_t/1.cc	2 May 2005 17:08:55 -0000
@@ -56,7 +56,7 @@
   const w_codecvt* 	cvt = &use_facet<w_codecvt>(loc);

   // unshift
-  strcpy(e_arr, e_lit);
+  memcpy(e_arr, e_lit, size + 1);
   w_codecvt::state_type state03;
   zero_state(state03);
   result r3 = cvt->unshift(state03, e_arr, e_arr + size, eto_next);


In the following test case the test case showed undefined behaviour
earlier as one more character than allocated was written so I am quite
unsure about what it is that was tested.

Index: libstdc++-v3/testsuite/27_io/basic_streambuf/overflow/char/1.cc
===================================================================
RCS file: /cvsroot/gcc/gcc/libstdc++-v3/testsuite/27_io/basic_streambuf/overflow/char/1.cc,v
retrieving revision 1.3
diff -u -r1.3 1.cc
--- libstdc++-v3/testsuite/27_io/basic_streambuf/overflow/char/1.cc	23 Sep 2003 20:03:18 -0000	1.3
+++ libstdc++-v3/testsuite/27_io/basic_streambuf/overflow/char/1.cc	2 May 2005 17:08:57 -0000
@@ -93,8 +93,7 @@
   typedef testbuf::int_type int_type;

   bool test __attribute__((unused)) = true;
-  char lit01[52];
-  strcpy(lit01, "chicago underground trio/possible cube on delmark");
+  char lit01[52] = "chicago underground trio/possible cube on delmark";
   testbuf buf01;

   // pbackfail
Index: libstdc++-v3/testsuite/27_io/basic_streambuf/sgetc/char/1.cc
===================================================================
RCS file: /cvsroot/gcc/gcc/libstdc++-v3/testsuite/27_io/basic_streambuf/sgetc/char/1.cc,v
retrieving revision 1.3
diff -u -r1.3 1.cc
--- libstdc++-v3/testsuite/27_io/basic_streambuf/sgetc/char/1.cc	23 Sep 2003 20:03:18 -0000	1.3
+++ libstdc++-v3/testsuite/27_io/basic_streambuf/sgetc/char/1.cc	2 May 2005 17:08:58 -0000
@@ -93,8 +93,7 @@
   typedef testbuf::int_type int_type;

   bool test __attribute__((unused)) = true;
-  char lit01[52];
-  strcpy(lit01, "chicago underground trio/possible cube on delmark");
+  char lit01[52] = "chicago underground trio/possible cube on delmark";
   testbuf buf01;

   // 27.5.2.3.1 get area
Index: libstdc++-v3/testsuite/27_io/basic_streambuf/sgetn/char/1.cc
===================================================================
RCS file: /cvsroot/gcc/gcc/libstdc++-v3/testsuite/27_io/basic_streambuf/sgetn/char/1.cc,v
retrieving revision 1.3
diff -u -r1.3 1.cc
--- libstdc++-v3/testsuite/27_io/basic_streambuf/sgetn/char/1.cc	23 Sep 2003 20:03:19 -0000	1.3
+++ libstdc++-v3/testsuite/27_io/basic_streambuf/sgetn/char/1.cc	2 May 2005 17:08:58 -0000
@@ -94,10 +94,8 @@

   bool test __attribute__((unused)) = true;

-  const char* lit00 = "chicago underground trio/possible cube on delmark";
-  size_t i01 = traits_type::length(lit00);
-  char lit01[i01];
-  strcpy(lit01, lit00);
+  char lit01[] = "chicago underground trio/possible cube on delmark";
+  size_t i01 = traits_type::length(lit01);

   testbuf buf01;

Index: libstdc++-v3/testsuite/27_io/basic_streambuf/sputn/char/1.cc
===================================================================
RCS file: /cvsroot/gcc/gcc/libstdc++-v3/testsuite/27_io/basic_streambuf/sputn/char/1.cc,v
retrieving revision 1.3
diff -u -r1.3 1.cc
--- libstdc++-v3/testsuite/27_io/basic_streambuf/sputn/char/1.cc	23 Sep 2003 20:03:19 -0000	1.3
+++ libstdc++-v3/testsuite/27_io/basic_streambuf/sputn/char/1.cc	2 May 2005 17:08:58 -0000
@@ -98,8 +98,8 @@
   // sputn/xsputn
   const char* lit01 = "isotope 217: the unstable molecule on thrill jockey";
   const int i02 = std::strlen(lit01);
-  char lit02[i02];
-  std::strcpy(lit02, lit01);
+  char lit02[i02 + 1];
+  std::memcpy(lit02, lit01, i02 + 1);

   char carray[i02 + 1];
   std::memset(carray, 0, i02 + 1);

/MF