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

apply strict enum checking #1254

Merged
merged 18 commits into from
Nov 26, 2019
Merged

apply strict enum checking #1254

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5626a3a
apply strict enum checking
ixje Nov 20, 2019
39e9042
i hate Windows
ixje Nov 20, 2019
2f79770
process feedback
ixje Nov 20, 2019
81a994b
Update neo.csproj
shargon Nov 20, 2019
6ec2df1
Merge branch 'strict_enum' of https://github.com/ixje/neo into strict…
ixje Nov 20, 2019
bd1dee2
change exception type
ixje Nov 20, 2019
510a06c
remove helper, only use Enum.IsDefined
ixje Nov 21, 2019
fb79e87
missed refactor
ixje Nov 21, 2019
b435a5d
Merge branch 'master' into strict_enum
lock9 Nov 21, 2019
bab1c8f
Optimize VerificationContract
shargon Nov 21, 2019
3421ec7
remove checking for flag enum, change check location to constructor o…
ixje Nov 22, 2019
19cfa32
Merge branch 'master' into strict_enum
erikzhang Nov 22, 2019
ae0c79e
Update ConsensusMessage.cs
erikzhang Nov 22, 2019
a162d00
Update ContractParameterDefinition.cs
erikzhang Nov 22, 2019
24e7a51
Merge branch 'master' into strict_enum
shargon Nov 25, 2019
5105174
move projects
erikzhang Nov 26, 2019
450c04a
Merge branch 'master' into strict_enum
erikzhang Nov 26, 2019
54c30e1
Remove unnecessary checks
erikzhang Nov 26, 2019
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
3 changes: 3 additions & 0 deletions neo/Consensus/ChangeView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.IO;

namespace Neo.Consensus
Expand Down Expand Up @@ -32,6 +33,8 @@ public override void Deserialize(BinaryReader reader)
base.Deserialize(reader);
Timestamp = reader.ReadUInt64();
Reason = (ChangeViewReason)reader.ReadByte();
if (!Enum.IsDefined(typeof(ChangeViewReason), Reason))
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to check it here. If we add new reason in the future, it can be compatible with the current version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's been a very long time that i looked at the consensus code, but what prevents me from sending a ConsensusPayload with an invalid message that gets processed unnecessarily (see #1254 (comment) what I mean with unnecessary processing)?

Copy link
Member

Choose a reason for hiding this comment

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

The consensus message doesn't need to be dropped even if I don't know the changeview reason. So this check is unnecessary because we may add new reasons in the future.

throw new FormatException();
}

public override void Serialize(BinaryWriter writer)
Expand Down
6 changes: 5 additions & 1 deletion neo/Consensus/ConsensusMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ protected ConsensusMessage(ConsensusMessageType type)

public virtual void Deserialize(BinaryReader reader)
{
if (Type != (ConsensusMessageType)reader.ReadByte())
ConsensusMessageType cmt = (ConsensusMessageType)reader.ReadByte();
if (!Enum.IsDefined(typeof(ConsensusMessageType), cmt))
ixje marked this conversation as resolved.
Show resolved Hide resolved
throw new FormatException();

if (Type != cmt)
throw new FormatException();
ViewNumber = reader.ReadByte();
}
Expand Down
4 changes: 4 additions & 0 deletions neo/Network/P2P/Message.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ void ISerializable.Deserialize(BinaryReader reader)
{
Flags = (MessageFlags)reader.ReadByte();
Command = (MessageCommand)reader.ReadByte();
if (!Enum.IsDefined(typeof(MessageCommand), Command))
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to check it here. If we add new command in the future, it can be compatible with the current version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea of strict enum checking is to fail early. Without this check here I can create a message with a fake command that

  1. the system will first try to decompress
  2. next all IP2PPlugins will receive it
  3. and finally it will silently be ignored because there is no matching case in the protocolhandler.

Instead with this check it will never waste any resources on decompressing and prevents it reaching P2PPlugins. It will drop the remote node on the first invalid message format, which is the same behaviour we have when a payload inside the message is invalid.

Copy link
Member

Choose a reason for hiding this comment

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

That is because we don't know the message format, but maybe the IP2PPlugin may know it. This could be a protocol extension plugin.

throw new FormatException();
_payload_compressed = reader.ReadVarBytes(PayloadMaxSize);
DecompressPayload();
}
Expand Down Expand Up @@ -152,6 +154,8 @@ internal static int TryDeserialize(ByteString data, out Message msg)
Command = (MessageCommand)header[1],
_payload_compressed = length <= 0 ? new byte[0] : data.Slice(payloadIndex, (int)length).ToArray()
};
if (!Enum.IsDefined(typeof(MessageCommand), msg.Command))
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to check it here. If we add new command in the future, it can be compatible with the current version.

throw new FormatException();
msg.DecompressPayload();

return payloadIndex + (int)length;
Expand Down
2 changes: 2 additions & 0 deletions neo/Network/P2P/Payloads/Cosigner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ void ISerializable.Deserialize(BinaryReader reader)
{
Account = reader.ReadSerializable<UInt160>();
Scopes = (WitnessScope)reader.ReadByte();
if (!Enum.IsDefined(typeof(WitnessScope), Scopes))
ixje marked this conversation as resolved.
Show resolved Hide resolved
throw new FormatException();
AllowedContracts = Scopes.HasFlag(WitnessScope.CustomContracts)
? reader.ReadSerializableArray<UInt160>(MaxSubitems)
: new UInt160[0];
Expand Down
4 changes: 3 additions & 1 deletion neo/Network/P2P/Payloads/TransactionAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ public JObject ToJson()
public static TransactionAttribute FromJson(JObject json)
{
TransactionAttribute transactionAttribute = new TransactionAttribute();
transactionAttribute.Usage = (TransactionAttributeUsage)(byte.Parse(json["usage"].AsString()));
transactionAttribute.Usage = (TransactionAttributeUsage)byte.Parse(json["usage"].AsString());
if (!Enum.IsDefined(typeof(TransactionAttributeUsage), transactionAttribute.Usage))
throw new ArgumentException();
transactionAttribute.Data = Convert.FromBase64String(json["data"].AsString());
return transactionAttribute;
}
Expand Down
2 changes: 1 addition & 1 deletion neo/SmartContract/Manifest/ContractParameterDefinition.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public static ContractParameterDefinition FromJson(JObject json)
return new ContractParameterDefinition
{
Name = json["name"].AsString(),
Type = (ContractParameterType)Enum.Parse(typeof(ContractParameterType), json["type"].AsString()),
Type = (ContractParameterType)Enum.Parse(typeof(ContractParameterType), json["type"].AsString())
};
}

Expand Down
8 changes: 7 additions & 1 deletion neo/Wallets/SQLite/VerificationContract.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@ public class VerificationContract : SmartContract.Contract, IEquatable<Verificat
public void Deserialize(BinaryReader reader)
{
reader.ReadSerializable<UInt160>();
ParameterList = reader.ReadVarBytes().Select(p => (ContractParameterType)p).ToArray();
ParameterList = reader.ReadVarBytes().Select(p =>
{
var ret = (ContractParameterType)p;
if (!Enum.IsDefined(typeof(ContractParameterType), ret))
throw new FormatException();
return ret;
}).ToArray();
Script = reader.ReadVarBytes();
}

Expand Down