search

open-watcom / open-watcom-v2

Open Watcom V2.0 - Source code repository, Wiki, Latest Binary build, Archived builds including all installers for download.
Other
957 stars 157 forks source link

Win16 IDE File open bug #677

Open HnkGitHub opened 3 years ago

HnkGitHub commented 3 years ago

Platform: 16 bit windows, Windows 3.1

the Windows 3.1 IDE crashes on opening a file for a project. Makes the IDE unusable. Looks nice. Just can't use it.

jmalak commented 3 years ago

I can not reproduce. I am able open project file. could you give me example with more detailed info?

HnkGitHub commented 3 years ago

Let me try on a native system. Crashes in DOSBox DOS emulator. But Borland's and Microsoft's ide"s work fine. I'll uninstall and reinstall and let it set the environment: config.sys, autoexec.bat, etc. I did install the Win32s subsystem update. 1.03a.

jmalak commented 3 years ago

I need understand on which operation it crash. If I understand correctly then you use DOSBOX DOS-emulator with Windows 3.x and Win32s. It is untested environment, OW suppose native DOS. Do you have installed wdebug.386 VXD driver? Did you tried WIN32 version of IDE or 16-bit Windows 3.x version?

HnkGitHub commented 3 years ago

I believe it crashes on processing the FOD_ADD or FOD_ADDALL window messages in vfilelist.cpp. I checked the ini file and wdebug.386 is loaded. I tested it on a windows 3.1 machine and the behavior is identical to the DOSBox environment. I ran the 16 bit wini86 ide.exe in a windows 2003 system, which supports 16 bit exes, and the problem disappears.

I would guess that the code is using unsupported win32 APIs

HnkGitHub commented 3 years ago

It can open an existing project with existing files. But when you insert or add source files to a project, then it crashes. I hope this explains it better.

This is the wini86 version crashing in a windows 3.1 environment. The same 16 bit ide runs Ok in a win32 environment (2003 or XP32): The 32 bit ide does not have the same problem.

jmalak commented 3 years ago

Thanks for detailed explanation. I will try to reproduce it on native Windows 3.1. It can be incompatibility in file name processing (path separator or file name component length) or some buffer overflow. What Windows version (exactly) you use 3.1 or 3.11?

HnkGitHub commented 3 years ago

Crashes on both. 3.1 and 3.11. Tried with and without Win32s subsystem. Didn't make a difference..

HnkGitHub commented 3 years ago

Tracked it down to the #include line 43 in bld/ide/ide/cpp/vfilelist.cpp. This include file cannot be found through the usual means. The file simply does not exist in the visible source code. So, I thought it was a typographical error.

But it's not. The successful compilation of the ide.exe depends on the existence of wwindows.h. I believe this file is created on the fly by wmake or the wcc compiler from the wi163464.h header file found in open-watcom-v2/bld/watcom/h. I'm guessing that either of these executables has code creating the wwindows.h header file for use in files that reference the wwindows.h include file.

GET_DLGDATA is referenced from this file and it defines it as a WIN32 API GetWindowLong(HWND, DWL_USER) . However, the Win32 version of GetWindowLong cannot be used in a Win16 application. The win16 GetWindowLong API call uses an int for the second parameter: 2 bytes in a Windows 16-bit OS environment. The file only defines 2 versions of GET_DLGDATA: one for WIN32, the other for WIN64: The wi163264.h file does not define a Win16 version of GET_DLGDATA. Apparently it uses, the WIN32 version.

This file must be an old file from the original source code and has the message saying, "If you figure out what this file does, please explain it here."

I don't think that this is the only 16 bit gui exe using the wwindows.h file. And I think that there could be a few more defines in this file that are incorrectly setting WIN32 API calls or defines when it should be setting WIN16 API calls or WIN16 defines.

The file is not that long and probably should be phased out separating code out for the specific OS as it is recommended in the file: TODO: This file is mostly specific to MS Windows. We need to make a platform neutral version and move the contents to the correct platform specific directories.

jmalak commented 3 years ago

Sorry GetWindowLong is standard 16-bit Windows API Entry, that your conclusion is wrong. also wwindows.h is located in bld/watcom/h subdirectory, it is transparent. Read carefully win163264.h file which is compatibility layer which always refer on appropriate windows.h header file. Next compatibility layer is WPI in bld/wpi sub-directory (OS/2 PM and Windows). For WIN386 extender is important _win386.h and wclbproc.h header file.

jmalak commented 3 years ago

it looks like bug in wclass sub-project where all file/path names are handled.

HnkGitHub commented 3 years ago

Both the Win32 API and the Windows 3.1 API have a function called GetWindowLong(HWND, int). They have the same declaration; however, they are not identical: both the HWND and int parameter are 2 bytes in size in the Windows 3.1 API, whereas they're 4 bytes in the WIN32 API. In 64 bit windows, I believe the two values are 8 bytes in size.

The C language does not allow overloading. The C++ language does.

jmalak commented 3 years ago

Below is prototype for various versions of Windows:

LONG WINAPI                         GetWindowLong( HWND, int );         16-bit Windows
WINUSERAPI LONG WINAPI    GetWindowLongA( HWND, int );       32-bit Windows
WINUSERAPI LONG WINAPI    GetWindowLongA(HWND, int);         64-bit Windows

LONG and HWND are Windows types defined by windows.h and int is C type. You can see they use exactly same prototype, there can not be any problem.

HnkGitHub commented 3 years ago

I was just wondering what the target type is for the wini86 ide: watcom extender, or win16. Changing from Win16 to watcom extender changes the size of integer from 2 to 4 bytes.

I thought that this might be the root of the problem. But checked the export definitions of win16x.dll and can only find GETWINDOWLONG32. So, I guess the OS knows what to do with differing sizes based on the type of executable. It's kind of hard to see what's really going on without using a dependable debugger in this environment.

jmalak commented 3 years ago

It is simple wini86 subdirectory is always 16-bit application for 16-bit Windows. win386 is 32-bit application for 16-bit Windows (bind with Watcom 32-bit Extender) nt386 is 32-bit application for 32/64-bit Windows ntx64 is 64-bit appůication for 64-bit Windows.

ide is only 16-bit application for 16-bit WIndows.

jmalak commented 3 years ago

If you are interested in WIN386 32-bit mode then you should study bld/win386 project where WIN386 Extender code and appropriate libraries with chunks for calling 16-bit WINAPI functions from 32-bit code.

pwroberts commented 1 year ago

I've been looking into this after encountering various similar crashes in the 16-bit IDE. I believe there is a whole category of crash bugs related to the file select dialog (vfilelst.cpp) caused by heap corruption of some kind. The crash occurs in the 16-bit allocator (allocseg.c/growseg.c) and seems to involve the allocator trying to read its own enabled state (__heap_enabled) from beyond the end of the current data segment.

To clarify a misunderstanding in the original few posts, the crashes don't occur on opening a project file, but rather on trying to add files ("sources" in Watcom-speak) to an already-open project.

Here are the steps I use to repro:

I also get a crash if I try to actually add a file in the file select dialog, but it's simpler/quicker to just immediately click Close on that dialog and trigger the crash. I believe the failure mode (and root cause) are the same.

I did some "printf debugging" with MessageBox and narrowed the crash down to GetResults(). The calls to various flavors of GetWindowLong succeed, and the values coming back from them (window handles, counts, text lengths) look OK, so I think the investigation of wwindows.h and GetWindowLong earlier in the thread was a red herring.

The crash location seemed to move around depending on my debugging code, which suggested heap corruption to me.

I built a debug build of the IDE (thanks @jmalak for Discord help building individual targets with debug info) and was eventually able to source-level debug it using Watcom 11.0c on Windows 3.1 (after various failed attempts with combinations of CodeView and OW 1.9 on Windows XP, 98, 95, and 3.1).

With a working debugger, I confirmed the crash usually (for me) occurs when GetResults() tries to allocate a small char array: https://github.com/open-watcom/open-watcom-v2/blob/master/bld/ide/ide/cpp/vfilelst.cpp#L360

"len" is zero here (for me), so the code (validly) attempts to allocate a char array of length 1. The call stack looks something like this:

We crash near the start of GrowSeg() where we try to read heap_enabled: https://github.com/open-watcom/open-watcom-v2/blob/a6882b7710215993d3fe64bf4fb4364b546cf091/bld/clib/heap/c/growseg.c#L67

In case it was a bug with small allocations, I tried new char[11], and got a crash in AllocSeg() instead of GrowSeg(), but as before, it crashed trying to read __heap_enabled: https://github.com/open-watcom/open-watcom-v2/blob/a6882b7710215993d3fe64bf4fb4364b546cf091/bld/clib/heap/c/allocseg.c#L77

When I stepped into GrowSeg() disassembly, I saw it try to read heap_enabled from 147f:1e8c, but segment 147f ended at offset 1a9f, i.e. it tried to read __heap_enabled from approximately 0x400 bytes beyond the end of the segment.

I added heap check canaries (try to call new char[1] and then delete []) throughout vfilelst and the modules that call into it, and I found that these small test allocations succeed immediately before the call to GetOpenFileName (which opens the dialog) and crash immediately after the WM_INITDIALOG message is received.

So things are already in a bad state when the dialog appears. Assuming no false negatives from the test allocations, things may go bad somewhere in the call stack between calling GetOpenFileName() and receiving the AddSrcDlgProc callback with the WM_INITDIALOG message. Unfortunately that call stack is tall, opaque, and not in application code, but there may be a problem with the dialog resource or how OW is populating the OPENFILENAME struct in vfilelst.