fix: add guards to avoid crash when node is not created (#1497)

library/libstorage.nim

@@ -48,12 +48,14 @@ logScope:
 template checkLibstorageParams*(
     ctx: ptr StorageContext, callback: StorageCallback, userData: pointer
 ) =
-  if not isNil(ctx):
-    ctx[].userData = userData
-
   if isNil(callback):
     return RET_MISSING_CALLBACK
 
+  if isNil(ctx):
+    return callback.error("The context is not created.", userData)
+
+  ctx[].userData = userData
+
 proc malloc(size: csize_t): pointer {.importc, header: "<stdlib.h>".}
 
 proc asNewCString(s: string): ptr cchar =
@@ -277,6 +279,10 @@ proc storage_close(
 
 proc storage_destroy(ctx: ptr StorageContext): cint {.dynlib, exportc.} =
   initializeLibrary()
+
+  if isNil(ctx):
+    return RET_ERR
+
   let res = storage_context.destroyStorageContext(ctx)
   if res.isErr: RET_ERR else: RET_OK
 
@@ -578,5 +584,9 @@ proc storage_set_event_callback(
     ctx: ptr StorageContext, callback: StorageCallback, userData: pointer
 ) {.dynlib, exportc.} =
   initializeLibrary()
+
+  if isNil(ctx):
+    return
+
   ctx[].eventCallback = cast[pointer](callback)
   ctx[].eventUserData = userData

library/storage_thread_requests/requests/node_debug_request.nim

@@ -96,6 +96,12 @@ proc process*(
   defer:
     destroyShared(self)
 
+  # LOG_LEVEL does not require a node.
+  if storage[].isNil and self.operation != NodeDebugMsgType.LOG_LEVEL:
+    const msg = "Failed to process: the node is not created."
+    error msg
+    return err(msg)
+
   case self.operation
   of NodeDebugMsgType.DEBUG:
     let res = (await getDebug(storage))

library/storage_thread_requests/requests/node_download_request.nim

@@ -299,6 +299,11 @@ proc process*(
   defer:
     destroyShared(self)
 
+  if storage[].isNil:
+    const msg = "Failed to process: the node is not created."
+    error msg
+    return err(msg)
+
   case self.operation
   of NodeDownloadMsgType.INIT:
     let res = (await init(storage, self.cid, self.chunkSize, self.local))

library/storage_thread_requests/requests/node_info_request.nim

@@ -55,6 +55,12 @@ proc process*(
   defer:
     destroyShared(self)
 
+  # METRICS does not need a node.
+  if storage[].isNil and self.operation != METRICS:
+    const msg = "Failed to process: the node is not created."
+    error msg
+    return err(msg)
+
   case self.operation
   of REPO:
     let res = (await getRepo(storage))

library/storage_thread_requests/requests/node_lifecycle_request.nim

@@ -181,21 +181,40 @@ proc process*(
       error "Failed to CREATE_NODE.", error = error
       return err($error)
   of START_NODE:
+    if storage[].isNil:
+      const msg = "Failed to START_NODE, the node is not created."
+      error msg
+      return err(msg)
     try:
       await storage[].start()
     except Exception as e:
       error "Failed to START_NODE.", error = e.msg
       return err(e.msg)
   of STOP_NODE:
+    if storage[].isNil:
+      const msg = "Failed to STOP_NODE, the node is not created."
+      error msg
+      return err(msg)
     try:
       await storage[].stop()
     except Exception as e:
       error "Failed to STOP_NODE.", error = e.msg
       return err(e.msg)
   of CLOSE_NODE:
+    if storage[].isNil:
+      const msg = "Failed to CLOSE_NODE, the node is not created."
+      error msg
+      return err(msg)
+
+    # Set to nil. Even if the close fails, it could fail partially
+    # and we don't want to keep a reference to a node that is in an unknown state.
+    defer:
+      storage[] = nil
+
     try:
       await storage[].close()
     except Exception as e:
       error "Failed to CLOSE_NODE.", error = e.msg
       return err(e.msg)
+
   return ok("")

library/storage_thread_requests/requests/node_mix_request.nim

@@ -24,6 +24,11 @@ proc process*(
   defer:
     destroyShared(self)
 
+  if storage[].isNil:
+    const msg = "Failed to process: the node is not created."
+    error msg
+    return err(msg)
+
   let previous = storage[].node.togglePrivateQueries(self.privateQueries).valueOr:
     return err(error.msg)
   return ok($previous)

library/storage_thread_requests/requests/node_p2p_request.nim

@@ -105,6 +105,11 @@ proc process*(
   defer:
     destroyShared(self)
 
+  if storage[].isNil:
+    const msg = "Failed to process: the node is not created."
+    error msg
+    return err(msg)
+
   case self.operation
   of NodeP2PMsgType.CONNECT:
     # connect() takes seq[cstring]; self.peerAddresses is a raw shared array,

library/storage_thread_requests/requests/node_storage_request.nim

@@ -147,6 +147,11 @@ proc process*(
   defer:
     destroyShared(self)
 
+  if storage[].isNil:
+    const msg = "Failed to process: the node is not created."
+    error msg
+    return err(msg)
+
   case self.operation
   of NodeStorageMsgType.LIST:
     let res = (await list(storage))

library/storage_thread_requests/requests/node_upload_request.nim

@@ -350,6 +350,11 @@ proc process*(
   defer:
     destroyShared(self)
 
+  if storage[].isNil:
+    const msg = "Failed to process: the node is not created."
+    error msg
+    return err(msg)
+
   case self.operation
   of NodeUploadMsgType.INIT:
     let res = (await init(storage, self.filepath, self.chunkSize))

tests/libstorage/requests.nim

@@ -0,0 +1,100 @@
+import std/json
+import std/monotimes
+import std/os
+import std/strutils
+
+import pkg/chronos
+import pkg/results
+
+import ../asynctest
+import ../checktest
+import ../../library/storage_thread_requests/requests/node_lifecycle_request
+import ../../library/storage_thread_requests/requests/node_info_request
+import ../../library/storage_thread_requests/requests/node_storage_request
+import ../../library/storage_thread_requests/requests/node_debug_request
+
+from ../../storage/storage import StorageServer
+
+asyncchecksuite "Libstorage - request on a node that is not created":
+  var uncreated: StorageServer
+
+  test "rejects a start":
+    let res =
+      await NodeLifecycleRequest.createShared(START_NODE).process(addr uncreated)
+
+    check res.isErr and "not created" in res.error
+
+  test "rejects a stop":
+    let res = await NodeLifecycleRequest.createShared(STOP_NODE).process(addr uncreated)
+
+    check res.isErr and "not created" in res.error
+
+  test "rejects a close":
+    let res =
+      await NodeLifecycleRequest.createShared(CLOSE_NODE).process(addr uncreated)
+
+    check res.isErr and "not created" in res.error
+
+  test "rejects an info request":
+    let res =
+      await NodeInfoRequest.createShared(NodeInfoMsgType.SPR).process(addr uncreated)
+
+    check res.isErr and "not created" in res.error
+
+  test "rejects a storage list request":
+    let res = await NodeStorageRequest.createShared(NodeStorageMsgType.LIST).process(
+      addr uncreated
+    )
+
+    check res.isErr and "not created" in res.error
+
+  test "accepts a metrics request, it reads the process registry":
+    let res = await NodeInfoRequest.createShared(NodeInfoMsgType.METRICS).process(
+      addr uncreated
+    )
+
+    check res.isOk
+
+  test "accepts a node creation":
+    let dataDir = getTempDir() / "libstorage-requests" / $getMonoTime()
+
+    defer:
+      removeDir(dataDir)
+
+    var created: StorageServer
+    let config = $ %*{"data-dir": dataDir}
+    let res = await NodeLifecycleRequest
+      .createShared(CREATE_NODE, config.cstring)
+      .process(addr created)
+
+    check res.isOk
+
+    let closed =
+      await NodeLifecycleRequest.createShared(CLOSE_NODE).process(addr created)
+    check closed.isOk
+
+  test "rejects a second close, the node is dropped by the first one":
+    let dataDir = getTempDir() / "libstorage-requests" / $getMonoTime()
+
+    defer:
+      removeDir(dataDir)
+
+    var created: StorageServer
+    let config = $ %*{"data-dir": dataDir}
+    check (
+      await NodeLifecycleRequest.createShared(CREATE_NODE, config.cstring).process(
+        addr created
+      )
+    ).isOk
+    check (await NodeLifecycleRequest.createShared(CLOSE_NODE).process(addr created)).isOk
+
+    let res = await NodeLifecycleRequest.createShared(CLOSE_NODE).process(addr created)
+
+    check res.isErr and "not created" in res.error
+
+  test "accepts a log level change":
+    let res = await NodeDebugRequest
+      .createShared(NodeDebugMsgType.LOG_LEVEL, logLevel = "WARN")
+      .process(addr uncreated)
+
+    check res.isOk

tests/testLibstorage.nim

@@ -1,5 +1,6 @@
 # Tests the Nim side of libstorage.
 import ./libstorage/config
 import ./libstorage/logosmetrics
+import ./libstorage/requests
 
 {.warning[UnusedImport]: off.}