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

login
register
mail settings
Submitter Janne Blomqvist
Date Jan. 21, 2011, 5:24 p.m.
Message ID <AANLkTinhTAfgxwPnHEtBndLrR8mGco+nvXXC7vG_1jjC@mail.gmail.com>
Download mbox | patch
Permalink /patch/79871/
State New
Headers show

Comments

Janne Blomqvist - Jan. 21, 2011, 5:24 p.m.
Hi,

POSIX does not require strerror() to be thread-safe (although some
implementations are), using it is thus a potential error in
libgfortran. The attached patch fixes this in the case that
strerror_r() is available; thanks to Jakub for the hint about
__builtin_classify_type.

Compared to the previous version of the patch at

http://gcc.gnu.org/ml/gcc-patches/2011-01/msg01400.html

this version does not use TLS, instead the API of gf_strerror() is now
similar to GNU strerror_r() and callers have been updated to supply a
buffer (that might, or might not, be used).

Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
Jakub Jelinek - Jan. 21, 2011, 5:36 p.m.
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.

	Jakub

Patch

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..754a031 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,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
+}
+
+
 /* show_locus()-- Print a line number and filename describing where
  * something went wrong */
 
@@ -192,6 +224,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 +233,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 +425,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 +439,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);