Skip to content

Handle non-byte-sized array element types in bounds_check_index#8843

Open
tautschnig wants to merge 1 commit intodiffblue:developfrom
tautschnig:bounds-check-integer
Open

Handle non-byte-sized array element types in bounds_check_index#8843
tautschnig wants to merge 1 commit intodiffblue:developfrom
tautschnig:bounds-check-integer

Conversation

@tautschnig
Copy link
Collaborator

@tautschnig tautschnig commented Feb 24, 2026

When the array element type has no byte size (e.g., arrays of __CPROVER_integer), object_descriptor_exprt::build cannot compute byte offsets. Fall back to a simple index-based lower-bound check instead of crashing. The upper bound check uses the array size directly and does not require byte offsets.

A DATA_INVARIANT ensures that arrays with non-byte-sized element types are not accessed via pointer dereference, making explicit the assumption that these arise only from internal modeling constructs.

Co-authored-by: Kiro kiro-agent@users.noreply.github.com

  • Each commit message has a non-empty body, explaining why the change was made.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • n/a My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes a crash in bounds_check_index that occurs when checking bounds for arrays with element types that have no computable byte size, such as arrays of __CPROVER_integer (mathematical integers) or unbounded arrays used in heap models.

The crash occurred because object_descriptor_exprt::build calls size_of_expr on the element type, and when that returns std::nullopt, the resulting ID_unknown offset causes a precondition violation in from_integer downstream.

Changes:

  • Modified bounds_check_index to check if the element type has a computable size before building the object descriptor
  • Introduced a simplified index-based lower-bound check when element size is unavailable
  • Wrapped object_descriptor_exprt in std::optional to conditionally build and use it
  • Added defensive DATA_INVARIANT to catch unexpected array expression types with unsized elements
  • Added regression test for arrays with mathematical integer element types

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/ansi-c/goto-conversion/goto_check_c.cpp Modified bounds_check_index to handle unsized array element types with conditional object descriptor building and simplified bounds checks
regression/cbmc/bounds_check_integer_index1/main.c Added test case with __CPROVER_integer array to verify bounds checking doesn't crash
regression/cbmc/bounds_check_integer_index1/test.desc Test configuration verifying both lower and upper bound checks are generated
regression/validate-trace-xml-schema/check.py Registered new test in validation exclusion list

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tautschnig tautschnig assigned tautschnig and unassigned kroening Feb 24, 2026
@tautschnig tautschnig force-pushed the bounds-check-integer branch 2 times, most recently from 3b0c6d1 to 1a1502c Compare March 4, 2026 21:56
@tautschnig tautschnig requested a review from Copilot March 4, 2026 21:57
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Mar 4, 2026

Codecov Report

❌ Patch coverage is 95.16129% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.03%. Comparing base (4eb741f) to head (b796cbc).

Files with missing lines Patch % Lines
src/ansi-c/goto-conversion/goto_check_c.cpp 95.16% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8843      +/-   ##
===========================================
+ Coverage    80.01%   80.03%   +0.02%     
===========================================
  Files         1700     1700              
  Lines       188342   188376      +34     
  Branches        73       73              
===========================================
+ Hits        150710   150776      +66     
+ Misses       37632    37600      -32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kroening
Copy link
Collaborator

kroening commented Mar 5, 2026

I would change the terminology from "unsized" to "dynamically sized". These arrays do have a size, it's just not a constant.

@tautschnig
Copy link
Collaborator Author

I would change the terminology from "unsized" to "dynamically sized". These arrays do have a size, it's just not a constant.

Hmm, when the element type is ID_integer (as one example) I don't think "dynamically sized" would seem right? Maybe "unsized" isn't right either, though. Any further ideas?

@kroening
Copy link
Collaborator

kroening commented Mar 5, 2026

Hmm, when the element type is ID_integer (as one example) I don't think "dynamically sized" would seem right? Maybe "unsized" isn't right either, though. Any further ideas?

If you mean the index type (not element type), then simply call them "unbounded"?

@tautschnig tautschnig force-pushed the bounds-check-integer branch from 1a1502c to add1701 Compare March 5, 2026 08:12
@tautschnig tautschnig changed the title Handle unsized array element types in bounds_check_index Handle non-byte-sized array element types in bounds_check_index Mar 5, 2026
@tautschnig
Copy link
Collaborator Author

If you mean the index type (not element type), then simply call them "unbounded"?

No, this genuinely is about the element type. I have updated both the title and the commit message (and PR description) to use the term "non-byte-sized".

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tautschnig tautschnig force-pushed the bounds-check-integer branch from add1701 to 1982f1d Compare March 5, 2026 09:57
When the array element type has no byte size (e.g., arrays of
__CPROVER_integer), object_descriptor_exprt::build cannot compute byte
offsets. Fall back to a simple index-based lower-bound check instead of
crashing. The upper bound check uses the array size directly and does
not require byte offsets.

A DATA_INVARIANT ensures that arrays with non-byte-sized element types
are not accessed via pointer dereference, making explicit the assumption
that these arise only from internal modeling constructs.

Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
@tautschnig tautschnig force-pushed the bounds-check-integer branch from 1982f1d to b796cbc Compare March 5, 2026 11:02
@tautschnig tautschnig assigned kroening and unassigned tautschnig Mar 6, 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.

3 participants