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

login
register
mail settings
Submitter Janne Blomqvist
Date Jan. 20, 2011, 6:11 p.m.
Message ID <AANLkTik9-NJ93hozTxpgasrO=v=qJBJcTtW3zoNe23Jy@mail.gmail.com>
Download mbox | patch
Permalink /patch/79746/
State New
Headers show

Comments

Janne Blomqvist - Jan. 20, 2011, 6:11 p.m.
Hello,

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.

Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
Janne Blomqvist - Jan. 21, 2011, 1 p.m.
On Thu, Jan 20, 2011 at 20:11, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> Hello,
>
> 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.
>
> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?

Some googling suggest TLS is not supported on all targets, so I'm
withdrawing this patch and coming up with another solution.
Jack Howarth - Jan. 21, 2011, 1:55 p.m.
On Fri, Jan 21, 2011 at 03:00:11PM +0200, Janne Blomqvist wrote:
> On Thu, Jan 20, 2011 at 20:11, Janne Blomqvist
> <blomqvist.janne@gmail.com> wrote:
> > Hello,
> >
> > 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.
> >
> > Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
> 
> Some googling suggest TLS is not supported on all targets, so I'm
> withdrawing this patch and coming up with another solution.
> 

Janne,
   Which targets don't have any form of tls? I thought all of the
targets currently had either tls or emutls support.
             Jack

> -- 
> Janne Blomqvist
Jakub Jelinek - Jan. 21, 2011, 1:58 p.m.
On Fri, Jan 21, 2011 at 08:55:29AM -0500, Jack Howarth wrote:
> On Fri, Jan 21, 2011 at 03:00:11PM +0200, Janne Blomqvist wrote:
> > On Thu, Jan 20, 2011 at 20:11, Janne Blomqvist
> > <blomqvist.janne@gmail.com> wrote:
> > > 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.
> > >
> > > Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
> > 
> > Some googling suggest TLS is not supported on all targets, so I'm
> > withdrawing this patch and coming up with another solution.
> > 
> 
>    Which targets don't have any form of tls? I thought all of the
> targets currently had either tls or emutls support.

Even if all of them do now, unless public API of libgfortran requires
it, using TLS for this is a big waste of resources when it is much
better to change the interla gfc_strerror API to something similar to either
POSIX or GNU strerror_r and adjust the callers to supply a buffer on their
stacks.

	Jakub
Janne Blomqvist - Jan. 21, 2011, 2:22 p.m.
On Fri, Jan 21, 2011 at 15:58, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jan 21, 2011 at 08:55:29AM -0500, Jack Howarth wrote:
>> On Fri, Jan 21, 2011 at 03:00:11PM +0200, Janne Blomqvist wrote:
>> > On Thu, Jan 20, 2011 at 20:11, Janne Blomqvist
>> > <blomqvist.janne@gmail.com> wrote:
>> > > 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.
>> > >
>> > > Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
>> >
>> > Some googling suggest TLS is not supported on all targets, so I'm
>> > withdrawing this patch and coming up with another solution.
>> >
>>
>>    Which targets don't have any form of tls? I thought all of the
>> targets currently had either tls or emutls support.

My quick googling came up with a number of posts that suggested TLS
doesn't work with Apple gcc on OSX. Perhaps FSF gcc works, I don't
know, nor do I know if other targets have similar problems. In any
case, considering that this is just a libgfortran internal API with
relatively few callers, IMHO it's safer to change the API rather than
risk causing problems on targets for which I have neither the means
nor any particular interest to fix.

> Even if all of them do now, unless public API of libgfortran requires
> it, using TLS for this is a big waste of resources when it is much
> better to change the interla gfc_strerror API to something similar to either
> POSIX or GNU strerror_r and adjust the callers to supply a buffer on their
> stacks.

Yes, this was exactly what I had in mind; the GNU strerror_r API
should work nicely both for the strerror_r() backed versions and the
strerror() fallback.
Jack Howarth - Jan. 21, 2011, 3:09 p.m.
On Fri, Jan 21, 2011 at 04:22:59PM +0200, Janne Blomqvist wrote:
> On Fri, Jan 21, 2011 at 15:58, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Fri, Jan 21, 2011 at 08:55:29AM -0500, Jack Howarth wrote:
> >> On Fri, Jan 21, 2011 at 03:00:11PM +0200, Janne Blomqvist wrote:
> >> > On Thu, Jan 20, 2011 at 20:11, Janne Blomqvist
> >> > <blomqvist.janne@gmail.com> wrote:
> >> > > 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.
> >> > >
> >> > > Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
> >> >
> >> > Some googling suggest TLS is not supported on all targets, so I'm
> >> > withdrawing this patch and coming up with another solution.
> >> >
> >>
> >>    Which targets don't have any form of tls? I thought all of the
> >> targets currently had either tls or emutls support.
> 
> My quick googling came up with a number of posts that suggested TLS
> doesn't work with Apple gcc on OSX. Perhaps FSF gcc works, I don't
> know, nor do I know if other targets have similar problems. In any
> case, considering that this is just a libgfortran internal API with
> relatively few callers, IMHO it's safer to change the API rather than
> risk causing problems on targets for which I have neither the means
> nor any particular interest to fix.

Janne,
  There is complete support for emutls on darwin for emutls. The tls testsuite
was revised to test exhaustively on emutls as well. Also, emutls was reworked
in gcc 4.6 to be fully compatible with lto.
       Jack

> 
> > Even if all of them do now, unless public API of libgfortran requires
> > it, using TLS for this is a big waste of resources when it is much
> > better to change the interla gfc_strerror API to something similar to either
> > POSIX or GNU strerror_r and adjust the callers to supply a buffer on their
> > stacks.
> 
> Yes, this was exactly what I had in mind; the GNU strerror_r API
> should work nicely both for the strerror_r() backed versions and the
> strerror() fallback.
> 
> 
> -- 
> Janne Blomqvist

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..44873f9 100644
--- a/libgfortran/intrinsics/gerror.c
+++ b/libgfortran/intrinsics/gerror.c
@@ -45,14 +45,13 @@  PREFIX(gerror) (char * msg, gfc_charlen_type msg_len)
 
   memset (msg, ' ', msg_len); /* Blank the string.  */
 
-  p = strerror (errno);
+  p = gf_strerror (errno);
   if (p == NULL)
     return;
 
   p_len = strlen (p);
   if (msg_len < p_len)
-    memcpy (msg, p, msg_len);
-  else
-    memcpy (msg, p, p_len);
+    p_len = msg_len;
+  memcpy (msg, p, p_len);
 }
 #endif
diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index fa64e20..b7eba0b 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -262,7 +262,7 @@  flush_if_preconnected (stream * s)
 const char *
 get_oserror (void)
 {
-  return strerror (errno);
+  return gf_strerror (errno);
 }
 
 
diff --git a/libgfortran/libgfortran.h b/libgfortran/libgfortran.h
index ac86492..d092a54 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>
@@ -756,6 +757,9 @@  internal_proto(notify_std);
 extern notification notification_std(int);
 internal_proto(notification_std);
 
+extern char *gf_strerror (int);
+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..a04f2c5 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() style function.  */
+
+char *
+gf_strerror (int errnum)
+{
+#ifdef HAVE_STRERROR_R
+#define GF_STRERROR_R_MAXSZ 256
+  static __thread char msg[GF_STRERROR_R_MAXSZ];
+  /* 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, msg, 0)) == 5)
+    {
+      /* GNU strerror_r()  */
+      return strerror_r (errnum, msg, GF_STRERROR_R_MAXSZ);
+    }
+  else
+    {
+      /* POSIX strerror_r ()  */
+      strerror_r (errnum, msg, GF_STRERROR_R_MAXSZ);
+      return msg;
+    }
+#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 */