Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Fix discovery exception handling #1160

Merged
merged 4 commits into from
Aug 22, 2018
Merged

Fix discovery exception handling #1160

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da7c9b6
Handle dicovery exceptions occurred in message processing
mkalinin Aug 20, 2018
1542abe
Fix NodeHandler.toString(): carefully check node id length
mkalinin Aug 20, 2018
2287929
Validate node id if node is created from 3rd party data
mkalinin Aug 20, 2018
8ee599b
Reduce log level for discovery processing errors
mkalinin Aug 21, 2018
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
8 changes: 7 additions & 1 deletion ethereumj-core/src/main/java/org/ethereum/net/rlpx/Node.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ public Node(byte[] id, String host, int port) {
this.port = port;
}

/**
* Instantiates node from RLP list containing node data.
* @throws IllegalArgumentException if node id is not a valid EC point.
*/
public Node(RLPList nodeRLP) {
byte[] hostB = nodeRLP.get(0).getRLPData();
byte[] portB = nodeRLP.get(1).getRLPData();
Expand All @@ -100,7 +104,9 @@ public Node(RLPList nodeRLP) {

this.host = bytesToIp(hostB);
this.port = port;
this.id = idB;

// a tricky way to check whether given data is a valid EC point or not
this.id = ECKey.fromNodeId(idB).getNodeId();
}

public Node(byte[] rlp) {
Expand Down
6 changes: 5 additions & 1 deletion ethereumj-core/src/main/java/org/ethereum/net/rlpx/discover/MessageHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ public void channelActive(ChannelHandlerContext ctx) throws Exception {

@Override
public void channelRead0(ChannelHandlerContext ctx, DiscoveryEvent event) throws Exception {
nodeManager.handleInbound(event);
try {
nodeManager.handleInbound(event);
} catch (Throwable t) {
logger.error("Failed to process incoming message: {}", event.getMessage(), t);
Copy link
Member

Choose a reason for hiding this comment

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

I would change log level to WARN or even INFO, since we can receive any garbage and this is not our error.
I'd also output t.toString() instead of t to not flood logs with traces in case of invalid messages

Ideally we can also try to differentiate the message handle problems onto MalformedMessageException and others. The first one is not error, the second one should be assumed as our error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to reduce logging level for the beginning. MalformedMessageException requires much more changes in the codebase.

}
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ethereumj-core/src/main/java/org/ethereum/net/rlpx/discover/NodeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ private void sendMessage(Message msg) {
@Override
public String toString() {
return "NodeHandler[state: " + state + ", node: " + node.getHost() + ":" + node.getPort() + ", id="
+ (node.getId().length > 0 ? Hex.toHexString(node.getId(), 0, 4) : "empty") + "]";
+ (node.getId().length >= 4 ? Hex.toHexString(node.getId(), 0, 4) : "empty") + "]";
}


Expand Down
15 changes: 12 additions & 3 deletions ethereumj-core/src/test/java/org/ethereum/net/rlpx/RLPXTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ public void test3() {
int port = 30303;

byte[] part1 = sha3("007".getBytes(Charset.forName("UTF-8")));
byte[] part2 = sha3("007".getBytes(Charset.forName("UTF-8")));
byte[] id = merge(part1, part2);
byte[] id = ECKey.fromPrivate(part1).getNodeId();

Node node = new Node(id, ip, port);

Expand Down Expand Up @@ -146,7 +145,7 @@ public void test5() {
@Test
public void test6() {

byte[] id_1 = sha3("+++".getBytes(Charset.forName("UTF-8")));
byte[] id_1 = ECKey.fromPrivate(sha3("+++".getBytes(Charset.forName("UTF-8")))).getNodeId();
String host_1 = "85.65.19.231";
int port_1 = 30303;

Expand Down Expand Up @@ -261,6 +260,16 @@ public void testCorrectIpPing() {
assertEquals(13272, msg1.getToPort());
assertEquals("58.136.8.186", msg1.getToHost());
}

@Test(expected = IllegalArgumentException.class)
public void testInvalidNodeId() {
byte[] id_1 = sha3("+++".getBytes(Charset.forName("UTF-8")));
String host_1 = "85.65.19.231";
int port_1 = 30303;

Node node_1 = new Node(id_1, host_1 , port_1);
new Node(node_1.getRLP());
}
}


Expand Down