Fix demand-loaded building artwork ownership

code/builtype.cpp

@@ -98,6 +98,12 @@ void const * BuildingTypeClass::WrenchShapes;
 
 BSurface * CloakingSurface;
 
+static void Free_Demand_Loaded_Shape(void const *& data)
+{
+	delete [] (char *)data;
+	data = NULL;
+}
+
 Cell const BuildingTypeClass::OccupyLists[BSIZE_COUNT][24] = {
 	/* BSIZE_11,	*/ { Cell(0,0),REFRESH_EOL },
 	/* BSIZE_21,	*/ { Cell(0,0),Cell(1,0),REFRESH_EOL },
@@ -366,12 +372,10 @@ BuildingTypeClass::BuildingTypeClass(char const * ininame) :
 BuildingTypeClass::~BuildingTypeClass(void)
 {
 	if (IsDemandLoad && ImageData != NULL) {
-		delete (ShapeSet *)ImageData;
-		ImageData = NULL;
+		Free_Demand_Loaded_Shape(ImageData);
 	}
 	if (IsDemandLoadBuildup && BuildupData != NULL) {
-		delete (ShapeSet *)BuildupData;
-		BuildupData = NULL;
+		Free_Demand_Loaded_Shape(BuildupData);
 	}
 	Detach_This_From_All(this, true);
 	BuildingTypes.Delete(this);
@@ -603,8 +607,7 @@ void BuildingTypeClass::Init(TheaterType theater)
 				classptr->ImageData = MFCD::Retrieve(fullname);
 			} else {
 				if (classptr->ImageData != NULL) {
-					delete (ShapeSet *)classptr->ImageData;
-					classptr->ImageData = NULL;
+					Free_Demand_Loaded_Shape(classptr->ImageData);
 				}
 			}
 
@@ -617,8 +620,7 @@ void BuildingTypeClass::Init(TheaterType theater)
 				classptr->BuildupData = MFCD::Retrieve(fullname);
 			} else {
 				if (classptr->BuildupData != NULL) {
-					delete (ShapeSet *)classptr->BuildupData;
-					classptr->BuildupData = NULL;
+					Free_Demand_Loaded_Shape(classptr->BuildupData);
 				}
 			}
 
@@ -633,14 +635,12 @@ void BuildingTypeClass::Init(TheaterType theater)
 		} else if (classptr->IsNewTheater) {
 			if (classptr->IsDemandLoad) {
 				if (classptr->ImageData != NULL) {
-					delete (ShapeSet *)classptr->ImageData;
-					classptr->ImageData = NULL;
+					Free_Demand_Loaded_Shape(classptr->ImageData);
 				}
 			}
 			if (classptr->IsDemandLoadBuildup) {
 				if (classptr->BuildupData != NULL) {
-					delete (ShapeSet *)classptr->BuildupData;
-					classptr->BuildupData = NULL;
+					Free_Demand_Loaded_Shape(classptr->BuildupData);
 				}
 			}
 			classptr->Fetch_Building_Normal_Image(theater);
@@ -1131,6 +1131,13 @@ bool BuildingTypeClass::Read_INI(CCINIClass const & ini)
 {
 	char buffer[128];
 
+	if (IsDemandLoad && ImageData != NULL) {
+		Free_Demand_Loaded_Shape(ImageData);
+	}
+	if (IsDemandLoadBuildup && BuildupData != NULL) {
+		Free_Demand_Loaded_Shape(BuildupData);
+	}
+
 	if (BASECLASS::Read_INI(ini)) {
 
 		HasSpotlight = ini.Get_Bool(Name(), "HasSpotlight", HasSpotlight);
@@ -1267,6 +1274,9 @@ bool BuildingTypeClass::Read_INI(CCINIClass const & ini)
 		IsDemandLoad = ArtINI.Get_Bool(Graphic_Name(), "DemandLoad", IsDemandLoad);
 		IsDemandLoadBuildup = ArtINI.Get_Bool(Graphic_Name(), "DemandLoadBuildup", IsDemandLoadBuildup);
 		IsFreeBuildup = ArtINI.Get_Bool(Graphic_Name(), "FreeBuildup", IsFreeBuildup);
+		if (IsDemandLoad) {
+			ImageData = NULL;
+		}
 
 		OccupyList = OccupyLists[Size];
 		ExitList = ExitLists[Size];
@@ -1722,7 +1732,9 @@ void BuildingTypeClass::Post_Load(void)
 	BASECLASS::Post_Load();
 
 	Fetch_Building_Voxel_Image();
-	Fetch_Normal_Image();
+	if (!IsDemandLoad) {
+		Fetch_Normal_Image();
+	}
 
 	ToTile = NULL;
 	OccupyList = OccupyLists[Size];
@@ -2047,10 +2059,9 @@ void const * BuildingTypeClass::Get_Buildup_Data(void) const
 /// </summary>
 void BuildingTypeClass::Free_Buildup_Data(void)
 {
-	if (IsFreeBuildup) {
+	if (IsFreeBuildup && IsDemandLoadBuildup) {
 		if (BuildupData != NULL) {
-			delete (ShapeSet *)BuildupData;
-			BuildupData = NULL;
+			Free_Demand_Loaded_Shape(BuildupData);
 		}
 	}
 }

manual/changes/demand-loaded-building-art-ownership.md

@@ -0,0 +1,32 @@
+---
+title: Keep demand-loaded structure artwork under its owner's lifetime
+category: fix
+release: 0.1.0
+targets:
+- type: key
+  id: DemandLoad
+  scope: buildingtype
+  effect: changed
+- type: key
+  id: DemandLoadBuildup
+  effect: changed
+- type: key
+  id: FreeBuildup
+  effect: changed
+credit: [Krisztiaan]
+---
+
+A structure type with `DemandLoad=yes` now detaches the archive-owned shape found before
+that setting is read. The type loads its own copy when the shape is first needed and
+releases only that copy when the theater changes or the type is destroyed. A theater-aware
+structure previously tried to release the archive's shared memory during theater setup,
+which could stop a skirmish before play began.
+
+Demand-loaded structure shapes and construction animations are now released as the byte
+arrays that the file loader allocated. Their former scalar release did not match that
+allocation and could corrupt the heap during theater changes, construction-art cleanup or
+shutdown.
+
+`FreeBuildup=yes` now releases construction artwork only when `DemandLoadBuildup=yes` gave
+the structure type its own copy. On its own the setting leaves archive-owned artwork in
+place instead of freeing shared memory and leaving later construction animations empty.

manual/content/formats/mix.md

@@ -32,7 +32,7 @@ Whether an archive is cached decides how its members can be reached:
 - A member of a cached archive can be handed out as a pointer straight into the memory the archive is already holding. Nothing is allocated for the member and nothing is copied. Shapes, fonts, palettes and sound samples are fetched this way, so those files have to live in an archive that was cached — a loose file, or a member of an archive that was mounted without being cached, is not found by that path at all.
 - Opening a member as a file works either way. From a cached archive the file object becomes a window onto that same memory and a read copies out of it; from an archive that is not cached the archive file itself is opened and every read is biased to the member's position within it.
 
-The memory belongs to the archive rather than to whatever asked for the member, so a pointer fetched the first way stays good only while that archive stays cached and mounted, and handing it back is not something its holder may do. [`FreeBuildup`](/keys/freebuildup/) describes what a type that hands one back costs the rest of the game.
+The memory belongs to the archive rather than to whatever asked for the member, so a pointer fetched the first way stays good only while that archive stays cached and mounted, and handing it back is not something its holder may do. [`FreeBuildup`](/keys/freebuildup/) releases only a separately loaded copy of construction artwork.
 
 :::danger[An archive too short to hold a header is mounted from uninitialized memory]
 The number of members and the size of the data block are taken from the first bytes of the file without testing that any bytes were read. An archive that cannot supply them — an empty file most obviously — is mounted with a member count and an index taken from whatever that memory last held. Allocating an index for an implausible count is not survivable; where the count is small enough to allocate, the archive joins the search carrying meaningless entries, and a request that matches one is handed an offset and a size that describe no file.

manual/content/keys/demandload--buildingtype.md

@@ -8,12 +8,8 @@ when_omitted:
   value: "no"
 ---
 
-A structure's shape is fetched from the archives twice while the rules are read: once under its [Image ID](/keys/image/), before this setting has been read at all, and once afterwards under the [main-shape basename](/keys/image/#scope-buildingtype). Only the second fetch is skipped. A structure whose two names agree — the ordinary case — is therefore already holding its artwork by the time the flag is consulted, and setting it changes nothing about when the shape is loaded. Deferral takes effect only where the first name resolves to no file.
+A structure's shape is first found in the archives under its [Image ID](/keys/image/), before this setting has been read. With the flag set, the structure type detaches that archive-owned shape without releasing it, records the [main-shape basename](/keys/image/#scope-buildingtype) for the current theater, and leaves its own shape empty.
 
-Where it does take effect, the shape is read from disk the first time something asks to draw a structure of the type, and is then held for the rest of the session. A type nothing ever draws allocates nothing, and a structure whose shape cannot be found is not drawn at all.
+The shape is read from disk the first time something asks to draw a structure of the type, and is held until the theater changes or the type is destroyed. A type nothing ever draws allocates nothing, and a structure whose shape cannot be found is not drawn at all.
 
 The construction animation is a separate setting, [`DemandLoadBuildup`](/keys/demandloadbuildup/). The deploying, door, under-door, bib and Z-shape overlay artwork is fetched with the rules whatever this is set to.
-
-:::danger[The release hands back memory the type may never have allocated]
-A type carrying this flag releases whatever its shape pointer holds when the type is destroyed, and again as the theater is set up if the type is also [`Theater=yes`](/keys/theater/) or [`NewTheater=yes`](/keys/newtheater/). Only a shape this flag actually deferred is a block the type allocated for itself; the pointer left by the earlier fetch belongs to the archive it was read from, and handing that back corrupts the heap: the game may fault there or at a later, unrelated allocation.
-:::

manual/content/keys/freebuildup.md

@@ -7,8 +7,6 @@ when_omitted:
   value: "no"
 ---
 
-With the flag set, the construction artwork is released at three moments: as each structure of the type is created and has been asked whether it may ever be sold, as a structure of the type finishes its buildup and opens, and as a structure of the type is taken off the map. Only [`DemandLoadBuildup=yes`](/keys/demandloadbuildup/) fetches it again afterwards.
+With this flag and [`DemandLoadBuildup=yes`](/keys/demandloadbuildup/) both set, the construction artwork is released at three moments: as each structure of the type is created and has been asked whether it may ever be sold, as a structure of the type finishes its buildup and opens, and as a structure of the type is taken off the map. The next structure that needs it loads it again.
 
-:::danger[On its own the release hands back memory the type never allocated]
-Without `DemandLoadBuildup=yes` the artwork was read straight out of the archive that holds it, and what is released is a pointer into that archive's own block. Handing it back corrupts the heap, and the game may fault there or at a later, unrelated allocation. Nothing reloads the artwork either, so every structure of the type is drawn as nothing while it builds, and every one after the first is marked unsellable as well. Set the two together or leave both off.
-:::
+Without `DemandLoadBuildup=yes`, this flag has no effect. The construction artwork remains attached to its archive and is not released or reloaded.