Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arithmetic overflow for sdiv #1087

Closed
nmushegian opened this issue Sep 12, 2016 · 4 comments
Closed

Arithmetic overflow for sdiv #1087

nmushegian opened this issue Sep 12, 2016 · 4 comments

Comments

@nmushegian
Copy link

nmushegian commented Sep 12, 2016

I was very surprised to learn DIV does not generate VM-level exceptions. I do see division by 0 is handled by the compiler now.

The "subleties" document only outlines the division by 0 case for SDIV:

https://github.com/ethereum/wiki/wiki/Subtleties

DIV, SDIV, MOD and SMOD with dividend (second argument) equal to 0 push 0 to the stack.

So what happens with SDIV(MININT, -1)? If I'm seeing this right, this behavior should absolutely be handled by the compiler as it is far less intuitive than the 0 case - nobody expects over/underflows for division like they do for multiplication.

Here's a test that works in latest browser-solidity. The result of these two functions is the same, and now the only time SDIV returns an incorrect (arbitrarily un-undefined) signed division value.

pragma solidity ^0.4.0;
contract test {
        function minInt() returns (int) {
            return -2**255;
        }
        function testSDIV() returns (int) {
        int minus1 = -1;
        int result = minInt() / minus1;
        return result;
    }
}
@chriseth
Copy link
Contributor

Thanks for catching this!

@chriseth chriseth modified the milestone: soo Sep 12, 2016
@chriseth chriseth added the soon label Sep 12, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Sep 13, 2016
@pirapira pirapira self-assigned this Sep 13, 2016
@pirapira pirapira added in progress and removed soon labels Sep 13, 2016
@chriseth
Copy link
Contributor

Having thought a bit more about this, I would actually say that this falls into the "throw on arithmetic overflow" category and not into the "throw on division by zero" category.

@pirapira
Copy link
Member

Yes, because -(minInt) is too big.

pirapira added a commit to pirapira/solidity that referenced this issue Sep 23, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Oct 5, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Oct 6, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Oct 10, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Oct 10, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Oct 12, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Oct 12, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Oct 13, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Oct 13, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Oct 13, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Oct 20, 2016
@chriseth chriseth changed the title The other exceptional DIV condition Arithmetic overflow for sdiv Oct 24, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Nov 17, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Nov 30, 2016
pirapira added a commit to pirapira/solidity that referenced this issue Dec 12, 2016
pirapira added a commit that referenced this issue Jan 13, 2017
pirapira added a commit to pirapira/solidity that referenced this issue Feb 2, 2017
pirapira added a commit that referenced this issue Feb 3, 2017
pirapira added a commit that referenced this issue Mar 3, 2017
pirapira added a commit to pirapira/solidity that referenced this issue Mar 3, 2017
pirapira added a commit to pirapira/solidity that referenced this issue Mar 9, 2017
pirapira added a commit to pirapira/solidity that referenced this issue Mar 10, 2017
pirapira added a commit to pirapira/solidity that referenced this issue Mar 13, 2017
pirapira added a commit to pirapira/solidity that referenced this issue Mar 22, 2017
pirapira added a commit to pirapira/solidity that referenced this issue Apr 11, 2017
pirapira added a commit to pirapira/solidity that referenced this issue Apr 21, 2017
pirapira added a commit that referenced this issue Jun 22, 2017
@chriseth chriseth added this to To do in 0.5.0 Feb 16, 2018
@axic
Copy link
Member

axic commented Apr 20, 2018

Should we close this issue and consider it part of the big umbrella issue #796 ?

@chriseth chriseth moved this from To do to Optional in 0.5.0 Apr 23, 2018
0.5.0 automation moved this from Optional to Done Apr 23, 2018
axic pushed a commit that referenced this issue Nov 20, 2018
* Create net gas metering EIP

* Update and rename eip-X.md to eip-1087.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
0.5.0
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants