From f6537d0849f9596d4da170b883f1839358e8fbe0 Mon Sep 17 00:00:00 2001 From: Kalle Olavi Niemitalo <k...@iki.fi> Date: Thu, 21 Jan 2010 21:02:13 +0200 Subject: [PATCH] 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 | 163 ++++++++++++++++++++++++++++---------------
2 files changed, 123 insertions(+), 55 deletions(-)
diff --git a/engine/unit/script_unit.cpp b/engine/unit/script_unit.cpp
index af36a78..dac6a2b 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 = luaL_getn(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 3b6477c..3d8a99c 100644
--- a/engine/unit/unit_save.cpp
+++ b/engine/unit/unit_save.cpp
@@ -165,6 +165,113 @@ void SaveOrder(const COrder *order, CFile *file)
file->printf("}");
}
+/** Save the progress of the current order to a file.
+**
+** @param unit Unit pointer to be saved.
+** @param file Output file.
+**
+** Each unit must always have a current order, which is kept in
+** unit->Orders[0], and then possibly some other orders that it will
+** execute after the current order completes. When the unit begins
+** executing an order, it can save additional order-specific data to
+** some member of the unit->Data union. For example, the various
+** movement orders save a precalculated path in unit->Data.Move.
+** While the unit is executing the order, it can then read and update
+** this data.
+**
+** This function saves the appropriate member of unit->Data to the
+** specified file. unit->Orders[0]->Action controls which member that
+** is.
+*/
+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:
+ {
+ CConstructionFrame *cframe;
+ int frame;
+
+ cframe = unit->Type->Construction->Frames;
+ frame = 0;
+ while (cframe != unit->Data.Built.Frame) {
+ cframe = cframe->Next;
+ ++frame;
+ }
+ file->printf(",\n \"data-built\", {");
+
+ if (unit->Data.Built.Worker) {
+ file->printf("\"worker\", \"%s\", ",
+
UnitReference(unit->Data.Built.Worker).c_str());
+ }
+ file->printf("\"progress\", %d, \"frame\", %d,",
+ unit->Data.Built.Progress, frame);
+ if (unit->Data.Built.Cancel) {
+ file->printf(" \"cancel\",");
+ }
+ file->printf("}");
+ break;
+ }
+ case UnitActionTrain:
+ file->printf(",\n \"data-train\", {");
+ file->printf("\"ticks\", %d, ", unit->Data.Train.Ticks);
+ 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) {
+ file->printf("%s%d", (i ? ", " : ""),
unit->Data.Harvest.CurrentProduction[i]);
+ }
+ file->printf("}}");
+ break;
+ 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]);
+ }
+ file->printf("},");
+ }
+ file->printf("}");
+ break;
+ default:
+ DebugPrint("Unknown action in order\n");
+ }
+}
+
/**
** Save the state of a unit to file.
**
@@ -350,62 +457,8 @@ void SaveUnit(const CUnit *unit, CFile *file)
//
// Order data part
//
- switch (unit->Orders[0]->Action) {
- case UnitActionStill:
- break;
- case UnitActionBuilt:
- {
- CConstructionFrame *cframe;
- int frame;
- cframe = unit->Type->Construction->Frames;
- frame = 0;
- while (cframe != unit->Data.Built.Frame) {
- cframe = cframe->Next;
- ++frame;
- }
- file->printf(",\n \"data-built\", {");
-
- if (unit->Data.Built.Worker) {
- file->printf("\"worker\", \"%s\", ",
-
UnitReference(unit->Data.Built.Worker).c_str());
- }
- file->printf("\"progress\", %d, \"frame\", %d,",
- unit->Data.Built.Progress, frame);
- if (unit->Data.Built.Cancel) {
- file->printf(" \"cancel\",");
- }
- file->printf("}");
- break;
- }
- case UnitActionTrain:
- file->printf(",\n \"data-train\", {");
- file->printf("\"ticks\", %d, ", unit->Data.Train.Ticks);
- file->printf("}");
- break;
- case UnitActionResource:
- file->printf(",\n \"data-harvest\", {");
- file->printf("\"current-production\", {");
- for (i = 0; i < MaxCosts; ++i) {
- file->printf("%s%d", (i ? ", " : ""),
unit->Data.Harvest.CurrentProduction[i]);
- }
- file->printf("}}");
- break;
- default:
- file->printf(",\n \"data-move\", {");
- if (unit->Data.Move.Fast) {
- file->printf("\"fast\", ");
- }
- if (unit->Data.Move.Length > 0) {
- file->printf("\"path\", {");
- for (i = 0; i < unit->Data.Move.Length; ++i) {
- file->printf("%d, ",
unit->Data.Move.Path[i]);
- }
- file->printf("},");
- }
- file->printf("}");
- break;
- }
+ SaveOrderData(unit, file);
if (unit->Goal) {
file->printf(",\n \"goal\", %d", UnitNumber(unit->Goal));
--
1.6.6
pgp5SuxXa1Sb9.pgp
Description: PGP signature
