Fix operator precedence in StackLimitEvader memory guard assertion#16532
Fix operator precedence in StackLimitEvader memory guard assertion#16532cuiweixie wants to merge 1 commit intoargotorg:developfrom
Conversation
|
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
| // Make sure all calls to ``memoryguard`` we found have the same value as argument (otherwise, abort). | ||
| u256 reservedMemory = literalArgumentValue(*memoryGuardCalls.front()); | ||
| yulAssert(reservedMemory < u256(1) << 32 - 1, ""); | ||
| yulAssert(reservedMemory < (u256(1) << 32 - 1), ""); |
There was a problem hiding this comment.
What's wrong with precedence here? << and - have higher precedence than <, so the parenthesis is redundant.
There was a problem hiding this comment.
Ah, looking at the description now, you mean the precedence of << in relation to -. You have a bug in your code if you meant to fix that.
Though I'm not 100% this is the right fix anyway. The value that makes the most sense to me here is 2**32, which would allow the range 0..2**32-1. If you change it the way you're suggesting, we get 0..2**32-2, which is odd (why leave out the last 32-bit value?). There's also non-zero probability that the intention was actually 2**31, though I'd agree that writing it as 2**(32 - 1) would be unusual.
Can you check the wider context? The assertion is just a sanity check that verifies something that must be guarded elsewhere by a proper check or stem from some lower level limitation. If you find that, the right value should become apparent.
There was a problem hiding this comment.
change to
yulAssert(reservedMemory < (u256(1) << 32) - 1, "");
| // Make sure all calls to ``memoryguard`` we found have the same value as argument (otherwise, abort). | ||
| u256 reservedMemory = literalArgumentValue(*memoryGuardCalls.front()); | ||
| yulAssert(reservedMemory < u256(1) << 32 - 1, ""); | ||
| yulAssert(reservedMemory < (u256(1) << 32 - 1), ""); |
There was a problem hiding this comment.
Ah, looking at the description now, you mean the precedence of << in relation to -. You have a bug in your code if you meant to fix that.
Though I'm not 100% this is the right fix anyway. The value that makes the most sense to me here is 2**32, which would allow the range 0..2**32-1. If you change it the way you're suggesting, we get 0..2**32-2, which is odd (why leave out the last 32-bit value?). There's also non-zero probability that the intention was actually 2**31, though I'd agree that writing it as 2**(32 - 1) would be unusual.
Can you check the wider context? The assertion is just a sanity check that verifies something that must be guarded elsewhere by a proper check or stem from some lower level limitation. If you find that, the right value should become apparent.
cuiweixie
force-pushed
the
bugfix/op
branch
2 times, most recently
from
00f030c to
f021195
Compare
The expression u256(1) << 32 - 1 was incorrectly evaluated as u256(1) << 31 due to operator precedence (32 - 1 evaluated first). Add parentheses to correctly compute (u256(1) << 32) - 1, i.e. the maximum 32-bit value.
2 participants
The expression u256(1) << 32 - 1 was incorrectly evaluated as u256(1) << 31 due to operator precedence (32 - 1 evaluated first). Add parentheses to correctly compute (u256(1) << 32) - 1, i.e. the maximum 32-bit value.