fix: crash on upnp restart (#1486)

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 = (

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(),

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