From cb9e330cfaaa112215a2eb3e0af9bb50047eb3b0 Mon Sep 17 00:00:00 2001 From: Jeff Parsons Date: Fri, 11 Sep 2015 15:13:08 -0700 Subject: [PATCH] Fixed a stack-size bug uncovered by a PUSH SS/POP SS sequence in Windows 95 kernel --- modules/pcjs/lib/x86cpu.js | 15 ++++++++++++++- modules/pcjs/lib/x86func.js | 11 ++++++++++- modules/pcjs/lib/x86op0f.js | 10 ++++++++++ modules/pcjs/lib/x86ops.js | 29 +++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 2 deletions(-) diff --git a/modules/pcjs/lib/x86cpu.js b/modules/pcjs/lib/x86cpu.js index d44a54858..73ef1b2de 100644 --- a/modules/pcjs/lib/x86cpu.js +++ b/modules/pcjs/lib/x86cpu.js @@ -2173,7 +2173,20 @@ X86CPU.prototype.setSS = function(sel, fInterruptable) var regESP = this.getSP(); var regLSP = this.segSS.load(sel); if (regLSP !== X86.ADDR_INVALID) { - this.regLSP = (regLSP + regESP)|0; + /* + * The safest way to update regLSP after a potential change to segSS.base is to call setSP() + * with the original stack pointer retrieved above via getSP(). When I tried to be clever and + * do this instead: + * + * this.regLSP = (regLSP + regESP)|0; + * + * 16-bit stacks began inadvertently using ESP instead of SP. The moral: don't be needlessly clever. + * + * Sprinkle the following assert throughout stack operations to catch that bug in the future: + * + * this.assert(!((this.regLSP - this.segSS.base) & ~this.segSS.maskAddr)); + */ + this.setSP(regESP); if (this.segSS.fExpDown) { this.regLSPLimit = (this.segSS.base + this.segSS.maskAddr)|0; this.regLSPLimitLow = (this.segSS.base + this.segSS.limit)|0; diff --git a/modules/pcjs/lib/x86func.js b/modules/pcjs/lib/x86func.js index 7c244b341..796b68186 100644 --- a/modules/pcjs/lib/x86func.js +++ b/modules/pcjs/lib/x86func.js @@ -1439,7 +1439,11 @@ X86.fnINT = function INT(nIDT, nError, nCycles) */ X86.fnIRET = function IRET() { + /* + * As discussed in fnRETF(), we temporarily set opLSP around operations that fnFault() may need to restart. + */ this.opLSP = this.regLSP; + this.nStepCycles -= this.cycleCounts.nOpCyclesIRet; if ((this.regCR0 & X86.CR0.MSW.PE) && (this.regPS & X86.PS.NT)) { @@ -2436,6 +2440,7 @@ X86.fnRCRd = function RCRd(dst, src) X86.fnRETF = function RETF(n) { this.opLSP = this.regLSP; + var newIP = this.popWord(); var newCS = this.popWord(); @@ -3916,13 +3921,17 @@ X86.fnFault = function(nFault, nError, fHalt, nCycles) * Prior to each new burst of instructions, stepCPU() sets fComplete to true, and the only (normal) way * for fComplete to become false is through stopCPU(), which isn't ordinarily called, except by the Debugger. */ + this.resetSizes(); this.setIP(this.opLIP - this.segCS.base); } else if (this.model >= X86.MODEL_80186) { if (this.nFault < 0) { /* - * Single-fault (error code is passed through, and the responsible instruction is restartable) + * Single-fault (error code is passed through, and the responsible instruction is restartable; + * the call to resetSizes() is critical, otherwise setIP() may update IP with the wrong size if + * the current instruction contains an OPERAND size override). */ + this.resetSizes(); this.setIP(this.opLIP - this.segCS.base); if (this.opLSP != X86.ADDR_INVALID) { this.setSP((this.regESP & ~this.segSS.maskAddr) | (this.opLSP - this.segSS.base)); diff --git a/modules/pcjs/lib/x86op0f.js b/modules/pcjs/lib/x86op0f.js index e521e5e05..9a7830185 100644 --- a/modules/pcjs/lib/x86op0f.js +++ b/modules/pcjs/lib/x86op0f.js @@ -1100,6 +1100,11 @@ X86.opSETNLE = function SETNLE() */ X86.opPUSHFS = function PUSHFS() { + /* + * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4, + * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write + * a 32-bit value, effectively zeroing the 2 upper bytes. Need to confirm this. + */ this.pushWord(this.segFS.sel); this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg; }; @@ -1167,6 +1172,11 @@ X86.opSHLDcl = function SHLDcl() */ X86.opPUSHGS = function PUSHGS() { + /* + * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4, + * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write + * a 32-bit value, effectively zeroing the 2 upper bytes. Need to confirm this. + */ this.pushWord(this.segGS.sel); this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg; }; diff --git a/modules/pcjs/lib/x86ops.js b/modules/pcjs/lib/x86ops.js index dba27038d..0d5cbbf6c 100644 --- a/modules/pcjs/lib/x86ops.js +++ b/modules/pcjs/lib/x86ops.js @@ -126,6 +126,11 @@ X86.opADDAX = function ADDAX() */ X86.opPUSHES = function PUSHES() { + /* + * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4, + * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write + * a 32-bit value, effectively zeroing the 2 upper bytes. Need to confirm this. + */ this.pushWord(this.segES.sel); this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg; }; @@ -137,6 +142,9 @@ X86.opPUSHES = function PUSHES() */ X86.opPOPES = function POPES() { + /* + * As discussed in fnRETF(), we temporarily set opLSP around operations that fnFault() may need to restart. + */ this.opLSP = this.regLSP; this.setES(this.popWord()); this.nStepCycles -= this.cycleCounts.nOpCyclesPopReg; @@ -216,6 +224,11 @@ X86.opORAX = function ORAX() */ X86.opPUSHCS = function PUSHCS() { + /* + * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4, + * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write + * a 32-bit value, effectively zeroing the 2 upper bytes. Need to confirm this. + */ this.pushWord(this.segCS.sel); this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg; }; @@ -314,6 +327,11 @@ X86.opADCAX = function ADCAX() */ X86.opPUSHSS = function PUSHSS() { + /* + * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4, + * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write + * a 32-bit value, effectively zeroing the 2 upper bytes. Need to confirm this. + */ this.pushWord(this.segSS.sel); this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg; }; @@ -325,6 +343,9 @@ X86.opPUSHSS = function PUSHSS() */ X86.opPOPSS = function POPSS() { + /* + * As discussed in fnRETF(), we temporarily set opLSP around operations that fnFault() may need to restart. + */ this.opLSP = this.regLSP; this.setSS(this.popWord()); this.nStepCycles -= this.cycleCounts.nOpCyclesPopReg; @@ -404,6 +425,11 @@ X86.opSBBAX = function SBBAX() */ X86.opPUSHDS = function PUSHDS() { + /* + * TODO: Reportedly, when the OPERAND size is 32 bits, the 80386 will decrement the stack pointer by 4, + * write the selector into the 2 lower bytes, and leave the 2 upper bytes untouched, whereas we will write + * a 32-bit value, effectively zeroing the 2 upper bytes. Need to confirm this. + */ this.pushWord(this.segDS.sel); this.nStepCycles -= this.cycleCounts.nOpCyclesPushSeg; }; @@ -415,6 +441,9 @@ X86.opPUSHDS = function PUSHDS() */ X86.opPOPDS = function POPDS() { + /* + * As discussed in fnRETF(), we temporarily set opLSP around operations that fnFault() may need to restart. + */ this.opLSP = this.regLSP; this.setDS(this.popWord()); this.nStepCycles -= this.cycleCounts.nOpCyclesPopReg;