aCalc: fix off-by-two in INC() runtime stack-overflow guard#40
Open
physwkim wants to merge 1 commit into
Open
Conversation
The value stack is calloc(ACALC_STACKSIZE) with top = &stack[1], so the last writable element is stack[ACALC_STACKSIZE-1]. INC() advanced ps, then guarded with `(ps-top) > ACALC_STACKSIZE` before writing (ps)->numEl and (ps)->sourceDouble. Since ps addresses stack[1 + (ps-top)], the write lands out of bounds once (ps-top) reaches ACALC_STACKSIZE-1: the guard let indices ACALC_STACKSIZE and ACALC_STACKSIZE+1 be written before tripping. The postfix compiler caps a purely scalar expression at ACALC_STACKSIZE, so scalar depth alone never reaches the guard. NDERIV/DERIV/FITPOLY, however, push two or three scratch elements at run time (aCalcPerform.c NDERIV does DEC then three INCs) that the compiler's per-opcode `runtime_effect` accounting models only as a net delta, not the transient peak. A deep-enough expression such as `1+(1+(...+(NDERIV(AA,2))...))` therefore passes aCalcPostfix() yet drives the runtime stack past its end, writing out of bounds and crashing the IOC. Guard on `(ps-top) > ACALC_STACKSIZE-2` so INC() rejects before the write can exceed stack[ACALC_STACKSIZE-1]. Valid depths are unchanged. Add acalcTest cases: 15 levels of nesting around NDERIV evaluate; 16 levels are rejected by the runtime guard. Without the fix the k=16 case writes out of bounds and terminates acalcTest with SIGSEGV.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The aCalc value stack is
calloc(ACALC_STACKSIZE)withtop = &stack[1],so the last writable element is
stack[ACALC_STACKSIZE-1].INC()advances
ps, then guards withBecause
psaddressesstack[1 + (ps-top)], the write lands out ofbounds once
(ps-top)reachesACALC_STACKSIZE-1. The guard admitsindices
ACALC_STACKSIZEandACALC_STACKSIZE+1before it trips —an off-by-two heap overflow of two
stackElements.The postfix compiler caps a purely scalar expression at
ACALC_STACKSIZE, so scalar depth alone never reaches the runtime guard.But
NDERIV/DERIV/FITPOLYpush two or three scratch elements at runtime (e.g.
NDERIVdoesDECthen threeINCs inaCalcPerform.c) thatthe compiler's per-opcode
runtime_effectaccounting models only as anet delta, not the transient peak. A deep-but-compiler-accepted
expression such as
therefore passes
aCalcPostfix()yet drives the runtime stack past itsend, writing out of bounds and crashing the IOC (SIGSEGV).
Fix
Guard on
(ps-top) > ACALC_STACKSIZE-2, soINC()rejects before thewrite can exceed
stack[ACALC_STACKSIZE-1]. Valid stack depths areunchanged (an expression that legitimately reached the old limit was
already writing out of bounds).
Test
Adds two
acalcTestcases aroundNDERIV: 15 levels of nesting evaluatenormally; 16 levels are rejected by the runtime guard (
aCalcPerformreturns non-zero) instead of overflowing. Without the fix the 16-level
case writes out of bounds and terminates
acalcTestwith SIGSEGV; withthe fix the full suite passes (150/150).