diff mbox

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

Message ID AANLkTi=bnvGpgQMthCDsNimeV++y=X_zDVjkMS6cyeXA@mail.gmail.com
State New
Headers show

Commit Message

Janne Blomqvist Jan. 21, 2011, 5:42 p.m. UTC 
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?

Comments

Jerry DeLisle Jan. 21, 2011, 7:29 p.m. UTC | #1 
On 01/21/2011 09:42 AM, Janne Blomqvist wrote:
> 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?
>
OK, thanks.

Jerry
Janne Blomqvist Jan. 21, 2011, 10:42 p.m. UTC | #2 
On Fri, Jan 21, 2011 at 21:29, Jerry DeLisle <jvdelisle@frontier.com> wrote:
> On 01/21/2011 09:42 AM, Janne Blomqvist wrote:
>>
>> 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?
>>
> OK, thanks.
>
> Jerry

Thanks,

Sending        libgfortran/ChangeLog
Sending        libgfortran/config.h.in
Sending        libgfortran/configure
Sending        libgfortran/configure.ac
Sending        libgfortran/intrinsics/gerror.c
Sending        libgfortran/io/unix.c
Sending        libgfortran/libgfortran.h
Sending        libgfortran/runtime/error.c
Transmitting file data ........
Committed revision 169110.
diff mbox

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..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);