public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Make interrupting tab-completion safe.
@ 2011-06-11  0:19 Sterling Augustine
  2011-06-12 12:12 ` Jan Kratochvil
  0 siblings, 1 reply; 26+ messages in thread
From: Sterling Augustine @ 2011-06-11  0:19 UTC (permalink / raw)
  To: gdb-patches

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

As discussed on IRC, gdb can crash on the following sequence:

gdb <really big program>
b <tab><tab>
[ctrl-c before tab-completion is done]
b <tab>
(segmentation fault)

The problem comes because the dwarf2read.c tries to discover the full
linkage name of symbols, and assumes that it won't be interrupted.
But the *_type_print* and *_print_type* functions do contain calls to
QUIT.

I'm sure it also occurs at times other than tab-completion--any time a
psymtab being converted to a symtab is interrupted.

This patch adjusts the functions in question to conditionally call
quit based on the variable show, which is -1 when they are called to
discover the full linkage name--among other times.

Sterling

2011-06-10  Sterling Augustine  <saugustine@google.com>

	* typeprint.h (TYPE_PRINT_QUIT): New macro.
	* psymtab.c (map_symbol_filenames_psymtab): Call QUIT.
	* p-typeprint.c (pascal_type_print_varspec_prefix): Call
	TYPE_PRINT_QUIT instead of QUIT.
	(pascal_type_print_varspec_suffix): Likewise.
	(pascal_type_print_base): Likewise.
	* m2-typeprint.c (m2_print_type): Likewise.
	* jv-typeprint.c (java_type_print_base): Likewise.
	* f-typeprint.c: Include typeprint.h.
	(f_type_print_varspec_prefix): Call
	TYPE_PRINT_QUIT instead of QUIT.
	(f_type_print_varspec_suffix): Likewise.
	(f_type_print_base): Likewise.
	* c-typeprint.c (c_type_print_varspec_prefix): Likewise.
	(c_type_print_varspec_suffix): Likewise.
	(c_type_print_base): Likewise. Remove extraneous calls to QUIT.
	* ada-typeprint.c (print_enum_type): Add show parameter. Call
	TYPE_PRINT_QUIT.
	(ada_print_type): Likewise.

[-- Attachment #2: conditional-type-print-quit.patch --]
[-- Type: text/x-patch, Size: 9286 bytes --]

Index: ada-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-typeprint.c,v
retrieving revision 1.40
diff -u -r1.40 ada-typeprint.c
--- ada-typeprint.c	1 Jan 2011 15:32:56 -0000	1.40
+++ ada-typeprint.c	11 Jun 2011 00:07:16 -0000
@@ -271,7 +271,7 @@
 /* Print enumerated type TYPE on STREAM.  */
 
 static void
-print_enum_type (struct type *type, struct ui_file *stream)
+print_enum_type (struct type *type, struct ui_file *stream, int show)
 {
   int len = TYPE_NFIELDS (type);
   int i, lastval;
@@ -282,7 +282,7 @@
   lastval = 0;
   for (i = 0; i < len; i++)
     {
-      QUIT;
+      TYPE_PRINT_QUIT (show);
       if (i)
 	fprintf_filtered (stream, ", ");
       wrap_here ("    ");
@@ -570,7 +570,7 @@
 
   for (i = fld0; i <= fld1; i += 1)
     {
-      QUIT;
+      TYPE_PRINT_QUIT (show);
 
       if (ada_is_parent_field (type, i) || ada_is_ignored_field (type, i))
 	;
@@ -846,7 +846,7 @@
 	if (show < 0)
 	  fprintf_filtered (stream, "(...)");
 	else
-	  print_enum_type (type, stream);
+	  print_enum_type (type, stream, show);
 	break;
       case TYPE_CODE_STRUCT:
 	if (ada_is_array_descriptor_type (type))
Index: c-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/c-typeprint.c,v
retrieving revision 1.70
diff -u -r1.70 c-typeprint.c
--- c-typeprint.c	22 Mar 2011 17:35:22 -0000	1.70
+++ c-typeprint.c	11 Jun 2011 00:07:16 -0000
@@ -247,7 +247,7 @@
   if (TYPE_NAME (type) && show <= 0)
     return;
 
-  QUIT;
+  TYPE_PRINT_QUIT (show);
 
   switch (TYPE_CODE (type))
     {
@@ -613,7 +613,7 @@
   if (TYPE_NAME (type) && show <= 0)
     return;
 
-  QUIT;
+  TYPE_PRINT_QUIT (show);
 
   switch (TYPE_CODE (type))
     {
@@ -730,7 +730,7 @@
   int need_access_label = 0;
   int j, len2;
 
-  QUIT;
+  TYPE_PRINT_QUIT (show);
 
   wrap_here ("    ");
   if (type == NULL)
@@ -842,7 +842,6 @@
 
 	  if (TYPE_DECLARED_CLASS (type))
 	    {
-	      QUIT;
 	      len = TYPE_NFIELDS (type);
 	      for (i = TYPE_N_BASECLASSES (type); i < len; i++)
 		if (!TYPE_FIELD_PRIVATE (type, i))
@@ -850,7 +849,6 @@
 		    need_access_label = 1;
 		    break;
 		  }
-	      QUIT;
 	      if (!need_access_label)
 		{
 		  len2 = TYPE_NFN_FIELDS (type);
@@ -871,7 +869,6 @@
 	    }
 	  else
 	    {
-	      QUIT;
 	      len = TYPE_NFIELDS (type);
 	      for (i = TYPE_N_BASECLASSES (type); i < len; i++)
 		if (TYPE_FIELD_PRIVATE (type, i)
@@ -880,13 +877,12 @@
 		    need_access_label = 1;
 		    break;
 		  }
-	      QUIT;
 	      if (!need_access_label)
 		{
 		  len2 = TYPE_NFN_FIELDS (type);
 		  for (j = 0; j < len2; j++)
 		    {
-		      QUIT;
+		      TYPE_PRINT_QUIT (show);
 		      len = TYPE_FN_FIELDLIST_LENGTH (type, j);
 		      for (i = 0; i < len; i++)
 			if (TYPE_FN_FIELD_PROTECTED (TYPE_FN_FIELDLIST1 (type,
@@ -911,7 +907,7 @@
 	  vptr_fieldno = get_vptr_fieldno (type, &basetype);
 	  for (i = TYPE_N_BASECLASSES (type); i < len; i++)
 	    {
-	      QUIT;
+	      TYPE_PRINT_QUIT (show);
 
 	      /* If we have a virtual table pointer, omit it.  Even if
 		 virtual table pointers are not specifically marked in
@@ -1011,7 +1007,7 @@
 		  if (TYPE_FN_FIELD_ARTIFICIAL (f, j))
 		    continue;
 
-		  QUIT;
+		  TYPE_PRINT_QUIT (show);
 		  if (TYPE_FN_FIELD_PROTECTED (f, j))
 		    {
 		      if (section_type != s_protected)
@@ -1192,7 +1188,7 @@
 	  lastval = 0;
 	  for (i = 0; i < len; i++)
 	    {
-	      QUIT;
+	      TYPE_PRINT_QUIT (show);
 	      if (i)
 		fprintf_filtered (stream, ", ");
 	      wrap_here ("    ");
Index: f-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/f-typeprint.c,v
retrieving revision 1.35
diff -u -r1.35 f-typeprint.c
--- f-typeprint.c	7 Jan 2011 19:36:16 -0000	1.35
+++ f-typeprint.c	11 Jun 2011 00:07:16 -0000
@@ -32,6 +32,7 @@
 #include "gdbcore.h"
 #include "target.h"
 #include "f-lang.h"
+#include "typeprint.h"
 
 #include "gdb_string.h"
 #include <errno.h>
@@ -101,7 +102,7 @@
   if (TYPE_NAME (type) && show <= 0)
     return;
 
-  QUIT;
+  TYPE_PRINT_QUIT (show);
 
   switch (TYPE_CODE (type))
     {
@@ -163,7 +164,7 @@
   if (TYPE_NAME (type) && show <= 0)
     return;
 
-  QUIT;
+  TYPE_PRINT_QUIT (show);
 
   switch (TYPE_CODE (type))
     {
@@ -261,7 +262,7 @@
   int upper_bound;
   int index;
 
-  QUIT;
+  TYPE_PRINT_QUIT (show);
 
   wrap_here ("    ");
   if (type == NULL)
Index: jv-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/jv-typeprint.c,v
retrieving revision 1.22
diff -u -r1.22 jv-typeprint.c
--- jv-typeprint.c	9 Jan 2011 03:08:57 -0000	1.22
+++ jv-typeprint.c	11 Jun 2011 00:07:16 -0000
@@ -91,7 +91,7 @@
   char *mangled_name;
   char *demangled_name;
 
-  QUIT;
+  TYPE_PRINT_QUIT (show);
   wrap_here ("    ");
 
   if (type == NULL)
@@ -165,7 +165,7 @@
 	  len = TYPE_NFIELDS (type);
 	  for (i = TYPE_N_BASECLASSES (type); i < len; i++)
 	    {
-	      QUIT;
+              TYPE_PRINT_QUIT (show);
 	      /* Don't print out virtual function table.  */
 	      if (strncmp (TYPE_FIELD_NAME (type, i), "_vptr", 5) == 0
 		  && is_cplus_marker ((TYPE_FIELD_NAME (type, i))[5]))
@@ -239,7 +239,7 @@
                     = (is_constructor_name (physname)
                        || is_destructor_name (physname));
 
-		  QUIT;
+                  TYPE_PRINT_QUIT (show);
 
 		  print_spaces_filtered (level + 4, stream);
 
Index: m2-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/m2-typeprint.c,v
retrieving revision 1.28
diff -u -r1.28 m2-typeprint.c
--- m2-typeprint.c	9 Jan 2011 03:20:33 -0000	1.28
+++ m2-typeprint.c	11 Jun 2011 00:07:16 -0000
@@ -75,7 +75,7 @@
 
   CHECK_TYPEDEF (type);
 
-  QUIT;
+  TYPE_PRINT_QUIT (show);
 
   wrap_here ("    ");
   if (type == NULL)
@@ -560,7 +560,7 @@
 
       for (i = TYPE_N_BASECLASSES (type); i < len; i++)
 	{
-	  QUIT;
+          TYPE_PRINT_QUIT (show);
 
 	  print_spaces_filtered (level + 4, stream);
 	  fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
@@ -603,7 +603,7 @@
       lastval = 0;
       for (i = 0; i < len; i++)
 	{
-	  QUIT;
+          TYPE_PRINT_QUIT (show);
 	  if (i > 0)
 	    fprintf_filtered (stream, ", ");
 	  wrap_here ("    ");
Index: p-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/p-typeprint.c,v
retrieving revision 1.40
diff -u -r1.40 p-typeprint.c
--- p-typeprint.c	10 Mar 2011 20:25:44 -0000	1.40
+++ p-typeprint.c	11 Jun 2011 00:07:17 -0000
@@ -215,7 +215,7 @@
   if (TYPE_NAME (type) && show <= 0)
     return;
 
-  QUIT;
+  TYPE_PRINT_QUIT (show);
 
   switch (TYPE_CODE (type))
     {
@@ -349,7 +349,7 @@
   if (TYPE_NAME (type) && show <= 0)
     return;
 
-  QUIT;
+  TYPE_PRINT_QUIT (show);
 
   switch (TYPE_CODE (type))
     {
@@ -451,7 +451,7 @@
     }
   section_type;
 
-  QUIT;
+  TYPE_PRINT_QUIT (show);
   wrap_here ("    ");
   if (type == NULL)
     {
@@ -562,7 +562,8 @@
 	  len = TYPE_NFIELDS (type);
 	  for (i = TYPE_N_BASECLASSES (type); i < len; i++)
 	    {
-	      QUIT;
+              TYPE_PRINT_QUIT (show);
+
 	      /* Don't print out virtual function table.  */
 	      if ((strncmp (TYPE_FIELD_NAME (type, i), "_vptr", 5) == 0)
 		  && is_cplus_marker ((TYPE_FIELD_NAME (type, i))[5]))
@@ -643,7 +644,8 @@
 		  int is_constructor = (strncmp (physname, "__ct__", 6) == 0);
 		  int is_destructor = (strncmp (physname, "__dt__", 6) == 0);
 
-		  QUIT;
+                  TYPE_PRINT_QUIT (show);
+
 		  if (TYPE_FN_FIELD_PROTECTED (f, j))
 		    {
 		      if (section_type != s_protected)
@@ -747,7 +749,7 @@
 	  lastval = 0;
 	  for (i = 0; i < len; i++)
 	    {
-	      QUIT;
+              TYPE_PRINT_QUIT (show);
 	      if (i)
 		fprintf_filtered (stream, ", ");
 	      wrap_here ("    ");
Index: psymtab.c
===================================================================
RCS file: /cvs/src/src/gdb/psymtab.c,v
retrieving revision 1.26.2.1
diff -u -r1.26.2.1 psymtab.c
--- psymtab.c	20 Apr 2011 20:10:29 -0000	1.26.2.1
+++ psymtab.c	11 Jun 2011 00:07:17 -0000
@@ -1086,7 +1086,7 @@
 
       if (ps->readin)
 	continue;
-
+      QUIT;
       fullname = psymtab_to_fullname (ps);
       (*fun) (ps->filename, fullname, data);
     }
Index: typeprint.h
===================================================================
RCS file: /cvs/src/src/gdb/typeprint.h,v
retrieving revision 1.11
diff -u -r1.11 typeprint.h
--- typeprint.h	1 Jan 2011 15:33:18 -0000	1.11
+++ typeprint.h	11 Jun 2011 00:07:17 -0000
@@ -29,4 +29,16 @@
 				  int, int);
 
 void c_type_print_args (struct type *, struct ui_file *, int, enum language);
+
+/* The variable show will be negative when being used to print the
+   full linkage name of a variable (among other times). Code that
+   calls *_type_print* and *_print_type* to discover a full linkage
+   name assumes that the process will not be interrupted--especially
+   from inside dwarf2read.c.  Using this macro to control calls to
+   QUIT allows, for example, a ptype command to be interrupted safely,
+   but not other operations that cannot be interrupted safely.  */
+
+#define TYPE_PRINT_QUIT(show) \
+  do { if (show >= 0) QUIT; } while (0)
+
 #endif

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] Make interrupting tab-completion safe.
  2011-06-11  0:19 [PATCH] Make interrupting tab-completion safe Sterling Augustine
@ 2011-06-12 12:12 ` Jan Kratochvil
  2011-06-13 17:45   ` Sterling Augustine
  0 siblings, 1 reply; 26+ messages in thread
From: Jan Kratochvil @ 2011-06-12 12:12 UTC (permalink / raw)
  To: Sterling Augustine; +Cc: gdb-patches

On Sat, 11 Jun 2011 02:19:05 +0200, Sterling Augustine wrote:
> gdb <really big program>
> b <tab><tab>
> [ctrl-c before tab-completion is done]
> b <tab>
> (segmentation fault)

I do not have it reproducuble, does not happen for you for ./gdb -nx ./gdb?
Tried also Firefox, both processing runs long enough to CTRL-C it.

It would be good to have a testcase for regressions anyway.


> The problem comes because the dwarf2read.c tries to discover the full
> linkage name of symbols, and assumes that it won't be interrupted.

I had rather an idea to make it interruption-safe as this way another
forgotten or newly introduced QUIT may regression it.  But I do not have
a reproducer to check where the crash happens.


Thanks,
Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] Make interrupting tab-completion safe.
  2011-06-12 12:12 ` Jan Kratochvil
@ 2011-06-13 17:45   ` Sterling Augustine
  2011-06-26 22:22     ` [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.] Jan Kratochvil
  2011-07-11 18:53     ` [PATCH] Make interrupting tab-completion safe Sterling Augustine
  0 siblings, 2 replies; 26+ messages in thread
From: Sterling Augustine @ 2011-06-13 17:45 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: gdb-patches

On Sun, Jun 12, 2011 at 5:11 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Sat, 11 Jun 2011 02:19:05 +0200, Sterling Augustine wrote:
>> gdb <really big program>
>> b <tab><tab>
>> [ctrl-c before tab-completion is done]
>> b <tab>
>> (segmentation fault)
>
> I do not have it reproducuble, does not happen for you for ./gdb -nx ./gdb?
> Tried also Firefox, both processing runs long enough to CTRL-C it.

gdb itself isn't big enough to make it easy to reproduce (~24k
symbols). The smallest programs I see this on are about 5x that size
and don't use shared libraries. (Not sure about mozilla, or the
interaction between shared libraries and this.) Certainly, the bigger
the binary, the easier it is to reproduce. If you haven't noticed that
tab-completion (immediately on launch with no init script) is unusably
slow, then the binary probably isn't big enough to trip this bug
easily.

The race is between the conversion from psymtab to full symtab and you
hitting ctrl-c. Some of this time is also spent handling filename
completion, which doesn't have the problem. You need to be sure to
interrupt c_type_print_args, when called from dwarf2read.c:5049.

> It would be good to have a testcase for regressions anyway.

Is there an existing test-case I can model this one on? (One that
sends an asynchronous sigint to gdb is probably enough.) I can't seem
to find any, but my deja-gnu foo is weak.

>> The problem comes because the dwarf2read.c tries to discover the full
>> linkage name of symbols, and assumes that it won't be interrupted.
>
> I had rather an idea to make it interruption-safe as this way another
> forgotten or newly introduced QUIT may regression it.  But I do not have
> a reproducer to check where the crash happens.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [readline patch, gdb-7.3?] Avoid free from a signal handler  [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-06-13 17:45   ` Sterling Augustine
@ 2011-06-26 22:22     ` Jan Kratochvil
  2011-06-27 16:03       ` Joel Brobecker
  2011-06-29 13:54       ` [Bug-readline] " Chet Ramey
  2011-07-11 18:53     ` [PATCH] Make interrupting tab-completion safe Sterling Augustine
  1 sibling, 2 replies; 26+ messages in thread
From: Jan Kratochvil @ 2011-06-26 22:22 UTC (permalink / raw)
  To: bug-readline; +Cc: Sterling Augustine, gdb-patches

GDB reproducer for the readline bug:
------------------------------------

On Mon, 13 Jun 2011 19:44:57 +0200, Sterling Augustine wrote:
> gdb itself isn't big enough to make it easy to reproduce (~24k
> symbols).

The goal is to do very many memory allocation functions in GDB, no matter
which ones.  I found for example libwebkitgtk.so.debug as good sample data,
optionally added .gdb_index is OK for faster startup, it should contain C++
functions for more memory operations (therefore GDB itself is not usable as
the sample data).  Used this GDB .exp file, reproducible in several seconds:

set binfile "/home/jkratoch/t/rh575292-gdbindex.debug"
gdb_exit
gdb_start
gdb_load ${binfile}
while 1 {
    send_gdb "b \t\t"
    sleep 0.1
    send_gdb "\003"
    gdb_test "" "^b \\^CQuit" "Quit seen"
}

b ^CQuit
(gdb) PASS: gdb.base/quit.exp: Quit seen
b *** glibc detected *** /home/jkratoch/redhat/gdb-clean/gdb/testsuite/../../gdb/gdb: munmap_chunk(): invalid pointer: 0x0000000005b950b0 ***

One can also add stub expensive mallinfo calls into GDB xmalloc/xfree/etc. for
better reproducibility.


The readline bug:
-----------------

readline now calls memory allocation/free functions from the readline signal
handler.  This is not permitted by POSIX and it does corrupt memory such as
during SiGINT:

#0  __lll_lock_wait_private () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:100
#1  in _L_lock_10461 () from /lib64/libc.so.6
#2  in __libc_malloc (bytes=139919578014176) at malloc.c:3657
#3  in __libc_message (do_abort=2, fmt=0x7f41909aebe8 "*** glibc detected *** %s: %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:137
#4  in malloc_printerr (action=3, str=0x7f41909aec18 "munmap_chunk(): invalid pointer", ptr=<optimized out>) at malloc.c:6283
#5  in xfree (ptr=0x5b950b0) at utils.c:1303
#6  in rl_free_undo_list () at undo.c:119
#7  in rl_free_line_state () at signals.c:503
#8  in _rl_handle_signal (sig=2) at signals.c:188
#9  in rl_signal_handler (sig=2) at signals.c:149
#10 <signal handler called>
#11 _int_malloc (av=0x7f4190bea1e0, bytes=177) at malloc.c:4727
#12 in __libc_malloc (bytes=177) at malloc.c:3660
#13 in reallochook (ptr=<optimized out>, size=128, caller=0xd6f531) at mcheck.c:335
#14 in d_growable_string_resize (dgs=0x7fff34b0d150, need=122) at ./cp-demangle.c:3247
#15 in d_growable_string_init (dgs=0x7fff34b0d150, estimate=122) at ./cp-demangle.c:3226
#16 in cplus_demangle_print (options=3, dc=0x4114fe8, estimate=122, palc=0x7fff34b0d198) at ./cp-demangle.c:3416
#17 in cp_comp_to_string (result=0x4114fe8, estimated_len=122) at cp-name-parser.y:1965
#18 in cp_canonicalize_string (string=0x93f4e20 "WebCore::FrameLoader::loadProvisionalItemFromCachedPage(void)") at cp-support.c:139
[...]

Unfortunately it leaks a bit now and also for example during SIGWINCH readline
calls xmalloc which cannot be ignored/delayed so easily as xfree.

Please postpone any memory management function only after the signal handler.


Thanks,
Jan


readline/
2011-06-27  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Avoid free from a signal handler.
	* Makefile.in (xfree.o): Add readline.h.
	* xfree.c: Include stdio.h and readline.h.
	(xfree): Return on RL_STATE_SIGHANDLER.
	* xmalloc.h (xfree): New definition.

--- a/readline/Makefile.in
+++ b/readline/Makefile.in
@@ -422,7 +422,7 @@ vi_mode.o: rldefs.h ${BUILD_DIR}/config.h rlconf.h
 vi_mode.o: readline.h keymaps.h rltypedefs.h chardefs.h tilde.h
 vi_mode.o: history.h ansi_stdlib.h rlstdc.h
 xfree.o: ${BUILD_DIR}/config.h
-xfree.o: ansi_stdlib.h
+xfree.o: ansi_stdlib.h readline.h
 xmalloc.o: ${BUILD_DIR}/config.h
 xmalloc.o: ansi_stdlib.h
 
--- a/readline/xfree.c
+++ b/readline/xfree.c
@@ -31,7 +31,10 @@
 #  include "ansi_stdlib.h"
 #endif /* HAVE_STDLIB_H */
 
+#include <stdio.h>
+
 #include "xmalloc.h"
+#include "readline.h"
 
 /* **************************************************************** */
 /*								    */
@@ -45,6 +48,10 @@ void
 xfree (string)
      PTR_T string;
 {
+  /* Leak a bit.  */
+  if (RL_ISSTATE(RL_STATE_SIGHANDLER))
+    return;
+
   if (string)
     free (string);
 }
--- a/readline/xmalloc.h
+++ b/readline/xmalloc.h
@@ -38,6 +38,9 @@
 
 #endif /* !PTR_T */
 
+/* xmalloc and xrealloc should be also protected from RL_STATE_SIGHANDLER.  */
+#define xfree xfree_readline
+
 extern PTR_T xmalloc PARAMS((size_t));
 extern PTR_T xrealloc PARAMS((void *, size_t));
 extern void xfree PARAMS((void *));

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [readline patch, gdb-7.3?] Avoid free from a signal handler  [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-06-26 22:22     ` [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.] Jan Kratochvil
@ 2011-06-27 16:03       ` Joel Brobecker
  2011-06-29 21:49         ` Jan Kratochvil
  2011-06-29 13:54       ` [Bug-readline] " Chet Ramey
  1 sibling, 1 reply; 26+ messages in thread
From: Joel Brobecker @ 2011-06-27 16:03 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: bug-readline, Sterling Augustine, gdb-patches

> readline/
> 2011-06-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Avoid free from a signal handler.
> 	* Makefile.in (xfree.o): Add readline.h.
> 	* xfree.c: Include stdio.h and readline.h.
> 	(xfree): Return on RL_STATE_SIGHANDLER.
> 	* xmalloc.h (xfree): New definition.

Interesting. OK for gdb-7.3 if OK for readline...

-- 
Joel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-06-26 22:22     ` [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.] Jan Kratochvil
  2011-06-27 16:03       ` Joel Brobecker
@ 2011-06-29 13:54       ` Chet Ramey
  2011-06-29 20:35         ` Jan Kratochvil
  1 sibling, 1 reply; 26+ messages in thread
From: Chet Ramey @ 2011-06-29 13:54 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: bug-readline, gdb-patches, Sterling Augustine

On 6/26/11 6:21 PM, Jan Kratochvil wrote:
> GDB reproducer for the readline bug:
> ------------------------------------
> 
> On Mon, 13 Jun 2011 19:44:57 +0200, Sterling Augustine wrote:
>> gdb itself isn't big enough to make it easy to reproduce (~24k
>> symbols).
> 
> The goal is to do very many memory allocation functions in GDB, no matter
> which ones.  I found for example libwebkitgtk.so.debug as good sample data,
> optionally added .gdb_index is OK for faster startup, it should contain C++
> functions for more memory operations (therefore GDB itself is not usable as
> the sample data).  Used this GDB .exp file, reproducible in several seconds:
> 
> set binfile "/home/jkratoch/t/rh575292-gdbindex.debug"
> gdb_exit
> gdb_start
> gdb_load ${binfile}
> while 1 {
>     send_gdb "b \t\t"
>     sleep 0.1
>     send_gdb "\003"
>     gdb_test "" "^b \\^CQuit" "Quit seen"
> }
> 
> b ^CQuit
> (gdb) PASS: gdb.base/quit.exp: Quit seen
> b *** glibc detected *** /home/jkratoch/redhat/gdb-clean/gdb/testsuite/../../gdb/gdb: munmap_chunk(): invalid pointer: 0x0000000005b950b0 ***
> 
> One can also add stub expensive mallinfo calls into GDB xmalloc/xfree/etc. for
> better reproducibility.
> 
> 
> The readline bug:
> -----------------
> 
> readline now calls memory allocation/free functions from the readline signal
> handler.  This is not permitted by POSIX and it does corrupt memory such as
> during SiGINT:
> 
> #0  __lll_lock_wait_private () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:100
> #1  in _L_lock_10461 () from /lib64/libc.so.6
> #2  in __libc_malloc (bytes=139919578014176) at malloc.c:3657
> #3  in __libc_message (do_abort=2, fmt=0x7f41909aebe8 "*** glibc detected *** %s: %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:137
> #4  in malloc_printerr (action=3, str=0x7f41909aec18 "munmap_chunk(): invalid pointer", ptr=<optimized out>) at malloc.c:6283
> #5  in xfree (ptr=0x5b950b0) at utils.c:1303
> #6  in rl_free_undo_list () at undo.c:119
> #7  in rl_free_line_state () at signals.c:503
> #8  in _rl_handle_signal (sig=2) at signals.c:188
> #9  in rl_signal_handler (sig=2) at signals.c:149
> #10 <signal handler called>
> #11 _int_malloc (av=0x7f4190bea1e0, bytes=177) at malloc.c:4727
> #12 in __libc_malloc (bytes=177) at malloc.c:3660
> #13 in reallochook (ptr=<optimized out>, size=128, caller=0xd6f531) at mcheck.c:335
> #14 in d_growable_string_resize (dgs=0x7fff34b0d150, need=122) at ./cp-demangle.c:3247
> #15 in d_growable_string_init (dgs=0x7fff34b0d150, estimate=122) at ./cp-demangle.c:3226
> #16 in cplus_demangle_print (options=3, dc=0x4114fe8, estimate=122, palc=0x7fff34b0d198) at ./cp-demangle.c:3416
> #17 in cp_comp_to_string (result=0x4114fe8, estimated_len=122) at cp-name-parser.y:1965
> #18 in cp_canonicalize_string (string=0x93f4e20 "WebCore::FrameLoader::loadProvisionalItemFromCachedPage(void)") at cp-support.c:139
> [...]
> 
> Unfortunately it leaks a bit now and also for example during SIGWINCH readline
> calls xmalloc which cannot be ignored/delayed so easily as xfree.
> 
> Please postpone any memory management function only after the signal handler.

If you feel that you need to make this change for the current version of
gdb, go ahead.  I will keep going in the direction readline-6.2 began:
using the RL_CHECK_SIGNALS macro to run signal handling code outside the
signal handler and minimizing the use of _rl_interrupt_immediately without
destroying performance.

The big problem is to allow operations that read through the file system
(like completion) to be interrupted without running unsafe functions from
a signal handler.  Wrapping completion functions is the current remaining
use of _rl_interrupt_immediately.  It does the user no good to keep
setting the "I got SIGINT" flag if he's trying to complete files on a dead
or otherwise unreachable file server.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-06-29 13:54       ` [Bug-readline] " Chet Ramey
@ 2011-06-29 20:35         ` Jan Kratochvil
  2011-06-30 14:38           ` Chet Ramey
  0 siblings, 1 reply; 26+ messages in thread
From: Jan Kratochvil @ 2011-06-29 20:35 UTC (permalink / raw)
  To: Chet Ramey; +Cc: bug-readline, gdb-patches, Sterling Augustine

On Wed, 29 Jun 2011 15:54:11 +0200, Chet Ramey wrote:
> If you feel that you need to make this change for the current version of
> gdb, go ahead.

OK.  It is not a complete fix but hopefully the one people hit the most.
I have also seen unreproducible crash(es) on a completion CTRL-C myself.


> The big problem is to allow operations that read through the file system
> (like completion) to be interrupted without running unsafe functions from
> a signal handler.

I do not see the problem, readline already does not use SA_RESTART and the
application also gets SIGINT passed by _rl_handle_signal so it aborts the
completion reading on its own, as GDB does by the QUIT macro.


> It does the user no good to keep setting the "I got SIGINT" flag if he's
> trying to complete files on a dead or otherwise unreachable file server.

The syscall should get EINTR without SA_RESTART.  But I admit I may not see
the problem now.


Thanks,
Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [readline patch, gdb-7.3?] Avoid free from a signal handler  [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-06-27 16:03       ` Joel Brobecker
@ 2011-06-29 21:49         ` Jan Kratochvil
  0 siblings, 0 replies; 26+ messages in thread
From: Jan Kratochvil @ 2011-06-29 21:49 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Sterling Augustine, gdb-patches

On Mon, 27 Jun 2011 18:03:07 +0200, Joel Brobecker wrote:
> > readline/
> > 2011-06-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Avoid free from a signal handler.
> > 	* Makefile.in (xfree.o): Add readline.h.
> > 	* xfree.c: Include stdio.h and readline.h.
> > 	(xfree): Return on RL_STATE_SIGHANDLER.
> > 	* xmalloc.h (xfree): New definition.
> 
> Interesting. OK for gdb-7.3 if OK for readline...

Checked in the original patch for trunk:
	http://sourceware.org/ml/gdb-cvs/2011-06/msg00168.html

For gdb-7.3 it is more complicated as it still has readline-5.1, checked in
there the patch below, it is not nice but I find it OK for the branch.


Thanks,
Jan


gdb_7_3-branch:
http://sourceware.org/ml/gdb-cvs/2011-06/msg00169.html

--- src/readline/ChangeLog.gdb	2011/03/04 18:12:47	1.38
+++ src/readline/ChangeLog.gdb	2011/06/29 21:46:43	1.38.2.1
@@ -1,3 +1,12 @@
+2011-06-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Avoid free from a signal handler.
+	* Makefile.in (xmalloc.o): Add readline.h.
+	* xmalloc.c: Include readline.h.
+	(xmalloc, xrealloc): Disable them by #if 0.
+	(xfree): Return on RL_STATE_SIGHANDLER, #undef free.
+	* xmalloc.h (xfree, free): New definition.
+
 2011-03-04  Michael Snyder  <msnyder@vmware.com>
 
 	* bind.c (rl_function_dumper): Free allocated memory.
--- src/readline/Makefile.in	2009/07/30 22:53:17	1.11
+++ src/readline/Makefile.in	2011/06/29 21:46:43	1.11.8.1
@@ -417,7 +417,7 @@
 vi_mode.o: readline.h keymaps.h rltypedefs.h chardefs.h tilde.h
 vi_mode.o: history.h ansi_stdlib.h rlstdc.h
 xmalloc.o: ${BUILD_DIR}/config.h
-xmalloc.o: ansi_stdlib.h
+xmalloc.o: ansi_stdlib.h readline.h
 
 bind.o: rlshell.h
 histfile.o: rlshell.h
--- src/readline/xmalloc.c	2006/04/20 20:13:20	1.5
+++ src/readline/xmalloc.c	2011/06/29 21:46:43	1.5.40.1
@@ -33,6 +33,7 @@
 #endif /* HAVE_STDLIB_H */
 
 #include "xmalloc.h"
+#include "readline.h"
 
 /* **************************************************************** */
 /*								    */
@@ -40,6 +41,9 @@
 /*								    */
 /* **************************************************************** */
 
+/* xmalloc and xrealloc are provided by GDB.  */
+#if 0
+
 static void
 memory_error_and_abort (fname)
      char *fname;
@@ -77,12 +81,20 @@
   return (temp);
 }
 
+/* xmalloc and xrealloc are provided by GDB.  */
+#endif /* 0 */
+
 /* Use this as the function to call when adding unwind protects so we
    don't need to know what free() returns. */
 void
 xfree (string)
      PTR_T string;
 {
+  /* Leak a bit.  */
+  if (RL_ISSTATE(RL_STATE_SIGHANDLER))
+    return;
+
+#undef free
   if (string)
     free (string);
 }
--- src/readline/xmalloc.h	2006/04/20 20:13:20	1.4
+++ src/readline/xmalloc.h	2011/06/29 21:46:43	1.4.40.1
@@ -39,6 +39,12 @@
 
 #endif /* !PTR_T */
 
+/* xmalloc and xrealloc should be also protected from RL_STATE_SIGHANDLER.  */
+#define xfree xfree_readline
+
+/* readline-5.1 backport.  */
+#define free xfree
+
 extern PTR_T xmalloc PARAMS((size_t));
 extern PTR_T xrealloc PARAMS((void *, size_t));
 extern void xfree PARAMS((void *));

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-06-29 20:35         ` Jan Kratochvil
@ 2011-06-30 14:38           ` Chet Ramey
  2011-07-06 16:03             ` Jan Kratochvil
  0 siblings, 1 reply; 26+ messages in thread
From: Chet Ramey @ 2011-06-30 14:38 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: bug-readline, gdb-patches, Sterling Augustine, chet.ramey

On 6/29/11 4:34 PM, Jan Kratochvil wrote:
> On Wed, 29 Jun 2011 15:54:11 +0200, Chet Ramey wrote:
>> If you feel that you need to make this change for the current version of
>> gdb, go ahead.
> 
> OK.  It is not a complete fix but hopefully the one people hit the most.
> I have also seen unreproducible crash(es) on a completion CTRL-C myself.
> 
> 
>> The big problem is to allow operations that read through the file system
>> (like completion) to be interrupted without running unsafe functions from
>> a signal handler.
> 
> I do not see the problem, readline already does not use SA_RESTART and the
> application also gets SIGINT passed by _rl_handle_signal so it aborts the
> completion reading on its own, as GDB does by the QUIT macro.
> 
> 
>> It does the user no good to keep setting the "I got SIGINT" flag if he's
>> trying to complete files on a dead or otherwise unreachable file server.
> 
> The syscall should get EINTR without SA_RESTART.  But I admit I may not see
> the problem now.

I have seen cases where the user hits ^C while readline or a filename
completion function is attempting to traverse a file system on a dead
NFS server, the signal handler gets hit, but the system call doesn't
get interrupted.  I haven't seen those cases in a while, though.

Let's try this.  Instead of introducing a leak in free(), remove the
references to _rl_interrupt_immediately in complete.c.  Those are the
only two places where the interrupt handler is called synchronously.
I have a couple more changes to make if that doesn't provide the
necessary responsiveness.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-06-30 14:38           ` Chet Ramey
@ 2011-07-06 16:03             ` Jan Kratochvil
  2011-07-06 16:07               ` Chet Ramey
  0 siblings, 1 reply; 26+ messages in thread
From: Jan Kratochvil @ 2011-07-06 16:03 UTC (permalink / raw)
  To: Chet Ramey; +Cc: bug-readline, gdb-patches, Sterling Augustine

On Thu, 30 Jun 2011 16:38:21 +0200, Chet Ramey wrote:
> I have seen cases where the user hits ^C while readline or a filename
> completion function is attempting to traverse a file system on a dead
> NFS server, the signal handler gets hit, but the system call doesn't
> get interrupted.  I haven't seen those cases in a while, though.

I have tried to reproduce it but I think it is outside of the scope of
readline and/or gdb.

Running
	ip6tables -I INPUT 1 -i lo -p tcp --dport 2049 -j DROP
before rl_filename_completion_function's readdir() call will cause (in strace):

rt_sigaction(SIGINT, {0x6518cb=handle_sigint, [INT], SA_RESTORER|SA_RESTART, 0x7f79cf0bd490}, NULL, 8) = 0
rt_sigaction(SIGQUIT, {0x65196e=handle_sigquit, [QUIT], SA_RESTORER|SA_RESTART, 0x7f79cf0bd490}, NULL, 8) = 0
rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
[...]
getdents(5, <hang>

and no CTRL-C makes any change.  It was mounted
on kernel-2.6.35.13-92.fc14.x86_64 with options:
	localhost:/... /... nfs ro,relatime,vers=3,rsize=1048576,wsize=1048576,namlen=255,acregmin=0,acregmax=0,acdirmin=0,acdirmax=0,soft,proto=tcp6,timeo=600,retrans=2,sec=sys,mountaddr=::1,mountvers=3,mountport=59099,mountproto=udp6,addr=::1 0 0

where I used "intr" but "intr" / "nointr" is not listed at all and man says:
	The intr / nointr mount option is deprecated after kernel 2.6.25.
	Only SIGKILL can interrupt a pending NFS operation on these kernels,
	and if specified, this mount option is ignored to provide backwards
	compatibility with older kernels.


> remove the references to _rl_interrupt_immediately

I think _rl_interrupt_immediately should never exist as you cannot do anything
much from the signal handler anyway.
man 7 signak "Async-signal-safe functions"


Thanks,
Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-07-06 16:03             ` Jan Kratochvil
@ 2011-07-06 16:07               ` Chet Ramey
  2011-07-06 17:42                 ` Jan Kratochvil
  0 siblings, 1 reply; 26+ messages in thread
From: Chet Ramey @ 2011-07-06 16:07 UTC (permalink / raw)
  To: jan.kratochvil; +Cc: chet.ramey, bug-readline, gdb-patches, saugustine, chet

> On Thu, 30 Jun 2011 16:38:21 +0200, Chet Ramey wrote:
> > I have seen cases where the user hits ^C while readline or a filename
> > completion function is attempting to traverse a file system on a dead
> > NFS server, the signal handler gets hit, but the system call doesn't
> > get interrupted.  I haven't seen those cases in a while, though.
> 
> I have tried to reproduce it but I think it is outside of the scope of
> readline and/or gdb.
> 
> Running
> 	ip6tables -I INPUT 1 -i lo -p tcp --dport 2049 -j DROP
> before rl_filename_completion_function's readdir() call will cause (in strace):
> 
> rt_sigaction(SIGINT, {0x6518cb=handle_sigint, [INT], SA_RESTORER|SA_RESTART, 0x7f79cf0bd490}, NULL, 8) = 0
> rt_sigaction(SIGQUIT, {0x65196e=handle_sigquit, [QUIT], SA_RESTORER|SA_RESTART, 0x7f79cf0bd490}, NULL, 8) = 0
> rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
> [...]
> getdents(5, <hang>
> 
> and no CTRL-C makes any change.  It was mounted
> on kernel-2.6.35.13-92.fc14.x86_64 with options:
> 	localhost:/... /... nfs ro,relatime,vers=3,rsize=1048576,wsize=1048576,namlen=255,acregmin=0,acregmax=0,acdirmin=0,acdirmax=0,soft,proto=tcp6,timeo=600,retrans=2,sec=sys,mountaddr=::1,mountvers=3,mountport=59099,mountproto=udp6,addr=::1 0 0
> 
> where I used "intr" but "intr" / "nointr" is not listed at all and man says:
> 	The intr / nointr mount option is deprecated after kernel 2.6.25.
> 	Only SIGKILL can interrupt a pending NFS operation on these kernels,
> 	and if specified, this mount option is ignored to provide backwards
> 	compatibility with older kernels.

Other systems besides Linux still implement the `intr' mount option and
allow system calls referencing unresponsive remote file systems to be
interrupted (FreeBSD and Mac OS X, for example).  I'm willing to accept
that bugs in prior versions of these systems have been fixed and the
options work as documented.

> > remove the references to _rl_interrupt_immediately
> 
> I think _rl_interrupt_immediately should never exist as you cannot do anything
> much from the signal handler anyway.
> man 7 signak "Async-signal-safe functions"

If you believe that you either have no hope (can't interrupt an operation on
an unresponsive server) or will get proper EINTR behavior, then you are right.
Otherwise, it's your only chance.

As I said, I'm willing to remove these references and see what happens.  Since
you have a way to readily reproduce the problem, I was hoping you'd do it
and let me know what you found.

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-07-06 16:07               ` Chet Ramey
@ 2011-07-06 17:42                 ` Jan Kratochvil
  2011-07-07 13:40                   ` Chet Ramey
  0 siblings, 1 reply; 26+ messages in thread
From: Jan Kratochvil @ 2011-07-06 17:42 UTC (permalink / raw)
  To: Chet Ramey; +Cc: bug-readline, gdb-patches, saugustine, chet

On Wed, 06 Jul 2011 17:58:26 +0200, Chet Ramey wrote:
> As I said, I'm willing to remove these references and see what happens.  Since
> you have a way to readily reproduce the problem, I was hoping you'd do it
> and let me know what you found.

I do not think any testing matters here.  This is a difficult to reproduce
race + memory corruption.  While a crash proves it is wrong no crash does not
prove anything.

Even if no existing system ever crashes the code is still wrong because it
violates POSIX:
http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html
	The following table defines a set of functions that shall be either
	reentrant or non-interruptible by signals and shall be async-signal-safe.

Static code analysis is the only valid verification.  Currently the signal
code calls free() which is not listed in the safe syscalls list above,
therefore the code is not correct.

I do not know if it is possible to code _rl_handle_signal in a way which uses
only the safe syscalls and only atomic operations on volatile data structures.
Anyway even if it would be possible I find such code very fragile and
I believe the signals should be always delayed through _rl_caught_signal.

Sure it then has to depend on the application which should properly return to
readline callers immediately if it sees any EINTR.


Thanks,
Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-07-06 17:42                 ` Jan Kratochvil
@ 2011-07-07 13:40                   ` Chet Ramey
  2011-07-08 16:03                     ` Chet Ramey
  2011-10-19 17:02                     ` Jan Kratochvil
  0 siblings, 2 replies; 26+ messages in thread
From: Chet Ramey @ 2011-07-07 13:40 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: bug-readline, gdb-patches, Sterling Augustine, chet.ramey

On 7/6/11 12:44 PM, Jan Kratochvil wrote:
> On Wed, 06 Jul 2011 17:58:26 +0200, Chet Ramey wrote:
>> As I said, I'm willing to remove these references and see what happens.  Since
>> you have a way to readily reproduce the problem, I was hoping you'd do it
>> and let me know what you found.
> 
> I do not think any testing matters here.  This is a difficult to reproduce
> race + memory corruption.  While a crash proves it is wrong no crash does not
> prove anything.

The impression I got from your earlier message is that is is very easy
to reproduce using a GDB .exp file:

"Used this GDB .exp file, reproducible in several seconds"

All I am asking you do to is to check whether you can reproduce it using
the same .exp file after removing references to _rl_interrupt_immediately
in complete.c.

> 
> Even if no existing system ever crashes the code is still wrong because it
> violates POSIX:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html
> 	The following table defines a set of functions that shall be either
> 	reentrant or non-interruptible by signals and shall be async-signal-safe.
> 
> Static code analysis is the only valid verification.  Currently the signal
> code calls free() which is not listed in the safe syscalls list above,
> therefore the code is not correct.
> 
> I do not know if it is possible to code _rl_handle_signal in a way which uses
> only the safe syscalls and only atomic operations on volatile data structures.
> Anyway even if it would be possible I find such code very fragile and
> I believe the signals should be always delayed through _rl_caught_signal.

Ironically, I changed it to respond immediately to signals when in callback
mode because of a bug you filed from gdb.  When readline was reading input
using rl_callback_read_char it did not respond quickly enough to SIGINT,
and gdb didn't catch it.  You will have to check and make sure the
conditions have changed enough to make it acceptable to delay signal
handling.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-07-07 13:40                   ` Chet Ramey
@ 2011-07-08 16:03                     ` Chet Ramey
  2011-10-19 20:30                       ` Jan Kratochvil
  2011-10-19 17:02                     ` Jan Kratochvil
  1 sibling, 1 reply; 26+ messages in thread
From: Chet Ramey @ 2011-07-08 16:03 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: chet.ramey, bug-readline, gdb-patches, Sterling Augustine

On 7/7/11 8:10 AM, I wrote:

>> I do not know if it is possible to code _rl_handle_signal in a way which uses
>> only the safe syscalls and only atomic operations on volatile data structures.
>> Anyway even if it would be possible I find such code very fragile and
>> I believe the signals should be always delayed through _rl_caught_signal.
> 
> Ironically, I changed it to respond immediately to signals when in callback
> mode because of a bug you filed from gdb.  When readline was reading input
> using rl_callback_read_char it did not respond quickly enough to SIGINT,
> and gdb didn't catch it.  You will have to check and make sure the
> conditions have changed enough to make it acceptable to delay signal
> handling.

It occurs to me that this will not work unless I make changes to readline's
callback implementation.  Currently readline installs its signal handlers
as part of the callback setup (rl_callback_handler_install ->
_rl_callback_newline).  This results in a situation where readline's signal
handlers are active when the application has flow control.  I imagine the
most likely scenario is that the application (gdb) is in a select() call
waiting for input when the signal arrives, readline sets a flag, and the
select call either restarts or returns -1/EINTR.  Either way, gdb can't do
anything about it except guess.

The most straightforward solution would be to move the signal setup into
rl_callback_read_char, so readline's signal handlers are in place only
when readline has control.  It's still important that the application
call rl_callback_handler_remove to restore the original signal handlers.

Once that's done, readline will not have to call signal handlers
synchronously when running in callback mode.  Thoughts?

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] Make interrupting tab-completion safe.
  2011-06-13 17:45   ` Sterling Augustine
  2011-06-26 22:22     ` [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.] Jan Kratochvil
@ 2011-07-11 18:53     ` Sterling Augustine
  2011-07-11 18:54       ` Jan Kratochvil
  1 sibling, 1 reply; 26+ messages in thread
From: Sterling Augustine @ 2011-07-11 18:53 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: gdb-patches

On Mon, Jun 13, 2011 at 10:44 AM, Sterling Augustine
<saugustine@google.com> wrote:
> On Sun, Jun 12, 2011 at 5:11 AM, Jan Kratochvil
> <jan.kratochvil@redhat.com> wrote:

> The race is between the conversion from psymtab to full symtab and you
> hitting ctrl-c. Some of this time is also spent handling filename
> completion, which doesn't have the problem. You need to be sure to
> interrupt c_type_print_args, when called from dwarf2read.c:5049.
>
>> It would be good to have a testcase for regressions anyway.
>
> Is there an existing test-case I can model this one on? (One that
> sends an asynchronous sigint to gdb is probably enough.) I can't seem
> to find any, but my deja-gnu foo is weak.

(Ping on this patch)

Writing a generic reproducible test-case for this is pretty hard.
Calling QUIT from dwarf2read.c:5049 is necessary, but not sufficient.
Counting QUITs is not a good solution, because recompiling the target
program can change the counts.

If I modify the source such that dwarf2read.c:5049 is the only place
that calls QUIT, reproducing it is easy, but that clearly isn't a good
way of writing a test case.

My understanding is that dwarf reading is not expected to be
interruptable, so I would expect something like this to be acceptable
without a new test.

Is there some other approach--short of rewriting all the *type_print*
stuff--that would be acceptable? I'd love to close out this problem.

Sterling

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] Make interrupting tab-completion safe.
  2011-07-11 18:53     ` [PATCH] Make interrupting tab-completion safe Sterling Augustine
@ 2011-07-11 18:54       ` Jan Kratochvil
       [not found]         ` <CAEG7qUxFvEoJ-E2YsoFPL-tKoK4kD3-pKn-h31uUeXQoDD2Gaw@mail.gmail.com>
  0 siblings, 1 reply; 26+ messages in thread
From: Jan Kratochvil @ 2011-07-11 18:54 UTC (permalink / raw)
  To: Sterling Augustine; +Cc: gdb-patches

On Mon, 11 Jul 2011 20:42:23 +0200, Sterling Augustine wrote:
> Writing a generic reproducible test-case for this is pretty hard.

I agree about it, a testcase is possible with unchanged FSF GDB as shown in
	http://sourceware.org/ml/gdb-patches/2011-06/msg00387.html

but the memory corruption is too random to rely on it in a testcase.


> Is there some other approach--short of rewriting all the *type_print*
> stuff--that would be acceptable? I'd love to close out this problem.

I find the user-facing bug as already fixed since the check-in:
	http://sourceware.org/ml/gdb-patches/2011-06/msg00475.html

There are some outstanding problems being discussed (it is my turn now):
	http://sourceware.org/ml/gdb-patches/2011-07/msg00233.html
but that is more for the next readline release and I hope users no longer hit
these crashes.

If you still have it reproducible with FSF GDB HEAD or 7.3 snapshots please
provide some backtrace etc.


Thanks,
Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [dwarf2_mark_helper patch] Re: [PATCH] Make interrupting tab-completion safe.
       [not found]         ` <CAEG7qUxFvEoJ-E2YsoFPL-tKoK4kD3-pKn-h31uUeXQoDD2Gaw@mail.gmail.com>
@ 2011-07-12 15:59           ` Jan Kratochvil
  2011-07-12 17:48             ` Sterling Augustine
                               ` (2 more replies)
  0 siblings, 3 replies; 26+ messages in thread
From: Jan Kratochvil @ 2011-07-12 15:59 UTC (permalink / raw)
  To: Sterling Augustine; +Cc: gdb-patches

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

On Mon, 11 Jul 2011 23:36:24 +0200, Sterling Augustine wrote:
> On head, synced this morning, I still get the failure.

OK, this is a different kind of crash than I reproduced before.
Re-posting back to the list with the off-list mail attached (+reformatted).

It is reproducible only for:
 * either cross-CU DIE reference by its offset but that is never produced by
   GCC so I do not consider it here.
 * or .debug_types, therefore -gdwarf-4 possibly with -fdebug-types-section.

And then also it must have:
 * missing DW_AT_MIPS_linkage_name which I guess only Google is using.
   One can simulate it in gdb by -ex 'set debug check-physname'.
   Otherwise C++ parameters printing would not get called.

And for the artificial reproducibility:
 * The referenced CUs must contain C++ parameters.
   To make possibly CTRL-C application while reading them in.
 * The referencing CU should not contain C++ parameters.
   Otherwise CTRL-C could apply too early.

Recommending some `ulimit -v 2000000' otherwise GDB can eat out the memory.

perl -le '$n=1000;print "class C$_ { public: void m (C$_ *c) {} } c$_;" for 1..$n;print "int main () { ";print "c$_.m (&c$_);" for 1..$n;print "}";'|g++ -gdwarf-4 -fdebug-types-section -Wall -x c++ -;./gdb -nx -ex 'set debug check-physname' ./a.out
g++ (GCC) 4.7.0 20110712 (experimental)
GNU gdb (GDB) 7.3.50.20110711-cvs
(gdb) b ^CQuit
(gdb) b <SEGV>
by:
(gdb) b <tab><fast ctrl-c>
(gdb) b <tab>

One can also use $n=10000 to have more than 1sec for <ctrl-c>.


> The segmentation fault happens because per_cu->cu == NULL.

While the fix is doing just the straightforward thing I do not see what better
fix could be made.


Thanks,
Jan


gdb/
2011-07-12  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix occasional crash of CTRL-C during DWARF read in.
	* dwarf2read.c (dwarf2_mark_helper): Return on NULL CU.

--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -15455,6 +15455,13 @@ dwarf2_mark_helper (void **slot, void *data)
   struct dwarf2_per_cu_data *per_cu;
 
   per_cu = (struct dwarf2_per_cu_data *) *slot;
+
+  /* cu->dependencies references may not yet have been ever read if QUIT aborts
+     reading of the chain.  As such dependencies remain valid there is not much
+     useful to track and undo them during QUIT cleanups.  */
+  if (per_cu->cu == NULL)
+    return 1;
+
   if (per_cu->cu->mark)
     return 1;
   per_cu->cu->mark = 1;

[-- Attachment #2: cu-trace --]
[-- Type: text/plain, Size: 11462 bytes --]

On Mon, Jul 11, 2011 at 11:53 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> If you still have it reproducible with FSF GDB HEAD or 7.3 snapshots please
> provide some backtrace etc.

On head, synced this morning, I still get the failure. Two stack
traces are below. The first is a trace from the actual successful quit
call. The second is the actual crash.

After each "b" on the command line, I type <tab><tab>, which of course
doesn't show up in the log.

The segmentation fault happens because per_cu->cu == NULL.

Sterling


*******

Starting program:
/usr/local/google/users/saugustine/gdb-tot/build/gdb/gdb
really_big_program
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/libthread_db.so.1".
GNU gdb (GDB) 7.3.50.20110711-cvs
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from really_big_program...done.
warning: Missing auto-load scripts referenced in section .debug_gdb_scripts
of file really_big_program
Use `info auto-load-scripts [REGEXP]' to list them.
(gdb) b
Program received signal SIGINT, Interrupt.
^C
Program received signal SIGINT, Interrupt.

Breakpoint 1, quit () at /home/saugustine/tot-gdb/src/gdb/utils.c:1192
1192	{
(top) where
#0  quit () at /home/saugustine/tot-gdb/src/gdb/utils.c:1192
#1  0x00000000005f890f in c_type_print_base (type=0x1bcb0290, stream=0x19ff6d00, show=-1, level=0) at /home/saugustine/tot-gdb/src/gdb/c-typeprint.c:730
#2  0x00000000005fa5f9 in c_print_type (type=0x1bcb0290, varstring=0x70622b "", stream=0x19ff6d00, show=-1, level=0) at /home/saugustine/tot-gdb/src/gdb/c-typeprint.c:63
#3  0x00000000005fa7c6 in c_type_print_args (type=0x1bd8f610, stream=0x19ff6d00, linkage_name=<optimized out>, language=language_cplus) at /home/saugustine/tot-gdb/src/gdb/c-typeprint.c:439
#4  0x00000000005b1fc2 in dwarf2_compute_name (name=<optimized out>, die=0x1bc9d8c0, cu=0x169a83e0, physname=1) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:5121
#5  0x00000000005b23dc in dwarf2_physname (name=0x7fffdcd38bfe "scoped_ptr", die=0x1bc9d8c0, cu=0x169a83e0) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:5241
#6  0x00000000005b6276 in compute_delayed_physnames (cu=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:4651
#7  process_full_comp_unit (per_cu=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:4729
#8  process_queue (objfile=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:4445
#9  dw2_do_instantiate_symtab (objfile=<optimized out>, per_cu=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:1812
#10 0x00000000005b6f73 in dwarf2_psymtab_to_symtab (pst=0x15c475a0) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:4402
#11 0x0000000000543954 in psymtab_to_symtab (pst=0x15c475a0) at /home/saugustine/tot-gdb/src/gdb/psymtab.c:752
#12 0x00000000005448dd in expand_symtabs_matching_via_partial (objfile=0xc68ed0, file_matcher=0, name_matcher=<optimized out>, kind=ALL_DOMAIN, data=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/psymtab.c:1271
#13 0x0000000000543573 in expand_partial_symbol_names (fun=0x53cbe0 <expand_partial_symbol_name>, data=0x7fffffffd8e0) at /home/saugustine/tot-gdb/src/gdb/psymtab.c:1913
#14 0x0000000000540385 in default_make_symbol_completion_list_break_on (text=0x7fffffffd9c2 "", word=0x7fffffffd9c2 "", break_on=0x70622b "") at /home/saugustine/tot-gdb/src/gdb/symtab.c:3897
#15 0x0000000000574059 in location_completer (ignore=<optimized out>, text=0x7fffffffd9c2 "", word=0x7fffffffd9c2 "") at /home/saugustine/tot-gdb/src/gdb/completer.c:286
#16 0x0000000000573623 in complete_line_internal (text=<optimized out>, line_buffer=<optimized out>, point=<optimized out>, reason=handle_completions) at /home/saugustine/tot-gdb/src/gdb/completer.c:791
#17 0x0000000000573afc in line_completion_function (point=<optimized out>, line_buffer=<optimized out>, matches=<optimized out>, text=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/completer.c:885
#18 readline_line_completion_function (text=0x364b130 "", matches=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/completer.c:102
#19 0x000000000063d673 in rl_completion_matches (text=0x364b130 "", entry_function=<optimized out>) at /home/saugustine/tot-gdb/src/readline/complete.c:1997
#20 0x000000000063ec5b in rl_complete_internal (what_to_do=9) at /home/saugustine/tot-gdb/src/readline/complete.c:1835
#21 0x00000000006366db in _rl_dispatch_subseq (key=9, map=0xab0400, got_subseq=0) at /home/saugustine/tot-gdb/src/readline/readline.c:774
#22 0x0000000000636eb7 in readline_internal_char () at /home/saugustine/tot-gdb/src/readline/readline.c:552
#23 0x000000000064b145 in rl_callback_read_char () at /home/saugustine/tot-gdb/src/readline/callback.c:201
#24 0x0000000000571f89 in rl_callback_read_char_wrapper (client_data=0x1bcb0290) at /home/saugustine/tot-gdb/src/gdb/event-top.c:177
#25 0x0000000000570818 in process_event () at /home/saugustine/tot-gdb/src/gdb/event-loop.c:402
#26 0x0000000000571c1a in gdb_do_one_event (data=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/event-loop.c:467
#27 0x000000000056b9db in catch_errors (func=<optimized out>, func_args=<optimized out>, errstring=<optimized out>, mask=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/exceptions.c:506
#28 0x00000000004df7d0 in tui_command_loop (data=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/tui/tui-interp.c:172
#29 0x0000000000458069 in captured_command_loop (data=0x1bcb0290) at /home/saugustine/tot-gdb/src/gdb/main.c:230
#30 0x000000000056b9db in catch_errors (func=<optimized out>, func_args=<optimized out>, errstring=<optimized out>, mask=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/exceptions.c:506
#31 0x0000000000458ca6 in captured_main (data=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/main.c:937
#32 0x000000000056b9db in catch_errors (func=<optimized out>, func_args=<optimized out>, errstring=<optimized out>, mask=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/exceptions.c:506
#33 0x0000000000458054 in gdb_main (args=0x19ff6d00) at /home/saugustine/tot-gdb/src/gdb/main.c:946
#34 0x000000000045801e in main (argc=<optimized out>, argv=0x19ff6d00) at /home/saugustine/tot-gdb/src/gdb/gdb.c:35
(top) c
Continuing.
Quit
(gdb) b
Program received signal SIGSEGV, Segmentation fault.
0x00000000005a757b in dwarf2_mark_helper (slot=0x1bae5e20, data=0x0)
at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:15462
15462	  if (per_cu->cu->mark)
(top) where
#0  0x00000000005a757b in dwarf2_mark_helper (slot=0x1bae5e20, data=0x0) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:15462
#1  0x00000000006e36f8 in htab_traverse_noresize (htab=<optimized out>, callback=0x5a7570 <dwarf2_mark_helper>, info=0x0) at /home/saugustine/tot-gdb/src/libiberty/hashtab.c:784
#2  0x00000000005a75a8 in dwarf2_mark_helper (slot=<optimized out>, data=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:15467
#3  0x00000000006e36f8 in htab_traverse_noresize (htab=<optimized out>, callback=0x5a7570 <dwarf2_mark_helper>, info=0x0) at /home/saugustine/tot-gdb/src/libiberty/hashtab.c:784
#4  0x00000000005a74a8 in dwarf2_mark (cu=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:15482
#5  age_cached_comp_units () at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:15228
#6  0x00000000005b6408 in dw2_do_instantiate_symtab (objfile=0x0, per_cu=0x9b6a780) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:1816
#7  0x00000000005b6f73 in dwarf2_psymtab_to_symtab (pst=0x15c46a50) at /home/saugustine/tot-gdb/src/gdb/dwarf2read.c:4402
#8  0x0000000000543954 in psymtab_to_symtab (pst=0x15c46a50) at /home/saugustine/tot-gdb/src/gdb/psymtab.c:752
#9  0x00000000005448dd in expand_symtabs_matching_via_partial (objfile=0xc68ed0, file_matcher=0, name_matcher=<optimized out>, kind=ALL_DOMAIN, data=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/psymtab.c:1271
#10 0x0000000000543573 in expand_partial_symbol_names (fun=0x53cbe0 <expand_partial_symbol_name>, data=0x7fffffffd8e0) at /home/saugustine/tot-gdb/src/gdb/psymtab.c:1913
#11 0x0000000000540385 in default_make_symbol_completion_list_break_on (text=0x7fffffffd9c2 "", word=0x7fffffffd9c2 "", break_on=0x70622b "") at /home/saugustine/tot-gdb/src/gdb/symtab.c:3897
#12 0x0000000000574059 in location_completer (ignore=<optimized out>, text=0x7fffffffd9c2 "", word=0x7fffffffd9c2 "") at /home/saugustine/tot-gdb/src/gdb/completer.c:286
#13 0x0000000000573623 in complete_line_internal (text=<optimized out>, line_buffer=<optimized out>, point=<optimized out>, reason=handle_completions) at /home/saugustine/tot-gdb/src/gdb/completer.c:791
#14 0x0000000000573afc in line_completion_function (point=<optimized out>, line_buffer=<optimized out>, matches=<optimized out>, text=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/completer.c:885
#15 readline_line_completion_function (text=0xe5675f0 "", matches=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/completer.c:102
#16 0x000000000063d673 in rl_completion_matches (text=0xe5675f0 "", entry_function=<optimized out>) at /home/saugustine/tot-gdb/src/readline/complete.c:1997
#17 0x000000000063ec5b in rl_complete_internal (what_to_do=9) at /home/saugustine/tot-gdb/src/readline/complete.c:1835
#18 0x00000000006366db in _rl_dispatch_subseq (key=9, map=0xab0400, got_subseq=0) at /home/saugustine/tot-gdb/src/readline/readline.c:774
#19 0x0000000000636eb7 in readline_internal_char () at /home/saugustine/tot-gdb/src/readline/readline.c:552
#20 0x000000000064b145 in rl_callback_read_char () at /home/saugustine/tot-gdb/src/readline/callback.c:201
#21 0x0000000000571f89 in rl_callback_read_char_wrapper (client_data=0x1bae5e20) at /home/saugustine/tot-gdb/src/gdb/event-top.c:177
#22 0x0000000000570818 in process_event () at /home/saugustine/tot-gdb/src/gdb/event-loop.c:402
#23 0x0000000000571c1a in gdb_do_one_event (data=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/event-loop.c:467
#24 0x000000000056b9db in catch_errors (func=<optimized out>, func_args=<optimized out>, errstring=<optimized out>, mask=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/exceptions.c:506
#25 0x00000000004df7d0 in tui_command_loop (data=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/tui/tui-interp.c:172
#26 0x0000000000458069 in captured_command_loop (data=0x1bae5e20) at /home/saugustine/tot-gdb/src/gdb/main.c:230
#27 0x000000000056b9db in catch_errors (func=<optimized out>, func_args=<optimized out>, errstring=<optimized out>, mask=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/exceptions.c:506
#28 0x0000000000458ca6 in captured_main (data=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/main.c:937
#29 0x000000000056b9db in catch_errors (func=<optimized out>, func_args=<optimized out>, errstring=<optimized out>, mask=<optimized out>) at /home/saugustine/tot-gdb/src/gdb/exceptions.c:506
#30 0x0000000000458054 in gdb_main (args=0x0) at /home/saugustine/tot-gdb/src/gdb/main.c:946
#31 0x000000000045801e in main (argc=<optimized out>, argv=0x0) at /home/saugustine/tot-gdb/src/gdb/gdb.c:35
(top)

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dwarf2_mark_helper patch] Re: [PATCH] Make interrupting tab-completion safe.
  2011-07-12 15:59           ` [dwarf2_mark_helper patch] " Jan Kratochvil
@ 2011-07-12 17:48             ` Sterling Augustine
  2011-07-12 18:56             ` Jan Kratochvil
  2011-07-12 21:18             ` [commit] " Jan Kratochvil
  2 siblings, 0 replies; 26+ messages in thread
From: Sterling Augustine @ 2011-07-12 17:48 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: gdb-patches

FWIW, this patch works great for me. Thanks.

Sterling

On Tue, Jul 12, 2011 at 4:14 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
...

> gdb/
> 2011-07-12  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>        Fix occasional crash of CTRL-C during DWARF read in.
>        * dwarf2read.c (dwarf2_mark_helper): Return on NULL CU.
>
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -15455,6 +15455,13 @@ dwarf2_mark_helper (void **slot, void *data)
>   struct dwarf2_per_cu_data *per_cu;
>
>   per_cu = (struct dwarf2_per_cu_data *) *slot;
> +
> +  /* cu->dependencies references may not yet have been ever read if QUIT aborts
> +     reading of the chain.  As such dependencies remain valid there is not much
> +     useful to track and undo them during QUIT cleanups.  */
> +  if (per_cu->cu == NULL)
> +    return 1;
> +
>   if (per_cu->cu->mark)
>     return 1;
>   per_cu->cu->mark = 1;
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [dwarf2_mark_helper patch] Re: [PATCH] Make interrupting tab-completion safe.
  2011-07-12 15:59           ` [dwarf2_mark_helper patch] " Jan Kratochvil
  2011-07-12 17:48             ` Sterling Augustine
@ 2011-07-12 18:56             ` Jan Kratochvil
  2011-07-12 21:18             ` [commit] " Jan Kratochvil
  2 siblings, 0 replies; 26+ messages in thread
From: Jan Kratochvil @ 2011-07-12 18:56 UTC (permalink / raw)
  To: Sterling Augustine; +Cc: gdb-patches

On Tue, 12 Jul 2011 13:14:40 +0200, Jan Kratochvil wrote:
> And then also it must have:
>  * missing DW_AT_MIPS_linkage_name which I guess only Google is using.
>    One can simulate it in gdb by -ex 'set debug check-physname'.
>    Otherwise C++ parameters printing would not get called.

Neither the hypothetical Google modification of GCC (for .debug size
reduction) nor 'set debug check-physname' should be needed.

Any GCCs except HEAD did not provide DW_AT_MIPS_linkage_name for constructors
and destructors.  GDB could crash on computing physname of these ones.


Regards,
Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [commit] Re: [dwarf2_mark_helper patch] Re: [PATCH] Make interrupting tab-completion safe.
  2011-07-12 15:59           ` [dwarf2_mark_helper patch] " Jan Kratochvil
  2011-07-12 17:48             ` Sterling Augustine
  2011-07-12 18:56             ` Jan Kratochvil
@ 2011-07-12 21:18             ` Jan Kratochvil
  2011-07-12 21:42               ` Tom Tromey
  2 siblings, 1 reply; 26+ messages in thread
From: Jan Kratochvil @ 2011-07-12 21:18 UTC (permalink / raw)
  To: Sterling Augustine; +Cc: gdb-patches

On Tue, 12 Jul 2011 13:14:40 +0200, Jan Kratochvil wrote:
> gdb/
> 2011-07-12  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix occasional crash of CTRL-C during DWARF read in.
> 	* dwarf2read.c (dwarf2_mark_helper): Return on NULL CU.

After some discussion on IRC whether QUIT should or should not be allowed for
dwarf2read this patch should be OK and I have checked it in:
	http://sourceware.org/ml/gdb-cvs/2011-07/msg00122.html
and for 7.3 as this kind of patch cannot have a regression:
	http://sourceware.org/ml/gdb-cvs/2011-07/msg00123.html

There could be a testcase, either one gets contributed or I hopefully write
one later.


Thanks,
Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [commit] Re: [dwarf2_mark_helper patch] Re: [PATCH] Make interrupting tab-completion safe.
  2011-07-12 21:18             ` [commit] " Jan Kratochvil
@ 2011-07-12 21:42               ` Tom Tromey
  2011-07-12 22:51                 ` Jan Kratochvil
  0 siblings, 1 reply; 26+ messages in thread
From: Tom Tromey @ 2011-07-12 21:42 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Sterling Augustine, gdb-patches

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> After some discussion on IRC whether QUIT should or should not be
Jan> allowed for dwarf2read this patch should be OK and I have checked
Jan> it in:

To recap, based on existing code we believe it is not only safe to have
a QUIT in the full symbol reader, but we think more should probably be
added.

psymtab reading appears to remain unsafe, or at least unexplored.
However, allowing QUITs here would also be nice.

QUITs in symtab reading imply resource leaks; however there are already
known resource leaks in this code (Jan has details).  My belief is that
we should move away from obstack allocation here and to something more
fine-grained like an alloc pool.

Tom

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [commit] Re: [dwarf2_mark_helper patch] Re: [PATCH] Make interrupting tab-completion safe.
  2011-07-12 21:42               ` Tom Tromey
@ 2011-07-12 22:51                 ` Jan Kratochvil
  0 siblings, 0 replies; 26+ messages in thread
From: Jan Kratochvil @ 2011-07-12 22:51 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Sterling Augustine, gdb-patches

On Tue, 12 Jul 2011 23:18:16 +0200, Tom Tromey wrote:
> QUITs in symtab reading imply resource leaks; however there are already
> known resource leaks in this code (Jan has details).

Besides small leaks here and there (such as allocating CU objects from
objfile_obstack - objfile_obstack has objfile lifetime, comp_unit_obstack has
CU lifetime; CU can get loaded+freed multiple times for one objfile).

There is also a more general problem that symtab can be read once and symtab
can never be freed.  This means that with tbreak or batch control of GDB etc.
in fact `-readnow' gets into effect which costs on libwekitgtk 5GB of memory
and it would be larger on real world apps.  It is a subpart of PR 12828 filed
by me.


> My belief is that we should move away from obstack allocation here and to
> something more fine-grained like an alloc pool.

There should be at least symtab_obstack.  But obstack is not great in general
as it is incompatible with valgrind.


Thanks,
Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-07-07 13:40                   ` Chet Ramey
  2011-07-08 16:03                     ` Chet Ramey
@ 2011-10-19 17:02                     ` Jan Kratochvil
  2011-10-19 17:51                       ` Pedro Alves
  2011-10-19 18:50                       ` Chet Ramey
  1 sibling, 2 replies; 26+ messages in thread
From: Jan Kratochvil @ 2011-10-19 17:02 UTC (permalink / raw)
  To: Chet Ramey; +Cc: bug-readline, gdb-patches, Sterling Augustine

On Thu, 07 Jul 2011 14:10:08 +0200, Chet Ramey wrote:
> The impression I got from your earlier message is that is is very easy
> to reproduce using a GDB .exp file:
> 
> "Used this GDB .exp file, reproducible in several seconds"
> 
> All I am asking you do to is to check whether you can reproduce it using
> the same .exp file after removing references to _rl_interrupt_immediately
> in complete.c.

After removing the workaround:
	https://lists.gnu.org/archive/html/bug-readline/2011-06/msg00003.html

and removing the changes of _rl_interrupt_immediately in complete.c the
memory corruption is still reproducible:
*** glibc detected *** .../gdb/testsuite/../../gdb/gdb: munmap_chunk(): invalid pointer: 0x000000000718ef70 ***


Thanks,
Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-10-19 17:02                     ` Jan Kratochvil
@ 2011-10-19 17:51                       ` Pedro Alves
  2011-10-19 18:50                       ` Chet Ramey
  1 sibling, 0 replies; 26+ messages in thread
From: Pedro Alves @ 2011-10-19 17:51 UTC (permalink / raw)
  To: gdb-patches; +Cc: Jan Kratochvil, Chet Ramey, bug-readline, Sterling Augustine

On Wednesday 19 October 2011 17:32:57, Jan Kratochvil wrote:
> On Thu, 07 Jul 2011 14:10:08 +0200, Chet Ramey wrote:
> > The impression I got from your earlier message is that is is very easy
> > to reproduce using a GDB .exp file:
> > 
> > "Used this GDB .exp file, reproducible in several seconds"
> > 
> > All I am asking you do to is to check whether you can reproduce it using
> > the same .exp file after removing references to _rl_interrupt_immediately
> > in complete.c.
> 
> After removing the workaround:
> 	https://lists.gnu.org/archive/html/bug-readline/2011-06/msg00003.html
> 
> and removing the changes of _rl_interrupt_immediately in complete.c the
> memory corruption is still reproducible:
> *** glibc detected *** .../gdb/testsuite/../../gdb/gdb: munmap_chunk(): invalid pointer: 0x000000000718ef70 ***

This is gdb's readline copy, but:

 static RETSIGTYPE
 rl_signal_handler (sig)
      int sig;
 {
   if (_rl_interrupt_immediately || RL_ISSTATE(RL_STATE_CALLBACK))
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     {
       _rl_interrupt_immediately = 0;
       _rl_handle_signal (sig);
     }
   else
     _rl_caught_signal = sig;
 
   SIGHANDLER_RETURN;
 }

and GDB uses readline's callback interface.

-- 
Pedro Alves

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-10-19 17:02                     ` Jan Kratochvil
  2011-10-19 17:51                       ` Pedro Alves
@ 2011-10-19 18:50                       ` Chet Ramey
  1 sibling, 0 replies; 26+ messages in thread
From: Chet Ramey @ 2011-10-19 18:50 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: bug-readline, gdb-patches, Sterling Augustine, chet.ramey

On 10/19/11 12:32 PM, Jan Kratochvil wrote:
> On Thu, 07 Jul 2011 14:10:08 +0200, Chet Ramey wrote:
>> The impression I got from your earlier message is that is is very easy
>> to reproduce using a GDB .exp file:
>>
>> "Used this GDB .exp file, reproducible in several seconds"
>>
>> All I am asking you do to is to check whether you can reproduce it using
>> the same .exp file after removing references to _rl_interrupt_immediately
>> in complete.c.
> 
> After removing the workaround:
> 	https://lists.gnu.org/archive/html/bug-readline/2011-06/msg00003.html
> 
> and removing the changes of _rl_interrupt_immediately in complete.c the
> memory corruption is still reproducible:
> *** glibc detected *** .../gdb/testsuite/../../gdb/gdb: munmap_chunk(): invalid pointer: 0x000000000718ef70 ***

Wow, a blast from the past. :-)  The next version of readline will do this
a different way, avoiding executing very much code in a signal handling
context.

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [Bug-readline] [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.]
  2011-07-08 16:03                     ` Chet Ramey
@ 2011-10-19 20:30                       ` Jan Kratochvil
  0 siblings, 0 replies; 26+ messages in thread
From: Jan Kratochvil @ 2011-10-19 20:30 UTC (permalink / raw)
  To: Chet Ramey; +Cc: bug-readline, Sterling Augustine, gdb-patches

On Fri, 08 Jul 2011 16:26:21 +0200, Chet Ramey wrote:
> The most straightforward solution would be to move the signal setup into
> rl_callback_read_char, so readline's signal handlers are in place only
> when readline has control.  It's still important that the application
> call rl_callback_handler_remove to restore the original signal handlers.

Why to keep the signal handler installed after rl_callback_read_char returns
to its caller?

The application (GDB) can do some rl_stuff_char(3) if it sees SIGINT and call
rl_callback_read_char() then.  This way all the SIGINT-hooked operations do
not have to be executed from the signal handler.

But you may have solved it some way as you wrote now, thanks.


Regards,
Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2011-10-19 18:50 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-11  0:19 [PATCH] Make interrupting tab-completion safe Sterling Augustine
2011-06-12 12:12 ` Jan Kratochvil
2011-06-13 17:45   ` Sterling Augustine
2011-06-26 22:22     ` [readline patch, gdb-7.3?] Avoid free from a signal handler [Re: [PATCH] Make interrupting tab-completion safe.] Jan Kratochvil
2011-06-27 16:03       ` Joel Brobecker
2011-06-29 21:49         ` Jan Kratochvil
2011-06-29 13:54       ` [Bug-readline] " Chet Ramey
2011-06-29 20:35         ` Jan Kratochvil
2011-06-30 14:38           ` Chet Ramey
2011-07-06 16:03             ` Jan Kratochvil
2011-07-06 16:07               ` Chet Ramey
2011-07-06 17:42                 ` Jan Kratochvil
2011-07-07 13:40                   ` Chet Ramey
2011-07-08 16:03                     ` Chet Ramey
2011-10-19 20:30                       ` Jan Kratochvil
2011-10-19 17:02                     ` Jan Kratochvil
2011-10-19 17:51                       ` Pedro Alves
2011-10-19 18:50                       ` Chet Ramey
2011-07-11 18:53     ` [PATCH] Make interrupting tab-completion safe Sterling Augustine
2011-07-11 18:54       ` Jan Kratochvil
     [not found]         ` <CAEG7qUxFvEoJ-E2YsoFPL-tKoK4kD3-pKn-h31uUeXQoDD2Gaw@mail.gmail.com>
2011-07-12 15:59           ` [dwarf2_mark_helper patch] " Jan Kratochvil
2011-07-12 17:48             ` Sterling Augustine
2011-07-12 18:56             ` Jan Kratochvil
2011-07-12 21:18             ` [commit] " Jan Kratochvil
2011-07-12 21:42               ` Tom Tromey
2011-07-12 22:51                 ` Jan Kratochvil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).