Fix for Emacs Crash - revisited

Fix for Emacs Crash - revisited

[Ben Key]

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

  The patch attached to this message fixes to following entry in
etc/PROBLEMS:

 * Emacs built on Windows 9x/ME crashes at startup on Windows XP,
 or Emacs built on XP crashes at startup on Windows 9x/ME.

The following changes are made by this patch:

1.  init_user_info in w32.c has been modified so that OpenProcessToken
    is no longer called on systems running Windows 9x.  Although
    OpenProcessToken exists in the version of advapi32.dll that is
    found in Windows 9x, it apparently generates an exception if
    called on Windows 9x.  This exception, if not handled causes Emacs
    to crash.

2.  I modified w32_wnd_proc in w32fns.c so that the function pointer
    track_mouse_event_fn is reinitialized in the WM_SETFOCUS
    handler.  I found that even though track_mouse_event_fn was being
    initialized in syms_of_w32fns which is called from main, the
    function pointer obtained in syms_of_w32fns was invalid.  Because
    of this, Emacs was crashing the first time a WM_MOUSEMOVE message
    was received.  I did not want to reinitialize this function
    pointer every time a WM_MOUSEMOVE message is received but I
    wanted to make certain that the function pointer got
    reinitialized as soon after the creation of the Emacs window as
    possible.  That is why I chose WM_SETFOCUS.

3.  I modified initialize_frame_menubar in w32menu.c to reinitialize
    the function pointers get_menu_item_info and set_menu_item_info.
    I found that even though these function pointers were being
    initialized in syms_of_w32menu which is called from main, the
    function pointers obtained in syms_of_w32menu were invalid.  This
    was causing Emacs to crash the first time either of these
    function pointers were used.  This happened to be in add_menu_item
    (which is why I initially came to my earlier conclusions,
    discussed in my earlier message "Fix for Emacs Crash").  I chose
    to reinitialize these function pointers here because I did not
    want to have to do it every time the function pointers were used
    and it seemed that initialize_frame_menubar was the entry point
    for all menu creation.

I have been trying to determine why these function pointers needed to
be reinitialized.  At first I thought that it might have something to
do will DLL relocation, but I no longer think that after discussing
my ideas with Glen Gordan, one of my coworkers.  He told me that when
Windows determine that DLL relocation needs to occur, it happens
before main gets called, not after.  I am uncomfortable in situations
where I cannot determine why a change that fixes a problem is
necessary.  For this reason I have tested my fix very throughly.  I
tested a build done on Windows XP using MSVC 6.0 both on Windows 98
and on Windows ME.  I also tested a build done on Windows XP using
MinGW 2.0 / MSYS 1.08 on Windows ME.  And just to make certain I was
covering all of my bases, I tested a build done on Windows ME using
MinGW 2.0 / MSYS 1.08 on Windows XP.  In all of these test
situations, Emacs worked without a problem.

[-- Attachment #2: dopatch21_3.sh --]
[-- Type: application/octet-stream, Size: 398 bytes --]

#!/bin/sh
if [ -f emacs-21.3.50-w32-play-sound.diff ]
then
  echo "Applying patch emacs-21.3.50-w32-play-sound.diff"
  patch --unified --quiet --strip=1 --input=emacs-21.3.50-w32-play-sound.diff
fi
if [ -f emacs-21.3.50-w32menu.c.diff ]
then
  echo "Applying patch emacs-21.3.50-windows-crash-fix.diff"
  patch --unified --quiet --strip=1 --input=emacs-21.3.50-windows-crash-fix.diff
fi

[-- Attachment #3: emacs-21.3.50-windows-crash-fix.diff --]
[-- Type: application/octet-stream, Size: 6581 bytes --]

--- _21.3/src/w32.c	2002-07-14 20:00:37.000000000 -0400
+++ 21.3/src/w32.c	2002-11-09 23:31:19.000000000 -0500
@@ -254,11 +254,22 @@
   HANDLE          token = NULL;
   SID_NAME_USE    user_type;
 
-  if (OpenProcessToken (GetCurrentProcess (), TOKEN_QUERY, &token)
-      && GetTokenInformation (token, TokenUser,
+  if (
+			/*
+				the test for os_subtype was added here to prevent a
+				crash that occurs when OpenProcessToken is called and
+				Emacs is running on Windows 9x when it was built on
+				Windows NT.
+			*/
+			os_subtype == OS_NT
+			&& OpenProcessToken (GetCurrentProcess (), TOKEN_QUERY, &token)
+      && GetTokenInformation (
+					token, TokenUser,
 			      (PVOID) user_sid, sizeof (user_sid), &trash)
-      && LookupAccountSid (NULL, *((PSID *) user_sid), name, &length,
-			   domain, &dlength, &user_type))
+      && LookupAccountSid (
+					NULL, *((PSID *) user_sid), name, &length,
+			   domain, &dlength, &user_type)
+			)
     {
       strcpy (the_passwd.pw_name, name);
       /* Determine a reasonable uid value. */
--- _21.3/src/w32.h	2002-05-03 16:41:03.000000000 -0400
+++ 21.3/src/w32.h	2002-11-09 23:31:19.000000000 -0500
@@ -124,5 +124,10 @@
 
 extern void init_ntproc ();
 extern void term_ntproc ();
+extern void syms_of_w32term ();
+extern void syms_of_w32fns ();
+extern void syms_of_w32select ();
+extern void syms_of_w32menu ();
+extern void syms_of_fontset ();
 
 #endif /* EMACS_W32_H */
--- _21.3/src/w32fns.c	2002-10-23 12:55:07.000000000 -0400
+++ 21.3/src/w32fns.c	2002-11-09 23:31:19.000000000 -0500
@@ -283,7 +283,12 @@
 
 /* Window that is tracking the mouse.  */
 static HWND track_mouse_window;
-FARPROC track_mouse_event_fn;
+
+typedef BOOL (WINAPI * TrackMouseEvent_Proc) (
+    IN OUT LPTRACKMOUSEEVENT lpEventTrack
+    );
+
+TrackMouseEvent_Proc track_mouse_event_fn=NULL;
 
 /* W95 mousewheel handler */
 unsigned int msh_mousewheel = 0;
@@ -4929,6 +4934,30 @@
       goto dflt;
 
     case WM_SETFOCUS:
+      /*
+        Reinitialize the function pointer track_mouse_event_fn here.
+        This is required even though it is initialized in syms_of_w32fns
+        which is called in main (emacs.c).
+        Reinitialize the function pointer track_mouse_event_fn here.
+        Even though this function pointer is initialized in
+        syms_of_w32fns which is called from main (emacs.c),
+        we need to initialize it again here in order to prevent
+        a crash that occurs in Windows 9x (possibly only when Emacs
+        was built on Windows NT / 2000 / XP?) when handling the
+        WM_MOUSEMOVE message.
+        The crash occurs when attempting to call the Win32 API
+        function TrackMouseEvent through the function pointer.
+        It appears as if the function pointer that is obtained when
+        syms_of_w32fns is called from main is no longer valid
+        (possibly due to DLL relocation?).
+        To resolve this issue, I have placed a call to reinitialize
+        this function pointer here because this message gets received
+        when the Emacs window gains focus.        
+       */
+      track_mouse_event_fn =
+        (TrackMouseEvent_Proc) GetProcAddress (
+            GetModuleHandle ("user32.dll"),
+            "TrackMouseEvent");
       dpyinfo->faked_key = 0;
       reset_modifiers ();
       register_hot_keys (hwnd);
@@ -14801,7 +14830,7 @@
 
   /* TrackMouseEvent not available in all versions of Windows, so must load
      it dynamically.  Do it once, here, instead of every time it is used.  */
-  track_mouse_event_fn = GetProcAddress (user32_lib, "TrackMouseEvent");
+  track_mouse_event_fn = (TrackMouseEvent_Proc) GetProcAddress (user32_lib, "TrackMouseEvent");
   track_mouse_window = NULL;
 
   w32_visible_system_caret_hwnd = NULL;
--- _21.3/src/w32menu.c	2002-08-05 12:33:44.000000000 -0400
+++ 21.3/src/w32menu.c	2002-11-09 23:31:19.000000000 -0500
@@ -129,8 +129,23 @@
 
 static HMENU current_popup_menu;
 
-FARPROC get_menu_item_info;
-FARPROC set_menu_item_info;
+void syms_of_w32menu ();
+
+typedef BOOL (WINAPI * GetMenuItemInfoA_Proc) (
+    IN HMENU,
+    IN UINT,
+    IN BOOL,
+    IN OUT LPMENUITEMINFOA
+    );
+typedef BOOL (WINAPI * SetMenuItemInfoA_Proc) (
+    IN HMENU,
+    IN UINT,
+    IN BOOL,
+    IN LPCMENUITEMINFOA
+    );
+
+GetMenuItemInfoA_Proc get_menu_item_info=NULL;
+SetMenuItemInfoA_Proc set_menu_item_info=NULL;
 
 Lisp_Object Vmenu_updating_frame;
 
@@ -1591,6 +1606,26 @@
 initialize_frame_menubar (f)
      FRAME_PTR f;
 {
+  HMODULE user32 = GetModuleHandle ("user32.dll");
+  /*
+    Reinitialize the function pointers set_menu_item_info and
+    get_menu_item_info here.
+    Even though these function pointers are initialized in
+    syms_of_w32menu which is called from main (emacs.c),
+    we need to initialize them again here in order to prevent
+    a crash that occurs in Windows 9x (possibly only when Emacs
+    was built on Windows NT / 2000 / XP?) in add_menu_item.
+    The crash occurs when attempting to call the Win32 API
+    function SetMenuItemInfo through the function pointer.
+    It appears as if the function pointer that is obtained when
+    syms_of_w32menu is called from main is no longer valid
+    (possibly due to DLL relocation?).
+    To resolve this issue, I have placed calls to reinitialize
+    these function pointers here because this function is the
+    entry point for menu creation.
+   */
+  get_menu_item_info = (GetMenuItemInfoA_Proc) GetProcAddress (user32, "GetMenuItemInfoA");
+  set_menu_item_info = (SetMenuItemInfoA_Proc) GetProcAddress (user32, "SetMenuItemInfoA");
   /* This function is called before the first chance to redisplay
      the frame.  It has to be, so the frame will have the right size.  */
   FRAME_MENU_BAR_ITEMS (f) = menu_bar_items (FRAME_MENU_BAR_ITEMS (f));
@@ -2355,13 +2390,12 @@
 
 #endif /* HAVE_MENUS */
 
-\f
-syms_of_w32menu ()
+void syms_of_w32menu ()
 {
   /* See if Get/SetMenuItemInfo functions are available.  */
   HMODULE user32 = GetModuleHandle ("user32.dll");
-  get_menu_item_info = GetProcAddress (user32, "GetMenuItemInfoA");
-  set_menu_item_info = GetProcAddress (user32, "SetMenuItemInfoA");
+  get_menu_item_info = (GetMenuItemInfoA_Proc) GetProcAddress (user32, "GetMenuItemInfoA");
+  set_menu_item_info = (SetMenuItemInfoA_Proc) GetProcAddress (user32, "SetMenuItemInfoA");
 
   staticpro (&menu_items);
   menu_items = Qnil;

Re: Fix for Emacs Crash - revisited

[Juanma Barranquero]

On Sun, 10 Nov 2002 03:45:34 -0500, "Ben Key" <Bkey1@tampabay.rr.com> wrote:

> 1.  init_user_info in w32.c has been modified so that OpenProcessToken
>     is no longer called on systems running Windows 9x.

In fact, with your patch OpenProcessToken is called *only* if the system
identifies itself as an OS_NT. Not likely to change, perhaps, but it
could break in future Windows OSes.

> I have been trying to determine why these function pointers needed to
> be reinitialized.  At first I thought that it might have something to
> do will DLL relocation, but I no longer think that after discussing
> my ideas with Glen Gordan, one of my coworkers.  He told me that when
> Windows determine that DLL relocation needs to occur, it happens
> before main gets called, not after.

My opinion is that we should research it further.  Perhpas it is some
kind of memory corruption, that your patch would hide.

It is particularly interesting that it happens only in 9x vs. XP builds.
AFAIK, Andrew's been building the binary releases on NT for quite a
while and it's never been such a problem running those releases on 9x.

It is a 21.3.50 thing or does it also happen with 21.[12] releases
and/or the RC branch?

                                                           /L/e/k/t/u

Re: Fix for Emacs Crash - revisited

[Juanma Barranquero]

On Mon, 11 Nov 2002 08:55:26 US/Eastern, bkey1@tampabay.rr.com wrote:

> For all of the current Windows NT Style OSes, Windows NT 4.0,
> Windows 2000, and Windows XP (possibly even Windows NT 3.51),
> ossubtype is equal to OS_NT.

Yes, I know.

> However, if there is a concern that this
> may break in a future OS, I could easily change my code to
> call GetVersionEx and change the test from 
> ossubtype==OS_NT
> to os_version_info.dwPlatformId==VER_PLATFORM_WIN32_NT.

IMHO the test should *exclude* the platforms where you know the call to
OpenProcessToken fails, not include the ones where it works, because the
first subset is more likely to remain stable than the second one.

> While I was debugging these crashes I found that when I set a breakpoint in
> the functions syms_of_w32menu or syms_of_w32fns the debugger
> failed to stop at the break point.  Neither were calls to 
> OutputDebugString I placed in these functions processed.  In
> addition, when I set breakpoints in main where these functions
> were called, the debugger failed to stop at these break points.
> However, I was able to verify that these functions were being
> called by setting a global variable to a specific value and
> setting a break point in initialize_frame_menubar and verifying
> that the global variable had that specific value.  I am not
> certain what caused this inability to get the debugger to stop
> at these breakpoints

Usually it means that it consists of dead code, i.e., that no machine code
was generated for those source locations... Or there's something I'm
missing?

> He was probably using MinGW, not MSVC 6.0.

AFAIK, Andrew switches between MinGW and MSVC from one release to the
next. But you're right, the problem obviously existed before as you
found it in etc/PROBLEMS. My mistake.

Anyway, it is still interesting trying to determine why it does happens
in XP vs. 9x/Me and not, for example, 2K vs 9x/Me.

As an aside: According to the Platform SDK, OpenProcessToken only exists
in NT 3.1 and later, not in the 9x series...


                                                           /L/e/k/t/u

Re: Fix for Emacs Crash - revisited

[Juanma Barranquero]

On Mon, 11 Nov 2002 10:32:49 US/Eastern, bkey1@tampabay.rr.com wrote:

> I would agree with this if it where not for the global variable test
> I did (I set a global variable to a specific value in syms_of_w32menu,
> set a breakpoint in initialize_frame_menubar which is called after 
> syms_of_w32menu, and found that at this point the global variable had
> that specific value).

That's really weird.

Excuse me for asking (perhaps obvious) questions, but: wouldn't you
accidentally mix optimized and not-optimized modules? I've had weird
things happening in those cases. And it's really strange not to be able
to put a breakpoint in existing code...

> As I said before,
> I am uncomfortable when I cannot determine why a change that fixes a crash
> is necessary.  That is why I tested this change as throughly as I did.

No doubt. You're making a great effort on all this. Please don't think
I'm thinking or saying otherwise.

> The Platform SDK documentation says it is only Supported in Windows
> NT / 2000 / XP.  This does not necessarily mean it does not exist in
> other versions of Windows.

I know. But I don't think we should be relying on
undocumented/unsupported features, so the question would be: Why was it
used in the code? For some reason we don't know or simply because of an
oversight?

> If OpenProcessToken did not exist in Windows 9x, Emacs would not even
> run in Windows 9x.  Instead, a dialog complaining about OpenProcessToken
> not being found would be displayed and then Emacs would exit unless we
> changed the code to call OpenProcessToken via a function pointer rather
> than calling it directly.

I know ;)

> NOTE: The call to OpenProcessToken was there
> before I made my change.

That was obvious from your patch :)

                                                           /L/e/k/t/u

Re: Fix for Emacs Crash - revisited

[bkey1]

  > IMHO the test should *exclude* the platforms where you know the call to
  > OpenProcessToken fails, not include the ones where it works, because the
  > first subset is more likely to remain stable than the second one.
Good idea, I will make this change tonight.

  > 
  > Usually it means that it consists of dead code, i.e., that no machine code
  > was generated for those source locations... Or there's something I'm
  > missing?
I would agree with this if it where not for the global variable test
I did (I set a global variable to a specific value in syms_of_w32menu,
set a breakpoint in initialize_frame_menubar which is called after 
syms_of_w32menu, and found that at this point the global variable had
that specific value).

  > 
  > Anyway, it is still interesting trying to determine why it does happens
  > in XP vs. 9x/Me and not, for example, 2K vs 9x/Me.
One of the grand mysteries of the universe.  I also would like to know
the answer to this question, but I was unable to find it.  As I said before,
I am uncomfortable when I cannot determine why a change that fixes a crash
is necessary.  That is why I tested this change as throughly as I did.

> 
> As an aside: According to the Platform SDK, OpenProcessToken only exists
> in NT 3.1 and later, not in the 9x series...
The Platform SDK documentation says it is only Supported in Windows
NT / 2000 / XP.  This does not necessarily mean it does not exist in
other versions of Windows.  I found out that it does exist in the
version of advapi32.dll found on Windows 9x by using dumpbin /exports.
If OpenProcessToken did not exist in Windows 9x, Emacs would not even
run in Windows 9x.  Instead, a dialog complaining about OpenProcessToken
not being found would be displayed and then Emacs would exit unless we
changed the code to call OpenProcessToken via a function pointer rather
than calling it directly.  NOTE: The call to OpenProcessToken was there
before I made my change.

Re: Fix for Emacs Crash - revisited

[Jason Rumney]

"Ben Key" <Bkey1@tampabay.rr.com> writes:

>     I found that even though track_mouse_event_fn was being
>     initialized in syms_of_w32fns which is called from main,

I think you will find that syms_of_w32fns and syms_of_w32menu are only
called when Emacs is dumped, and that is why looking up the addresses
of dynamic library functions from them causes problems when the binary
is run on a different version of Windows.

But rather than moving the initialization to where a system message
is processed, perhaps there is another suitable w32 specific
initialization function that is called every time Emacs runs. I think
there is one in w32term.c at least, although that would involve
making the function pointers non-static.