From 643c692784ac41016d505a4829df6220f06c64d8 Mon Sep 17 00:00:00 2001 From: "sentry[bot]" <39604003+sentry[bot]@users.noreply.github.com> Date: Mon, 1 Jun 2026 10:33:15 +0000 Subject: [PATCH] Fix: Universal access port TOCTOU race and disconnect issues (#22554) * Please describe the intent of your changes in a clear fashion. This PR addresses two related bugs concerning the universal access port and cable: **1. TOCTOU Race Condition Leading to Crash (SERVER-PROD-X9)** * **Root Cause:** When inserting a universal access cable into an IPC's access port, a check for `access_port.internal_port` occupancy occurs before a `do_mob` delay (which can include a consent dialog and a 2-second wait). If another action or player inserts something into the port during this delay, the subsequent call to `insert_item()` would find `internal_port` already occupied and trigger a `crash_with()`, leading to a server crash. * **Fix:** * A re-check for `access_port.internal_port` occupancy and the continued existence of the `access_port` itself has been added immediately after the `do_mob` delay in `access_cable/attack()`. If the port is now occupied or no longer exists, the insertion attempt is gracefully aborted with a user message. * The `crash_with()` call in `insert_item()` has been replaced with a `log_debug()` and an early return, ensuring that any future unexpected state in `internal_port` leads to graceful degradation rather than a server crash. **2. Stale `internal_port` on Cable Disconnect** * **Root Cause:** The `access_port` did not override the generic `remove_cable()` proc. When a cable was retracted or removed, `access_port.internal_port` was not explicitly cleared, causing the port to appear permanently occupied even after the cable was gone. * **Fix:** * An override for `remove_cable()` has been added to `/obj/item/organ/internal/machine/access_port`. This ensures that `clear_port()` is called, properly nulling `internal_port` when the cable is removed. * Please make sure that, in the case of mapping changes, you include images of these changes in the PR's description. * Please make sure to mark your PR as wip or review required by making a comment with !wip or !review required * If you include sprites/sounds/... (assets) that you have not created yourself specify the license and original author below. * Ensure that you also credit them in the appropriate location / changelog as specified in the contributor guidelines ### Asset Licenses The following assets that **have not** been created by myself are included in this PR: | Path | Original Author | License | | --- | --- | --- | | icons/example.dmi | ExamplePerson (Example Station) | CC0 | Fixes SERVER-PROD-X9 --------- Co-authored-by: sentry[bot] <39604003+sentry[bot]@users.noreply.github.com> Co-authored-by: VMSolidus --- .../internal/species/machine/access_port.dm | 20 ++++++++++++++++++- .../hellfirejag-ipc-cable-fixes.yml | 4 ++++ 2 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 html/changelogs/hellfirejag-ipc-cable-fixes.yml diff --git a/code/modules/organs/internal/species/machine/access_port.dm b/code/modules/organs/internal/species/machine/access_port.dm index 62001dfa307..303da885afe 100644 --- a/code/modules/organs/internal/species/machine/access_port.dm +++ b/code/modules/organs/internal/species/machine/access_port.dm @@ -77,11 +77,13 @@ SIGNAL_HANDLER if(internal_port) crash_with("Insert_item with [jack] on access port called with [internal_port] of [owner] already present!") + return FALSE internal_port = jack jack.forceMove(src) RegisterSignal(internal_port, COMSIG_QDELETING, PROC_REF(clear_port)) to_chat(owner, SPAN_MACHINE_WARNING("Internal firewall notice: [internal_port] inserted into [src].")) + return TRUE /** * This proc is called whenever the access cable is, for some reason, qdeleted (like with an explosion). @@ -111,9 +113,16 @@ /obj/item/organ/internal/machine/access_port/insert_cable(obj/item/access_cable/cable, mob/user) . = ..() - insert_item(cable) + if (!insert_item(cable)) + return + cable.create_cable(owner) +/obj/item/organ/internal/machine/access_port/remove_cable(obj/item/access_cable/cable) + . = ..() + if(internal_port == cable) + clear_port() + /obj/item/organ/internal/machine/access_port/cable_interact(obj/item/access_cable/cable, mob/user) var/obj/item/organ/internal/machine/internal_diagnostics/diagnostics_unit = owner.internal_organs_by_name[BP_DIAGNOSTICS_SUITE] if(!diagnostics_unit) @@ -277,6 +286,15 @@ user.visible_message(SPAN_WARNING("[user] tries to jack \the [src] into [human]'s access port...")) if(!do_mob(user, human, 2 SECONDS)) return + // Re-check after the delay: port state may have changed during consent/wait window + var/obj/item/organ/internal/machine/access_port/fresh_port = human.internal_organs_by_name[BP_ACCESS_PORT] + if(!fresh_port) + to_chat(user, SPAN_WARNING("[human] no longer has an access port!")) + return + if(fresh_port.internal_port) + to_chat(user, SPAN_WARNING("The access port is now occupied by [fresh_port.internal_port]!")) + return + access_port = fresh_port user.visible_message(SPAN_WARNING("[user] jacks \the [src] into [human]'s access port!")) else diff --git a/html/changelogs/hellfirejag-ipc-cable-fixes.yml b/html/changelogs/hellfirejag-ipc-cable-fixes.yml new file mode 100644 index 00000000000..4b87bbc629d --- /dev/null +++ b/html/changelogs/hellfirejag-ipc-cable-fixes.yml @@ -0,0 +1,4 @@ +author: Hellfirejag +delete-after: True +changes: + - bugfix: "Fixed some situations where an IPC datacable can get 'stuck'."