Skip to content

Fix readFieldText not validating coordinates against screen bounds (#4)#7

Open
p0dalirius wants to merge 1 commit intomainfrom
bugfix-read-field-text-bounds
Open

Fix readFieldText not validating coordinates against screen bounds (#4)#7
p0dalirius wants to merge 1 commit intomainfrom
bugfix-read-field-text-bounds

Conversation

@p0dalirius
Copy link
Copy Markdown
Contributor

Linked Issue

Closes #4

Root Cause

ScriptExecutor::readFieldText(row, col) forwarded the caller-supplied coordinates straight to the host's ScreenInterface::readFieldText, even though the neighbouring helper readScreenText(row, col, length) already performs an equivalent bounds check (row < 0 || row >= m_screen->rows() || col < 0). EXPECT FIELD AT ... and EXTRACT $var FIELD AT ... therefore pushed negative or out-of-screen coordinates — including the -2, -2 pair that results from AT 0 0 after the 1-based → 0-based conversion — across the library boundary, leaving behaviour entirely to the host implementation.

Fix Description

Add the same style of bounds check that readScreenText already uses, extended with col >= m_screen->cols() (applicable here because this helper takes no length argument). When the coordinates are out of range, return an empty QString and do not dispatch to the screen. This is consistent with the existing "no field found" return contract documented on ScreenInterface::readFieldText and keeps the library defensive regardless of the host implementation's own robustness.

How Verified

  • Tests: added tests/test_script_executor_bounds.cpp::readFieldTextFiltersOutOfRangeCoordinates. The test installs a RecordingScreen ScreenInterface stub whose readFieldText records every (row, col) it is called with, then runs a script with four EXTRACT $x FIELD AT ... lines (three out of range, one in range). It asserts that exactly one call reaches the screen and that the one call's coordinates match the in-range extract.
  • Regression check: I temporarily replaced the bounds check with if (false) and reran the test — it correctly failed with "Actual: 4, Expected: 1" at the QCOMPARE(screen.m_fieldCalls.size(), 1) assertion. Restoring the check made it pass again. This confirms the test exercises the bug, not a tautology.
  • Existing tests: test_script_lexer and test_script_parser both still pass.

Test Coverage

Added: tests/test_script_executor_bounds.cpp::readFieldTextFiltersOutOfRangeCoordinates — uses a recording ScreenInterface stub to observe that out-of-range EXTRACT FIELD AT / EXPECT FIELD AT coordinates no longer reach the host screen. A new test_script_executor_bounds test target is added to CMakeLists.txt.

Scope of Change

  • Files changed: src/script_executor.cpp, tests/test_script_executor_bounds.cpp (new), CMakeLists.txt
  • Submodule pointer updated: no
  • Behavioral changes outside the bug fix: none

Risk and Rollout

Local change. In-range queries behave exactly as before. Out-of-range queries that previously produced host-dependent behavior now consistently return an empty QString, which the EXPECT FIELD CONTAINS comparison already handles correctly (returns false — the negated variant returns true — so NOT semantics are preserved) and EXTRACT FIELD stores as an empty variable value.

@p0dalirius p0dalirius self-assigned this Apr 18, 2026
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.

readFieldText does not validate row/col against screen bounds

1 participant