Baron-von-Riedesel / DOS-debug

Debug and DebugX (short: Debug/X) are extended versions of MS DEBUG
58 stars 12 forks source link

Clearing TF on stack should use B bit of stack segment to determine use of esp #6

Closed ecm-pushbx closed 2 years ago

ecm-pushbx commented 2 years ago

https://github.com/Baron-von-Riedesel/DOS-debug/blob/b60a75d445f08f9a20abc48ccfb906225194bb41/src/DEBUG.ASM#L7089

ecm-pushbx commented 2 years ago

The fix is simple:

diff --git a/DEBUG.ASM b/DEBUG.ASM
--- a/DEBUG.ASM
+++ b/DEBUG.ASM
@@ -7086,7 +7086,7 @@
        mov es,[regs.rSS]
 if ?PM
        mov bx,es
-       call getseglimit
+       call getseldefsize
        jz @F
        .386
        mov ebx,dword ptr [regs.rSP]

I made a test case as follows:

  1. Set ss limit to 128 KiB - 1 B.
  2. Set esp to 1_0000h
  3. Clear ss B bit
  4. pushfd
  5. pop into register
  6. Check whether the TF appears to be set

When tracing through this sequence using current DebugX, running on HDPMI32 on qemu, it will leave TF set in the flags, though presumably corrupting memory in the other 64 KiB.

(On dosemu2 it appears that esp is always used even when the ss B bit is clear. This is an unrelated bug. However, it means we cannot check for this on dosemu2.)

Baron-von-Riedesel commented 2 years ago

Yes, this is actually a regression in v1.28.

ecm-pushbx commented 2 years ago

Your changelog entry says:

regression in DEBUGX, v1.28: trace flag onto stack wasn't cleared if debuggee was in protected-mode, its stack was 32-bit and ESP was greater 0ffffh

However, this is not accurate. An esp that is intended to be used beyond 64 KiB implies a segment limit that is also beyond 64 KiB (and B bit set). This case worked as expected. What didn't work was an ss limit beyond 64 KiB but the B bit being clear and the high word of esp not being zero. According to the B bit only the 16-bit sp should be used, but prior to the fix the function would use the 32-bit esp.

Anyway, the bug wasn't simply an apparent TF of 1 on the stack, the debugger would also corrupt a bit of memory in the stack segment other than what was meant to be cleared.