Re: [Patch, libfortran] PR46267 strerror() might not be thread-safe, take 2

[Janne Blomqvist <blomqvist dot janne at gmail dot com>]

On Fri, Jan 21, 2011 at 19:36, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jan 21, 2011 at 07:24:56PM +0200, Janne Blomqvist wrote:
>> @@ -141,6 +141,38 @@ gfc_xtoa (GFC_UINTEGER_LARGEST n, char *buffer, size_t len)
>> Â Âreturn p;
>> Â}
>>
>> +
>> +/* Hopefully thread-safe wrapper for a strerror() style function. Â*/
>> +
>> +char *
>> +gf_strerror (int errnum,
>> +#ifdef HAVE_STRERROR_R
>> + Â Â Â Â Âchar * buf, size_t buflen)
>> +{
>> + Â/* TODO: How to prevent the compiler warning due to strerror_r of
>> + Â Â the untaken branch having the wrong return type? Â*/
>> + Âif (__builtin_classify_type (strerror_r (0, buf, 0)) == 5)
>> + Â Â{
>> + Â Â Â/* GNU strerror_r() Â*/
>> + Â Â Âreturn strerror_r (errnum, buf, buflen);
>> + Â Â}
>> + Âelse
>> + Â Â{
>> + Â Â Â/* POSIX strerror_r () Â*/
>> + Â Â Âstrerror_r (errnum, buf, buflen);
>> + Â Â Âreturn buf;
>> + Â Â}
>> +#else
>> + Â Â Â Â Â Â char * buf __attribute__((unused)),
>> + Â Â Â Â Âsize_t buflen __attribute__((unused)))
>> +{
>> + Â/* strerror () is not necessarily thread-safe, but should at least
>> + Â Â be available everywhere. Â*/
>> + Âreturn strerror (errnum);
>> +#endif
>> +}
>
> The #ifdef inside of the parameters ending inside function
> looks very ugly, you can use unused attribute unconditionally
> and start #ifdef only within function body.

Fair enough; updated patch attached. Otherwise no changes. Ok for trunk?



-- 
Janne Blomqvist

diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index 7b28f12..4f137e4 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -249,7 +249,7 @@ AC_CHECK_FUNCS(chdir strerror getlogin gethostname kill link symlink perror)
 AC_CHECK_FUNCS(sleep time ttyname signal alarm ctime clock access fork execl)
 AC_CHECK_FUNCS(wait setmode execvp pipe dup2 close fdopen strcasestr getrlimit)
 AC_CHECK_FUNCS(gettimeofday stat fstat lstat getpwuid vsnprintf dup getcwd)
-AC_CHECK_FUNCS(localtime_r gmtime_r)
+AC_CHECK_FUNCS(localtime_r gmtime_r strerror_r)
 
 # Check for glibc backtrace functions
 AC_CHECK_FUNCS(backtrace backtrace_symbols)
diff --git a/libgfortran/intrinsics/gerror.c b/libgfortran/intrinsics/gerror.c
index ccb5c3e..6feadc9 100644
--- a/libgfortran/intrinsics/gerror.c
+++ b/libgfortran/intrinsics/gerror.c
@@ -43,16 +43,17 @@ PREFIX(gerror) (char * msg, gfc_charlen_type msg_len)
   int p_len;
   char *p;
 
-  memset (msg, ' ', msg_len); /* Blank the string.  */
-
-  p = strerror (errno);
-  if (p == NULL)
-    return;
-
+  p = gf_strerror (errno, msg, msg_len);
   p_len = strlen (p);
-  if (msg_len < p_len)
-    memcpy (msg, p, msg_len);
-  else
-    memcpy (msg, p, p_len);
+  /* The returned pointer p might or might not be the same as the msg
+     argument.  */
+  if (p != msg)
+    {
+      if (msg_len < p_len)
+	p_len = msg_len;
+      memcpy (msg, p, p_len);
+    }
+  if (msg_len > p_len)
+    memset (&msg[p_len], ' ', msg_len - p_len);
 }
 #endif
diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index fa64e20..950b7a2 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -256,16 +256,6 @@ flush_if_preconnected (stream * s)
 }
 
 
-/* get_oserror()-- Get the most recent operating system error.  For
- * unix, this is errno. */
-
-const char *
-get_oserror (void)
-{
-  return strerror (errno);
-}
-
-
 /********************************************************************
 Raw I/O functions (read, write, seek, tell, truncate, close).
 
diff --git a/libgfortran/libgfortran.h b/libgfortran/libgfortran.h
index ac86492..c9d3f37 100644
--- a/libgfortran/libgfortran.h
+++ b/libgfortran/libgfortran.h
@@ -1,5 +1,6 @@
 /* Common declarations for all of libgfortran.
-   Copyright (C) 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010
+   Copyright (C) 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010,
+   2011
    Free Software Foundation, Inc.
    Contributed by Paul Brook <paul@nowt.org>, and
    Andy Vaught <andy@xena.eas.asu.edu>
@@ -738,9 +739,6 @@ extern void internal_error (st_parameter_common *, const char *)
   __attribute__ ((noreturn));
 internal_proto(internal_error);
 
-extern const char *get_oserror (void);
-internal_proto(get_oserror);
-
 extern const char *translate_error (int);
 internal_proto(translate_error);
 
@@ -756,6 +754,9 @@ internal_proto(notify_std);
 extern notification notification_std(int);
 internal_proto(notification_std);
 
+extern char *gf_strerror (int, char *, size_t);
+internal_proto(gf_strerror);
+
 /* fpu.c */
 
 extern void set_fpu (void);
diff --git a/libgfortran/runtime/error.c b/libgfortran/runtime/error.c
index 1baf9d3..06c144a 100644
--- a/libgfortran/runtime/error.c
+++ b/libgfortran/runtime/error.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 2002, 2003, 2005, 2006, 2007, 2009, 2010
+/* Copyright (C) 2002, 2003, 2005, 2006, 2007, 2009, 2010, 2011
    Free Software Foundation, Inc.
    Contributed by Andy Vaught
 
@@ -141,6 +141,36 @@ gfc_xtoa (GFC_UINTEGER_LARGEST n, char *buffer, size_t len)
   return p;
 }
 
+
+/* Hopefully thread-safe wrapper for a strerror_r() style function.  */
+
+char *
+gf_strerror (int errnum, 
+             char * buf __attribute__((unused)), 
+	     size_t buflen __attribute__((unused)))
+{
+#ifdef HAVE_STRERROR_R
+  /* TODO: How to prevent the compiler warning due to strerror_r of
+     the untaken branch having the wrong return type?  */
+  if (__builtin_classify_type (strerror_r (0, buf, 0)) == 5)
+    {
+      /* GNU strerror_r()  */
+      return strerror_r (errnum, buf, buflen);
+    }
+  else
+    {
+      /* POSIX strerror_r ()  */
+      strerror_r (errnum, buf, buflen);
+      return buf;
+    }
+#else
+  /* strerror () is not necessarily thread-safe, but should at least
+     be available everywhere.  */
+  return strerror (errnum);
+#endif
+}
+
+
 /* show_locus()-- Print a line number and filename describing where
  * something went wrong */
 
@@ -192,6 +222,8 @@ recursion_check (void)
 }
 
 
+#define STRERR_MAXSZ 256
+
 /* os_error()-- Operating system error.  We get a message from the
  * operating system, show it and leave.  Some operating system errors
  * are caught and processed by the library.  If not, we come here. */
@@ -199,8 +231,10 @@ recursion_check (void)
 void
 os_error (const char *message)
 {
+  char errmsg[STRERR_MAXSZ];
   recursion_check ();
-  st_printf ("Operating system error: %s\n%s\n", get_oserror (), message);
+  st_printf ("Operating system error: %s\n%s\n", 
+	     gf_strerror (errno, errmsg, STRERR_MAXSZ), message);
   sys_exit (1);
 }
 iexport(os_error);
@@ -389,6 +423,7 @@ translate_error (int code)
 void
 generate_error (st_parameter_common *cmp, int family, const char *message)
 {
+  char errmsg[STRERR_MAXSZ];
 
   /* If there was a previous error, don't mask it with another
      error message, EOF or EOR condition.  */
@@ -402,7 +437,8 @@ generate_error (st_parameter_common *cmp, int family, const char *message)
 
   if (message == NULL)
     message =
-      (family == LIBERROR_OS) ? get_oserror () : translate_error (family);
+      (family == LIBERROR_OS) ? gf_strerror (errno, errmsg, STRERR_MAXSZ) :
+      translate_error (family);
 
   if (cmp->flags & IOPARM_HAS_IOMSG)
     cf_strcpy (cmp->iomsg, cmp->iomsg_len, message);