From 3132bbb077c47ec02692d3dd987dde0a834d4b9f Mon Sep 17 00:00:00 2001
From: Kalle Olavi Niemitalo <kon@iki.fi>
Date: Thu, 21 Jan 2010 21:02:13 +0200
Subject: [PATCH 2/2] Fix buffer overflow in unit->Data.Move.Path[]

When saving a game, save unit->Data.Move only if the current order
seems to use that.  In particular, don't save it when the action
of the current order is UnitActionDie (which doesn't use any data),
nor when the unit hasn't yet started executing the current order.
This fix is incomplete in that it never saves unit->Data.Move for
UnitActionResource, even though that does use it for some values
of unit->SubAction; but this is not a new bug.

When loading a game, raise an error if the path data is too long to
fit in unit->Data.Move.Path[].  This avoids memory corruption that
could have security implications.  However, if the current action is
UnitActionDie, which wouldn't use unit->Data.Move in any case, then
ignore any path data so that games saved before this patch can still
be loaded.
---
 engine/unit/script_unit.cpp |   15 +++++++++++++++
 engine/unit/unit_save.cpp   |   29 ++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 1 deletions(-)

diff --git a/engine/unit/script_unit.cpp b/engine/unit/script_unit.cpp
index c0e39cc..dcaeb79 100644
--- a/engine/unit/script_unit.cpp
+++ b/engine/unit/script_unit.cpp
@@ -384,6 +384,17 @@ static void CclParseMove(lua_State *l, CUnit *unit)
 	int args;
 	int j;
 
+	// Bos Wars 2.5 used to save unit->Data.Move as "data-move"
+	// even for UnitActionDie, which doesn't actually use
+	// unit->Data.Move at all and leaves it uninitialized.
+	// This could cause out-of-range values in save files.
+	// To work around the bug and allow loading such files,
+	// ignore "data-move" when the unit is already dying.
+	if (!unit->Orders.empty()
+	    && unit->Orders[0]->Action == UnitActionDie) {
+		return;
+	}
+
 	if (!lua_istable(l, -1)) {
 		LuaError(l, "incorrect argument");
 	}
@@ -405,6 +416,10 @@ static void CclParseMove(lua_State *l, CUnit *unit)
 				LuaError(l, "incorrect argument");
 			}
 			subargs = lua_objlen(l, -1);
+			if (subargs > MAX_PATH_LENGTH) {
+				// unit->Data.Move.Path[] would overflow.
+				LuaError(l, "path data is too long");
+			}
 			for (k = 0; k < subargs; ++k) {
 				lua_rawgeti(l, -1, k + 1);
 				unit->Data.Move.Path[k] = LuaToNumber(l, -1);
diff --git a/engine/unit/unit_save.cpp b/engine/unit/unit_save.cpp
index e86ac6b..49291e4 100644
--- a/engine/unit/unit_save.cpp
+++ b/engine/unit/unit_save.cpp
@@ -187,8 +187,19 @@ static void SaveOrderData(const CUnit *unit, CFile *file)
 {
 	int i;
 
+	// If the unit has not yet begun executing this order, then
+	// unit->Data can still contain data from the previous order.
+	// Do not attempt to save the data in that case, because
+	// integers in it may be out of range, and pointers in it can
+	// point to invalid addresses.
+	if (unit->SubAction == 0)
+		return;
+	
 	switch (unit->Orders[0]->Action) {
+		case UnitActionNone:
 		case UnitActionStill:
+		case UnitActionStandGround:
+		case UnitActionDie:
 			break;
 		case UnitActionBuilt:
 			{
@@ -221,6 +232,9 @@ static void SaveOrderData(const CUnit *unit, CFile *file)
 			file->printf("}");
 			break;
 		case UnitActionResource:
+			/// @bug Should save unit->Data.Move instead,
+			/// if unit->SubAction == SUB_MOVE_TO_RESOURCE.
+			/// However, that macro is not visible here.
 			file->printf(",\n  \"data-harvest\", {");
 			file->printf("\"current-production\", {");
 			for (i = 0; i < MaxCosts; ++i) {
@@ -228,12 +242,23 @@ static void SaveOrderData(const CUnit *unit, CFile *file)
 			}
 			file->printf("}}");
 			break;
-		default:
+		case UnitActionFollow:
+		case UnitActionMove:
+		case UnitActionAttack:
+		case UnitActionAttackGround:
+		case UnitActionSpellCast:
+		case UnitActionBoard:
+		case UnitActionUnload:
+		case UnitActionPatrol:
+		case UnitActionBuild:
+		case UnitActionRepair:
 			file->printf(",\n  \"data-move\", {");
 			if (unit->Data.Move.Fast) {
 				file->printf("\"fast\", ");
 			}
 			if (unit->Data.Move.Length > 0) {
+				Assert(unit->Data.Move.Length <= MAX_PATH_LENGTH);
+
 				file->printf("\"path\", {");
 				for (i = 0; i < unit->Data.Move.Length; ++i) {
 					file->printf("%d, ", unit->Data.Move.Path[i]);
@@ -242,6 +267,8 @@ static void SaveOrderData(const CUnit *unit, CFile *file)
 			}
 			file->printf("}");
 			break;
+		default:
+			DebugPrint("Unknown action in order\n");
 	}
 }
 
-- 
1.6.6

