Skip to content

sCalc: fix out-of-bounds read in lrc() on an empty operand#39

Open
physwkim wants to merge 1 commit into
epics-modules:masterfrom
physwkim:fix/scalc-lrc-empty-operand
Open

sCalc: fix out-of-bounds read in lrc() on an empty operand#39
physwkim wants to merge 1 commit into
epics-modules:masterfrom
physwkim:fix/scalc-lrc-empty-operand

Conversation

@physwkim

Copy link
Copy Markdown

Problem

lrc() in sCalcPerform.c loops with

for (i=0, lrc=0; i<strlen(rawInput)-1; i+=2) {
    lrc += hex(rawInput[i])*0x10 + hex(rawInput[i+1]);

strlen() returns size_t (unsigned). For an empty operand,
strlen(rawInput)-1 wraps to SIZE_MAX, so i < SIZE_MAX is always
true and the loop reads rawInput[] far out of bounds.

The LRC and AMODBUS operators call lrc() with whatever string is
on the stack (case LRC: / case AMODBUS: in sCalcPerform.c), so an
scalcout expression like LRC('') — or LRC applied to an empty
string field — crashes the IOC with SIGSEGV.

Fix

Compute the length once into a signed int and loop while i+1 < len.
An empty operand now yields an empty loop (LRC "00"); non-empty
operands are unchanged. The sibling xor8() already guards len == 0;
this brings lrc() in line.

Test

Adds two scalcTest cases:

  • LRC('')"00"
  • LRC('F7031389000A')"60" (the LRC 0x60 from the AMODBUS
    worked example in the source comment)

Without the fix scalcTest terminates with SIGSEGV on the empty-operand
case; with the fix the full suite passes (145/145).

lrc() looped with `i < strlen(rawInput)-1`.  strlen() returns size_t,
so for an empty operand strlen(rawInput)-1 wraps to SIZE_MAX and the
loop condition is always true, reading rawInput[] far out of bounds.

The LRC and AMODBUS operators reach lrc() with any string on the stack
(sCalcPerform.c case LRC/AMODBUS), so an scalcout expression such as
LRC('') or LRC applied to an empty field crashes the IOC with SIGSEGV.

Compute the length once into a signed int and loop while `i+1 < len`,
so an empty operand yields an empty loop (LRC "00") and non-empty
operands are unchanged.  The sibling xor8() already guards len==0.

Add scalcTest cases: LRC('') -> "00" and LRC('F7031389000A') -> "60"
(the LRC 0x60 from the AMODBUS example in the source comment).  Without
the fix scalcTest terminates with SIGSEGV on the empty-operand case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant