Re: allocate_string_data memory corruption

[Richard M. Stallman]

    How do we get around this?  Is it OK for x_catch_errors to return
    without doing anything, if it is called in a signal handler?

I don't think so.  Catching errors is the only way to recover
from certain errors that the X server reports.

    And, as I've asked previously, do you see any disadvantage to simply
    using BLOCK_INPUT in the string/cons allocation functions?

The reason I'd rather not do it is simply that it is a big change in
the design paradigm that has been followed thus far.  It is safer to
keep the paradigm unchanged, and fix whatever place fails to follow
it.

After the release, we could look at changing this paradigm, perhaps to
use SYNC_INPUT.


One way to fix it now is to make a different function, which uses
preallocated data, for use in a signal handler.  Does this patch fix
the problem?


*** xterm.c     19 Jan 2006 12:29:03 -0500      1.891
--- xterm.c     21 Jan 2006 16:30:44 -0500      
***************
*** 336,341 ****
--- 336,347 ----
  void x_wm_set_window_state P_ ((struct frame *, int));
  void x_wm_set_icon_pixmap P_ ((struct frame *, int));
  void x_initialize P_ ((void));
+ 
+ static int x_signal_catch_errors P_ ((Display *));
+ static void x_signal_uncatch_errors P_ ((int));
+ static int x_signal_had_errors_p P_ (());
+ static Lisp_Object x_signal_catch_errors_unwind P_ (());
+ 
  static void x_font_min_bounds P_ ((XFontStruct *, int *, int *));
  static int x_compute_min_glyph_bounds P_ ((struct frame *));
  static void x_update_end P_ ((struct frame *));
***************
*** 3725,3731 ****
           structure is changing at the same time this function
           is running.  So at least we must not crash from them.  */
  
!       count = x_catch_errors (FRAME_X_DISPLAY (*fp));
  
        if (FRAME_X_DISPLAY_INFO (*fp)->grabbed && last_mouse_frame
            && FRAME_LIVE_P (last_mouse_frame))
--- 3731,3737 ----
           structure is changing at the same time this function
           is running.  So at least we must not crash from them.  */
  
!       count = x_signal_catch_errors (FRAME_X_DISPLAY (*fp));
  
        if (FRAME_X_DISPLAY_INFO (*fp)->grabbed && last_mouse_frame
            && FRAME_LIVE_P (last_mouse_frame))
***************
*** 3791,3800 ****
  #endif /* USE_X_TOOLKIT */
          }
  
!       if (x_had_errors_p (FRAME_X_DISPLAY (*fp)))
          f1 = 0;
  
!       x_uncatch_errors (FRAME_X_DISPLAY (*fp), count);
  
        /* If not, is it one of our scroll bars?  */
        if (! f1)
--- 3797,3806 ----
  #endif /* USE_X_TOOLKIT */
          }
  
!       if (x_signal_had_errors_p ())
          f1 = 0;
  
!       x_signal_uncatch_errors (count);
  
        /* If not, is it one of our scroll bars?  */
        if (! f1)
***************
*** 5711,5717 ****
                      Display *d = event.xclient.display;
                      /* Catch and ignore errors, in case window has been
                         iconified by a window manager such as GWM.  */
!                     int count = x_catch_errors (d);
                      XSetInputFocus (d, event.xclient.window,
                                      /* The ICCCM says this is
                                         the only valid choice.  */
--- 5717,5723 ----
                      Display *d = event.xclient.display;
                      /* Catch and ignore errors, in case window has been
                         iconified by a window manager such as GWM.  */
!                     int count = x_signal_catch_errors (d);
                      XSetInputFocus (d, event.xclient.window,
                                      /* The ICCCM says this is
                                         the only valid choice.  */
***************
*** 5720,5726 ****
                      /* This is needed to detect the error
                         if there is an error.  */
                      XSync (d, False);
!                     x_uncatch_errors (d, count);
                    }
                  /* Not certain about handling scroll bars here */
  #endif /* 0 */
--- 5726,5732 ----
                      /* This is needed to detect the error
                         if there is an error.  */
                      XSync (d, False);
!                     x_signal_uncatch_errors (count);
                    }
                  /* Not certain about handling scroll bars here */
  #endif /* 0 */
***************
*** 7591,7596 ****
--- 7597,7667 ----
  #endif /* ! 0 */
  
  
+ Display *x_signal_catch_errors_dpy;
+ 
+ Lisp_Object x_signal_error_message_string;
+ 
+ static int
+ x_signal_catch_errors (dpy)
+      Display *dpy;
+ {
+   int count = SPECPDL_INDEX ();
+ 
+   /* Make sure any errors from previous requests have been dealt with.  */
+   XSync (dpy, False);
+ 
+   x_signal_catch_errors_dpy = dpy;
+   record_unwind_protect (x_signal_catch_errors_unwind, Qnil);
+ 
+   SSET (x_signal_error_message_string, 0, 0);
+ 
+   return count;
+ }
+ 
+ /* Unbind the binding that we made to check for X errors.  */
+ 
+ static Lisp_Object
+ x_signal_catch_errors_unwind (old_val)
+      Lisp_Object old_val;
+ {
+   /* The display may have been closed before this function is called.
+      Check if it is still open before calling XSync.  */
+   if (x_display_info_for_display (x_signal_catch_errors_dpy) != 0)
+     {
+       BLOCK_INPUT;
+       XSync (x_signal_catch_errors_dpy, False);
+       UNBLOCK_INPUT;
+     }
+ 
+   return Qnil;
+ }
+ 
+ /* Nonzero if we had any X protocol errors
+    since we did x_signal_catch_errors.  */
+ 
+ static int
+ x_signal_had_errors_p ()
+ {
+   Display *dpy = x_signal_catch_errors_dpy;
+ 
+   /* Make sure to catch any errors incurred so far.  */
+   XSync (dpy, False);
+ 
+   return SREF (x_signal_error_message_string, 0) != 0;
+ }
+ 
+ /* Stop catching X protocol errors and let them make Emacs die.
+    DPY should be the display that was passed to x_catch_errors.
+    COUNT should be the value that was returned by
+    the corresponding call to x_catch_errors.  */
+ 
+ static void
+ x_signal_uncatch_errors (count)
+      int count;
+ {
+   unbind_to (count, Qnil);
+ }
+ 
  /* Handle SIGPIPE, which can happen when the connection to a server
     simply goes away.  SIGPIPE is handled by x_connection_signal.
     Don't need to do anything, because the write which caused the
***************
*** 10837,10842 ****
--- 10908,10916 ----
  
    staticpro (&last_mouse_press_frame);
    last_mouse_press_frame = Qnil;
+ 
+   staticpro (&x_signal_error_message_string);
+   x_signal_error_message_string = make_uninit_string (X_ERROR_MESSAGE_SIZE);
  
    DEFVAR_BOOL ("x-use-underline-position-properties",
               &x_use_underline_position_properties,