bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[PATCH 1/3] strerror_r: enforce POSIX recommendations


From: Eric Blake
Subject: [PATCH 1/3] strerror_r: enforce POSIX recommendations
Date: Tue, 24 May 2011 13:47:30 -0600

POSIX recommends (but does not require) that strerror_r populate
buf even on error.  But since we guarantee this behavior for
strerror, we might as well also guarantee it for strerror_r.

* lib/strerror_r.c (safe_copy): New helper method.
(strerror_r): Guarantee a non-empty string.
* tests/test-strerror_r.c (main): Enhance tests to incorporate
recent POSIX rulings and to match our strerror guarantees.
* doc/posix-functions/strerror_r.texi (strerror_r): Document this.

Signed-off-by: Eric Blake <address@hidden>
---

I've updated this patch, as well as tested this series on:

glibc 2.13, Solaris 10, FreeBSD 8.2, AIX 5.1, Irix 6.5, HP-UX 10.20
(not using the native strerror_r, but I'm still working that),
cygwin (still failing, but that's coming).  mingw testing still
underway.  perror needs to be replaced in more situations (I'm
still hoping to get to that soon), but this is now far enough and
passing strerror/_r on enough platforms that I feel good pushing it,
and it also matches glibc.git behavior as of Uli's patch last week
to guarantee that strerror_r populates buf even on error.

 ChangeLog                           |    9 +++
 doc/posix-functions/strerror_r.texi |   18 ++++--
 lib/strerror_r.c                    |  114 +++++++++++++---------------------
 tests/test-strerror_r.c             |   88 ++++++++++++++++-----------
 4 files changed, 118 insertions(+), 111 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4ec6463..bc838bd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2011-05-24  Eric Blake  <address@hidden>
+
+       strerror_r: enforce POSIX recommendations
+       * lib/strerror_r.c (safe_copy): New helper method.
+       (strerror_r): Guarantee a non-empty string.
+       * tests/test-strerror_r.c (main): Enhance tests to incorporate
+       recent POSIX rulings and to match our strerror guarantees.
+       * doc/posix-functions/strerror_r.texi (strerror_r): Document this.
+
 2011-05-24  Jim Meyering  <address@hidden>

        test-perror2.c: avoid warning about unused variable
diff --git a/doc/posix-functions/strerror_r.texi 
b/doc/posix-functions/strerror_r.texi
index 21e4181..91026ef 100644
--- a/doc/posix-functions/strerror_r.texi
+++ b/doc/posix-functions/strerror_r.texi
@@ -51,15 +51,21 @@ strerror_r
 platforms:
 HP-UX 11.31.
 @item
-When the buffer is too small, this function does not fail, but instead
-truncates the result and returns 0 on some platforms:
-OSF/1 5.1.
+When the buffer is too small and the value is in range, this function
+does not fail, but instead truncates the result and returns 0 on some
+platforms:
+AIX 6.1, OSF/1 5.1.
address@hidden
+When the value is not in range or the buffer is too small, this
+function fails to leave a NUL-terminated string in the buffer on some
+platforms:
+glibc 2.13, FreeBSD 8.2.
 @end itemize

 Portability problems not fixed by Gnulib:
 @itemize
 @item
-When the buffer is too small, this function does not fail, but instead
-truncates the result and returns 0 on some platforms:
-AIX 6.1.
+Calling this function can clobber the buffer used by @code{strerror}
+on some platforms:
+Cygwin 1.7.9.
 @end itemize
diff --git a/lib/strerror_r.c b/lib/strerror_r.c
index f0b59c7..40ebc59 100644
--- a/lib/strerror_r.c
+++ b/lib/strerror_r.c
@@ -92,6 +92,30 @@ gl_lock_define_initialized(static, strerror_lock)

 #endif

+/* Copy as much of MSG into BUF as possible, without corrupting errno.
+   Return 0 if MSG fit in BUFLEN, otherwise return ERANGE.  */
+static int
+safe_copy (char *buf, size_t buflen, const char *msg)
+{
+  size_t len = strlen (msg);
+  int ret;
+
+  if (len < buflen)
+    {
+      /* Although POSIX allows memcpy() to corrupt errno, we don't
+         know of any implementation where this is a real problem.  */
+      memcpy (buf, msg, len + 1);
+      ret = 0;
+    }
+  else
+    {
+      memcpy (buf, msg, buflen - 1);
+      buf[buflen - 1] = '\0';
+      ret = ERANGE;
+    }
+  return ret;
+}
+

 int
 strerror_r (int errnum, char *buf, size_t buflen)
@@ -102,9 +126,10 @@ strerror_r (int errnum, char *buf, size_t buflen)
   if (buflen <= 1)
     {
       if (buflen)
-        *buf = 0;
+        *buf = '\0';
       return ERANGE;
     }
+  *buf = '\0';

 #if GNULIB_defined_ETXTBSY \
     || GNULIB_defined_ESOCK \
@@ -413,19 +438,7 @@ strerror_r (int errnum, char *buf, size_t buflen)
       }

     if (msg)
-      {
-        int saved_errno = errno;
-        size_t len = strlen (msg);
-        int ret = ERANGE;
-
-        if (len < buflen)
-          {
-            memcpy (buf, msg, len + 1);
-            ret = 0;
-          }
-        errno = saved_errno;
-        return ret;
-      }
+      return safe_copy (buf, buflen, msg);
   }
 #endif

@@ -441,6 +454,13 @@ strerror_r (int errnum, char *buf, size_t buflen)
       ret = __xpg_strerror_r (errnum, buf, buflen);
       if (ret < 0)
         ret = errno;
+      if (!*buf)
+        {
+          /* glibc 2.13 would not touch buf on err, so we have to fall
+             back to GNU strerror_r which always returns a thread-safe
+             untruncated string to (partially) copy into our buf.  */
+          safe_copy (buf, buflen, strerror_r (errnum, buf, buflen));
+        }
     }

 #elif USE_SYSTEM_STRERROR_R
@@ -453,18 +473,11 @@ strerror_r (int errnum, char *buf, size_t buflen)
     {
       char stackbuf[80];

-      if (buflen < sizeof (stackbuf))
+      if (buflen < sizeof stackbuf)
         {
-          ret = strerror_r (errnum, stackbuf, sizeof (stackbuf));
+          ret = strerror_r (errnum, stackbuf, sizeof stackbuf);
           if (ret == 0)
-            {
-              size_t len = strlen (stackbuf);
-
-              if (len < buflen)
-                memcpy (buf, stackbuf, len + 1);
-              else
-                ret = ERANGE;
-            }
+            ret = safe_copy (buf, buflen, stackbuf);
         }
       else
         ret = strerror_r (errnum, buf, buflen);
@@ -479,19 +492,7 @@ strerror_r (int errnum, char *buf, size_t buflen)

     /* FreeBSD rejects 0; see http://austingroupbugs.net/view.php?id=382.  */
     if (errnum == 0 && ret == EINVAL)
-      {
-        if (buflen <= strlen ("Success"))
-          {
-            ret = ERANGE;
-            if (buflen)
-              buf[0] = 0;
-          }
-        else
-          {
-            ret = 0;
-            strcpy (buf, "Success");
-          }
-      }
+      ret = safe_copy (buf, buflen, "Success");

 #else /* USE_SYSTEM_STRERROR */

@@ -528,17 +529,7 @@ strerror_r (int errnum, char *buf, size_t buflen)
         if (errmsg == NULL || *errmsg == '\0')
           ret = EINVAL;
         else
-          {
-            size_t len = strlen (errmsg);
-
-            if (len < buflen)
-              {
-                memcpy (buf, errmsg, len + 1);
-                ret = 0;
-              }
-            else
-              ret = ERANGE;
-          }
+          ret = safe_copy (buf, buflen, errmsg);
 #  if HAVE_CATGETS && (defined __NetBSD__ || defined __hpux)
         if (catd != (nl_catd)-1)
           catclose (catd);
@@ -558,17 +549,7 @@ strerror_r (int errnum, char *buf, size_t buflen)
         if (errmsg == NULL || *errmsg == '\0')
           ret = EINVAL;
         else
-          {
-            size_t len = strlen (errmsg);
-
-            if (len < buflen)
-              {
-                memcpy (buf, errmsg, len + 1);
-                ret = 0;
-              }
-            else
-              ret = ERANGE;
-          }
+          ret = safe_copy (buf, buflen, errmsg);
       }
     else
       ret = EINVAL;
@@ -586,17 +567,7 @@ strerror_r (int errnum, char *buf, size_t buflen)
       if (errmsg == NULL || *errmsg == '\0')
         ret = EINVAL;
       else
-        {
-          size_t len = strlen (errmsg);
-
-          if (len < buflen)
-            {
-              memcpy (buf, errmsg, len + 1);
-              ret = 0;
-            }
-          else
-            ret = ERANGE;
-        }
+        ret = safe_copy (buf, buflen, errmsg);
     }

     gl_lock_unlock (strerror_lock);
@@ -605,6 +576,9 @@ strerror_r (int errnum, char *buf, size_t buflen)

 #endif

+    if (ret == EINVAL && !*buf)
+      snprintf (buf, buflen, "Unknown error %d", errnum);
+
     errno = saved_errno;
     return ret;
   }
diff --git a/tests/test-strerror_r.c b/tests/test-strerror_r.c
index 9c4874f..1f23ba8 100644
--- a/tests/test-strerror_r.c
+++ b/tests/test-strerror_r.c
@@ -36,21 +36,24 @@ main (void)

   errno = 0;
   buf[0] = '\0';
-  ASSERT (strerror_r (EACCES, buf, sizeof (buf)) == 0);
+  ASSERT (strerror_r (EACCES, buf, sizeof buf) == 0);
   ASSERT (buf[0] != '\0');
   ASSERT (errno == 0);
+  ASSERT (strlen (buf) < sizeof buf);

   errno = 0;
   buf[0] = '\0';
-  ASSERT (strerror_r (ETIMEDOUT, buf, sizeof (buf)) == 0);
+  ASSERT (strerror_r (ETIMEDOUT, buf, sizeof buf) == 0);
   ASSERT (buf[0] != '\0');
   ASSERT (errno == 0);
+  ASSERT (strlen (buf) < sizeof buf);

   errno = 0;
   buf[0] = '\0';
-  ASSERT (strerror_r (EOVERFLOW, buf, sizeof (buf)) == 0);
+  ASSERT (strerror_r (EOVERFLOW, buf, sizeof buf) == 0);
   ASSERT (buf[0] != '\0');
   ASSERT (errno == 0);
+  ASSERT (strlen (buf) < sizeof buf);

   /* POSIX requires strerror (0) to succeed.  Reject use of "Unknown
      error", but allow "Success", "No error", or even Solaris' "Error
@@ -58,54 +61,69 @@ main (void)
      http://austingroupbugs.net/view.php?id=382  */
   errno = 0;
   buf[0] = '\0';
-  ret = strerror_r (0, buf, sizeof (buf));
+  ret = strerror_r (0, buf, sizeof buf);
   ASSERT (ret == 0);
   ASSERT (buf[0]);
   ASSERT (errno == 0);
   ASSERT (strstr (buf, "nknown") == NULL);

-  /* Test results with out-of-range errnum and enough room.  */
-
+  /* Test results with out-of-range errnum and enough room.  POSIX
+     allows an empty string on success, and allows an unchanged buf on
+     error, but these are not useful, so we guarantee contents.  */
   errno = 0;
   buf[0] = '^';
-  ret = strerror_r (-3, buf, sizeof (buf));
+  ret = strerror_r (-3, buf, sizeof buf);
   ASSERT (ret == 0 || ret == EINVAL);
-  if (ret == 0)
-    ASSERT (buf[0] != '^');
+  ASSERT (buf[0] != '^');
+  ASSERT (*buf);
   ASSERT (errno == 0);
+  ASSERT (strlen (buf) < sizeof buf);
+
+  /* Test results with a too small buffer.  POSIX requires an error;
+     only ERANGE for 0 and valid errors, and a choice of ERANGE or
+     EINVAL for out-of-range values.  On error, POSIX permits buf to
+     be empty, unchanged, or unterminated, but these are not useful,
+     so we guarantee NUL-terminated truncated contents for all but
+     size 0.  http://austingroupbugs.net/view.php?id=398  */
+  {
+    int errs[] = { EACCES, 0, -3, };
+    int j;
+    for (j = 0; j < SIZEOF (errs); j++)
+      {
+        int err = errs[j];
+        char buf2[sizeof buf] = "";
+        size_t len;
+        size_t i;

-  /* Test results with a too small buffer.  */
+        strerror_r (err, buf2, sizeof buf2);
+        len = strlen (buf2);

-  ASSERT (strerror_r (EACCES, buf, sizeof (buf)) == 0);
-  {
-    size_t len = strlen (buf);
-    size_t i;
+        for (i = 0; i <= len; i++)
+          {
+            strcpy (buf, "BADFACE");
+            errno = 0;
+            ret = strerror_r (err, buf, i);
+            ASSERT (errno == 0);
+            if (err < 0)
+              ASSERT (ret == ERANGE || ret == EINVAL);
+            else
+              ASSERT (ret == ERANGE);
+            if (i == 0)
+              ASSERT (strcmp (buf, "BADFACE") == 0);
+            else
+              {
+                ASSERT (strncmp (buf, buf2, i - 1) == 0);
+                ASSERT (buf[i - 1] == '\0');
+              }
+          }

-    for (i = 0; i <= len; i++)
-      {
         strcpy (buf, "BADFACE");
         errno = 0;
-        ret = strerror_r (EACCES, buf, i);
+        ret = strerror_r (err, buf, len + 1);
+        ASSERT (ret != ERANGE);
         ASSERT (errno == 0);
-        if (ret == 0)
-          {
-            /* Truncated result.  POSIX allows this, and it actually
-               happens on AIX 6.1 and Cygwin.  */
-            ASSERT ((strcmp (buf, "BADFACE") == 0) == (i == 0));
-          }
-        else
-          {
-            /* Failure.  */
-            ASSERT (ret == ERANGE);
-            /* buf is clobbered nevertheless, on FreeBSD and MacOS X.  */
-          }
+        ASSERT (strcmp (buf, buf2) == 0);
       }
-
-    strcpy (buf, "BADFACE");
-    errno = 0;
-    ret = strerror_r (EACCES, buf, len + 1);
-    ASSERT (ret == 0);
-    ASSERT (errno == 0);
   }

 #if GNULIB_STRERROR
-- 
1.7.4.4




reply via email to

[Prev in Thread] Current Thread [Next in Thread]