From 06821e944818e7f064aeb95cbdef5f12dd85a729 Mon Sep 17 00:00:00 2001
From: Simon Howard
Date: Thu, 27 Nov 2014 16:54:05 -0500
Subject: hexen: audit calls to P_StartACS().

Scripts can have a maximum of three arguments when started
(see ACS_Execute / #80 linedef special type). This means that the
array of arguments passed must be at least three elements in
length. When loading scripts, check the argument count never
exceeds 3, and print a warning message if it does.

Don't pass pointers to structure members implicitly treating them
as elements of an array (as this relies on compiler-dependent
behavior as to how structure members are laid out). Instead, copy
values into a temporary array to make the intended behavior explicit.

This fixes #477. Thanks to Quasar for reporting it.
---
 src/hexen/p_acs.c   | 25 +++++++++++++++++++++----
 src/hexen/p_enemy.c |  5 +++--
 src/hexen/p_inter.c |  5 ++---
 src/hexen/p_map.c   | 12 ++++++++++--
 src/hexen/p_spec.c  | 14 ++++++++++++--
 5 files changed, 48 insertions(+), 13 deletions(-)

(limited to 'src/hexen')

diff --git a/src/hexen/p_acs.c b/src/hexen/p_acs.c
index f76fa167..b9fd8f08 100644
--- a/src/hexen/p_acs.c
+++ b/src/hexen/p_acs.c
@@ -27,6 +27,7 @@
 
 // MACROS ------------------------------------------------------------------
 
+#define MAX_SCRIPT_ARGS 3
 #define SCRIPT_CONTINUE 0
 #define SCRIPT_STOP 1
 #define SCRIPT_TERMINATE 2
@@ -195,7 +196,7 @@ static acs_t *NewScript;
 
 static int (*PCodeCmds[]) (void) =
 {
-CmdNOP,
+        CmdNOP,
         CmdTerminate,
         CmdSuspend,
         CmdPushNumber,
@@ -294,7 +295,10 @@ CmdNOP,
         CmdSoundSequence,
         CmdSetLineTexture,
         CmdSetLineBlocking,
-        CmdSetLineSpecial, CmdThingSound, CmdEndPrintBold};
+        CmdSetLineSpecial,
+        CmdThingSound,
+        CmdEndPrintBold,
+};
 
 // CODE --------------------------------------------------------------------
 
@@ -336,6 +340,16 @@ void P_LoadACScripts(int lump)
         info->argCount = LONG(*buffer);
         ++buffer;
 
+        if (info->argCount > MAX_SCRIPT_ARGS)
+        {
+            fprintf(stderr, "Warning: ACS script #%i has %i arguments, more "
+                            "than the maximum of %i. Enforcing limit.\n"
+                            "If you are seeing this message, please report "
+                            "the name of the WAD where you saw it.\n",
+                            i, info->argCount, MAX_SCRIPT_ARGS);
+            info->argCount = MAX_SCRIPT_ARGS;
+        }
+
         if (info->number >= OPEN_SCRIPTS_BASE)
         {                       // Auto-activate
             info->number -= OPEN_SCRIPTS_BASE;
@@ -414,6 +428,9 @@ void P_CheckACSStore(void)
 //
 // P_StartACS
 //
+// Start an ACS script. The 'args' array should be at least MAX_SCRIPT_ARGS
+// elements in length.
+//
 //==========================================================================
 
 static char ErrorMsg[128];
@@ -458,7 +475,7 @@ boolean P_StartACS(int number, int map, byte * args, mobj_t * activator,
     script->side = side;
     script->ip = ACSInfo[infoIndex].address;
     script->thinker.function = T_InterpretACS;
-    for (i = 0; i < ACSInfo[infoIndex].argCount; i++)
+    for (i = 0; i < MAX_SCRIPT_ARGS && i < ACSInfo[infoIndex].argCount; i++)
     {
         script->vars[i] = args[i];
     }
@@ -503,7 +520,7 @@ static boolean AddToACSStore(int map, int number, byte * args)
     }
     ACSStore[index].map = map;
     ACSStore[index].script = number;
-    memcpy(ACSStore[index].args, args, sizeof(int));
+    memcpy(ACSStore[index].args, args, MAX_SCRIPT_ARGS);
     return true;
 }
 
diff --git a/src/hexen/p_enemy.c b/src/hexen/p_enemy.c
index 1ad87032..18cc79eb 100644
--- a/src/hexen/p_enemy.c
+++ b/src/hexen/p_enemy.c
@@ -4781,7 +4781,8 @@ void A_FreezeDeath(mobj_t * actor)
         }
     }
     else if (actor->flags & MF_COUNTKILL && actor->special)
-    {                           // Initiate monster death actions
+    {
+        // Initiate monster death actions.
         P_ExecuteLineSpecial(actor->special, actor->args, NULL, 0, actor);
     }
 }
@@ -4933,7 +4934,7 @@ void A_KoraxChase(mobj_t * actor)
 {
     mobj_t *spot;
     int lastfound;
-    byte args[3] = { 0, 0, 0 };
+    byte args[3] = {0, 0, 0};
 
     if ((!actor->special2.i) &&
         (actor->health <= (actor->info->spawnhealth / 2)))
diff --git a/src/hexen/p_inter.c b/src/hexen/p_inter.c
index 15dfa5f8..587614ff 100644
--- a/src/hexen/p_inter.c
+++ b/src/hexen/p_inter.c
@@ -1280,7 +1280,7 @@ mobj_t *ActiveMinotaur(player_t * master)
 
 void P_KillMobj(mobj_t * source, mobj_t * target)
 {
-    int dummy;
+    byte dummyArgs[3] = {0, 0, 0};
     mobj_t *master;
 
     target->flags &= ~(MF_SHOOTABLE | MF_FLOAT | MF_SKULLFLY | MF_NOGRAVITY);
@@ -1292,8 +1292,7 @@ void P_KillMobj(mobj_t * source, mobj_t * target)
     {                           // Initiate monster death actions
         if (target->type == MT_SORCBOSS)
         {
-            dummy = 0;
-            P_StartACS(target->special, 0, (byte *) & dummy, target, NULL, 0);
+            P_StartACS(target->special, 0, dummyArgs, target, NULL, 0);
         }
         else
         {
diff --git a/src/hexen/p_map.c b/src/hexen/p_map.c
index c8172555..11aa23e0 100644
--- a/src/hexen/p_map.c
+++ b/src/hexen/p_map.c
@@ -2003,6 +2003,7 @@ static boolean PuzzleActivated;
 boolean PTR_PuzzleItemTraverse(intercept_t * in)
 {
     mobj_t *mobj;
+    byte args[3];
     int sound;
 
     if (in->isaline)
@@ -2045,8 +2046,14 @@ boolean PTR_PuzzleItemTraverse(intercept_t * in)
         {                       // Item type doesn't match
             return false;
         }
-        P_StartACS(in->d.line->arg2, 0, &in->d.line->arg3,
-                   PuzzleItemUser, in->d.line, 0);
+
+        // Construct an args[] array that would contain the values from
+        // the line that would be passed by Vanilla Hexen.
+        args[0] = in->d.line->arg3;
+        args[1] = in->d.line->arg4;
+        args[2] = in->d.line->arg5;
+
+        P_StartACS(in->d.line->arg2, 0, args, PuzzleItemUser, in->d.line, 0);
         in->d.line->special = 0;
         PuzzleActivated = true;
         return false;           // Stop searching
@@ -2061,6 +2068,7 @@ boolean PTR_PuzzleItemTraverse(intercept_t * in)
     {                           // Item type doesn't match
         return true;
     }
+
     P_StartACS(mobj->args[1], 0, &mobj->args[2], PuzzleItemUser, NULL, 0);
     mobj->special = 0;
     PuzzleActivated = true;
diff --git a/src/hexen/p_spec.c b/src/hexen/p_spec.c
index f9d69367..e2069fb9 100644
--- a/src/hexen/p_spec.c
+++ b/src/hexen/p_spec.c
@@ -498,6 +498,9 @@ Events are operations triggered by using, crossing, or shooting special lines, o
 //
 // P_ExecuteLineSpecial
 //
+// Invoked when crossing a linedef. The args[] array should be at least
+// 5 elements in length.
+//
 //============================================================================
 
 boolean P_ExecuteLineSpecial(int special, byte * args, line_t * line,
@@ -842,6 +845,7 @@ boolean P_ExecuteLineSpecial(int special, byte * args, line_t * line,
 boolean P_ActivateLine(line_t * line, mobj_t * mo, int side,
                        int activationType)
 {
+    byte args[5];
     int lineActivation;
     boolean repeat;
     boolean buttonSuccess;
@@ -863,8 +867,14 @@ boolean P_ActivateLine(line_t * line, mobj_t * mo, int side,
     repeat = line->flags & ML_REPEAT_SPECIAL;
     buttonSuccess = false;
 
-    buttonSuccess = P_ExecuteLineSpecial(line->special, &line->arg1, line,
-                                         side, mo);
+    // Construct args[] array to contain the arguments from the line, as we
+    // cannot rely on struct field ordering and layout.
+    args[0] = line->arg1;
+    args[1] = line->arg2;
+    args[2] = line->arg3;
+    args[3] = line->arg4;
+    args[4] = line->arg5;
+    buttonSuccess = P_ExecuteLineSpecial(line->special, args, line, side, mo);
     if (!repeat && buttonSuccess)
     {                           // clear the special on non-retriggerable lines
         line->special = 0;
-- 
cgit v1.2.3