Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CauseWay DOS-extender bugs #1361

Open
Baron-von-Riedesel opened this issue Dec 20, 2024 · 16 comments
Open

CauseWay DOS-extender bugs #1361

Baron-von-Riedesel opened this issue Dec 20, 2024 · 16 comments
Labels

Comments

@Baron-von-Riedesel
Copy link

Baron-von-Riedesel commented Dec 20, 2024

Hello,

since OW 2.0 has apparently switched to CauseWay as its preferred DOS extender, and since the source of the app is available, it's probably time to fix some bugs in it.

This small test program ( compiled with wcc386, linked with "wlink system causeway" )

#include <stdio.h>

char getif( void )
{
    char rc;
    _asm {
        //int 3
        push ebp
        mov ebp, 0fff00000h
        mov ax,902h
        int 31h
        pop ebp
        mov rc,al
    }
    return rc;
}

int main ( void )
{
    printf("IF=%u\n", getif() );
    _asm cli;
    printf("IF=%u\n", getif() );
    _asm sti;
    printf("IF=%u\n", getif() );
    return( 0 );
}

should print the current state of the CPU's interrupt flag, using DPMI function 0x902. In Open Watcom, it causes page faults if it runs in a "raw" or VCPI environment ( that is, NOT in a Windows DOS box or DosEmu ).

It's because of a bug in the CauseWay extender, file raw_vcpi.asm, function RawDPMIPatch():

rv46_DPMI_0902:
        cmp     al,02h          ;get virtual interupt state?
        jnz     rv46_NotOurs
        push    ebp
        mov     ebp, esp        ; <<<< this line was missing
        assume ds:nothing
        test    BYTE PTR cs:DpmiEmuSystemFlags,1
        assume ds:_cwDPMIEMU
        jz      rv46_3
        movzx   ebp,bp
;        mov     al,[ebp+(4)+(2+2+2)+(2+2)+1]  ; <<<< displacement too large by 3*2
        mov     al,[ebp+(4)+(2+2)+1]
        and     al,2
        shr     al,1
        pop     ebp
        clc
        jmp     rv46_Done
rv46_3:
;        mov     al,[ebp+(4)+(4+4+4)+(4+4)+1]  ; <<<< displacement too large by 3*4
        mov     al,[ebp+4+(4+4)+1]
        and     al,2
        shr     al,1

There's an uninitialized register (EBP) and wrong displacements, the code parts probably have been copied from somewhere and then weren't tested. The very same bug occurs for DPMI functions 0x900 and 0x901 - althouth those are a bit more severe, since the interrupt flag is modified by them, meaning a hazardous write to memory.

@jmalak jmalak added the bug label Dec 20, 2024
@jmalak
Copy link
Member

jmalak commented Dec 20, 2024

Thank you for your bug report and analysis.

@rdos314
Copy link
Member

rdos314 commented Dec 20, 2024

The code seems a bit strange. Why is there a movzx ebp,bp in it? This will mean that the stack must be less than 64k, otherwise it will read random garbage. I thought CauseWay was a 32-bit DOS extender, and thus should not use a 16-bit stack.

Reading the IF flag from the stack implies it's trying to read it from the interrupt frame generated by int 31h, but why would the positions differ based on DpmiEmuSystemsFlags?

@Baron-von-Riedesel
Copy link
Author

I thought CauseWay was a 32-bit DOS extender, and thus should not use a 16-bit stack.

OW uses CauseWay just to create 32-bit zero-based flat binaries, but the original CauseWay extender is more powerful - has support for 16- and 32-bit small model and 32-bit flat model. The CW linker, WL32, is a 16-bit "small" app, for example.

jmalak added a commit that referenced this issue Dec 21, 2024
@Baron-von-Riedesel
Copy link
Author

Baron-von-Riedesel commented Jan 2, 2025

Another issue of the CauseWay extender: it may "jump into the wild", that is, jump to a code address that is in a conventional memory region that has been released to DOS. Such errors often vary in their appearance, depending on the DOS memory structure.

Here's a test case:

/* generate "not present" exception */

void CreateNPExc( void )
{
    _asm {
    mov cx,1
    mov ax,0
    int 31h
    mov ebx,eax
    mov ecx,cs
    lar ecx,ecx
    shr ecx,8
    and cl,7Fh  ;reset present bit
    mov ax,9
    int 31h

    push ebx
    pop fs
    };
    return;
}

void AllocDosMem( void )
{
    _asm {
    mov bx,0ffffh
    mov ax,100h
    int 31h
    mov ax,100h
    int 31h     // second call should allocate all conv. dos mem
    jc exit
    push es     // now clear the allocated memory
    pushad
    cld
    xor edi, edi
    mov es, edx
    movzx ecx, bx
    shl ecx, 4
    mov al, 0Fh  // clear with 0Fs
    rep stosb
    popad
    pop es
exit:
    };
    return;
}

int main ( void )
{
    AllocDosMem();
    CreateNPExc();
    return( 0 );
}

This sample generates an exception 0x0B, after allocating - and "clearing" - all free DOS memory.
Compile with "wcc386 sample.c", link with "wlink system causeway f sample.obj". When running the program (in "raw/vcpi" mode, that is, NOT in a DOS box of Windows or DosEmu), It may display various sorts of errors, unable to recover.

The problem is in file interrup.asm:

;
;Now switch back to exit code.
;
if 0 // current code
        mov     ax,InitDS
        mov     ds,ax
        assume ds:_cwInit
        mov     WORD PTR IErrorNumber,0
        mov     ax,InitCS
        push    ax
        mov     ax,offset InitError
        push    ax
        db 66h
        retf
else // fix
        mov     ax,MainDS
        mov     ds,ax
        assume ds:_cwMain
        jmp     f[TerminationHandler]
endif

Problem with the current code is that it tries to jump to a function in _cwInit - this segment is only present during intialization.
Note that some more "usual" exceptions ( like 0x0D or 0x0E ) are handled on a higher level ( by code in exceptn.asm ), so that plain DPMI-mode is also covered - that's why this error won't show up too often...

@jmalak
Copy link
Member

jmalak commented Jan 2, 2025

Thank you again for bug report and issue analysis.

@jmalak jmalak closed this as completed Jan 2, 2025
@jmalak jmalak reopened this Jan 2, 2025
@Baron-von-Riedesel
Copy link
Author

One more issue about exceptions. The previous test case was about exceptions that generate an error code. Some do not, and CauseWay "tends" to handle those as simple interrupts, that is, if they aren't handled by a protected-mode interrupt handler, they are routed to real-mode. Interrupts 0 and 1 are handled differently, they are always treated as exceptions.

This is - at least - a problem with interrupt 6 ( invalid opcode ). This is a fault, not a trap, so it MUST be handled, and it cannot be handled properly in real-mode ( DOS or the V86 monitor program may simply terminate the application, leaving the system in an unstable state ).

The - rather simple - test case reveals the problem if the program runs in raw/vcpi mode:

/* generate "invalid opcode" exception */

void CreateExc06( void )
{
    _asm {
        mov ax,0
        mov cs,ax
    };
    return;
}

int main ( void )
{
    CreateExc06();
    return( 0 );
}

compiled as before with "wcc386 sample.c", linked with "wlink system causeway f sample.obj".

As for now, to fix this I'd suggest to handle int 6 - equally to int 0 - as an exception. The default CauseWay exception handler will then terminate the application in protected-mode, with an error msg ( and file cw.err written ). A few other exceptions without error code ( int 4, 5, 7 ) may perhaps need further investigation...

This would be the proposed fix, in file interrup.asm:

inter14_NoCode:
        and     w[esp+(4+4)+(4)+(4+4)],0011111111010101b
        mov     eax,[esp+(4+4)+(4)+(4+4)]       ;Get flags.
        and     eax,not 65536
        mov     ExceptionFlags,eax
        cmp ExceptionIndex,0           ;int 0
        jz inter14_ForceException
if 1
        cmp ExceptionIndex,6            ;<<<< int 06 is to be an exception!
        jz inter14_ForceException      ;<<<<
endif
        cmp     ExceptionIndex,1        ;int 1
        jnz     inter14_SortedCode
inter14_ForceException:

@jmalak
Copy link
Member

jmalak commented Jan 5, 2025

Thanks for your next bug report.
I think the proposed fix is not correct, because "ForceException" cause processing always as Exception even if it is INTx.
But Exception and Interrupt processing is control for INT by InterruptTable and for Exception by ExceptionTable. I think the issue is that stack frame is handled incorrectly also there is confusing bit in ExceptionFlag (bit 16) which should signal Exception with Error Code but it looks like it is used with different meaning and code is wrong. stack depth is handled correctly but it looks like pointers to frame menbers are incorrect. I will check code in more details to fix these mismatches.

jmalak added a commit to jmalak/open-watcom-v2 that referenced this issue Jan 6, 2025
jmalak added a commit to jmalak/open-watcom-v2 that referenced this issue Jan 6, 2025
jmalak added a commit that referenced this issue Jan 6, 2025
@Baron-von-Riedesel
Copy link
Author

Baron-von-Riedesel commented Jan 6, 2025

I think the proposed fix is not correct, because "ForceException" cause processing always as Exception even if it is INTx.

Well, yes, it's not exactly what DPMI host do - hdpmi also tries to distinguish between exception 06 and interrupt 06, just to be compatible with what the Win9x host does. But is it really worth it? CauseWay isn't a DPMI host, and the important thing is that an unhandled exc 06 should terminate the app, safely.

@Baron-von-Riedesel
Copy link
Author

Baron-von-Riedesel commented Jan 10, 2025

Here's a CauseWay memory management bug that should make the machine either freeze or reboot:

/* alloc memory */

#include <stdio.h>

void *AllocDPMIMemory( unsigned int amount )
{
    void *rc = 0;
    _asm {
        mov ecx,amount
        shld ebx,ecx,16
        mov ax,501h
        int 31h
        jc error
        push bx
        push cx
        pop eax
        mov rc, eax
    error:
    };
    return rc;
}

int main ( void )
{
    void * p1;
    void * p2;
    p1 = AllocDPMIMemory(0x20000000);
    printf("1. alloc = %p\n", p1 );
    p2 = AllocDPMIMemory(0xf0000000);
    printf("2. alloc = %p\n", p2 );

    return( 0 );
}

The problem is that the causeway kernel encounters an "out of address space" error here, but is unable to detect it.

Proposed fix in memory.asm, proc ExtendLinearMemory:

        mov     eax,ecx
        shl     eax,12
        dec     eax
        add     eax,LinearLimit
        jc      mem10_error              ;<<<<< line added
        dec     eax
        sub     eax,LinearBase
        cmp     eax,MaxMemLin
        jnc     mem10_error

EDIT: I forgot to mention: the test case assumes that the first alloc ( 512 MB ) succeeds. So - in case you want to try it on a VM - you'll have to setup the machine with at least 520 MB

@Baron-von-Riedesel
Copy link
Author

Baron-von-Riedesel commented Jan 12, 2025

A problem with CauseWays DPMI emulation, again causing a crash:

/* call an int 31h function not implemented by CauseWay DPMI emulator */

#include <stdio.h>

short int GetFPEmuStatus( void )
{
    short int rc = -1;
    _asm {
        mov ax,0E00h
        int 31h
        jc error
        mov rc, ax
    error:
    };
    return rc;
}

int main ( void )
{
    printf("int 31h, ax=0E00h returned = %X\n", GetFPEmuStatus() );
    return( 0 );
}

The problem is that the CauseWay "DPMI emulator", if it doesn't know how to handle the Int 31h, routes it to "the previous" handler". Since there is no real "previous handler", the interrupt finally arrives in real-mode ... and the real-mode IVT vector 31h is not an address supposed to be called.

IMO the fix would be in raw_vcpi.asm

rv46_NotOurs:
if 0        
        ;Not a function recognised by us so pass control to previous handler.
        ;
        assume ds:nothing
        jmp     FWORD PTR cs:[OldInt31h]         ;pass it onto previous handler.
        assume ds:_cwDPMIEMU
        ;
OldInt31h       dd offset IntNN386Catch+(31h*8)
        dw DpmiEmuCS
else
;--- just return with Carry set.
        stc
        jmp rv46_Done
endif

@jmalak
Copy link
Member

jmalak commented Jan 17, 2025

Thanks for your info and analysis again.
I have only basic knowledge about VCPI and DPMI that I need think little more.
It looks like the difference between INTx and Exception handling by Interrupt and Exception tables is not well done that what you proposed is only solution to handle the issue.
I will implement your proposed change.
I am preparing also introduction of structures for INT/EXC stack frames to do handling transparently without manual calculated offsets to fix some wrong offsets.

@Baron-von-Riedesel
Copy link
Author

It looks like the difference between INTx and Exception handling by Interrupt and Exception tables is not well done that what you proposed is only solution to handle the issue.

I'm not disappointed that you didn't find a proper solution - I'm also convinced that a 100% safe distinction between exception 06 and a coded INT 06 doesn't exist. Long ago I even wrote a little test program just to see what all the DPMI host do in thoses cases - cwsdpmi is the strangest beast in this regard, btw.

Back to CauseWay: there's a (minor) bug in proc RawCallBack (file raw_vcpi.asm):

RawCallBack     proc    near
        pushf
        cli
        ;
        ;Check if Windows enhanced mode has been started.
        ;
        assume ds:nothing
        cmp     cs:InWindows,0
        assume ds:_cwRaw
        jz      rv30_Normal
        popf
        add sp,2                  ;<----- line missing
        retf
        ;
rv30_Normal:

It's interesting that Michael Devore cared about Win3 enhanced mode - however, his "fix" surely was never tested: proc RawCallBack is always called NEAR ( to have a means to calculate the callback number ), so the stack has to be adjusted before the RETF.

Inside RawCallBack one could do a small optimization

        ;
        ;Check if this call back is int &| busy.
        ;
        mov     ax,RetAdd               ;get return address.
        sub     ax,CallBackSize ;back to start of call back entry.
        sub     ax,offset CallBackList  ;offset from start of list.
        xor     dx,dx
        mov     bx,CallBackSize
        div     bx              ;entry number.
        mov     bx,size CallBackStruc
        mul     bx              ;get offset into table.
        mov     bx,offset CallBackTable
        add     bx,ax           ;point to this entry.
        ;
        [snip]
        ;
        mov     ax,RetAdd               ;get return address.
        sub     ax,CallBackSize ;back to start of call back entry.
        sub     ax,offset CallBackList  ;offset from start of list.
        jz      rv30_zero
        xor     dx,dx
        mov     bx,CallBackSize
        div     bx              ;entry number.
rv30_zero:
        mov     bx,size CallBackStruc
        mul     bx              ;get offset into table.
        mov     bx,offset CallBackTable
        add     bx,ax           ;point to this entry.

The code to calculate the callback number exists twice - one can remove one of the instances ( register BX is never modified by the code between, so the second calculation is redundant ).

@Baron-von-Riedesel
Copy link
Author

Another (small) bug in CauseWay's DPMI emulator - function "release descriptor" ( int 31h, ax=1 ). The test case:

#include <stdio.h>

int ReleaseDescriptor( unsigned short selector )
{
    int rc;
    _asm {
        mov bx,selector
        mov ax,1
        int 31h
        mov eax,0
        jc error
        mov eax,1
    error:
        mov rc, eax
    };
    return rc;
}

int main ( void )
{
    /* release a descriptor that hasn't been allocated;
     * should return 0.
     */
    printf("ReleaseDescriptor(0x1007)=%u\n", ReleaseDescriptor( 0x1007 ) );
    return( 0 );
}

Currently '1' is returned by function ReleaseDescriptor if the program runs in raw/vcpi mode. In a Windows DOS box, '0' will be returned.

The problem is: the function that handles the request doesn't check if the descriptor that's to be released is a valid one and if it has been allocated at all.
My suggestion for a fix in ldt.asm:

RawRelDescriptor proc near
        push    eax
        push    ecx
        push    esi
        push    ds
        push    es
        mov     ax,KernalDS     ;access _cwRaw segment
        mov     ds,eax
        assume ds:_cwRaw
        mov     ax,KernalZero
        mov     es,eax
        movzx   esi,bx          ;Get selector to use.
        shr     esi,3           ;/8 for descriptor number.

        test  bl,4                     ; must be a LDT entry  
        jz      error
        cmp     esi,GDT_Entries ;don't release GDT entries
        jc      error

        add     esi,MDTLinear+4

        cmp     BYTE PTR es:[esi],0  ;don't release unallocated descriptors!
        jz      error

        mov     BYTE PTR es:[esi],0  ;mark this entry as free.
        movzx   esi,bx
        and     esi,not 7
        add     esi,MDTLinear
        xor     eax,eax
        mov     es:[esi+0],eax
        mov     es:[esi+4],eax

        xor     ecx,ecx         ;check GS/FS/ES/DS registers and reset them if 
        mov     esi,gs          ;they've become invalid
        lar     eax,esi
        jz      @F
        mov     gs,ecx
@@:
        mov     esi,fs
        lar     eax,esi
        jz      @F
        mov     fs,ecx
@@:
        pop     esi
        lar     eax,esi
        jz      @F
        mov     esi,ecx
@@:
        mov     es,esi
        pop     esi
        lar     eax,esi
        jz      @F
        mov     esi,ecx
@@:
        mov     ds,esi
        ;
        clc
        jmp     rrd_9
error:
        pop     es
        pop     ds
        stc
rrd_9: 
        pop     esi
        pop     ecx
        pop     eax
        ret
        assume ds:_cwDPMIEMU
RawRelDescriptor endp

The line "cmp esi,GDT_Entries ;don't release GDT entries" perhaps deserves an explanation: CauseWay maps GDT and LDT to the very same linear address space - no idea why. The first entries (about 20) in the LDT are therefore actually "GDT descriptors" - to "release" them most likely causes an immediate crash.

@jmalak
Copy link
Member

jmalak commented Jan 23, 2025

You are right there is some redundant and odd code.
Now I am working on code to be more readable by using new macros.
I will add new structures instead of hardcoded offsets, especially on stack.
After this I will apply your proposed changes.

@Baron-von-Riedesel
Copy link
Author

Baron-von-Riedesel commented Jan 29, 2025

After this I will apply your proposed changes.

There's no hurry - none of the bugs described so far are critical.

Another issue ( or at least something for discussion ): CauseWay emits DOS int 21h calls for its swapfile support without checking if the InDos flag is set. Usually the swapfile is only accessed when memory conditions are "low", but there is one exception: for DPMI function 0x500 (get free memory info), CauseWay emits an int 21h, ah=36h to detect the current free disk space ( unless the NOVM option has been set ). This should probably be fixed , IMO.

@jmalak
Copy link
Member

jmalak commented Jan 29, 2025

Back to exceptions versus interrupt detection I am afraid that we need to prefer exception handling as you proposed, probably nobody will be raise appropriate interrupt.
I did some code cleaning that it should be a little bit readable then before.
Anyway thanks for all your hints and comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants