Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 9 additions & 1 deletion core/peer/record.go
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ func addrsFromProtobuf(addrs []*pb.PeerRecord_AddressInfo) []ma.Multiaddr {
out := make([]ma.Multiaddr, 0, len(addrs))
for _, addr := range addrs {
a, err := ma.NewMultiaddrBytes(addr.Multiaddr)
if err != nil {
if err != nil || len(a) == 0 {
continue
}
out = append(out, a)
Expand All @@ -245,6 +245,14 @@ func addrsFromProtobuf(addrs []*pb.PeerRecord_AddressInfo) []ma.Multiaddr {
func addrsToProtobuf(addrs []ma.Multiaddr) []*pb.PeerRecord_AddressInfo {
out := make([]*pb.PeerRecord_AddressInfo, 0, len(addrs))
for _, addr := range addrs {
// Drop empty multiaddrs. Since go-multiaddr v0.15 made Multiaddr a
// slice type, a zero-value Multiaddr has no components and encodes
// to zero bytes on the wire. Receivers that skip the empty-input
// check decode those bytes as "/" and reject the address.
// See https://github.com/libp2p/js-libp2p/issues/3478#issuecomment-4322093929
if len(addr) == 0 {
continue
}
out = append(out, &pb.PeerRecord_AddressInfo{Multiaddr: addr.Bytes()})
}
return out
Expand Down
44 changes: 44 additions & 0 deletions core/peer/record_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
. "github.com/libp2p/go-libp2p/core/peer"
"github.com/libp2p/go-libp2p/core/record"
"github.com/libp2p/go-libp2p/core/test"
ma "github.com/multiformats/go-multiaddr"
)

func TestPeerRecordConstants(t *testing.T) {
Expand Down Expand Up @@ -53,6 +54,49 @@ func TestSignedPeerRecordFromEnvelope(t *testing.T) {
})
}

// Regression: PeerRecord must not write empty multiaddrs onto the wire.
// A zero-component Multiaddr encodes to zero bytes, and peers that skip
// the empty-input check render those bytes as "/". go-libp2p's own
// decoder errors on empty input via NewMultiaddrBytes and drops the
// entry, so a round-trip on go-libp2p alone hides the bug. The test
// inspects marshaled record bytes instead.
//
// See https://github.com/libp2p/js-libp2p/issues/3478#issuecomment-4322093929
func TestPeerRecordDropsEmptyMultiaddrs(t *testing.T) {
priv, _, err := test.RandTestKeyPair(crypto.Ed25519, 256)
test.AssertNilError(t, err)
id, err := IDFromPrivateKey(priv)
test.AssertNilError(t, err)

good, err := ma.NewMultiaddr("/ip4/127.0.0.1/tcp/4001")
test.AssertNilError(t, err)

rec := &PeerRecord{
PeerID: id,
Addrs: []ma.Multiaddr{nil, good, {}, good},
Seq: TimestampSeq(),
}
recBytes, err := rec.MarshalRecord()
test.AssertNilError(t, err)

rec2 := &PeerRecord{}
if err := rec2.UnmarshalRecord(recBytes); err != nil {
t.Fatal(err)
}
if len(rec2.Addrs) != 2 {
t.Fatalf("expected 2 non-empty addrs after round-trip, got %d", len(rec2.Addrs))
}

// Re-encode and compare bytes. Without the fix, the original record
// holds zero-length addr fields that get dropped on decode, so the
// re-encoded bytes are shorter than the original.
rec2Bytes, err := rec2.MarshalRecord()
test.AssertNilError(t, err)
if !bytes.Equal(recBytes, rec2Bytes) {
t.Fatal("PeerRecord wire form contained empty multiaddr entries")
}
}

// This is pretty much guaranteed to pass on Linux no matter how we implement it, but Windows has
// low clock precision. This makes sure we never get a duplicate.
func TestTimestampSeq(t *testing.T) {
Expand Down
7 changes: 7 additions & 0 deletions p2p/host/basic/addrs_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,13 @@ func (a *addrsManager) makeSignedPeerRecord(addrs []ma.Multiaddr) (*record.Envel
if a.signKey == nil {
return nil, errors.New("signKey is nil")
}
// Drop empty multiaddrs before sealing. A zero-component Multiaddr
// would otherwise enter the signed envelope and reach peers as "/"
// when they decode the wire bytes.
// See https://github.com/libp2p/js-libp2p/issues/3478#issuecomment-4322093929
addrs = slices.DeleteFunc(slices.Clone(addrs), func(m ma.Multiaddr) bool {
return len(m) == 0
})
// Limit the length of currentAddrs to ensure that our signed peer records aren't rejected
peerRecordSize := 64 // HostID
k, err := a.signKey.Raw()
Expand Down
Loading