Invalid MA is ignored (#881)

libp2p/multiaddress.nim

@@ -15,7 +15,7 @@ else:
   {.push raises: [].}
 {.push public.}
 
-import pkg/chronos
+import pkg/chronos, chronicles
 import std/[nativesockets, hashes]
 import tables, strutils, sets, stew/shims/net
 import multicodec, multihash, multibase, transcoder, vbuffer, peerid,
@@ -23,6 +23,9 @@ import multicodec, multihash, multibase, transcoder, vbuffer, peerid,
 import stew/[base58, base32, endians2, results]
 export results, minprotobuf, vbuffer, utility
 
+logScope:
+  topics = "libp2p multiaddress"
+
 type
   MAKind* = enum
     None, Fixed, Length, Path, Marker
@@ -1117,6 +1120,10 @@ proc getField*(pb: ProtoBuffer, field: int,
 proc getRepeatedField*(pb: ProtoBuffer, field: int,
                        value: var seq[MultiAddress]): ProtoResult[bool] {.
      inline.} =
+  ## Read repeated field from protobuf message. ``field`` is field number. If the message is malformed, an error is returned.
+  ## If field is not present in message, then ``ok(false)`` is returned and value is empty. If field is present,
+  ## but no items could be parsed, then ``err(ProtoError.IncorrectBlob)`` is returned and value is empty.
+  ## If field is present and some item could be parsed, then ``true`` is returned and value contains the parsed values.
   var items: seq[seq[byte]]
   value.setLen(0)
   let res = ? pb.getRepeatedField(field, items)
@@ -1128,6 +1135,8 @@ proc getRepeatedField*(pb: ProtoBuffer, field: int,
       if ma.isOk():
         value.add(ma.get())
       else:
-        value.setLen(0)
-        return err(ProtoError.IncorrectBlob)
-    ok(true)
+        debug "Not supported MultiAddress in blob", ma = item
+    if value.len == 0:
+      err(ProtoError.IncorrectBlob)
+    else:
+      ok(true)

tests/testmultiaddress.nim

@@ -1,3 +1,19 @@
+# Nim-LibP2P
+# Copyright (c) 2023 Status Research & Development GmbH
+# Licensed under either of
+#  * Apache License, version 2.0, ([LICENSE-APACHE](LICENSE-APACHE))
+#  * MIT license ([LICENSE-MIT](LICENSE-MIT))
+# at your option.
+# This file may not be copied, modified, or distributed except according to
+# those terms.
+
+when (NimMajor, NimMinor) < (1, 4):
+  {.push raises: [Defect].}
+else:
+  {.push raises: [].}
+
+import std/sequtils
+
 import unittest2
 import stew/byteutils
 import ../libp2p/[multicodec, multiaddress]
@@ -417,4 +433,36 @@ suite "MultiAddress test suite":
     check not tcp.matchPartial(ma)
     check IP4.matchPartial(ma)
 
+  test "getRepeatedField does not fail when all addresses are valid":
+    var pb = initProtoBuffer()
+    let mas = SuccessVectors.mapIt(MultiAddress.init(it).get())
+    for ma in mas:
+      pb.write(1, ma)
+    pb.finish()
 
+    var decoded = newSeq[MultiAddress]()
+    check pb.getRepeatedField(1, decoded).isOk()
+    check decoded == mas
+
+  test "getRepeatedField does not fail when some addresses are invalid":
+    var pb = initProtoBuffer()
+    var mas = @[MultiAddress.init("/ip4/1.2.3.4" ).get(), MultiAddress()]
+    for ma in mas:
+      pb.write(1, ma)
+    pb.finish()
+
+    var decoded = newSeq[MultiAddress]()
+    check pb.getRepeatedField(1, decoded).isOk()
+    check decoded == @[MultiAddress.init("/ip4/1.2.3.4" ).get()]
+
+  test "getRepeatedField fails when all addresses are invalid":
+    var pb = initProtoBuffer()
+    var mas = @[MultiAddress(), MultiAddress()]
+    for ma in mas:
+      pb.write(1, ma)
+    pb.finish()
+
+    var decoded = newSeq[MultiAddress]()
+    let error = pb.getRepeatedField(1, decoded).error()
+    check error == ProtoError.IncorrectBlob
+    check decoded.len == 0