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

test large overflows and underflows for EnsureIntegerInRange #1170

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Hecate2
Copy link
Contributor

@Hecate2 Hecate2 commented Sep 18, 2024

The correctness of OpCode.AND depends on the representation of BigInteger in bytes, and I am really suspicious whether AND is correct. Here are some examples of bytes representations.
https://learn.microsoft.com/en-us/dotnet/api/system.numerics.biginteger.tobytearray?view=net-8.0
MOD is safe, no matter how BigInteger is implemented in bytes.

shargon
shargon previously approved these changes Sep 18, 2024
@Jim8y
Copy link
Contributor

Jim8y commented Sep 18, 2024

Need unit test to demonstrate the problem.

@Hecate2
Copy link
Contributor Author

Hecate2 commented Sep 18, 2024

Need unit test to demonstrate the problem.

I have not come up with any test case to demonstrate that AND is wrong, but I will try it in 24 hrs

@vncoelho
Copy link
Member

For BigIntenger Negative values it may be the error if I understood correctly

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

We should ensure that is well covered with the current UT

@Hecate2
Copy link
Contributor Author

Hecate2 commented Sep 19, 2024

In tests I found that AND works well, while MOD is wrong. In C#, (-1 mod 5) returns -1. In other languages the result is often 4 😰

adjustTarget.Instruction = Push(mask);
AddInstruction(OpCode.AND);
adjustTarget.Instruction = Push(mask + 1);
AddInstruction(OpCode.MOD);
Copy link

Choose a reason for hiding this comment

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

Expression x AND mask is not equal to x MOD (mask + 1) when x is negative, so MOD cannot replace AND here?
For expample:

var x = new BigInteger(-3);
var y = new BigInteger(255);


Console.WriteLine($"mod {x % (y+1)}, and {x & y}");

x = new BigInteger(3);
y = new BigInteger(255);

Console.WriteLine($"mod {x % (y+1)}, and {x & y}");

Output:

mod -3, and 253
mod 3, and 3

@nan01ab
Copy link

nan01ab commented Sep 19, 2024

In tests I found that AND works well, while MOD is wrong. In C#, (-1 mod 5) returns -1. In other languages the result is often 4 😰

Other languages are Python? C, Java, Rust etc. have the same semantics as C#.

@Hecate2
Copy link
Contributor Author

Hecate2 commented Sep 19, 2024

My fault. OpCode.AND is always correct in tests. I will add tests for large overflows and underflows.

@Hecate2 Hecate2 changed the title fix EnsureIntegerInRange /*fix*/ test large overflows and underflows for EnsureIntegerInRange Sep 19, 2024
@Hecate2 Hecate2 changed the title /*fix*/ test large overflows and underflows for EnsureIntegerInRange test large overflows and underflows for EnsureIntegerInRange Sep 19, 2024
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.

5 participants