Repository navigation
x86: bound reads both of its operands - #3109
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #3109 +/- ##
=======================================
Coverage ? 61.09%
=======================================
Files ? 243
Lines ? 56756
Branches ? 10063
=======================================
Hits ? 34675
Misses ? 16871
Partials ? 5210 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Weirdly I did write it by hand. I'll try harder next time. |
Collaborator
Sorry, I didn't mean to offend you. |
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.
Detailed description
Detailed description
The
boundinstruction is currently marks its register operand as written to but that is incorrect.BOUNDonly reads its operands: it compares the value in the register against a pair of bounds stored in memory. If the value is outside those bounds, it raises a#BRexception. It does not write a result anywhere.This incorrect access information also causes confusing results between Intel and AT&T syntax:
The fix is to mark both operands as read-only for the
X86_BOUNDS16rmandX86_BOUNDS32rmentries inX86MappingInsnOp.incandX86MappingInsnOp_reduce.inc.The AT&T printer normally reverses the operand-access information because AT&T syntax generally reverses the operand order compared with Intel syntax. However,
boundis unusual because its AT&T representation keeps the same operand order as Intel syntax. (enter,lcall, andljmpbehave similarly, but their mapping rows do not contain access information.)Once both
boundoperands are correctly marked as reads, reversing their access information makes no difference because both values are identical. Therefore, there is no need to change the AT&T printer's general reversal behavior.Test plan
Add tests to
tests/details/x86.yamlfor:62 03using Intel syntax, which exercisesBOUNDS32rm.62 03using AT&T syntax, also exercisingBOUNDS32rm.66 62 03, which exercisesBOUNDS16rm.The tests check that both operands are marked as reads and that
regs_readcontains the correct registers. All three tests fail on the currentnextbranch. If either mapping row is reverted after the fix, only the tests that use that particular instruction form fail, confirming that both rows are covered.cstestcannot assert thatregs_writeis empty, so there is no direct test for the absence of theRegisters modifiedoutput. Instead, this is indirectly covered by checking that the register operand is marked as read rather than write.(Note that the corresponding entries in
X86MappingInsnOp_reduce.incwere checked separately usingcstoolfrom a build withCAPSTONE_X86_REDUCE=ONbecausecstestdoes not link against that configuration).Closing issues
The same two incorrect mapping rows also exist in
v5and need the same fix.