From 2171267344116065159b5052a1f95502256b2d0a Mon Sep 17 00:00:00 2001 From: mbaxter Date: Mon, 3 Dec 2018 19:02:25 -0500 Subject: [PATCH] Add an UNKNOWN DisconnectReason representing disconnects with no reason (#359) --- .../ethereum/p2p/netty/ApiHandler.java | 2 +- .../ethereum/p2p/netty/NettyP2PNetwork.java | 2 +- .../p2p/wire/messages/DisconnectMessage.java | 56 +++++++----- .../ethereum/p2p/NettyP2PNetworkTest.java | 2 +- .../wire/messages/DisconnectMessageTest.java | 91 +++++++++++++++++++ 5 files changed, 129 insertions(+), 24 deletions(-) create mode 100644 ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/wire/messages/DisconnectMessageTest.java diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/ApiHandler.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/ApiHandler.java index a2f31045f7..2a395bc6b1 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/ApiHandler.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/ApiHandler.java @@ -74,7 +74,7 @@ protected void channelRead0(final ChannelHandlerContext ctx, final MessageData o break; case WireMessageCodes.DISCONNECT: final DisconnectMessage disconnect = DisconnectMessage.readFrom(message); - DisconnectReason reason = null; + DisconnectReason reason = DisconnectReason.UNKNOWN; try { reason = disconnect.getReason(); LOG.debug( diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetwork.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetwork.java index 55b8c1d094..d669416d1e 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetwork.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetwork.java @@ -241,7 +241,7 @@ protected void initChannel(final SocketChannel ch) { } // Reject incoming connections that are blacklisted if (peerBlacklist.contains(connection)) { - connection.disconnect(DisconnectReason.USELESS_PEER); + connection.disconnect(DisconnectReason.UNKNOWN); return; } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/wire/messages/DisconnectMessage.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/wire/messages/DisconnectMessage.java index 16e9c03e0e..0ad7585b12 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/wire/messages/DisconnectMessage.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/wire/messages/DisconnectMessage.java @@ -12,18 +12,15 @@ */ package tech.pegasys.pantheon.ethereum.p2p.wire.messages; -import static com.google.common.base.Preconditions.checkArgument; -import static tech.pegasys.pantheon.util.Preconditions.checkGuard; - import tech.pegasys.pantheon.ethereum.p2p.api.MessageData; import tech.pegasys.pantheon.ethereum.p2p.wire.AbstractMessageData; -import tech.pegasys.pantheon.ethereum.p2p.wire.WireProtocolException; import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput; import tech.pegasys.pantheon.ethereum.rlp.RLP; import tech.pegasys.pantheon.ethereum.rlp.RLPInput; import tech.pegasys.pantheon.ethereum.rlp.RLPOutput; import tech.pegasys.pantheon.util.bytes.BytesValue; +import java.util.Optional; import java.util.stream.Stream; public final class DisconnectMessage extends AbstractMessageData { @@ -75,16 +72,21 @@ public Data(final DisconnectReason reason) { public void writeTo(final RLPOutput out) { out.startList(); - out.writeByte(reason.getValue()); + out.writeBytesValue(reason.getValue()); out.endList(); } public static Data readFrom(final RLPInput in) { - final int size = in.enterList(); - checkGuard(size == 1, WireProtocolException::new, "Expected list size 1, got: %s", size); - final DisconnectReason reason = DisconnectReason.forCode(in.readByte()); + in.enterList(); + BytesValue reasonData = in.readBytesValue(); in.leaveList(); + // Disconnect reason should be at most 1 byte, otherwise, just return UNKNOWN + final DisconnectReason reason = + reasonData.size() == 1 + ? DisconnectReason.forCode(reasonData.get(0)) + : DisconnectReason.UNKNOWN; + return new Data(reason); } @@ -100,6 +102,7 @@ public DisconnectReason getReason() { * Protocol */ public enum DisconnectReason { + UNKNOWN(null), REQUESTED((byte) 0x00), TCP_SUBSYSTEM_ERROR((byte) 0x01), BREACH_OF_PROTOCOL((byte) 0x02), @@ -115,29 +118,40 @@ public enum DisconnectReason { SUBPROTOCOL_TRIGGERED((byte) 0x10); private static final DisconnectReason[] BY_ID; - private final byte code; + private final Optional code; static { final int maxValue = - Stream.of(DisconnectReason.values()).mapToInt(dr -> (int) dr.getValue()).max().getAsInt(); + Stream.of(DisconnectReason.values()) + .filter(r -> r.code.isPresent()) + .mapToInt(r -> (int) r.code.get()) + .max() + .getAsInt(); BY_ID = new DisconnectReason[maxValue + 1]; - Stream.of(DisconnectReason.values()).forEach(dr -> BY_ID[dr.getValue()] = dr); + Stream.of(DisconnectReason.values()) + .filter(r -> r.code.isPresent()) + .forEach(r -> BY_ID[r.code.get()] = r); } - public static DisconnectReason forCode(final byte taintedCode) { - final byte code = (byte) (taintedCode & 0xff); - checkArgument(code < BY_ID.length, "unrecognized disconnect reason"); - final DisconnectReason reason = BY_ID[code]; - checkArgument(reason != null, "unrecognized disconnect reason"); - return reason; + public static DisconnectReason forCode(final Byte code) { + if (code == null || code >= BY_ID.length || code < 0 || BY_ID[code] == null) { + // Be permissive and just return unknown if the disconnect reason is bad + return UNKNOWN; + } + return BY_ID[code]; + } + + DisconnectReason(final Byte code) { + this.code = Optional.ofNullable(code); } - DisconnectReason(final byte code) { - this.code = code; + public BytesValue getValue() { + return code.map(BytesValue::of).orElse(BytesValue.EMPTY); } - public byte getValue() { - return code; + @Override + public String toString() { + return getValue().toString() + " " + name(); } } } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NettyP2PNetworkTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NettyP2PNetworkTest.java index e3b12bf13b..bb7e2c7eb4 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NettyP2PNetworkTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NettyP2PNetworkTest.java @@ -378,7 +378,7 @@ public void rejectIncomingConnectionFromBlacklistedPeer() throws Exception { assertThat(connectFuture.get(5L, TimeUnit.SECONDS).getPeer().getNodeId()).isEqualTo(localId); assertThat(peerFuture.get(5L, TimeUnit.SECONDS).getPeer().getNodeId()).isEqualTo(localId); assertThat(reasonFuture.get(5L, TimeUnit.SECONDS)) - .isEqualByComparingTo(DisconnectReason.USELESS_PEER); + .isEqualByComparingTo(DisconnectReason.UNKNOWN); } } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/wire/messages/DisconnectMessageTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/wire/messages/DisconnectMessageTest.java new file mode 100644 index 0000000000..bcf0094278 --- /dev/null +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/wire/messages/DisconnectMessageTest.java @@ -0,0 +1,91 @@ +/* + * Copyright 2018 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.ethereum.p2p.wire.messages; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; + +import tech.pegasys.pantheon.ethereum.p2p.api.MessageData; +import tech.pegasys.pantheon.ethereum.p2p.wire.RawMessage; +import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason; +import tech.pegasys.pantheon.util.bytes.BytesValue; + +import org.junit.Test; + +public class DisconnectMessageTest { + + @Test + public void readFromWithReason() { + MessageData messageData = + new RawMessage(WireMessageCodes.DISCONNECT, BytesValue.fromHexString("0xC103")); + DisconnectMessage disconnectMessage = DisconnectMessage.readFrom(messageData); + + DisconnectReason reason = disconnectMessage.getReason(); + assertThat(reason).isEqualTo(DisconnectReason.USELESS_PEER); + } + + @Test + public void readFromWithNoReason() { + MessageData messageData = + new RawMessage(WireMessageCodes.DISCONNECT, BytesValue.fromHexString("0xC180")); + DisconnectMessage disconnectMessage = DisconnectMessage.readFrom(messageData); + + DisconnectReason reason = disconnectMessage.getReason(); + assertThat(reason).isEqualTo(DisconnectReason.UNKNOWN); + } + + @Test + public void readFromWithInvalidReason() { + String[] invalidReasons = { + "0xC10C", + "0xC155", + // List containing a byte > 128 (negative valued) + "0xC281FF", + // List containing a multi-byte reason + "0xC3820101" + }; + + for (String invalidReason : invalidReasons) { + MessageData messageData = + new RawMessage(WireMessageCodes.DISCONNECT, BytesValue.fromHexString(invalidReason)); + DisconnectMessage disconnectMessage = DisconnectMessage.readFrom(messageData); + DisconnectReason reason = disconnectMessage.getReason(); + assertThat(reason).isEqualTo(DisconnectReason.UNKNOWN); + } + } + + @Test + public void readFromWithWrongMessageType() { + MessageData messageData = + new RawMessage(WireMessageCodes.PING, BytesValue.fromHexString("0xC103")); + assertThatThrownBy(() -> DisconnectMessage.readFrom(messageData)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Message has code 2 and thus is not a DisconnectMessage"); + } + + @Test + public void createWithReason() { + DisconnectMessage disconnectMessage = DisconnectMessage.create(DisconnectReason.USELESS_PEER); + + assertThat(disconnectMessage.getReason()).isEqualTo(DisconnectReason.USELESS_PEER); + assertThat(disconnectMessage.getData().toString()).isEqualToIgnoringCase("0xC103"); + } + + @Test + public void createWithUnknownReason() { + DisconnectMessage disconnectMessage = DisconnectMessage.create(DisconnectReason.UNKNOWN); + + assertThat(disconnectMessage.getReason()).isEqualTo(DisconnectReason.UNKNOWN); + assertThat(disconnectMessage.getData().toString()).isEqualToIgnoringCase("0xC180"); + } +}