diff --git a/storage/nat.nim b/storage/nat.nim index 60dedc49..a0b9ba23 100644 --- a/storage/nat.nim +++ b/storage/nat.nim @@ -9,7 +9,7 @@ {.push raises: [].} import - std/[options, os, times, net, atomics, exitprocs], + std/[options, os, times, net, atomics], nat_traversal/[miniupnpc, natpmp], json_serialization/std/net, results @@ -250,7 +250,7 @@ proc repeatPortMapping(args: PortMappingArgs) {.thread, raises: [ValueError].} = sleep(sleepDuration) -proc stopNatThreads() {.noconv.} = +proc stopNatThreads() = # stop the thread debug "Stopping NAT port mapping renewal threads" try: @@ -301,6 +301,14 @@ proc stopNatThreads() {.noconv.} = debug "NAT-PMP: deleted port mapping", externalPort = eport, internalPort = iport, protocol = protocol +proc stopNat*() = + stopNatThreads() + natThreads.setLen(0) + activeMappings.setLen(0) + extIp = IpAddress.none + strategy = NatStrategy.NatNone + natClosed.store(false) + proc redirectPorts*( strategy: NatStrategy, tcpPort, udpPort: Port, description: string ): Option[(Port, Port)] = @@ -325,10 +333,6 @@ proc redirectPorts*( natThreads[^1].createThread( repeatPortMapping, (strategy, externalTcpPort, externalUdpPort, description) ) - # atexit() in disguise - if natThreads.len == 1: - # we should register the thread termination function only once - addExitProc(stopNatThreads) except Exception as exc: warn "Failed to create NAT port mapping renewal thread", exc = exc.msg @@ -339,8 +343,10 @@ proc setupNat*( ## If any of this fails, we don't return any IP address but do return the ## original ports as best effort. ## TODO: Allow for tcp or udp port mapping to be optional. - if extIp.isNone: - extIp = getExternalIP(natStrategy) + + # getExternalIP initialises the threadvars upnp and npmp, + # so we need to call to make sure they are initialised in the current thread. + extIp = getExternalIP(natStrategy) if extIp.isSome: let ip = extIp.get let extPorts = ( diff --git a/storage/storage.nim b/storage/storage.nim index f794a2f5..6ef0bd47 100644 --- a/storage/storage.nim +++ b/storage/storage.nim @@ -160,6 +160,9 @@ proc stop*(s: StorageServer) {.async.} = notice "Stopping Storage node" + {.gcsafe.}: + stopNat() + var futures = @[ s.storageNode.switch.stop(), s.storageNode.stop(), diff --git a/tests/storage/testnat.nim b/tests/storage/testnat.nim index 21faa156..1ca3c05a 100644 --- a/tests/storage/testnat.nim +++ b/tests/storage/testnat.nim @@ -1,10 +1,11 @@ -import std/[unittest, net] +import std/[unittest, net, options] import pkg/chronos import pkg/libp2p/[multiaddress, multihash, multicodec] import pkg/results -import ../../storage/nat +import ../../storage/nat {.all.} import ../../storage/utils +import ../../storage/utils/natutils suite "NAT Address Tests": test "nattedAddress with local addresses": @@ -41,3 +42,30 @@ suite "NAT Address Tests": # Verify results check(discoveryAddrs == expectedDiscoveryAddrs) check(libp2pAddrs == expectedlibp2pAddrs) + +suite "NAT restart safety": + test "setupNat does not crash when extIp is cached from a previous start": + # Reproduces the restart crash (SIGSEGV) seen with nat=upnp. + # + # After a first start, extIp was cached and upnp was initialised. + # After a stop, extIp was still cached, but upnp was not initialised because + # the thread was destroyed using the destroy function. + # As a result, the next start crashed in setupNat when it tried to do port mapping + # because getExternalIp wasn't called anymore (it was cached) and upnp was not initialised. + extIp = some(parseIpAddress("1.2.3.4")) + strategy = NatStrategy.NatUpnp + + let res = setupNat(NatStrategy.NatUpnp, Port(8500), Port(8500), "storage") + check res.tcpPort.isSome + + test "stopNat resets NAT state so the next start is clean": + # Simulates leftover state + extIp = some(parseIpAddress("1.2.3.4")) + strategy = NatStrategy.NatNone + activeMappings.add(PortMappings()) + + stopNat() + + check extIp.isNone + check activeMappings.len == 0 + check natThreads.len == 0