#2155 [PATCH] Scite (4.3.0): IDM_QUIT from Lua triggers DEP on WinXP

[Dejan Budimir]

(* DEP = Data Execution Protection)

There seem to be no problems with:

user.shortcuts=Escape|IDM_QUIT|

and

quit.on.close.last=1

user.shortcuts=Escape|IDM_CLOSE|

but

command.name.49.*=lua_quit
command.49.*=lua_quit
command.subsystem.49.*=3
command.mode.49.*=savebefore:no
command.shortcut.49.*=Escape

with

function lua_quit(id)
    scite.MenuCommand(IDM_QUIT)
end

as well as

quit.on.close.last=1

command.name.49.*=lua_close
command.49.*=lua_close
command.subsystem.49.*=3
command.mode.49.*=savebefore:no
command.shortcut.49.*=Escape

with

function lua_close(id)
    scite.MenuCommand(IDM_CLOSE)
end

would cause 'USER32.DLL' to mess with an invalid address, and I wish that I could tell you why.

Looking at the message pump it became obvious that the 'WM_QUIT'message was never received and rewriting the message handling into something more idiomatic (in my view) seems to have fixed the issue.

(The patch also corrects the too optimistic interpretation of 'GetMessage''s "'BOOL'" return.)

[Dejan Budimir]

Better version of the patch, same code changes.

[Neil Hodgson]

The failures appear to me to be occurring because Scintilla is called after it has been destroyed. Specifically, after Lua has been called to quit, the menus are checked by asking Scintilla for state. This can be avoided by not performing the checks when the main window is null:

diff -r 0d8ee7bc03cf win32/SciTEWinBar.cxx
--- a/win32/SciTEWinBar.cxx Fri Jan 31 12:23:51 2020 +1100
+++ b/win32/SciTEWinBar.cxx Sat Feb 01 09:30:50 2020 +1100
@@ -530,10 +530,12 @@
        ::EnableMenuItem(::GetMenu(MainHWND()), wIDCheckItem, MF_DISABLED | MF_GRAYED | MF_BYCOMMAND);
    ::EnableButton(HwndOf(wToolBar), wIDCheckItem, val);
 }

 void SciTEWin::CheckMenus() {

+   if (!MainHWND())
+       return;
    SciTEBase::CheckMenus();
    ::CheckMenuRadioItem(::GetMenu(MainHWND()), IDM_EOL_CRLF, IDM_EOL_LF,
                 static_cast<int>(wEditor.EOLMode()) - static_cast<int>(SA::EndOfLine::CrLf) + IDM_EOL_CRLF, 0);
    ::CheckMenuRadioItem(::GetMenu(MainHWND()), IDM_ENCODING_DEFAULT, IDM_ENCODING_UCOOKIE,
                 CurrentBuffer()->unicodeMode + IDM_ENCODING_DEFAULT, 0);

[Dejan Budimir]

That makes much more sense. I'll be testing it tomorrow.

[Dejan Budimir]

Good job, Neil! The DEP no longer happens with your patch. Now the GetMessage() properly receives the WM_QUIT message which makes it return 0 and terminate the message pump as expected.

There are still a couple of issues, though, let me address them in another post.

[Dejan Budimir]

In SciTEWin::QuitProgram():

::PostQuitMessage(0);
wSciTE.Destroy();

This does two things: it

- adds a WM_QUIT message to the message queue, and
- calls into the `WM_DESTROY` of `SciTEWin::WndProc` (and waits for the return).

Currently, WM_DESTROY does nothing, so the call is a no-op and trivially safe. But there may be a future scenario where the message loop is supposed to outlive the main window for a while.

I believe that the proper way to shut down a windows app is to go through a couple stages via wndproc.

1. Call `::SendMessage(...,WM_CLOSE,..)` (not CloseWindow() which only `SW_HIDE`'s it)
2. Let `case WM_CLOSE` call `DestroyWindow()`
3. Let `case WM_DESTROY` potentially free some heap and call `PostQuitMessage(0)` when (and if) it's done.

Cheers!

[Neil Hodgson]

But there may be a future scenario where the message loop is supposed to outlive the main window for a while.

This doesn't seem likely to me.

Unless there is a demonstrable bug here, these changes may cause other problems so do not appear worthwhile.

[Dejan Budimir]

I was just hoping that they may solve some other problems ;). But I do appreciate your minimalism. in keeping what works and will try to abide by it

[Dejan Budimir]

The other issue is that GetMessage() may return a negative value (mostly at compile-time errors, as far as I can glean, so not really an issue ... but nevertheless). We should at least be testing for if (going > 0) { in SciTEWin::EventLoop(), if only to calm the future reader's anxiety.

[Dejan Budimir]

Because the return value can be nonzero, zero, or -1, avoid code like this: ...
MSDN

[Dejan Budimir]

Another minor issue is the use of the going variable in the EventLoop. First,
BOOL going = true; should probably read BOOL going = TRUE;. Second, it sort of hides a potentially infinite loop. Third, it isn't really needed here.

[Neil Hodgson]

Its not an infinite loop - its a loop that is controlled by the result of GetMessageW so it is most clearly coded as showing that dependency.

Other issues addressed with [e9c086].

[Dejan Budimir]

I really hate to waste your time any further. I'm saying that it's the only place where going is used. It would be better documented as if (0 < GetMessageW(...)) ... else break;.

[Neil Hodgson]

To me it is more explicit to have a control variable that is tested by the loop condition as that shows it is not an infinite loop and makes it easier to reason about the loop behaviour.

[Dejan Budimir]

I understand. I will adapt. My preference is for fewer symbols and for shorter scopes.

[Dejan Budimir]

Lastly, using ::GetMessageW but not ::TranslateAcceleratorW kinda feels shoddy. And we're better than that.

[Neil Hodgson]

Committed fix as [d52b46].