[PATCH] strerror_r: guarantee unchanged errno

[Eric Blake]

POSIX guarantees that strerror doesn't change errno on success,
and since we implement strerror by using strerror_r, it makes
sense to make the same guarantee for strerror_r (rather, going
one step further to say that sterror_r does not corrupt errno
even on failure, since it returns an error value explicitly).

See also http://austingroupbugs.net/view.php?id=447

* lib/strerror_r.c (strerror_r): Guarantee unchanged errno.
* lib/strerror.c (strerror): Set errno to match strerror_r
failure.
* tests/test-strerror_r.c (main): Enhance test.

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

Bruno's code was doing part of the effort for NetBSD, but
forgot that other calls outside the protection for catgets
(such as strcpy) can also corrupt errno; also, preserving
errno is important when replacing sterror_r on platforms
that return -1.

I'm also opening up a POSIX bug for the fact that strcpy()
and friend are not documented as leaving errno unchanged,
as it would have been easier to write the test by assuming
that string manipulation doesn't interfere with errno.

 ChangeLog               |    6 ++++++
 lib/strerror.c          |    1 +
 lib/strerror_r.c        |    4 ++--
 tests/test-strerror_r.c |   14 ++++++++++++++
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f5e455b..0c15e9c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2011-05-19  Eric Blake  <address@hidden>

+       strerror_r: guarantee unchanged errno
+       * lib/strerror_r.c (strerror_r): Guarantee unchanged errno.
+       * lib/strerror.c (strerror): Set errno to match strerror_r
+       failure.
+       * tests/test-strerror_r.c (main): Enhance test.
+
        strerror_r: fix on newer cygwin
        * lib/strerror_r.c (rpl_strerror_r): Cygwin now has
        __xpg_strerror_r, use it.
diff --git a/lib/strerror.c b/lib/strerror.c
index 6137552..ee7e088 100644
--- a/lib/strerror.c
+++ b/lib/strerror.c
@@ -50,6 +50,7 @@ strerror (int n)
     static char const fmt[] = "Unknown error (%d)";
     verify (sizeof (buf) >= sizeof (fmt) + INT_STRLEN_BOUND (n));
     sprintf (buf, fmt, n);
+    errno = ret;
     return buf;
   }
 }
diff --git a/lib/strerror_r.c b/lib/strerror_r.c
index fe1185b..0f05b94 100644
--- a/lib/strerror_r.c
+++ b/lib/strerror_r.c
@@ -418,6 +418,7 @@ strerror_r (int errnum, char *buf, size_t buflen)

   {
     int ret;
+    int saved_errno = errno;

 #if USE_SYSTEM_STRERROR_R

@@ -519,7 +520,6 @@ strerror_r (int errnum, char *buf, size_t buflen)
     if (errnum >= 0 && errnum < sys_nerr)
       {
 #  if HAVE_CATGETS && (defined __NetBSD__ || defined __hpux)
-        int saved_errno = errno;
 #   if defined __NetBSD__
         nl_catd catd = catopen ("libc", NL_CAT_LOCALE);
         const char *errmsg =
@@ -554,7 +554,6 @@ strerror_r (int errnum, char *buf, size_t buflen)
 #  if HAVE_CATGETS && (defined __NetBSD__ || defined __hpux)
         if (catd != (nl_catd)-1)
           catclose (catd);
-        errno = saved_errno;
 #  endif
       }
     else
@@ -618,6 +617,7 @@ strerror_r (int errnum, char *buf, size_t buflen)

 #endif

+    errno = saved_errno;
     return ret;
   }
 }
diff --git a/tests/test-strerror_r.c b/tests/test-strerror_r.c
index 7aad3c7..4828767 100644
--- a/tests/test-strerror_r.c
+++ b/tests/test-strerror_r.c
@@ -34,35 +34,45 @@ main (void)

   /* Test results with valid errnum and enough room.  */

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

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

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

   /* POSIX requires strerror (0) to succeed.  Reject use of "Unknown
      error", but allow "Success", "No error", or even Solaris' "Error
      0" which are distinct patterns from true out-of-range strings.
      http://austingroupbugs.net/view.php?id=382  */
+  errno = 0;
   buf[0] = '\0';
   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.  */

+  errno = 0;
   buf[0] = '^';
   ret = strerror_r (-3, buf, sizeof (buf));
   ASSERT (ret == 0 || ret == EINVAL);
   if (ret == 0)
     ASSERT (buf[0] != '^');
+  ASSERT (errno == 0);

   /* Test results with a too small buffer.  */

@@ -74,7 +84,9 @@ main (void)
     for (i = 0; i <= len; i++)
       {
         strcpy (buf, "BADFACE");
+        errno = 0;
         ret = strerror_r (EACCES, buf, i);
+        ASSERT (errno == 0);
         if (ret == 0)
           {
             /* Truncated result.  POSIX allows this, and it actually
@@ -90,8 +102,10 @@ main (void)
       }

     strcpy (buf, "BADFACE");
+    errno = 0;
     ret = strerror_r (EACCES, buf, len + 1);
     ASSERT (ret == 0);
+    ASSERT (errno == 0);
   }

   return 0;
-- 
1.7.4.4