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

Review correctness of integer division in floating point context #479

Closed
dekobon opened this issue Feb 8, 2019 · 5 comments
Closed

Review correctness of integer division in floating point context #479

dekobon opened this issue Feb 8, 2019 · 5 comments
Assignees

Comments

@dekobon
Copy link
Contributor

dekobon commented Feb 8, 2019

See:

AesCbcCipherDetails.java:124

@indianwhocodes indianwhocodes self-assigned this Feb 12, 2019
@indianwhocodes
Copy link
Contributor

indianwhocodes commented Mar 7, 2019

This was an extremely fun bug to verify since it's nature was mathematical, I also got a chance to use the Wolfram-Alpha-Calculator which is a fun-tool like MATLAB to perform complex mathematical operations. To surmise, Math.floor() works perfectly fine since its returns the lesser than closest integer towards +infinity which is essentially required to calculate the blocks for contentLength in AESCBCCiphers while running AesCbcCipherDetailsTest#calculateContentLengthWorks().
I understand why you reported this, since the aforementioned function is a bit generic nevertheless we can safely close #479.

@indianwhocodes
Copy link
Contributor

indianwhocodes commented Mar 7, 2019

Although, I suggest I should come up with a pull-request to define the test-group:unlimited-crypto in the testng.xml file.

@dekobon
Copy link
Contributor Author

dekobon commented Mar 12, 2019

Ok, I just took a look at this. This is interesting:

    public static void main(String[] argv) {
        int a = -902342342;
        int b = 3233337;
        int intDivision = a / b;
        double floor = Math.floor(a / b);
        double floorDiv = Math.floorDiv(a, b);
        double casted = (double)a / (double)b;
        double floorCasted = Math.floor(casted);

        System.out.printf("a / b as int = %d\n", intDivision);
        System.out.printf("floor(a / b) as double = %f\n", floor);
        System.out.printf("floorDiv(a / b) as double = %f\n", floorDiv);
        System.out.printf("floor((double)a / (double)b) as double = %f\n", floorCasted);
    }

Output:

a / b as int = -279
floor(a / b) as double = -279.000000
floorDiv(a / b) as double = -280.000000
floor((double)a / (double)b) as double = -280.000000

All three values are the same if a and b are positive. In our case, Math.floorDiv() might make the most sense because it converts the ints to a double and does the floor operation all at once.

What are your thoughts?

@indianwhocodes
Copy link
Contributor

indianwhocodes commented Mar 12, 2019

I agree, Also within this code context (AesCbcCipherDetails.java#L124) the return type for Math.floorDiv() would be long which is what we need here.
The aforementioned approach would also reduce the number of type-casting we perform supplemented by the fact that it's a relatively more accurate method of performing this mathematical operation and checks for Overflow-Error.

@indianwhocodes
Copy link
Contributor

Fixed by Pull-Request:#500 .

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

No branches or pull requests

2 participants