diff --git a/api.md b/api.md index 11d08fb..1fc7e59 100644 --- a/api.md +++ b/api.md @@ -98,13 +98,20 @@ Returns a `MappingResult` containing the mapping `id` (needed for `destroyMappin Returns an error if no NAT device is found, the mapping fails, or the timeout expires. +> **Warning:** `onStateChange` runs on libplum's internal C thread, not the +> chronos event loop. Do not call chronos APIs or touch non-thread-safe state +> from it; restrict it to thread-safe operations (e.g. `Atomic`, a channel). + ### destroyMapping ```nim proc destroyMapping*(id: cint) ``` -Removes a mapping. Must be called exactly once after a successful `createMapping`. +Removes a mapping. Must be called exactly once after a successful `createMapping`, +otherwise the mapping's internal handle is leaked for the lifetime of the process. +Calling it again, or with an unknown `id`, is a safe no-op. `cleanup` also releases +any mappings still active. ### hasMapping diff --git a/libplum/libplum.nim b/libplum/libplum.nim index 8de5c8b..9c3457e 100644 --- a/libplum/libplum.nim +++ b/libplum/libplum.nim @@ -15,17 +15,6 @@ const libraryPath = libplumPath & "/libplum.a" {.passc: "-I" & includePath & " -DPLUM_STATIC".} {.passl: libraryPath.} -# libplum declares some parameters as `const T*` in C (read-only pointer). -# Nim has no equivalent, so the generated C code drops the `const`, causing -# a type mismatch warning in GCC 15+. This pragma suppresses that warning -# only in this translation unit and is valid for both C and C++. -{. - emit: """ -#ifdef __GNUC__ -#pragma GCC diagnostic ignored "-Wno-incompatible-pointer-types" -#endif -""" -.} when defined(windows): {.passl: "-lws2_32 -liphlpapi -lbcrypt".} @@ -108,6 +97,8 @@ type # Import plum functions +{.push raises: [].} + proc plum_init*( config: ptr plum_config_t ): cint {.importc: "plum_init", header: "plum.h".} @@ -129,3 +120,5 @@ proc plum_destroy_mapping*( proc plum_get_local_address*( buffer: cstring, size: csize_t ): cint {.importc: "plum_get_local_address", header: "plum.h".} + +{.pop.} diff --git a/libplum/plum.nim b/libplum/plum.nim index cdcadeb..3399724 100644 --- a/libplum/plum.nim +++ b/libplum/plum.nim @@ -12,6 +12,10 @@ import chronos/threadsync import results import ./libplum +# libplum declares some parameters as `const T*` in C (read-only pointer). +# Nim has no equivalent, so the generated C code drops the `const`, causing +# a type mismatch warning in GCC 15+. This pragma suppresses that warning +# only in this translation unit and is valid for both C and C++. {. emit: """ #ifdef __GNUC__ @@ -24,11 +28,22 @@ export results {.pragma: callback, cdecl, raises: [], gcsafe.} +{.push raises: [].} + type PlumProtocol* = enum TCP = PLUM_IP_PROTOCOL_TCP.int UDP = PLUM_IP_PROTOCOL_UDP.int + PlumLogLevel* {.pure.} = enum + Verbose = PLUM_LOG_LEVEL_VERBOSE.int + Debug = PLUM_LOG_LEVEL_DEBUG.int + Info = PLUM_LOG_LEVEL_INFO.int + Warn = PLUM_LOG_LEVEL_WARN.int + Error = PLUM_LOG_LEVEL_ERROR.int + Fatal = PLUM_LOG_LEVEL_FATAL.int + None = PLUM_LOG_LEVEL_NONE.int + PlumState* = enum Destroyed = PLUM_STATE_DESTROYED.int Pending = PLUM_STATE_PENDING.int @@ -55,6 +70,9 @@ type mapping*: PlumMapping PlumStateCallback* = proc(state: PlumState, mapping: PlumMapping) {.callback.} + ## Invoked on mapping state changes after the initial result. Runs on + ## libplum's internal C thread, not the chronos loop: only touch + ## thread-safe state from it (e.g. Atomic), never chronos APIs. MappingHandle = ref object signal: ThreadSignalPtr @@ -70,8 +88,8 @@ type resolvedInternalPort: uint16 resolvedExternalPort: uint16 resolvedExternalHost: array[PLUM_MAX_HOST_LEN, char] - # Use abandoned pattern for memory freeing - abandoned: Atomic[bool] + # Refcount-like + signalReleases: Atomic[int] onStateChange: PlumStateCallback # libplum calls mappingCallback from its own C thread. Under refc, any thread @@ -79,15 +97,21 @@ type template foreignThreadGc(body: untyped) = when declared(setupForeignThreadGc): setupForeignThreadGc() - body - when declared(tearDownForeignThreadGc): - tearDownForeignThreadGc() + try: + body + finally: + when declared(tearDownForeignThreadGc): + tearDownForeignThreadGc() var activeMappingsLock: Lock var activeMappings {.guard: activeMappingsLock.}: Table[cint, MappingHandle] initLock(activeMappingsLock) +proc releaseSignal(handle: MappingHandle) = + if handle.signalReleases.fetchAdd(1) == 1: + discard handle.signal.close() + # We can be confident that the pattern is GC Safe using # a lock. template withSafeLock(body: untyped) = @@ -95,9 +119,7 @@ template withSafeLock(body: untyped) = withLock activeMappingsLock: body -proc mappingCallback( - id: cint, state: plum_state_t, raw: ptr plum_mapping_t -) {.cdecl, raises: [].} = +proc mappingCallback(id: cint, state: plum_state_t, raw: ptr plum_mapping_t) {.cdecl.} = ## Called from libplum's internal C thread on SUCCESS, FAILURE, and DESTROYED. foreignThreadGc: @@ -109,12 +131,7 @@ proc mappingCallback( if plumState == Destroyed: withSafeLock: activeMappings.del(id) - # The handle can be abandoned after a timeout during a - # mapping creation. - # In that case, destroy is called internally and the - # signal pointer can be closed. - if handle.abandoned.load(): - discard handle.signal.close() + handle.releaseSignal() # Release the pin set in createMapping: the C library is done with the # raw pointer and will never call this callback again for this mapping. GC_unref(handle) @@ -148,15 +165,15 @@ proc mappingCallback( handle.onStateChange(plumState, mapping) proc init*( - logLevel: plum_log_level_t = PLUM_LOG_LEVEL_NONE, - discoverTimeout: int = 0, - mappingTimeout: int = 0, - recheckPeriod: int = 0, -): Result[void, string] {.raises: [].} = + logLevel: PlumLogLevel = PlumLogLevel.None, + discoverTimeout: int32 = 0, + mappingTimeout: int32 = 0, + recheckPeriod: int32 = 0, +): Result[void, string] = ## init MUST be called to setup internal plum thread (plum_init). var config = plum_config_t( - log_level: logLevel, + log_level: plum_log_level_t(logLevel.int), log_callback: nil, dummytls_domain: nil, discover_timeout: discoverTimeout.cint, @@ -170,7 +187,7 @@ proc init*( else: err("plum_init failed: " & $res) -proc cleanup*(): Result[void, string] {.raises: [].} = +proc cleanup*(): Result[void, string] = ## cleanup MUST be called to stop the thread and clean the setup. let res = plum_cleanup() @@ -198,7 +215,6 @@ proc createMapping*( user_ptr: cast[pointer](handle), ) - # Avoid issue with refc. # Pin the handle to prevent GC: the C library holds a raw pointer to # it (user_ptr) and might use it until DESTROYED fires. GC_ref(handle) @@ -221,40 +237,23 @@ proc createMapping*( raise finally: if not completed: - # Timeout or cancellation: we cannot close the signal here because - # the C callback may fire later and call fireSync on it. - # Mark the handle as abandoned so the DESTROYED callback closes it. - # Access via activeMappings rather than the local `handle` ref, - # in order to make sure we have the valid reference. - withSafeLock: - let h = activeMappings.getOrDefault(id) - if not h.isNil: - h.abandoned.store(true) + # Timeout or cancellation: a late callback may still fireSync, so the + # mapping is torn down and the close is decided by releaseSignal. discard plum_destroy_mapping(id) - else: - # Signal fired normally, safe to close. - discard signal.close() + handle.releaseSignal() - # Reached only when completed = true (CancelledError skips this). + # Timeout path: the signal never fired within the deadline. if not completed: return err("plum: mapping " & $id & " timed out") - # Read result via activeMappings rather than the local `handle` ref in - # order to make sure we have the valid reference. - var resolvedState: PlumState - var resolvedMapping: PlumMapping - - withSafeLock: - let h = activeMappings.getOrDefault(id) - if not h.isNil: - resolvedState = h.resolvedState - resolvedMapping = PlumMapping( - protocol: h.resolvedProtocol, - mappingProtocol: h.resolvedMappingProtocol, - internalPort: h.resolvedInternalPort, - externalPort: h.resolvedExternalPort, - externalHost: $cast[cstring](unsafeAddr h.resolvedExternalHost), - ) + let resolvedState = handle.resolvedState + let resolvedMapping = PlumMapping( + protocol: handle.resolvedProtocol, + mappingProtocol: handle.resolvedMappingProtocol, + internalPort: handle.resolvedInternalPort, + externalPort: handle.resolvedExternalPort, + externalHost: $cast[cstring](unsafeAddr handle.resolvedExternalHost), + ) if resolvedState == Success: return ok(MappingResult(id: id, mapping: resolvedMapping)) @@ -262,16 +261,29 @@ proc createMapping*( discard plum_destroy_mapping(id) return err("plum: mapping " & $id & " failed") -proc destroyMapping*(id: cint) {.raises: [].} = +proc destroyMapping*(id: cint) = ## Must be called exactly once after a successful createMapping. + ## Safe to call again or on an unknown id + withSafeLock: + if id notin activeMappings: + return discard plum_destroy_mapping(id) -proc hasMapping*(id: cint): bool {.raises: [].} = - ## Returns true if the mapping exists and has not been destroyed yet. - withSafeLock: - result = id in activeMappings +proc hasMapping*(id: cint): bool = + ## Returns true if the mapping exists and is not being destroyed. + var st: plum_state_t + if plum_query_mapping(id, addr st, nil) == PLUM_ERR_SUCCESS: + PlumState(st.int) notin {Destroying, Destroyed} + else: + false -proc getLocalAddress*(): Result[string, string] {.raises: [].} = +proc activeMappingCount*(): int = + ## Number of mappings the wrapper still tracks. Drops to 0 once every + ## mapping has fired DESTROYED. Mainly useful to detect handle leaks. + withSafeLock: + result = activeMappings.len + +proc getLocalAddress*(): Result[string, string] = var buf = newString(PLUM_MAX_ADDRESS_LEN) let res = plum_get_local_address(buf.cstring, buf.len.csize_t) if res >= 0: @@ -279,3 +291,5 @@ proc getLocalAddress*(): Result[string, string] {.raises: [].} = ok(buf) else: err("plum_get_local_address failed: " & $res) + +{.pop.} diff --git a/tests/test_plum.nim b/tests/test_plum.nim index e8dfc12..430ee89 100644 --- a/tests/test_plum.nim +++ b/tests/test_plum.nim @@ -16,6 +16,8 @@ import chronos import libplum/plum import libplum/libplum +const miniupnp_protocol {.strdefine.} = "" + suite "plum": test "init and cleanup": let r = init() @@ -39,7 +41,37 @@ suite "plum": test "hasMapping returns false for unknown id": check not hasMapping(999) -const miniupnp_protocol {.strdefine.} = "" + # Only valid where no NAT device answers; the integration container runs + # miniupnpd, which would make the mapping succeed before the timeout. + when miniupnp_protocol == "": + test "createMapping times out without a NAT device": + require init().isOk() + defer: + discard cleanup() + + let r = waitFor createMapping(TCP, 8101, timeout = milliseconds(50)) + check r.isErr() + + test "cleanup while a createMapping is pending completes cleanly": + require init().isOk() + defer: + discard cleanup() + + # No NAT device answers, so the mapping stays PENDING until we cleanup. + let fut = createMapping(TCP, 8501, timeout = milliseconds(200)) + waitFor sleepAsync(50.milliseconds) + discard cleanup() + let r = waitFor fut + check r.isErr() + check activeMappingCount() == 0 + + test "destroyMapping is a no-op on an unknown id": + require init().isOk() + defer: + discard cleanup() + + destroyMapping(999.cint) + check not hasMapping(999) # The flag is passed by the Docker / Podman container. when miniupnp_protocol != "": @@ -54,7 +86,7 @@ when miniupnp_protocol != "": let mappingTimeout = seconds(40) let logLevel = - if getEnv("LIBPLUM_VERBOSE") == "1": PLUM_LOG_LEVEL_VERBOSE else: PLUM_LOG_LEVEL_NONE + if getEnv("LIBPLUM_VERBOSE") == "1": PlumLogLevel.Verbose else: PlumLogLevel.None var gRenewed: Atomic[bool] @@ -89,15 +121,13 @@ when miniupnp_protocol != "": suite "plum - " & miniupnp_protocol & " using miniupnp": test "createMapping TCP and destroyMapping": - require init(discoverTimeout = discoverMs, logLevel = logLevel).isOk() + require init(discoverTimeout = discoverMs.int32, logLevel = logLevel).isOk() defer: discard cleanup() let r = waitFor createMapping(TCP, 8101, timeout = mappingTimeout) require r.isOk() let res = r.value - defer: - destroyMapping(res.id) checkpoint miniupnp_protocol & " TCP: " & res.mapping.externalHost & ":" & $res.mapping.externalPort @@ -112,8 +142,13 @@ when miniupnp_protocol != "": else: check res.mapping.mappingProtocol == PCP + destroyMapping(res.id) + check not hasMapping(res.id) + # second destroy on a real id must be a safe no-op + destroyMapping(res.id) + test "createMapping UDP and destroying": - require init(discoverTimeout = discoverMs, logLevel = logLevel).isOk() + require init(discoverTimeout = discoverMs.int32, logLevel = logLevel).isOk() defer: discard cleanup() @@ -137,7 +172,9 @@ when miniupnp_protocol != "": test "mapping is renewed after miniupnpd restart": require init( - discoverTimeout = discoverMs, logLevel = logLevel, recheckPeriod = recheckMs + discoverTimeout = discoverMs.int32, + logLevel = logLevel, + recheckPeriod = recheckMs.int32, ) .isOk() defer: @@ -165,3 +202,13 @@ when miniupnp_protocol != "": startMiniupnpd(miniupnp_protocol) check waitRenewal() + + test "cleanup releases active mappings": + require init(discoverTimeout = discoverMs.int32, logLevel = logLevel).isOk() + + let r = waitFor createMapping(TCP, 8401, timeout = mappingTimeout) + require r.isOk() + + # no destroyMapping on purpose: cleanup must release everything + check cleanup().isOk() + check activeMappingCount() == 0 diff --git a/vendor/libplum b/vendor/libplum index b74757f..b5d5f7d 160000 --- a/vendor/libplum +++ b/vendor/libplum @@ -1 +1 @@ -Subproject commit b74757f88c2a98ea905f5b81ad040232bc019c35 +Subproject commit b5d5f7d31b319b21136ec861d59d95f02eaf207a