From 00ed6ad3124909d16815149c6c171810d9d260c6 Mon Sep 17 00:00:00 2001
From: Jordan Hrycaj <info@mjh-it.com>
Date: Thu, 26 May 2022 10:23:40 +0100
Subject: [PATCH] Dedicated RLP reader for `DisconnectionReason` (#507)

* Provide dedicated `DisconnectionReason` enum type RLP reader

why:
  Without this reader, the program communicating via RLPX will crash when
  receiving out of bound reason codes disconnect message.

  Out of bound value assignments to an enum causes a `RangeError`defect
  and consequently the program to terminate. This `RangeError` is avoided
  here and a `MalformedRlpError` catchable error raised.

* Using default exception type in bespoke `read(DisconnectionReason)`

why:
  This should not differ from the default enum parser. The particular
  message is different and more targeted, here.

  Note: The default RLP parser was not used because `
  `array[1,DisconnectionReason]` is currently not properly handled and
  should give a siliar error message as a `DisconnectionReason` error.

* De-clutter, custom read() was not needed

Co-authored-by: jordan <jordan@curd.mjh-it.com>
---
 eth/p2p/private/p2p_types.nim  |  2 ++
 eth/p2p/rlpx.nim               | 16 +++++++++-------
 tests/p2p/test_rlpx_thunk.json | 10 ++++++++++
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/eth/p2p/private/p2p_types.nim b/eth/p2p/private/p2p_types.nim
index 2398501..e9a2042 100644
--- a/eth/p2p/private/p2p_types.nim
+++ b/eth/p2p/private/p2p_types.nim
@@ -1,3 +1,4 @@
+
 # nim-eth
 # Copyright (c) 2018-2021 Status Research & Development GmbH
 # Licensed and distributed under either of
@@ -8,6 +9,7 @@
 import
   std/[deques, tables],
   bearssl, chronos,
+  stew/results,
   ".."/../[rlp, keys], ".."/../common/eth_types,
   ".."/[enode, kademlia, discovery, rlpxcrypt]
 
diff --git a/eth/p2p/rlpx.nim b/eth/p2p/rlpx.nim
index 8f694ef..6af208b 100644
--- a/eth/p2p/rlpx.nim
+++ b/eth/p2p/rlpx.nim
@@ -55,28 +55,30 @@ type
     value: DisconnectionReason
 
 proc read(rlp: var Rlp; T: type DisconnectionReasonList): T
-    {.raises: [Defect, RlpError].} =
+    {.gcsafe, raises: [RlpError, Defect].} =
   ## Rlp mixin: `DisconnectionReasonList` parser
+
   if rlp.isList:
     # Be strict here: The expression `rlp.read(DisconnectionReasonList)`
     # accepts lists with at least one item. The array expression wants
     # exactly one item.
-    let a = rlp.read(array[1, DisconnectionReason])
-    return DisconnectionReasonList(value: a[0])
+    return DisconnectionReasonList(
+      value: rlp.read(array[1,DisconnectionReason])[0])
 
   # Also accepted: a single byte reason code. Is is typically used
   # by variants of the reference implementation `Geth`
   if rlp.blobLen <= 1:
-    let n = rlp.read(int)
-    return DisconnectionReasonList(value: n.DisconnectionReason)
+    return DisconnectionReasonList(
+      value: rlp.read(DisconnectionReason))
 
   # Also accepted: a blob of a list (aka object) of reason code. It is
   # used by `bor`, a `geth` fork
   var subList = rlp.toBytes.rlpFromBytes
   if subList.isList:
     # Ditto, see above.
-    let a = subList.read(array[1,DisconnectionReason])
-    return DisconnectionReasonList(value: a[0])
+    return DisconnectionReasonList(
+      value: subList.read(array[1,DisconnectionReason])[0])
+
   raise newException(RlpTypeMismatch, "Single entry list expected")
 
 
diff --git a/tests/p2p/test_rlpx_thunk.json b/tests/p2p/test_rlpx_thunk.json
index a57e77f..ce51de3 100644
--- a/tests/p2p/test_rlpx_thunk.json
+++ b/tests/p2p/test_rlpx_thunk.json
@@ -59,6 +59,16 @@
     "error": "RlpTypeMismatch",
     "description": "listElem to assert on not a single entry list"
   },
+  "Listing single element list with entry off enum range": {
+    "payload": "01c116",
+    "error": "RlpTypeMismatch",
+    "description": "Disconnect reason code out of bounds 0..16 (got: 22)"
+  },
+  "Listing single element off enum range": {
+    "payload": "0116",
+    "error": "RlpTypeMismatch",
+    "description": "Disconnect reason code out of bounds 0..16 (got: 22)"
+  },
   "devp2p hello packet version 22 + additional list elements for EIP-8": {
     "payload": "00f87137916b6e6574682f76302e39312f706c616e39cdc5836574683dc6846d6f726b1682270fb840fda1cff674c90c9a197539fe3dfb53086ace64f83ed7c6eabec741f7f381cc803e52ab2cd55d5569bce4347107a310dfd5f88a010cd2ffd1005ca406f1842877c883666f6f836261720304"
   }