[Patch, libfortran] PR 67414 Improve error handling

[Patch, libfortran] PR 67414 Improve error handling

[Janne Blomqvist]

[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]

Hi,

the attached patch improves the error handling for backtrace failing,
by printing the error number or the error string in addition to the
message. It also fixes a potential null pointer crash in gf_strerror.

Regtested on x86_64-pc-linux-gnu, Ok for trunk?

2015-09-01  Janne Blomqvist  <jb@gcc.gnu.org>

    PR libfortran/67414
    * io/write.c (gfc_itoa): Move to runtime/string.c.
    * libgfortran.h (show_backtrace): Make arg bool.
    (gfc_itoa): New prototype.
    * runtime/backtrace.c (struct mystate): Change type of try_simple
    field, add in_signal_handler field.
    (error_callback): Print out error number, or if not in a signal
    handler, the error message.
    (show_backtrace): Change type of arg, change initialization of
    struct mystate.
    (backtrace): Call show_backtrace with correct arg type.
    * runtime/compile_options.c (backtrace_handler): Call with correct
    arg type.
    * runtime/error.c (sys_abort): Likewise.
    (gf_strerror): Handle newlocale() failure.
    * runtime/string.c (gfc_itoa): Function moved here from
    io/write.c.


-- 
Janne Blomqvist

[-- Attachment #2: allocbt.diff --]
[-- Type: text/plain, Size: 7420 bytes --]

diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
index 7599659..e226236 100644
--- a/libgfortran/io/write.c
+++ b/libgfortran/io/write.c
@@ -1032,47 +1032,6 @@ ztoa_big (const char *s, char *buffer, int len, GFC_UINTEGER_LARGEST *n)
   return buffer;
 }
 
-/* gfc_itoa()-- Integer to decimal conversion.
-   The itoa function is a widespread non-standard extension to standard
-   C, often declared in <stdlib.h>.  Even though the itoa defined here
-   is a static function we take care not to conflict with any prior
-   non-static declaration.  Hence the 'gfc_' prefix, which is normally
-   reserved for functions with external linkage.  */
-
-static const char *
-gfc_itoa (GFC_INTEGER_LARGEST n, char *buffer, size_t len)
-{
-  int negative;
-  char *p;
-  GFC_UINTEGER_LARGEST t;
-
-  assert (len >= GFC_ITOA_BUF_SIZE);
-
-  if (n == 0)
-    return "0";
-
-  negative = 0;
-  t = n;
-  if (n < 0)
-    {
-      negative = 1;
-      t = -n; /*must use unsigned to protect from overflow*/
-    }
-
-  p = buffer + GFC_ITOA_BUF_SIZE - 1;
-  *p = '\0';
-
-  while (t != 0)
-    {
-      *--p = '0' + (t % 10);
-      t /= 10;
-    }
-
-  if (negative)
-    *--p = '-';
-  return p;
-}
-
 
 void
 write_i (st_parameter_dt *dtp, const fnode *f, const char *p, int len)
diff --git a/libgfortran/libgfortran.h b/libgfortran/libgfortran.h
index 553cef1..3eb0d85 100644
--- a/libgfortran/libgfortran.h
+++ b/libgfortran/libgfortran.h
@@ -651,7 +651,7 @@ export_proto(store_exe_path);
 
 /* backtrace.c */
 
-extern void show_backtrace (int);
+extern void show_backtrace (bool);
 internal_proto(show_backtrace);
 
 
@@ -838,6 +838,9 @@ internal_proto(fc_strdup);
 extern char *fc_strdup_notrim(const char *, gfc_charlen_type);
 internal_proto(fc_strdup_notrim);
 
+extern const char *gfc_itoa(GFC_INTEGER_LARGEST, char *, size_t);
+internal_proto(gfc_itoa);
+
 /* io/intrinsics.c */
 
 extern void flush_all_units (void);
diff --git a/libgfortran/runtime/backtrace.c b/libgfortran/runtime/backtrace.c
index 0d7c1fc..3c81de6 100644
--- a/libgfortran/runtime/backtrace.c
+++ b/libgfortran/runtime/backtrace.c
@@ -26,6 +26,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 #include <string.h>
 #include <stdlib.h>
+#include <errno.h>
 
 #ifdef HAVE_UNISTD_H
 #include <unistd.h>
@@ -38,8 +39,9 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 /* Store our own state while backtracing.  */
 struct mystate
 {
-  int try_simple;
   int frame;
+  bool try_simple;
+  bool in_signal_handler;
 };
 
 
@@ -65,15 +67,27 @@ static void
 error_callback (void *data, const char *msg, int errnum)
 {
   struct mystate *state = (struct mystate *) data;
+  char errbuf[256];
+
   if (errnum < 0)
     {
-      state->try_simple = 1;
+      state->try_simple = true;
       return;
     }
 
-  estr_write ("\nSomething went wrong while printing the backtrace: ");
-  estr_write (msg);
-  estr_write ("\n");
+  if (state->in_signal_handler)
+    {
+      estr_write ("\nSomething went wrong while printing the backtrace: ");
+      estr_write (msg);
+      estr_write (", errno: ");
+      char *p = gfc_itoa (errnum, errbuf, sizeof (errbuf));
+      estr_write (p);
+      estr_write ("\n");
+    }
+  else
+    st_printf("\nSomething went wrong while printing "
+	      "the backtrace: %s: %s\n", msg,
+	      gf_strerror (errnum, errbuf, sizeof (errbuf)));
 }
 
 static int
@@ -110,10 +124,10 @@ full_callback (void *data, uintptr_t pc, const char *filename,
 /* Display the backtrace.  */
 
 void
-show_backtrace (int in_signal_handler)
+show_backtrace (bool in_signal_handler)
 {
   struct backtrace_state *lbstate;
-  struct mystate state = { 0, 0 };
+  struct mystate state = { 0, false, in_signal_handler };
  
   lbstate = backtrace_create_state (NULL, 1, error_callback, NULL);
 
@@ -147,6 +161,6 @@ export_proto (backtrace);
 void
 backtrace (void)
 {
-  show_backtrace (0);
+  show_backtrace (false);
 }
 
diff --git a/libgfortran/runtime/compile_options.c b/libgfortran/runtime/compile_options.c
index f44256b..087c070 100644
--- a/libgfortran/runtime/compile_options.c
+++ b/libgfortran/runtime/compile_options.c
@@ -126,7 +126,7 @@ backtrace_handler (int signum)
 
   show_signal (signum);
   estr_write ("\nBacktrace for this error:\n");
-  show_backtrace (1);
+  show_backtrace (true);
 
   /* Now reraise the signal.  We reactivate the signal's
      default handling, which is to terminate the process.
diff --git a/libgfortran/runtime/error.c b/libgfortran/runtime/error.c
index 9eb0764..4aabe4a 100644
--- a/libgfortran/runtime/error.c
+++ b/libgfortran/runtime/error.c
@@ -173,7 +173,7 @@ sys_abort (void)
       || (options.backtrace == -1 && compile_options.backtrace == 1))
     {
       estr_write ("\nProgram aborted. Backtrace:\n");
-      show_backtrace (0);
+      show_backtrace (false);
       signal (SIGABRT, SIG_DFL);
     }
 
@@ -221,8 +221,16 @@ gf_strerror (int errnum,
 #ifdef HAVE_STRERROR_L
   locale_t myloc = newlocale (LC_CTYPE_MASK | LC_MESSAGES_MASK, "",
 			      (locale_t) 0);
-  char *p = strerror_l (errnum, myloc);
-  freelocale (myloc);
+  char *p;
+  if (myloc)
+    {
+      p = strerror_l (errnum, myloc);
+      freelocale (myloc);
+    }
+  else
+    /* newlocale might fail e.g. due to running out of memory, fall
+       back to the simpler strerror.  */
+    p = strerror (errnum);
   return p;
 #elif defined(HAVE_STRERROR_R)
 #ifdef HAVE_USELOCALE
diff --git a/libgfortran/runtime/string.c b/libgfortran/runtime/string.c
index 3c339da..eec190b 100644
--- a/libgfortran/runtime/string.c
+++ b/libgfortran/runtime/string.c
@@ -1,7 +1,7 @@
 /* Copyright (C) 2002-2015 Free Software Foundation, Inc.
    Contributed by Paul Brook
 
-This file is part of the GNU Fortran 95 runtime library (libgfortran).
+This file is part of the GNU Fortran runtime library (libgfortran).
 
 Libgfortran is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License as published by
@@ -25,6 +25,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #include "libgfortran.h"
 #include <string.h>
 #include <stdlib.h>
+#include <assert.h>
 
 
 /* Given a fortran string, return its length exclusive of the trailing
@@ -167,3 +168,47 @@ find_option (st_parameter_common *cmp, const char *s1, gfc_charlen_type s1_len,
 
   return -1;
 }
+
+
+/* gfc_itoa()-- Integer to decimal conversion.
+   The itoa function is a widespread non-standard extension to
+   standard C, often declared in <stdlib.h>.  Even though the itoa
+   defined here is a static function we take care not to conflict with
+   any prior non-static declaration.  Hence the 'gfc_' prefix, which
+   is normally reserved for functions with external linkage.  Notably,
+   in contrast to the *printf() family of functions, this ought to be
+   async-signal-safe.  */
+
+const char *
+gfc_itoa (GFC_INTEGER_LARGEST n, char *buffer, size_t len)
+{
+  int negative;
+  char *p;
+  GFC_UINTEGER_LARGEST t;
+
+  assert (len >= GFC_ITOA_BUF_SIZE);
+
+  if (n == 0)
+    return "0";
+
+  negative = 0;
+  t = n;
+  if (n < 0)
+    {
+      negative = 1;
+      t = -n; /*must use unsigned to protect from overflow*/
+    }
+
+  p = buffer + GFC_ITOA_BUF_SIZE - 1;
+  *p = '\0';
+
+  while (t != 0)
+    {
+      *--p = '0' + (t % 10);
+      t /= 10;
+    }
+
+  if (negative)
+    *--p = '-';
+  return p;
+}

Re: [Patch, libfortran] PR 67414 Improve error handling

[Janne Blomqvist]

[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]

On Tue, Sep 1, 2015 at 1:04 AM, FX <fxcoudert@gmail.com> wrote:
>> the attached patch improves the error handling for backtrace failing,
>> by printing the error number or the error string in addition to the
>> message. It also fixes a potential null pointer crash in gf_strerror.
>>
>> Regtested on x86_64-pc-linux-gnu, Ok for trunk?
>
> Mostly OK. I have one question, though: in the specific case from the PR, the error in the backtrace is an OS error (allocating memory), which had set the errno variable. But in other cases, it is (I think) possible that libbacktrace itself fails and call error_callback() without errno being set. In that case, the errno message will be confusing.

Hmm, in that case errnum must be set to 0. What about the attached
patch, which prints the existing message if errnum == 0, and the new
and improved only for errnum > 0?

>
> So, for that part of the patch, I think it would make more sense to change libbacktrace to give out a reasonable error message (and not “mmap”). A quick grep through the libbacktrace sources reveal that most calls to error_callback() actually provide insightful error messages. The ones that need to be improved are:
>
> alloc.c:    error_callback (data, "malloc", errno);
> alloc.c:          error_callback (data, "realloc", errno);
> alloc.c:      error_callback (data, "realloc", errno);
> mmap.c: error_callback (data, "mmap", errno);
> mmapio.c:      error_callback (data, "mmap", errno);
> mmapio.c:    error_callback (data, "munmap", errno);
> posix.c:      error_callback (data, "close", errno);
> read.c:      error_callback (data, "lseek", errno);
> read.c:      error_callback (data, "read", errno);

This seems to be the unix tradition of terse error messages, I suppose
it depends on other libbacktrace users whether such a change would be
seen as desirable..


-- 
Janne Blomqvist

[-- Attachment #2: allocbt2.diff --]
[-- Type: text/plain, Size: 7251 bytes --]

diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
index 7599659..e226236 100644
--- a/libgfortran/io/write.c
+++ b/libgfortran/io/write.c
@@ -1032,47 +1032,6 @@ ztoa_big (const char *s, char *buffer, int len, GFC_UINTEGER_LARGEST *n)
   return buffer;
 }
 
-/* gfc_itoa()-- Integer to decimal conversion.
-   The itoa function is a widespread non-standard extension to standard
-   C, often declared in <stdlib.h>.  Even though the itoa defined here
-   is a static function we take care not to conflict with any prior
-   non-static declaration.  Hence the 'gfc_' prefix, which is normally
-   reserved for functions with external linkage.  */
-
-static const char *
-gfc_itoa (GFC_INTEGER_LARGEST n, char *buffer, size_t len)
-{
-  int negative;
-  char *p;
-  GFC_UINTEGER_LARGEST t;
-
-  assert (len >= GFC_ITOA_BUF_SIZE);
-
-  if (n == 0)
-    return "0";
-
-  negative = 0;
-  t = n;
-  if (n < 0)
-    {
-      negative = 1;
-      t = -n; /*must use unsigned to protect from overflow*/
-    }
-
-  p = buffer + GFC_ITOA_BUF_SIZE - 1;
-  *p = '\0';
-
-  while (t != 0)
-    {
-      *--p = '0' + (t % 10);
-      t /= 10;
-    }
-
-  if (negative)
-    *--p = '-';
-  return p;
-}
-
 
 void
 write_i (st_parameter_dt *dtp, const fnode *f, const char *p, int len)
diff --git a/libgfortran/libgfortran.h b/libgfortran/libgfortran.h
index 553cef1..3eb0d85 100644
--- a/libgfortran/libgfortran.h
+++ b/libgfortran/libgfortran.h
@@ -651,7 +651,7 @@ export_proto(store_exe_path);
 
 /* backtrace.c */
 
-extern void show_backtrace (int);
+extern void show_backtrace (bool);
 internal_proto(show_backtrace);
 
 
@@ -838,6 +838,9 @@ internal_proto(fc_strdup);
 extern char *fc_strdup_notrim(const char *, gfc_charlen_type);
 internal_proto(fc_strdup_notrim);
 
+extern const char *gfc_itoa(GFC_INTEGER_LARGEST, char *, size_t);
+internal_proto(gfc_itoa);
+
 /* io/intrinsics.c */
 
 extern void flush_all_units (void);
diff --git a/libgfortran/runtime/backtrace.c b/libgfortran/runtime/backtrace.c
index 0d7c1fc..12ad76a 100644
--- a/libgfortran/runtime/backtrace.c
+++ b/libgfortran/runtime/backtrace.c
@@ -26,6 +26,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 #include <string.h>
 #include <stdlib.h>
+#include <errno.h>
 
 #ifdef HAVE_UNISTD_H
 #include <unistd.h>
@@ -38,8 +39,9 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 /* Store our own state while backtracing.  */
 struct mystate
 {
-  int try_simple;
   int frame;
+  bool try_simple;
+  bool in_signal_handler;
 };
 
 
@@ -65,15 +67,35 @@ static void
 error_callback (void *data, const char *msg, int errnum)
 {
   struct mystate *state = (struct mystate *) data;
+#define ERRHDR "\nCould not print backtrace: "
+
   if (errnum < 0)
     {
-      state->try_simple = 1;
+      state->try_simple = true;
       return;
     }
-
-  estr_write ("\nSomething went wrong while printing the backtrace: ");
-  estr_write (msg);
-  estr_write ("\n");
+  else if (errnum == 0)
+    {
+      estr_write (ERRHDR);
+      estr_write (msg);
+      estr_write ("\n");
+    }
+  else
+    {
+      char errbuf[256];
+      if (state->in_signal_handler)
+	{
+	  estr_write (ERRHDR);
+	  estr_write (msg);
+	  estr_write (", errno: ");
+	  const char *p = gfc_itoa (errnum, errbuf, sizeof (errbuf));
+	  estr_write (p);
+	  estr_write ("\n");
+	}
+      else
+	st_printf (ERRHDR "%s: %s\n", msg,
+		  gf_strerror (errnum, errbuf, sizeof (errbuf)));
+    }
 }
 
 static int
@@ -110,10 +132,10 @@ full_callback (void *data, uintptr_t pc, const char *filename,
 /* Display the backtrace.  */
 
 void
-show_backtrace (int in_signal_handler)
+show_backtrace (bool in_signal_handler)
 {
   struct backtrace_state *lbstate;
-  struct mystate state = { 0, 0 };
+  struct mystate state = { 0, false, in_signal_handler };
  
   lbstate = backtrace_create_state (NULL, 1, error_callback, NULL);
 
@@ -147,6 +169,6 @@ export_proto (backtrace);
 void
 backtrace (void)
 {
-  show_backtrace (0);
+  show_backtrace (false);
 }
 
diff --git a/libgfortran/runtime/compile_options.c b/libgfortran/runtime/compile_options.c
index f44256b..087c070 100644
--- a/libgfortran/runtime/compile_options.c
+++ b/libgfortran/runtime/compile_options.c
@@ -126,7 +126,7 @@ backtrace_handler (int signum)
 
   show_signal (signum);
   estr_write ("\nBacktrace for this error:\n");
-  show_backtrace (1);
+  show_backtrace (true);
 
   /* Now reraise the signal.  We reactivate the signal's
      default handling, which is to terminate the process.
diff --git a/libgfortran/runtime/error.c b/libgfortran/runtime/error.c
index 9eb0764..4aabe4a 100644
--- a/libgfortran/runtime/error.c
+++ b/libgfortran/runtime/error.c
@@ -173,7 +173,7 @@ sys_abort (void)
       || (options.backtrace == -1 && compile_options.backtrace == 1))
     {
       estr_write ("\nProgram aborted. Backtrace:\n");
-      show_backtrace (0);
+      show_backtrace (false);
       signal (SIGABRT, SIG_DFL);
     }
 
@@ -221,8 +221,16 @@ gf_strerror (int errnum,
 #ifdef HAVE_STRERROR_L
   locale_t myloc = newlocale (LC_CTYPE_MASK | LC_MESSAGES_MASK, "",
 			      (locale_t) 0);
-  char *p = strerror_l (errnum, myloc);
-  freelocale (myloc);
+  char *p;
+  if (myloc)
+    {
+      p = strerror_l (errnum, myloc);
+      freelocale (myloc);
+    }
+  else
+    /* newlocale might fail e.g. due to running out of memory, fall
+       back to the simpler strerror.  */
+    p = strerror (errnum);
   return p;
 #elif defined(HAVE_STRERROR_R)
 #ifdef HAVE_USELOCALE
diff --git a/libgfortran/runtime/string.c b/libgfortran/runtime/string.c
index 3c339da..5bd0f61 100644
--- a/libgfortran/runtime/string.c
+++ b/libgfortran/runtime/string.c
@@ -1,7 +1,7 @@
 /* Copyright (C) 2002-2015 Free Software Foundation, Inc.
    Contributed by Paul Brook
 
-This file is part of the GNU Fortran 95 runtime library (libgfortran).
+This file is part of the GNU Fortran runtime library (libgfortran).
 
 Libgfortran is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License as published by
@@ -167,3 +167,48 @@ find_option (st_parameter_common *cmp, const char *s1, gfc_charlen_type s1_len,
 
   return -1;
 }
+
+
+/* gfc_itoa()-- Integer to decimal conversion.
+   The itoa function is a widespread non-standard extension to
+   standard C, often declared in <stdlib.h>.  Even though the itoa
+   defined here is a static function we take care not to conflict with
+   any prior non-static declaration.  Hence the 'gfc_' prefix, which
+   is normally reserved for functions with external linkage.  Notably,
+   in contrast to the *printf() family of functions, this ought to be
+   async-signal-safe.  */
+
+const char *
+gfc_itoa (GFC_INTEGER_LARGEST n, char *buffer, size_t len)
+{
+  int negative;
+  char *p;
+  GFC_UINTEGER_LARGEST t;
+
+  if (len < GFC_ITOA_BUF_SIZE)
+    sys_abort ();
+
+  if (n == 0)
+    return "0";
+
+  negative = 0;
+  t = n;
+  if (n < 0)
+    {
+      negative = 1;
+      t = -n; /*must use unsigned to protect from overflow*/
+    }
+
+  p = buffer + GFC_ITOA_BUF_SIZE - 1;
+  *p = '\0';
+
+  while (t != 0)
+    {
+      *--p = '0' + (t % 10);
+      t /= 10;
+    }
+
+  if (negative)
+    *--p = '-';
+  return p;
+}

Re: [Patch, libfortran] PR 67414 Improve error handling

[FX]

> Hmm, in that case errnum must be set to 0. What about the attached
> patch, which prints the existing message if errnum == 0, and the new
> and improved only for errnum > 0?

OK. Thanks for the patch.

FX