Skip to content

Check open integer-range bounds for shift-left overflow - #6647

Open
JanTvrdik wants to merge 1 commit into
phpstan:2.2.xfrom
JanTvrdik:jt-fix-bitshift-overflow
Open

JanTvrdik wants to merge 1 commit into
phpstan:2.2.xfrom
JanTvrdik:jt-fix-bitshift-overflow

Conversation

@JanTvrdik

@JanTvrdik JanTvrdik commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Check open integer-range bounds for shift-left overflow, using PHP_INT_MIN and PHP_INT_MAX when a bound is null.

The existing overflow check only checks finite bounds. For int<0, max> << 8, it checks zero but skips the maximum, so the result incorrectly remains non-negative. The same problem affects open lower bounds.

This causes a false smaller.alwaysFalse error in a big-endian u64() reader:

$value = 0;
for ($i = 0; $i < 8; $i++) {
    $value = ($value << 8) | ord($bytes[$i]);
}
if ($value < 0) {
    throw new RuntimeException('Value is too large for 64-bit signed integer');
}

The false positive first appears in 2.2.9. Git bisect identifies 1081ec4, whose bitwise OR range inference exposes the shift-left problem. The more recent finite-bound overflow check does not cover this case.

Tests

  • Added type-inference tests for the original reader and six open-bound variants.
  • Added a comparison-rule test that rejects the false positive.
  • Confirmed the new tests fail before the source fix and pass after it.
  • Updated two existing shift-left expectations that also assumed overflow was impossible.
  • Zero shifts and right shifts retain their existing range inference.

Local validation on PHP 8.5:

  • Full ParaTest suite: 21,765 tests, 97,025 assertions, 74 skipped, no failures.
  • PHPStan self-analysis: no errors.
  • Code style for the changed source and test class: passes.
  • The PHAR built by this PR's CI also passes the full passkeys analysis with its original configuration.

Original failure: https://github.com/shipmonk-rnd/passkeys/actions/runs/36873090362/job/110405538537

Task: https://app.asana.com/0/0/1219068476946134

Copilot AI balanced review requested due to automatic review settings October 1, 2026 14:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The focused fix correctly handles both open bounds and includes comprehensive regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes shift-left inference for open integer ranges by accounting for platform limits during overflow detection.

Changes:

  • Checks unbounded range endpoints using PHP_INT_MIN and PHP_INT_MAX.
  • Updates affected shift-left expectations.
  • Adds regression coverage for open ranges and the u64() reader false positive.
File Description
src/​Reflection/​InitializerExprTypeResolver.php Detects overflow at open integer-range bounds.
tests/​PHPStan/​Analyser/​nsrt/​shift-left-unbounded-range.php Covers open bounds, zero/right shifts, and the reader regression.
tests/​PHPStan/​Analyser/​nsrt/​integer-range-types.php Updates overflow-sensitive inferred types.
tests/​PHPStan/​Rules/​Comparison/​NumberComparisonOperatorsConstantConditionRuleTest.php Verifies the false comparison warning is eliminated.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@JanTvrdik

Copy link
Copy Markdown
Contributor Author

The core unit-test matrix, self-analysis matrix, generated baselines, and coding standard have passed. I also downloaded the PHAR built by this PR and ran the full passkeys analysis with its original configuration: no errors.

The broader PHAR integration matrix is not green, but its current failures also occur in the earlier run https://github.com/phpstan/phpstan-src/actions/runs/36867554483, before this PR:

I have not changed those unrelated checks or their baselines. This account has read-only access to the upstream repository, so I cannot rerun its jobs.

@JanTvrdik

Copy link
Copy Markdown
Contributor Author

Final CI update: the core unit tests, self-analysis, coding standard, and generated baselines pass, but not all remaining failures are pre-existing.

PocketMine is affected by this fix. At src/world/format/io/region/RegionLoader.php:153–154, it passes getSectorCount() << 12 to fread(). getSectorCount() is annotated only as positive-int, with no finite upper bound. The old inference assumed this shift stayed positive; the corrected inference returns int because it can overflow. Consequently, fread() now reports that its length must be int<1, max>. A finite upper bound on the sector count would allow the existing bounded-range inference to retain positivity. I have not suppressed this diagnostic or changed the integration baseline.

The other newly completed failures are:

  • Both mutation-testing jobs were interrupted by runner shutdown/cancellation while generating mutants; they did not report mutation-score failures.
  • The PHP 8.5 benchmark exceeded its timing thresholds for bug-14972.php and bug-14996.php. I have not established whether those timing failures are related to this change.

The earlier comment's pre-existing-failure evidence applies to the specific jobs listed there, not to every failure in the final matrix.

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.

2 participants