Reviewing codebase for stability and critical bugs (#195)

* fix: reject invalid switchNumber(-1), mutex init guard, ABS BUTTON_TEST/SYSTEM guard, CMD_SET_COMMS_MODE batch fix + 3 new tests

Agent-Logs-Url: https://github.com/dazzaXx/ModernJVS/sessions/7c312b9a-49b0-456f-ba5a-d9b806de9e20

Co-authored-by: dazzaXx <48131734+dazzaXx@users.noreply.github.com>

* Fix missing continue for unmapped devices (!autoDetect); fix misleading test comment

Agent-Logs-Url: https://github.com/dazzaXx/ModernJVS/sessions/67639733-14aa-44fb-b17d-4fa43fab2445

Co-authored-by: dazzaXx <48131734+dazzaXx@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: dazzaXx <48131734+dazzaXx@users.noreply.github.com>

Author: Copilot

src/controller/input.c

@@ -574,6 +574,19 @@ static void *deviceThread(void *_args)
                     if (args->inputs.abs[event.code].secondaryIO && args->jvsIO->chainedIO != NULL)
                         io = args->jvsIO->chainedIO;
 
+                    /* Mirror the EV_KEY BUTTON_TEST guard: BUTTON_TEST and BUTTON_3
+                     * share the same numeric value (0x80); if either is mapped to the
+                     * SYSTEM player on a HAT axis, route through the test-button latch
+                     * rather than calling setSwitch directly to keep behaviour consistent
+                     * with the key-based test button path. */
+                    if (args->inputs.abs[event.code].output == BUTTON_TEST &&
+                        args->inputs.abs[event.code].jvsPlayer == SYSTEM)
+                    {
+                        if (event.value != 0 && __atomic_load_n(&args->jvsIO->deviceID, __ATOMIC_ACQUIRE) != -1)
+                            __sync_fetch_and_xor(&testButtonActive, 1);
+                        continue;
+                    }
+
                     if (event.value == args->inputs.absMin[event.code])
                     {
                         /* Activate primary direction and clear secondary to prevent
@@ -618,6 +631,18 @@ static void *deviceThread(void *_args)
                         }
                         continue;
                     }
+
+                    /* Mirror the EV_KEY BUTTON_TEST guard (BUTTON_TEST == BUTTON_3 == 0x80):
+                     * route through the latch rather than calling setSwitch directly. */
+                    if (args->inputs.key[event.code].output == BUTTON_TEST &&
+                        args->inputs.key[event.code].jvsPlayer == SYSTEM)
+                    {
+                        if (event.value != args->inputs.absMin[event.code] &&
+                            __atomic_load_n(&args->jvsIO->deviceID, __ATOMIC_ACQUIRE) != -1)
+                            __sync_fetch_and_xor(&testButtonActive, 1);
+                        continue;
+                    }
+
                     else if (event.value == args->inputs.absMin[event.code])
                     {
                         setSwitch(io, args->inputs.key[event.code].jvsPlayer, args->inputs.key[event.code].output, 0);
@@ -1378,6 +1403,11 @@ JVSInputStatus initInputs(char *outputMappingPath, char *configPath, char *secon
                     continue;
                 }
             }
+            /* When autoDetect is off (or no generic map matched), skip any
+             * device that still has no mappings rather than starting a
+             * pointless thread that never produces JVS output. */
+            if (inputMappings.length == 0)
+                continue;
         }
 
         EVInputs evInputs = {0};

src/jvs/io.c

@@ -122,10 +122,13 @@ int initIO(JVSIO *io)
 	/* Destroy any previous mutex before re-initialising.  POSIX states that
 	 * calling pthread_mutex_init on an already-initialised mutex without a
 	 * preceding pthread_mutex_destroy is undefined behaviour and leaks the
-	 * kernel resource on Linux.  The destroy is a no-op if the mutex was
-	 * never initialised (the struct is zero-initialised in main). */
-	pthread_mutex_destroy(&io->state_mutex);
+	 * kernel resource on Linux.  Guard with mutexInitialized == 1 so that
+	 * the first call (where the field is zero or garbage) skips the destroy
+	 * and avoids UB on a non-zero-initialised struct. */
+	if (io->mutexInitialized == 1)
+		pthread_mutex_destroy(&io->state_mutex);
 	pthread_mutex_init(&io->state_mutex, NULL);
+	io->mutexInitialized = 1;
 
 	return 1;
 }
@@ -139,6 +142,15 @@ int setSwitch(JVSIO *io, JVSPlayer player, JVSInput switchNumber, int value)
 		return 0;
 	}
 
+	/* Reject an invalid (out-of-range) switch number.  jvsInputFromString()
+	 * returns (JVSInput)-1 when a button name is not found; using -1 in the
+	 * bitwise OR/AND below would corrupt the entire switch state word. */
+	if ((int)switchNumber < 0)
+	{
+		debug(0, "Error: Invalid switch number %d — likely an unrecognised button name in a mapping file.\n", (int)switchNumber);
+		return 0;
+	}
+
 	pthread_mutex_lock(&io->state_mutex);
 	if (value)
 		io->state.inputSwitch[player] |= switchNumber;

src/jvs/io.h

@@ -112,6 +112,11 @@ typedef struct
 typedef struct JVSIO
 {
     int deviceID;
+    /* Set to 1 by initIO after the first pthread_mutex_init call so that
+     * subsequent initIO calls can safely call pthread_mutex_destroy first.
+     * Checked with == 1 (not just != 0) so that a struct poisoned with
+     * memset(0xFF) skips the destroy, which is the correct safe behaviour. */
+    int mutexInitialized;
     int analogueRestBits;
     int gunXRestBits;
     int gunYRestBits;

src/jvs/jvs.c

@@ -512,17 +512,28 @@ JVSStatus processPacket(JVSIO *jvsIO)
 
 		/* CMD_SET_COMMS_MODE is a broadcast-only command (NODE NO.FF) and the JVS
 		 * spec defines its response size as 0 — no acknowledge should be sent.
-		 * Return immediately without calling writePacket so nothing is transmitted. */
+		 * If it arrives as the sole command in a packet, return immediately so
+		 * nothing is transmitted.  If it appears after other commands in a batch
+		 * (non-compliant but possible with buggy arcade hardware), break out of
+		 * the loop so the responses already assembled for the prior commands are
+		 * delivered rather than silently dropped. */
 		case CMD_SET_COMMS_MODE:
 		{
+			size = 2;  /* command byte + mode byte */
 			if (index + 1 >= (int)inputPacket.length - 1)
 			{
 				debug(0, "Error: CMD_SET_COMMS_MODE - packet too short\n");
 				return JVS_STATUS_ERROR;
 			}
 			debug(1, "CMD_SET_COMMS_MODE - Mode 0x%02X (no response required)\n", inputPacket.data[index + 1]);
-			return JVS_STATUS_SUCCESS;
+			/* outputPacket.length == 1 means only the STATUS_SUCCESS header byte
+			 * was written, i.e. no prior commands produced any output — safe to
+			 * return without sending anything. */
+			if (outputPacket.length <= 1)
+				return JVS_STATUS_SUCCESS;
+			/* Otherwise fall through to writePacket via the break. */
 		}
+		break;
 
 		/* Ask for the name of the IO board */
 		case CMD_REQUEST_ID:

src/testsuite/test_modernjvs.c

@@ -747,6 +747,73 @@ static void test_jvsInputFromString_unknown(void)
 
 /* ── New tests for PR bug-fixes ──────────────────────────────────────────── */
 
+/*
+ * setSwitch must reject a negative switchNumber.
+ * jvsInputFromString("BAD_NAME") returns (JVSInput)-1; before the fix that
+ * value was used directly in a bitwise OR, setting ALL bits of inputSwitch
+ * and making every button appear simultaneously pressed on the JVS bus.
+ */
+static void test_setSwitch_invalid_switch_number(void)
+{
+    TEST_BEGIN(test_setSwitch_invalid_switch_number);
+    JVSIO io = make_test_io();
+
+    /* Ensure the state starts clean */
+    ASSERT_EQ_INT(io.state.inputSwitch[1], 0, "P1 switch state starts at 0");
+
+    /* Pass the -1 sentinel returned by jvsInputFromString on lookup failure */
+    int r = setSwitch(&io, PLAYER_1, (JVSInput)-1, 1);
+    ASSERT_EQ_INT(r, 0, "setSwitch with switchNumber -1 must return 0");
+
+    /* The switch state must not be corrupted (must still be 0, not 0xFFFF) */
+    ASSERT_EQ_INT(io.state.inputSwitch[1], 0,
+                  "switch state must not be corrupted by invalid switchNumber");
+
+    /* Also verify the release path doesn't clear everything */
+    setSwitch(&io, PLAYER_1, BUTTON_START, 1);
+    setSwitch(&io, PLAYER_1, (JVSInput)-1, 0);
+    ASSERT(io.state.inputSwitch[1] & BUTTON_START,
+           "valid BUTTON_START bit must survive invalid-switch release");
+
+    TEST_PASS();
+}
+
+/*
+ * Calling initIO() twice on the same JVSIO must not produce undefined behaviour.
+ * The first call initialises the mutex; the second call must destroy then
+ * re-initialise it safely, confirmed by the mutexInitialized guard introduced
+ * to prevent pthread_mutex_destroy on a garbage-initialised struct.
+ */
+static void test_initIO_reinit_mutex_safety(void)
+{
+    TEST_BEGIN(test_initIO_reinit_mutex_safety);
+
+    JVSIO io;
+    memset(&io, 0, sizeof(io));
+    io.capabilities.players            = 2;
+    io.capabilities.analogueInChannels = 2;
+    io.capabilities.analogueInBits     = 10;
+    io.capabilities.coins              = 2;
+    io.capabilities.rotaryChannels     = 0;
+    io.capabilities.gunChannels        = 0;
+
+    int r1 = initIO(&io);
+    ASSERT_EQ_INT(r1, 1, "first initIO returns 1");
+    ASSERT_EQ_INT(io.mutexInitialized, 1, "mutexInitialized set after first init");
+
+    /* Set some state, then re-init; state must be zeroed again */
+    io.state.inputSwitch[1] = 0xFFFF;
+    int r2 = initIO(&io);
+    ASSERT_EQ_INT(r2, 1, "second initIO returns 1");
+    ASSERT_EQ_INT(io.state.inputSwitch[1], 0, "state zeroed by second initIO");
+
+    /* The mutex must still be usable after re-init */
+    int r3 = setSwitch(&io, PLAYER_1, BUTTON_START, 1);
+    ASSERT_EQ_INT(r3, 1, "setSwitch works after re-init");
+
+    TEST_PASS();
+}
+
 /*
  * setSwitch must reject a negative player index.
  * jvsPlayerFromString("INVALID") returns (JVSPlayer)-1, which before the fix
@@ -2508,12 +2575,13 @@ static void test_processPacket_cmd_remaining_payout_two_slots(void)
     ASSERT(r.valid == 1, "response valid");
     ASSERT_EQ_INT(r.data[0], STATUS_SUCCESS,  "STATUS_SUCCESS");
     ASSERT_EQ_INT(r.data[1], REPORT_SUCCESS,  "REPORT_SUCCESS");
-    /* 2 slots × 2 bytes each = 4 data bytes, all zero */
-    ASSERT_EQ_INT(r.data[2], 0x00, "slot1 high = 0");
-    ASSERT_EQ_INT(r.data[3], 0x00, "slot1 low = 0");
-    ASSERT_EQ_INT(r.data[4], 0x00, "slot2 high = 0");
-    ASSERT_EQ_INT(r.data[5], 0x00, "slot2 low = 0");
-    /* Total response data length: STATUS(1) + REPORT(1) + 2*2 = 6 */
+    /* Single-slot response: REPORT(1) + hopper_status(1) + remaining_hi(1)
+     * + remaining_mid(1) + remaining_lo(1) = 5 bytes; all zero because we
+     * always report 0 remaining.  Total: STATUS(1) + 5 = 6. */
+    ASSERT_EQ_INT(r.data[2], 0x00, "hopper_status = 0");
+    ASSERT_EQ_INT(r.data[3], 0x00, "remaining_hi = 0");
+    ASSERT_EQ_INT(r.data[4], 0x00, "remaining_mid = 0");
+    ASSERT_EQ_INT(r.data[5], 0x00, "remaining_lo = 0");
     ASSERT_EQ_INT(r.data_len, 6, "data_len = 6");
 
     close(sv[0]); close(sv[1]); serialIO = -1;
@@ -2997,6 +3065,46 @@ static void test_processPacket_cmd_set_comms_mode(void)
     TEST_PASS();
 }
 
+/*
+ * CMD_SET_COMMS_MODE appearing after another command in a batch must NOT
+ * silently discard the REPORT bytes already assembled for the prior command.
+ * Before the fix the handler did an unconditional `return`, dropping any
+ * partially-built response.
+ *
+ * Batch: CMD_COMMAND_VERSION | CMD_SET_COMMS_MODE 0x01
+ * Expected response: STATUS_SUCCESS + REPORT_SUCCESS + commandVersion byte
+ *   (the SET_COMMS_MODE does not add a REPORT byte but the version reply
+ *    must be present in the wire output).
+ */
+static void test_processPacket_cmd_set_comms_mode_batch_preserves_prior(void)
+{
+    TEST_BEGIN(test_processPacket_cmd_set_comms_mode_batch_preserves_prior);
+
+    JVSIO io = make_test_io();
+    io.deviceID = 0x01;
+    int sv[2];
+    int afd = open_test_socket(sv);
+    ASSERT(afd >= 0, "socketpair");
+
+    /* Build a two-command batch: CMD_COMMAND_VERSION (no args) followed by
+     * CMD_SET_COMMS_MODE 0x01. */
+    unsigned char cmd[] = {CMD_COMMAND_VERSION, CMD_SET_COMMS_MODE, 0x01};
+    JVSStatus s = run_processPacket(&io, afd, 0x01, cmd, 3);
+    ASSERT(s == JVS_STATUS_SUCCESS, "batch with CMD_SET_COMMS_MODE SUCCESS");
+
+    /* The response for CMD_COMMAND_VERSION must be present; without the fix
+     * the SET_COMMS_MODE early-return would have dropped it entirely. */
+    JVSResponse r = jvs_read_response(afd);
+    ASSERT(r.valid == 1, "response checksum valid");
+    ASSERT_EQ_INT(r.data[0], STATUS_SUCCESS, "STATUS_SUCCESS present");
+    ASSERT_EQ_INT(r.data[1], REPORT_SUCCESS, "REPORT_SUCCESS for CMD_COMMAND_VERSION");
+    ASSERT_EQ_INT(r.data[2], io.capabilities.commandVersion,
+                  "commandVersion byte preserved in response");
+
+    close(sv[0]); close(sv[1]); serialIO = -1;
+    TEST_PASS();
+}
+
 static void test_processPacket_cmd_read_keypad(void)
 {
     TEST_BEGIN(test_processPacket_cmd_read_keypad);
@@ -3459,6 +3567,7 @@ static const TestFn tests[] = {
     test_jvsPlayerFromString_known,
     test_jvsPlayerFromString_unknown,
     /* New bounds-check tests (PR bug-fixes) */
+    test_setSwitch_invalid_switch_number,
     test_setSwitch_negative_player,
     test_setAnalogue_negative_channel,
     test_setGun_negative_channel,
@@ -3468,6 +3577,7 @@ static const TestFn tests[] = {
     test_initIO_oversized_capabilities,
     test_initIO_zero_analogue_bits,
     test_initIO_oversized_analogue_bits,
+    test_initIO_reinit_mutex_safety,
     test_initJVS_right_align_bits,
     test_initJVS_chained_io_rest_bits,
     /* Config parsing */
@@ -3543,6 +3653,7 @@ static const TestFn tests[] = {
     test_processPacket_cmd_remaining_payout_two_slots,
     /* Additional command coverage */
     test_processPacket_cmd_set_comms_mode,
+    test_processPacket_cmd_set_comms_mode_batch_preserves_prior,
     test_processPacket_cmd_read_keypad,
     test_processPacket_cmd_write_gpo_bit,
     test_processPacket_cmd_write_analog,