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.

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

Jump to bottom

Check ABI for notifications #2810

Merged
merged 15 commits into from
Aug 18, 2023
Merged

Check ABI for notifications #2810

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e88062f
Check ABI for notifications
erikzhang Sep 2, 2022
f0b3071
Merge branch 'master' into check-abi-for-notifications
erikzhang Sep 7, 2022
c7e9ced
Merge branch 'master' into check-abi-for-notifications
shargon Sep 26, 2022
95ee47e
Merge branch 'master' into check-abi-for-notifications
shargon Oct 29, 2022
a426a29
Merge branch 'master' into check-abi-for-notifications
vncoelho Jan 15, 2023
e3db758
Merge branch 'master' into check-abi-for-notifications
shargon Mar 17, 2023
95b9983
Merge branch 'master' into check-abi-for-notifications
vncoelho Apr 17, 2023
8cb741b
Merge branch 'master' into check-abi-for-notifications
shargon Jul 5, 2023
75829a9
Patch checkabi (#2884)
Ashuaidehao Aug 9, 2023
080695d
Merge branch 'master' into check-abi-for-notifications
shargon Aug 9, 2023
b812394
Merge branch 'master' into check-abi-for-notifications
Jim8y Aug 15, 2023
93b4126
Check UTF8 string
shargon Aug 16, 2023
be2ef40
Cleaner way
shargon Aug 16, 2023
780103a
Fix Buffer
shargon Aug 16, 2023
3f10c96
Update src/Neo/SmartContract/ApplicationEngine.Runtime.cs
Jim8y Aug 18, 2023
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
92 changes: 87 additions & 5 deletions src/Neo/SmartContract/ApplicationEngine.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,17 @@
// Redistribution and use in source and binary forms with or without
// modifications are permitted.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Numerics;
using Neo.Cryptography.ECC;
using Neo.IO;
using Neo.Network.P2P.Payloads;
using Neo.SmartContract.Native;
using Neo.VM;
using Neo.VM.Types;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Numerics;
using Array = Neo.VM.Types.Array;

namespace Neo.SmartContract
Expand Down Expand Up @@ -332,6 +332,35 @@ protected internal void RuntimeLog(byte[] state)
/// <param name="eventName">The name of the event.</param>
/// <param name="state">The arguments of the event.</param>
protected internal void RuntimeNotify(byte[] eventName, Array state)
{
if (!IsHardforkEnabled(Hardfork.HF_Basilisk))
{
RuntimeNotifyV1(eventName, state);
return;
}
if (eventName.Length > MaxEventName) throw new ArgumentException(null, nameof(eventName));
string name = Utility.StrictUTF8.GetString(eventName);
ContractState contract = CurrentContext.GetState<ExecutionContextState>().Contract;
if (contract is null)
throw new InvalidOperationException("Notifications are not allowed in dynamic scripts.");
var @event = contract.Manifest.Abi.Events.FirstOrDefault(p => string.Equals(p.Name, name, StringComparison.Ordinal));
if (@event is null)
throw new InvalidOperationException($"Event `{name}` does not exist.");
if (@event.Parameters.Length != state.Count)
throw new InvalidOperationException("The number of the arguments does not match the formal parameters of the event.");
for (int i = 0; i < @event.Parameters.Length; i++)
{
var p = @event.Parameters[i];
if (!CheckItemType(state[i], p.Type))
throw new InvalidOperationException($"The type of the argument `{p.Name}` does not match the formal parameter.");
}
using MemoryStream ms = new(MaxNotificationSize);
using BinaryWriter writer = new(ms, Utility.StrictUTF8, true);
BinarySerializer.Serialize(writer, state, MaxNotificationSize);
SendNotification(CurrentScriptHash, name, state);
}

protected internal void RuntimeNotifyV1(byte[] eventName, Array state)
{
if (eventName.Length > MaxEventName) throw new ArgumentException(null, nameof(eventName));
if (CurrentContext.GetState<ExecutionContextState>().Contract is null)
Expand Down Expand Up @@ -384,5 +413,58 @@ protected internal void BurnGas(long gas)
throw new InvalidOperationException("GAS must be positive.");
AddGas(gas);
}

private static bool CheckItemType(StackItem item, ContractParameterType type)
{
StackItemType aType = item.Type;
if (aType == StackItemType.Pointer) return false;
switch (type)
{
case ContractParameterType.Any:
return true;
case ContractParameterType.Boolean:
return aType == StackItemType.Boolean;
case ContractParameterType.Integer:
return aType == StackItemType.Integer;
case ContractParameterType.ByteArray:
case ContractParameterType.String:
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to have some additional checks for the String type? The way it is now it's absolutely the same as ByteArray, maybe some differentiation can be useful like if String could only contain valid UTF-8 byte sequence.

Copy link
Member

Choose a reason for hiding this comment

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

@shargon, how about this additional check?

Copy link
Member

Choose a reason for hiding this comment

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

@AnnaShaleva Done

{
if (aType is StackItemType.Any or StackItemType.ByteString or StackItemType.Buffer)
Jim8y marked this conversation as resolved.
Show resolved Hide resolved
{
try
{
_ = Utility.StrictUTF8.GetString(item.GetSpan()); // Prevent any non-UTF8 string
return true;
}
catch { }
}
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

And the last question from my side: if we have Null stackitem passed as notification parameter and in manifest it is declared ContractParameterType.String, what should happen? The previous version allowed this, but seems that the current code will throw the InvalidCastException exception on item.GetSpan() call for Null stackitem. But I think it's valid to pass Null stackitem as ContractParameterType.String.

@shargon, @roman-khimov, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@AnnaShaleva, you're programming in Go, string can't be null there!

Jokes aside, I don't think I have any strong preference here. I'm slightly more in favor of making strings always have some contents (even if it's "", basically the way it is now), but if there are useful cases where Null is applicable, we can accept it too. We can also try having more strict policy for now and then relax it if needed (it's always easier than the other way around).

Copy link
Member

Choose a reason for hiding this comment

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

Let's then adjust a bit the current check so that it'll be more explicit:

 if (aType is StackItemType.ByteString or StackItemType.Buffer)

Copy link
Member

Choose a reason for hiding this comment

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

StackItemType.Any normal indicates NULL. But normally depends on where it is.
you can always check for StackItem.IsNull or StackItem.Null
Maybe treat strings the same as string.IsNullOrEmpty() does.

Copy link
Member

Choose a reason for hiding this comment

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

I think that should be empty, not null

Copy link
Member

Choose a reason for hiding this comment

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

@shargon, good, then see the #2810 (comment), please.

case ContractParameterType.Hash160:
if (aType == StackItemType.Any) return true;
if (aType != StackItemType.ByteString && aType != StackItemType.Buffer) return false;
return item.GetSpan().Length == UInt160.Length;
case ContractParameterType.Hash256:
if (aType == StackItemType.Any) return true;
if (aType != StackItemType.ByteString && aType != StackItemType.Buffer) return false;
return item.GetSpan().Length == UInt256.Length;
case ContractParameterType.PublicKey:
if (aType == StackItemType.Any) return true;
if (aType != StackItemType.ByteString && aType != StackItemType.Buffer) return false;
return item.GetSpan().Length == 33;
case ContractParameterType.Signature:
if (aType == StackItemType.Any) return true;
if (aType != StackItemType.ByteString && aType != StackItemType.Buffer) return false;
return item.GetSpan().Length == 64;
case ContractParameterType.Array:
return aType is StackItemType.Any or StackItemType.Array or StackItemType.Struct;
case ContractParameterType.Map:
return aType is StackItemType.Any or StackItemType.Map;
case ContractParameterType.InteropInterface:
return aType is StackItemType.Any or StackItemType.InteropInterface;
default:
return false;
}
}
}
}
36 changes: 31 additions & 5 deletions tests/Neo.UnitTests/SmartContract/UT_ApplicationEngine.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Neo.Network.P2P.Payloads;
using Neo.SmartContract;
using Neo.SmartContract.Manifest;
using System;
using System.Numerics;
using System.Text;

namespace Neo.UnitTests.SmartContract
{
Expand All @@ -24,36 +26,60 @@ public void TestNotSupportedNotification()
{
using var engine = ApplicationEngine.Create(TriggerType.Application, null, null, TestBlockchain.TheNeoSystem.GenesisBlock, settings: TestBlockchain.TheNeoSystem.Settings, gas: 1100_00000000);
engine.LoadScript(Array.Empty<byte>());
engine.CurrentContext.GetState<ExecutionContextState>().Contract = new();
engine.CurrentContext.GetState<ExecutionContextState>().Contract = new()
{
Manifest = new()
{
Abi = new()
{
Events = new[]
{
new ContractEventDescriptor
{
Name = "e1",
Parameters = new[]
{
new ContractParameterDefinition
{
Type = ContractParameterType.Array
}
}
}
}
}
}
};

// circular

VM.Types.Array array = new();
array.Add(array);

Assert.ThrowsException<NotSupportedException>(() => engine.RuntimeNotify(new byte[] { 0x01 }, array));
Assert.ThrowsException<NotSupportedException>(() => engine.RuntimeNotify(Encoding.ASCII.GetBytes("e1"), array));

// Buffer

array.Clear();
array.Add(new VM.Types.Buffer(1));
engine.CurrentContext.GetState<ExecutionContextState>().Contract.Manifest.Abi.Events[0].Parameters[0].Type = ContractParameterType.ByteArray;

engine.RuntimeNotify(new byte[] { 0x01 }, array);
engine.RuntimeNotify(Encoding.ASCII.GetBytes("e1"), array);
engine.Notifications[0].State[0].Type.Should().Be(VM.Types.StackItemType.ByteString);

// Pointer

array.Clear();
array.Add(new VM.Types.Pointer(Array.Empty<byte>(), 1));

Assert.ThrowsException<NotSupportedException>(() => engine.RuntimeNotify(new byte[] { 0x01 }, array));
Assert.ThrowsException<InvalidOperationException>(() => engine.RuntimeNotify(Encoding.ASCII.GetBytes("e1"), array));

// InteropInterface

array.Clear();
array.Add(new VM.Types.InteropInterface(new object()));
engine.CurrentContext.GetState<ExecutionContextState>().Contract.Manifest.Abi.Events[0].Parameters[0].Type = ContractParameterType.InteropInterface;

Assert.ThrowsException<NotSupportedException>(() => engine.RuntimeNotify(new byte[] { 0x01 }, array));
Assert.ThrowsException<NotSupportedException>(() => engine.RuntimeNotify(Encoding.ASCII.GetBytes("e1"), array));
}

[TestMethod]
Expand Down
53 changes: 50 additions & 3 deletions tests/Neo.UnitTests/SmartContract/UT_InteropService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,22 @@ public void Runtime_GetNotifications_Test()
scriptHash2 = script.ToArray().ToScriptHash();

snapshot.DeleteContract(scriptHash2);
snapshot.AddContract(scriptHash2, TestUtils.GetContract(script.ToArray(), TestUtils.CreateManifest("test", ContractParameterType.Any, ContractParameterType.Integer, ContractParameterType.Integer)));
ContractState contract = TestUtils.GetContract(script.ToArray(), TestUtils.CreateManifest("test", ContractParameterType.Any, ContractParameterType.Integer, ContractParameterType.Integer));
contract.Manifest.Abi.Events = new[]
{
new ContractEventDescriptor
{
Name = "testEvent2",
Parameters = new[]
{
new ContractParameterDefinition
{
Type = ContractParameterType.Any
}
}
}
};
snapshot.AddContract(scriptHash2, contract);
}

// Wrong length
Expand Down Expand Up @@ -93,7 +108,23 @@ public void Runtime_GetNotifications_Test()
// Execute

engine.LoadScript(script.ToArray());
engine.CurrentContext.GetState<ExecutionContextState>().Contract = new();
engine.CurrentContext.GetState<ExecutionContextState>().Contract = new()
{
Manifest = new()
{
Abi = new()
{
Events = new[]
{
new ContractEventDescriptor
{
Name = "testEvent1",
Parameters = System.Array.Empty<ContractParameterDefinition>()
}
}
}
}
};
var currentScriptHash = engine.EntryScriptHash;

Assert.AreEqual(VMState.HALT, engine.Execute());
Expand Down Expand Up @@ -146,7 +177,23 @@ public void Runtime_GetNotifications_Test()
// Execute

engine.LoadScript(script.ToArray());
engine.CurrentContext.GetState<ExecutionContextState>().Contract = new();
engine.CurrentContext.GetState<ExecutionContextState>().Contract = new()
{
Manifest = new()
{
Abi = new()
{
Events = new[]
{
new ContractEventDescriptor
{
Name = "testEvent1",
Parameters = System.Array.Empty<ContractParameterDefinition>()
}
}
}
}
};
var currentScriptHash = engine.EntryScriptHash;

Assert.AreEqual(VMState.HALT, engine.Execute());
Expand Down