Fix AROS64 XADOpen and CLI crashes

ChangeLog

@@ -159,6 +159,9 @@ AROS portability:
 - Fix AROS x86_64 FTP lister handles, AROS menu data IPTR casts,
   AROS-converted image data writable mapping, AROS debug builds
   and runtime init.
+- Fix AROS x86_64 XADOpen and internal CLI crashes caused by
+  pointer-sized ARexx message metadata and RawDoFmt string varargs
+  (issue #90).
 - diskinfo: resolve PI at runtime to dodge an AROS x86_64
   cross-compiler ICE under Rosetta.
 - ftp / diskinfo / icon / library: fix AROS x86_64 format-string

source/Library/rexx.c

@@ -24,10 +24,6 @@ For more information on Directory Opus for Windows please see:
 #include "dopuslib.h"
 #include "rexx.h"
 
-#ifdef __AROS__
-	#define rm_avail rm_Unused1
-#endif
-
 #ifdef __amigaos4__
 /* OS4 newlib does not provide amiga.lib's SetRexxVar/GetRexxVar. Wrap the
  * rexxsyslib interface (IRexxSys->Set/GetRexxVarFromMsg) to preserve the
@@ -76,6 +72,92 @@ LONG GetRexxVar(CONST struct RexxMsg *rexxmsg, CONST_STRPTR name, STRPTR *result
 
 static char *RexxMsgIdentifier = "DOPUS";
 
+#ifdef __AROS__
+struct DOpusRexxMsgTracker
+{
+	struct Node node;
+	struct RexxMsg *msg;
+	struct List *stems;
+};
+
+static struct List DOpusRexxMsgTrackers;
+static BOOL DOpusRexxMsgTrackersReady;
+
+static void rexx_init_stem_tracking(void)
+{
+	if (!DOpusRexxMsgTrackersReady)
+	{
+		NewList(&DOpusRexxMsgTrackers);
+		DOpusRexxMsgTrackersReady = TRUE;
+	}
+}
+
+static BOOL rexx_track_stem_list(struct RexxMsg *msg, struct List *list)
+{
+	struct DOpusRexxMsgTracker *tracker;
+
+	if (!(tracker = AllocVec(sizeof(*tracker), MEMF_CLEAR)))
+		return FALSE;
+
+	tracker->msg = msg;
+	tracker->stems = list;
+
+	Forbid();
+	rexx_init_stem_tracking();
+	AddTail(&DOpusRexxMsgTrackers, (struct Node *)tracker);
+	Permit();
+
+	return TRUE;
+}
+
+static struct List *rexx_find_stem_list(struct RexxMsg *msg, BOOL detach)
+{
+	struct DOpusRexxMsgTracker *tracker, *next;
+	struct List *list = 0;
+	struct DOpusRexxMsgTracker *found = 0;
+
+	Forbid();
+	if (DOpusRexxMsgTrackersReady)
+	{
+		for (tracker = (struct DOpusRexxMsgTracker *)DOpusRexxMsgTrackers.lh_Head; tracker->node.ln_Succ;
+			 tracker = next)
+		{
+			next = (struct DOpusRexxMsgTracker *)tracker->node.ln_Succ;
+			if (tracker->msg == msg)
+			{
+				list = tracker->stems;
+				if (detach)
+				{
+					Remove((struct Node *)tracker);
+					found = tracker;
+				}
+				break;
+			}
+		}
+	}
+	Permit();
+
+	if (found)
+		FreeVec(found);
+
+	return list;
+}
+
+static struct List *rexx_get_stem_list(struct RexxMsg *msg)
+{
+	return rexx_find_stem_list(msg, FALSE);
+}
+
+static struct List *rexx_detach_stem_list(struct RexxMsg *msg)
+{
+	return rexx_find_stem_list(msg, TRUE);
+}
+#else
+	#define rexx_track_stem_list(msg, list) ((msg)->rm_avail = (IPTR)(list), TRUE)
+	#define rexx_get_stem_list(msg) ((struct List *)(IPTR)(msg)->rm_avail)
+	#define rexx_detach_stem_list(msg) ((struct List *)(IPTR)(msg)->rm_avail)
+#endif
+
 // Free an ARexx message
 void LIBFUNC L_FreeRexxMsgEx(REG(a0, struct RexxMsg *msg))
 {
@@ -85,6 +167,9 @@ void LIBFUNC L_FreeRexxMsgEx(REG(a0, struct RexxMsg *msg))
 	if (!RexxSysBase)
 		return;
 
+	if (!msg)
+		return;
+
 	// Get argument count
 	args = msg->rm_Action & RXARGMASK;
 
@@ -101,7 +186,7 @@ void LIBFUNC L_FreeRexxMsgEx(REG(a0, struct RexxMsg *msg))
 		struct List *list;
 
 		// Get list pointer
-		if ((list = (struct List *)(IPTR)msg->rm_avail))
+		if ((list = rexx_detach_stem_list(msg)))
 		{
 			struct RexxStem *node, *next;
 
@@ -144,13 +229,18 @@ struct RexxMsg *LIBFUNC L_CreateRexxMsgEx(REG(a0, struct MsgPort *port),
 	if (!(list = AllocVec(sizeof(struct List), MEMF_CLEAR)))
 		return msg;
 
-	// Turn message into a special Opus one
-	msg->rm_Node.mn_Node.ln_Name = RexxMsgIdentifier;
-	msg->rm_avail = (IPTR)list;
-
 	// Initialise list
 	NewList(list);
 
+	if (!rexx_track_stem_list(msg, list))
+	{
+		FreeVec(list);
+		return msg;
+	}
+
+	// Turn message into a special Opus one
+	msg->rm_Node.mn_Node.ln_Name = RexxMsgIdentifier;
+
 	return msg;
 }
 
@@ -190,7 +280,7 @@ long LIBFUNC L_SetRexxVarEx(REG(a0, struct RexxMsg *msg),
 		return 10;
 
 	// Get list pointer
-	if (!(list = (struct List *)(IPTR)msg->rm_avail))
+	if (!(list = rexx_get_stem_list(msg)))
 		return 10;
 
 	// See if variable already exists
@@ -248,7 +338,7 @@ long LIBFUNC L_GetRexxVarEx(REG(a0, struct RexxMsg *msg), REG(a1, char *varname)
 		return 10;
 
 	// Get list pointer
-	if (!(list = (struct List *)(IPTR)msg->rm_avail))
+	if (!(list = rexx_get_stem_list(msg)))
 		return 10;
 
 	// See if variable exists

source/Library/tests/test_rexx_aros64_tracking.py

@@ -0,0 +1,42 @@
+#!/usr/bin/env python3
+"""Regression checks for AROS 64-bit DOpus ARexx message metadata."""
+
+from pathlib import Path
+import unittest
+
+
+ROOT = Path(__file__).resolve().parents[3]
+REXX_C = ROOT / "source" / "Library" / "rexx.c"
+
+
+def read_source():
+    return REXX_C.read_text(encoding="latin-1")
+
+
+class RexxAROS64TrackingTests(unittest.TestCase):
+    def test_aros_does_not_store_pointer_in_rm_unused1(self):
+        source = read_source()
+
+        self.assertNotIn("#define rm_avail rm_Unused1", source)
+        self.assertIn("struct DOpusRexxMsgTracker", source)
+        self.assertIn("struct RexxMsg *msg;", source)
+        self.assertIn("struct List *stems;", source)
+
+    def test_dopus_stem_lists_are_tracked_outside_rexxmsg_on_aros(self):
+        source = read_source()
+
+        self.assertIn("static BOOL rexx_track_stem_list(struct RexxMsg *msg, struct List *list)", source)
+        self.assertIn("static struct List *rexx_get_stem_list(struct RexxMsg *msg)", source)
+        self.assertIn("static struct List *rexx_detach_stem_list(struct RexxMsg *msg)", source)
+        self.assertIn("if (!rexx_track_stem_list(msg, list))", source)
+        self.assertIn("if ((list = rexx_detach_stem_list(msg)))", source)
+        self.assertIn("if (!(list = rexx_get_stem_list(msg)))", source)
+
+    def test_free_rexx_msg_accepts_null(self):
+        source = read_source()
+
+        self.assertIn("if (!msg)\n\t\treturn;", source)
+
+
+if __name__ == "__main__":
+    unittest.main()

source/Program/function_cli.c

@@ -48,6 +48,40 @@ BOOL cli_open(CLIData *);
 void cli_close(CLIData *);
 void cli_free(CLIData *);
 
+static void cli_ulong_to_string(ULONG value, char *buffer, int buffer_size)
+{
+	int pos = buffer_size - 1;
+
+	if (buffer_size < 2)
+		return;
+
+	buffer[pos] = 0;
+	do
+	{
+		buffer[--pos] = '0' + (value % 10);
+		value /= 10;
+	} while (value && pos > 0);
+
+	strcpy(buffer, buffer + pos);
+}
+
+static void cli_build_window_name(char *buffer, int buffer_size)
+{
+	char top[12];
+	char *screen = get_our_pubscreen();
+	ULONG top_edge = (GUI->screen_pointer) ? GUI->screen_pointer->BarHeight + 1 : 20;
+
+	cli_ulong_to_string(top_edge, top, sizeof(top));
+
+	buffer[0] = 0;
+	StrConcat(buffer, environment->env->output_device, buffer_size);
+	StrConcat(buffer, "0/", buffer_size);
+	StrConcat(buffer, top, buffer_size);
+	StrConcat(buffer, "/512/150/DOpus 5 CLI/CLOSE/SCREEN ", buffer_size);
+	if (screen)
+		StrConcat(buffer, screen, buffer_size);
+}
+
 // Internal command line interpreter
 DOPUS_FUNC(function_cli)
 {
@@ -59,11 +93,7 @@ DOPUS_FUNC(function_cli)
 	short eliza_state = 0;
 
 	// Output filename
-	lsprintf(handle->temp_buffer,
-			 "%s0/%ld/512/150/DOpus 5 CLI/CLOSE/SCREEN %s",
-			 environment->env->output_device,
-			 (GUI->screen_pointer) ? GUI->screen_pointer->BarHeight + 1 : 20,
-			 get_our_pubscreen());
+	cli_build_window_name(handle->temp_buffer, sizeof(handle->temp_buffer));
 
 	// Allocate name copy
 	if (!(data.name = AllocVec(strlen(handle->temp_buffer) + 1, 0)))

source/Program/tests/test_cli_console_cleanup.py

@@ -28,6 +28,15 @@ def test_cli_data_starts_zeroed(self):
 
         self.assertIn("CLIData data = {0};", source)
 
+    def test_cli_window_name_avoids_rawdofmt_string_varargs(self):
+        source = read_source()
+
+        self.assertIn("static void cli_build_window_name(char *buffer, int buffer_size)", source)
+        self.assertIn("cli_build_window_name(handle->temp_buffer, sizeof(handle->temp_buffer));", source)
+        self.assertIn('StrConcat(buffer, "/512/150/DOpus 5 CLI/CLOSE/SCREEN ", buffer_size);', source)
+        self.assertIn("char *screen = get_our_pubscreen();", source)
+        self.assertNotIn('"%s0/%ld/512/150/DOpus 5 CLI/CLOSE/SCREEN %s"', source)
+
     def test_input_open_failure_restores_console_state(self):
         source = read_source()
         body = function_body(source, "cli_open")