From a62490e02ec7af371734e61e4620f381a8072451 Mon Sep 17 00:00:00 2001 From: Tristan Wilson Date: Thu, 23 Jul 2026 09:13:47 +0200 Subject: [PATCH 1/2] holepunch: initialize timeout before registering notifiee The hole-punch network notifiee was registered before the service copied directDialTimeout onto it. An inbound relayed connection could therefore read the field concurrently and observe the zero value. Pass the configured timeout into the constructor so every field is initialized before Network.Notify publishes the object. Add a deterministic regression test that inspects the timeout at registration. Signed-off-by: Tristan Wilson --- p2p/protocol/holepunch/holepuncher.go | 15 +++-- p2p/protocol/holepunch/svc.go | 3 +- p2p/protocol/holepunch/svc_init_test.go | 85 +++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 9 deletions(-) create mode 100644 p2p/protocol/holepunch/svc_init_test.go diff --git a/p2p/protocol/holepunch/holepuncher.go b/p2p/protocol/holepunch/holepuncher.go index 1ae16bdfce..3c470709af 100644 --- a/p2p/protocol/holepunch/holepuncher.go +++ b/p2p/protocol/holepunch/holepuncher.go @@ -50,14 +50,15 @@ type holePuncher struct { filter AddrFilter } -func newHolePuncher(h host.Host, ids identify.IDService, listenAddrs func() []ma.Multiaddr, tracer *tracer, filter AddrFilter) *holePuncher { +func newHolePuncher(h host.Host, ids identify.IDService, listenAddrs func() []ma.Multiaddr, directDialTimeout time.Duration, tracer *tracer, filter AddrFilter) *holePuncher { hp := &holePuncher{ - host: h, - ids: ids, - active: make(map[peer.ID]struct{}), - tracer: tracer, - filter: filter, - listenAddrs: listenAddrs, + host: h, + ids: ids, + active: make(map[peer.ID]struct{}), + tracer: tracer, + filter: filter, + listenAddrs: listenAddrs, + directDialTimeout: directDialTimeout, } hp.ctx, hp.ctxCancel = context.WithCancel(context.Background()) h.Network().Notify((*netNotifiee)(hp)) diff --git a/p2p/protocol/holepunch/svc.go b/p2p/protocol/holepunch/svc.go index 0e4c90c7f9..924b28ad4a 100644 --- a/p2p/protocol/holepunch/svc.go +++ b/p2p/protocol/holepunch/svc.go @@ -148,8 +148,7 @@ func (s *Service) waitForPublicAddr() { // service is closed return } - s.holePuncher = newHolePuncher(s.host, s.ids, s.listenAddrs, s.tracer, s.filter) - s.holePuncher.directDialTimeout = s.directDialTimeout + s.holePuncher = newHolePuncher(s.host, s.ids, s.listenAddrs, s.directDialTimeout, s.tracer, s.filter) close(s.hasPublicAddrsChan) } diff --git a/p2p/protocol/holepunch/svc_init_test.go b/p2p/protocol/holepunch/svc_init_test.go new file mode 100644 index 0000000000..1ca41f0b5a --- /dev/null +++ b/p2p/protocol/holepunch/svc_init_test.go @@ -0,0 +1,85 @@ +package holepunch + +import ( + "context" + "testing" + "time" + + "github.com/libp2p/go-libp2p/core/host" + "github.com/libp2p/go-libp2p/core/network" + "github.com/libp2p/go-libp2p/core/peer" + "github.com/libp2p/go-libp2p/core/protocol" + + ma "github.com/multiformats/go-multiaddr" +) + +type notifyInspectNetwork struct { + network.Network + notify func(network.Notifiee) +} + +func (n *notifyInspectNetwork) Notify(notifiee network.Notifiee) { + n.notify(notifiee) +} + +type notifyInspectHost struct { + host.Host + network network.Network + addrs []ma.Multiaddr +} + +func (*notifyInspectHost) ID() peer.ID { return peer.ID("notify-inspect-host") } + +func (h *notifyInspectHost) Addrs() []ma.Multiaddr { + return h.addrs +} + +func (h *notifyInspectHost) Network() network.Network { + return h.network +} + +func (*notifyInspectHost) SetStreamHandler(protocol.ID, network.StreamHandler) {} + +func TestHolePuncherFullyInitializedBeforeNetworkNotify(t *testing.T) { + const timeout = 137 * time.Millisecond + + var ( + timeoutAtRegistration time.Duration + notifyCalls int + ) + testNetwork := ¬ifyInspectNetwork{ + notify: func(notifiee network.Notifiee) { + notifyCalls++ + registered, ok := notifiee.(*netNotifiee) + if !ok { + t.Fatalf("registered %T, want *netNotifiee", notifiee) + } + timeoutAtRegistration = (*holePuncher)(registered).directDialTimeout + }, + } + addrs := []ma.Multiaddr{ma.StringCast("/ip4/1.2.3.4/tcp/4001")} + testHost := ¬ifyInspectHost{network: testNetwork, addrs: addrs} + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + service := &Service{ + ctx: ctx, + host: testHost, + listenAddrs: func() []ma.Multiaddr { return addrs }, + directDialTimeout: timeout, + hasPublicAddrsChan: make(chan struct{}), + } + service.refCount.Add(1) + service.waitForPublicAddr() + if service.holePuncher != nil { + defer service.holePuncher.Close() + } + + if notifyCalls != 1 { + t.Fatalf("Notify calls = %d, want 1", notifyCalls) + } + if timeoutAtRegistration != timeout { + t.Fatalf("directDialTimeout at registration = %s, want %s", timeoutAtRegistration, timeout) + } +} From 894fa092c5453ab4da5f85976d0f4a9bc8ef93b5 Mon Sep 17 00:00:00 2001 From: sukun Date: Wed, 29 Jul 2026 20:37:14 +0530 Subject: [PATCH 2/2] holepunch: remove initialization order test The test depends too much on the package internals. The race window it checks is closed by the previous commit, and is infeasible to trigger with real hosts. Assisted-By: Claude Fable 5 --- p2p/protocol/holepunch/svc_init_test.go | 85 ------------------------- 1 file changed, 85 deletions(-) delete mode 100644 p2p/protocol/holepunch/svc_init_test.go diff --git a/p2p/protocol/holepunch/svc_init_test.go b/p2p/protocol/holepunch/svc_init_test.go deleted file mode 100644 index 1ca41f0b5a..0000000000 --- a/p2p/protocol/holepunch/svc_init_test.go +++ /dev/null @@ -1,85 +0,0 @@ -package holepunch - -import ( - "context" - "testing" - "time" - - "github.com/libp2p/go-libp2p/core/host" - "github.com/libp2p/go-libp2p/core/network" - "github.com/libp2p/go-libp2p/core/peer" - "github.com/libp2p/go-libp2p/core/protocol" - - ma "github.com/multiformats/go-multiaddr" -) - -type notifyInspectNetwork struct { - network.Network - notify func(network.Notifiee) -} - -func (n *notifyInspectNetwork) Notify(notifiee network.Notifiee) { - n.notify(notifiee) -} - -type notifyInspectHost struct { - host.Host - network network.Network - addrs []ma.Multiaddr -} - -func (*notifyInspectHost) ID() peer.ID { return peer.ID("notify-inspect-host") } - -func (h *notifyInspectHost) Addrs() []ma.Multiaddr { - return h.addrs -} - -func (h *notifyInspectHost) Network() network.Network { - return h.network -} - -func (*notifyInspectHost) SetStreamHandler(protocol.ID, network.StreamHandler) {} - -func TestHolePuncherFullyInitializedBeforeNetworkNotify(t *testing.T) { - const timeout = 137 * time.Millisecond - - var ( - timeoutAtRegistration time.Duration - notifyCalls int - ) - testNetwork := ¬ifyInspectNetwork{ - notify: func(notifiee network.Notifiee) { - notifyCalls++ - registered, ok := notifiee.(*netNotifiee) - if !ok { - t.Fatalf("registered %T, want *netNotifiee", notifiee) - } - timeoutAtRegistration = (*holePuncher)(registered).directDialTimeout - }, - } - addrs := []ma.Multiaddr{ma.StringCast("/ip4/1.2.3.4/tcp/4001")} - testHost := ¬ifyInspectHost{network: testNetwork, addrs: addrs} - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - service := &Service{ - ctx: ctx, - host: testHost, - listenAddrs: func() []ma.Multiaddr { return addrs }, - directDialTimeout: timeout, - hasPublicAddrsChan: make(chan struct{}), - } - service.refCount.Add(1) - service.waitForPublicAddr() - if service.holePuncher != nil { - defer service.holePuncher.Close() - } - - if notifyCalls != 1 { - t.Fatalf("Notify calls = %d, want 1", notifyCalls) - } - if timeoutAtRegistration != timeout { - t.Fatalf("directDialTimeout at registration = %s, want %s", timeoutAtRegistration, timeout) - } -}