Skip to content

[Fix]: check division overflow for int32 and int64 #1287

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 2, 2025
Merged

[Fix]: check division overflow for int32 and int64 #1287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0bb7ec
fix: always check division overflow for int32 and int64
nan01ab Jan 27, 2025
cf0d970
Merge branch 'master' into fix.div-overflow-checking
Jim8y Feb 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/Neo.Compiler.CSharp/MethodConvert/Expression/BinaryExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

extern alias scfx;

using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
Expand Down Expand Up @@ -91,6 +92,12 @@ private void ConvertBinaryExpression(SemanticModel model, BinaryExpressionSyntax
">=" => (OpCode.GE, false),
_ => throw new CompilationException(expression.OperatorToken, DiagnosticId.SyntaxNotSupported, $"Unsupported operator: {expression.OperatorToken}")
};

if (expression.OperatorToken.ValueText == "/")
{
CheckDivideOverflow(model.GetTypeInfo(expression).Type);
}

AddInstruction(opcode);
if (checkResult)
{
Expand All @@ -99,6 +106,39 @@ private void ConvertBinaryExpression(SemanticModel model, BinaryExpressionSyntax
}
}

private void CheckDivideOverflow(ITypeSymbol? type)
{
if (type is null) return;
while (type.NullableAnnotation == NullableAnnotation.Annotated)
{
// Supporting nullable integer like `byte?`
type = ((INamedTypeSymbol)type).TypeArguments.First();
}

// Check division overflow if checked statement.
// In C#, division overflow is checked or not in `unchecked` statement depends on the implementation.
if (!_checkedStack.Peek()) return;

// Only check overflow for int32 and int64
// NOTE: short / short -> int, ushort / ushort -> int, char / char -> int,
// sbyte / sbyte -> int, byte / byte -> int, so overflow check is not needed.
if (type.Name != "Int32" && type.Name != "Int64") return;

var minValue = type.Name == "Int64" ? long.MinValue : int.MinValue;
var endTarget = new JumpTarget();

AddInstruction(OpCode.DUP);
Push(-1);
Jump(OpCode.JMPNE_L, endTarget);

AddInstruction(OpCode.OVER);
Push(minValue);
Jump(OpCode.JMPNE_L, endTarget);

AddInstruction(OpCode.THROW);
Copy link
Contributor

@Jim8y Jim8y Jan 28, 2025

Choose a reason for hiding this comment

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

here you can push an exception message before throw, but not necesssary.

Copy link
Contributor Author

@nan01ab nan01ab Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

here you can push an exception message before throw, but not necesssary.

No exception message before other similar THROWs.
So no exception message here.

Copy link
Contributor

@Jim8y Jim8y Jan 29, 2025

Choose a reason for hiding this comment

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

here you can push an exception message before throw, but not necesssary.

No exception message before other similar THROWs. So no exception message here.

LOL, that is actually a problem in other places, having exception message is the correct behavior, but its ok, no need to update. .

Copy link
Contributor Author

@nan01ab nan01ab Jan 29, 2025

Choose a reason for hiding this comment

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

here you can push an exception message before throw, but not necesssary.

No exception message before other similar THROWs. So no exception message here.

LOL, that is actually a problem in other places, having exception message is the correct behavior, but its ok, no need to update. .

This and others will be fixed by later PRs.

Copy link
Member

@shargon shargon Jan 29, 2025

Choose a reason for hiding this comment

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

We should fix it in a different PR

Copy link
Contributor Author

@nan01ab nan01ab Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

here you can push an exception message before throw, but not necesssary.

No exception message before other similar THROWs. So no exception message here.

LOL, that is actually a problem in other places, having exception message is the correct behavior, but its ok, no need to update. .

Why LOL?
Many cases were written by you. 😏

endTarget.Instruction = AddInstruction(OpCode.NOP);
}

private void ConvertLogicalOrExpression(SemanticModel model, ExpressionSyntax left, ExpressionSyntax right)
{
JumpTarget rightTarget = new();
Expand Down
15 changes: 8 additions & 7 deletions src/Neo.Compiler.CSharp/MethodConvert/Expression/UnaryExpression.PrefixUnary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,19 +279,20 @@ private void EmitIncrementOrDecrement(SyntaxToken operatorToken, ITypeSymbol? ty

private void EmitNegativeInteger(ITypeSymbol? typeSymbol)
{
if (typeSymbol is null || (typeSymbol.Name != "Int32" && typeSymbol.Name != "Int64"))
{
// -sbyte, -byte, -short, -ushort, -char -> int, -int, -uint -> long
AddInstruction(OpCode.NEGATE); // Emit NEGATE for other integer types
return;
}

if (typeSymbol is null) return;
while (typeSymbol.NullableAnnotation == NullableAnnotation.Annotated)
{
// Supporting nullable integer like `byte?`
typeSymbol = ((INamedTypeSymbol)typeSymbol).TypeArguments.First();
}

if (typeSymbol.Name != "Int32" && typeSymbol.Name != "Int64")
{
// -sbyte, -byte, -short, -ushort, -char -> int, -int, -uint -> long
Copy link
Member

@shargon shargon Jan 27, 2025

Choose a reason for hiding this comment

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

unsigned types works with negate?

Copy link
Contributor Author

@nan01ab nan01ab Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

unsigned types works with negate?

- byte, -ushort -> int
- uint -> long
- ulong -> invalid operation

Copy link
Contributor

@Jim8y Jim8y Jan 28, 2025

Choose a reason for hiding this comment

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

He is right, - ushort => int

AddInstruction(OpCode.NEGATE); // Emit NEGATE for other integer types
return;
}

var minValue = typeSymbol.Name == "Int64" ? long.MinValue : int.MinValue; // int32 or int64

JumpTarget negateTarget = new(), endTarget = new();
Expand Down
4 changes: 4 additions & 0 deletions tests/Neo.Compiler.CSharp.TestContracts/Contract_Overflow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ public class Contract_Overflow : SmartContract.Framework.SmartContract
public static uint AddUInt(uint a, uint b) => a + b;
public static uint MulUInt(uint a, uint b) => a * b;

public static int DivInt(int a, int b) => checked(a / b);

public static int DivShort(short a, short b) => checked(a / b);

public static int NegateIntChecked(int a) => checked(-a);
public static int NegateInt(int a) => -a;

Expand Down
46 changes: 44 additions & 2 deletions tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Overflow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ public abstract class Contract_Overflow(Neo.SmartContract.Testing.SmartContractI
{
#region Compiled data

public static Neo.SmartContract.Manifest.ContractManifest Manifest => Neo.SmartContract.Manifest.ContractManifest.Parse(@"{""name"":""Contract_Overflow"",""groups"":[],""features"":{},""supportedstandards"":[],""abi"":{""methods"":[{""name"":""addInt"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":0,""safe"":false},{""name"":""mulInt"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":53,""safe"":false},{""name"":""addUInt"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":106,""safe"":false},{""name"":""mulUInt"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":141,""safe"":false},{""name"":""negateIntChecked"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":176,""safe"":false},{""name"":""negateInt"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":191,""safe"":false},{""name"":""negateLongChecked"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":206,""safe"":false},{""name"":""negateLong"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":225,""safe"":false},{""name"":""negateShortChecked"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":244,""safe"":false},{""name"":""negateShort"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":250,""safe"":false},{""name"":""negateAddInt"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":256,""safe"":false},{""name"":""negateAddIntChecked"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":319,""safe"":false},{""name"":""negateAddLong"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":354,""safe"":false},{""name"":""negateAddLongChecked"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":449,""safe"":false}],""events"":[]},""permissions"":[],""trusts"":[],""extra"":{""nef"":{""optimization"":""All""}}}");
public static Neo.SmartContract.Manifest.ContractManifest Manifest => Neo.SmartContract.Manifest.ContractManifest.Parse(@"{""name"":""Contract_Overflow"",""groups"":[],""features"":{},""supportedstandards"":[],""abi"":{""methods"":[{""name"":""addInt"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":0,""safe"":false},{""name"":""mulInt"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":53,""safe"":false},{""name"":""addUInt"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":106,""safe"":false},{""name"":""mulUInt"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":141,""safe"":false},{""name"":""divInt"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":176,""safe"":false},{""name"":""divShort"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":196,""safe"":false},{""name"":""negateIntChecked"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":216,""safe"":false},{""name"":""negateInt"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":231,""safe"":false},{""name"":""negateLongChecked"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":246,""safe"":false},{""name"":""negateLong"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":265,""safe"":false},{""name"":""negateShortChecked"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":284,""safe"":false},{""name"":""negateShort"",""parameters"":[{""name"":""a"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":290,""safe"":false},{""name"":""negateAddInt"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":296,""safe"":false},{""name"":""negateAddIntChecked"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":359,""safe"":false},{""name"":""negateAddLong"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":394,""safe"":false},{""name"":""negateAddLongChecked"",""parameters"":[{""name"":""a"",""type"":""Integer""},{""name"":""b"",""type"":""Integer""}],""returntype"":""Integer"",""offset"":489,""safe"":false}],""events"":[]},""permissions"":[],""trusts"":[],""extra"":{""nef"":{""optimization"":""All""}}}");

/// <summary>
/// Optimization: "All"
/// </summary>
public static Neo.SmartContract.NefFile Nef => Convert.FromBase64String(@"TkVGM1Rlc3RpbmdFbmdpbmUAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAP3wAVcAAnh5nkoCAAAAgC4EIgpKAv///38yHgP/////AAAAAJFKAv///38yDAMAAAAAAQAAAJ9AVwACeHmgSgIAAACALgQiCkoC////fzIeA/////8AAAAAkUoC////fzIMAwAAAAABAAAAn0BXAAJ4eZ5KEC4EIg5KA/////8AAAAAMgwD/////wAAAACRQFcAAnh5oEoQLgQiDkoD/////wAAAAAyDAP/////AAAAAJFAVwABeEoCAAAAgCoDOptAVwABeEoCAAAAgCoDQJtAVwABeEoDAAAAAAAAAIAqAzqbQFcAAXhKAwAAAAAAAACAKgNAm0BXAAF4m0BXAAF4m0BXAAJ4eZ5KAgAAAIAuBCIKSgL///9/Mh4D/////wAAAACRSgL///9/MgwDAAAAAAEAAACfSgIAAACAKgNAm0BXAAJ4eZ5KAgAAAIAuAzpKAv///38yAzpKAgAAAIAqAzqbQFcAAnh5nkoDAAAAAAAAAIAuBCIOSgP/////////fzIyBP//////////AAAAAAAAAACRSgP/////////fzIUBAAAAAAAAAAAAQAAAAAAAACfSgMAAAAAAAAAgCoDQJtAVwACeHmeSgMAAAAAAAAAgC4DOkoD/////////38yAzpKAwAAAAAAAACAKgM6m0D1Twbi").AsSerializable<Neo.SmartContract.NefFile>();
public static Neo.SmartContract.NefFile Nef => Convert.FromBase64String(@"TkVGM1Rlc3RpbmdFbmdpbmUAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAP0YAlcAAnh5nkoCAAAAgC4EIgpKAv///38yHgP/////AAAAAJFKAv///38yDAMAAAAAAQAAAJ9AVwACeHmgSgIAAACALgQiCkoC////fzIeA/////8AAAAAkUoC////fzIMAwAAAAABAAAAn0BXAAJ4eZ5KEC4EIg5KA/////8AAAAAMgwD/////wAAAACRQFcAAnh5oEoQLgQiDkoD/////wAAAAAyDAP/////AAAAAJFAVwACeHlKDyoLSwIAAACAKgM6oUBXAAJ4eUoPKgtLAgAAAIAqAzqhQFcAAXhKAgAAAIAqAzqbQFcAAXhKAgAAAIAqA0CbQFcAAXhKAwAAAAAAAACAKgM6m0BXAAF4SgMAAAAAAAAAgCoDQJtAVwABeJtAVwABeJtAVwACeHmeSgIAAACALgQiCkoC////fzIeA/////8AAAAAkUoC////fzIMAwAAAAABAAAAn0oCAAAAgCoDQJtAVwACeHmeSgIAAACALgM6SgL///9/MgM6SgIAAACAKgM6m0BXAAJ4eZ5KAwAAAAAAAACALgQiDkoD/////////38yMgT//////////wAAAAAAAAAAkUoD/////////38yFAQAAAAAAAAAAAEAAAAAAAAAn0oDAAAAAAAAAIAqA0CbQFcAAnh5nkoDAAAAAAAAAIAuAzpKA/////////9/MgM6SgMAAAAAAAAAgCoDOptA4RUGTQ==").AsSerializable<Neo.SmartContract.NefFile>();

#endregion

Expand Down Expand Up @@ -73,6 +73,48 @@ public abstract class Contract_Overflow(Neo.SmartContract.Testing.SmartContractI
[DisplayName("addUInt")]
public abstract BigInteger? AddUInt(BigInteger? a, BigInteger? b);

/// <summary>
/// Unsafe method
/// </summary>
/// <remarks>
/// Script: VwACeHlKDyoLSwIAAACAKgM6oUA=
/// INITSLOT 0002 [64 datoshi]
/// LDARG0 [2 datoshi]
/// LDARG1 [2 datoshi]
/// DUP [2 datoshi]
/// PUSHM1 [1 datoshi]
/// JMPNE 0B [2 datoshi]
/// OVER [2 datoshi]
/// PUSHINT32 00000080 [1 datoshi]
/// JMPNE 03 [2 datoshi]
/// THROW [512 datoshi]
/// DIV [8 datoshi]
/// RET [0 datoshi]
/// </remarks>
[DisplayName("divInt")]
public abstract BigInteger? DivInt(BigInteger? a, BigInteger? b);

/// <summary>
/// Unsafe method
/// </summary>
/// <remarks>
/// Script: VwACeHlKDyoLSwIAAACAKgM6oUA=
/// INITSLOT 0002 [64 datoshi]
/// LDARG0 [2 datoshi]
/// LDARG1 [2 datoshi]
/// DUP [2 datoshi]
/// PUSHM1 [1 datoshi]
/// JMPNE 0B [2 datoshi]
/// OVER [2 datoshi]
/// PUSHINT32 00000080 [1 datoshi]
/// JMPNE 03 [2 datoshi]
/// THROW [512 datoshi]
/// DIV [8 datoshi]
/// RET [0 datoshi]
/// </remarks>
[DisplayName("divShort")]
public abstract BigInteger? DivShort(BigInteger? a, BigInteger? b);

/// <summary>
/// Unsafe method
/// </summary>
Expand Down
13 changes: 13 additions & 0 deletions tests/Neo.Compiler.CSharp.UnitTests/UnitTest_Overflow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,5 +102,18 @@ public void Test_NegateChecked()
Assert.AreEqual(-9223372036854775808, Contract.NegateAddLong(-9223372036854775807, -1));
Assert.ThrowsException<TestException>(() => Contract.NegateAddLongChecked(-9223372036854775807, -1));
}

[TestMethod]
public void Test_DivOverflow()
{
Assert.AreEqual(int.MaxValue, Contract.DivInt(int.MaxValue, 1));
Assert.AreEqual(short.MaxValue, Contract.DivShort(short.MaxValue, 1));

// VMUnhandledException int.MinValue / -1
Assert.ThrowsException<TestException>(() => Contract.DivInt(int.MinValue, -1));

// short / -1 -> int, so no overflow
Assert.AreEqual(32768, Contract.DivShort(short.MinValue, -1));
}
}
}
Loading