diff --git a/core/peer/record.go b/core/peer/record.go index fce69ce00c..7f9b1defed 100644 --- a/core/peer/record.go +++ b/core/peer/record.go @@ -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) @@ -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 diff --git a/core/peer/record_test.go b/core/peer/record_test.go index b06f113404..865eb080e4 100644 --- a/core/peer/record_test.go +++ b/core/peer/record_test.go @@ -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) { @@ -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) { diff --git a/p2p/host/basic/addrs_manager.go b/p2p/host/basic/addrs_manager.go index 8fb19455a4..297a0f020c 100644 --- a/p2p/host/basic/addrs_manager.go +++ b/p2p/host/basic/addrs_manager.go @@ -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()