Fix demand-loaded building artwork ownership

code/animtype.cpp

@@ -64,6 +64,16 @@
 #include "warhead.h"
 
 
+/// <summary>
+/// Releases shape data that this type loaded through the file layer.
+/// </summary>
+static void Free_Demand_Loaded_Shape(void const *& data)
+{
+	delete [] (char *)data;
+	data = NULL;
+}
+
+
 /***********************************************************************************************
  * AnimTypeClass::AnimTypeClass -- Constructor for animation types.                            *
  *                                                                                             *
@@ -172,8 +182,7 @@ AnimTypeClass::AnimTypeClass(char const *ininame) :
 AnimTypeClass::~AnimTypeClass(void)
 {
 	if (IsDemandLoad && ImageData) {
-		delete [] (char*) ImageData;
-		ImageData = NULL;
+		Free_Demand_Loaded_Shape(ImageData);
 	}
 
 	AbstractTypePtrTracker.Delete(this);
@@ -219,8 +228,7 @@ void AnimTypeClass::Init(TheaterType theater)
 			} else {
 				if (anim->IsTheater || anim->IsNewTheater) {
 					if (anim->ImageData != NULL) {
-						delete [] (char*) anim->ImageData;
-						anim->ImageData = NULL;
+						Free_Demand_Loaded_Shape(anim->ImageData);
 					}
 				}
 			}
@@ -358,6 +366,14 @@ AnimType AnimTypeClass::From_Name(char const * name)
 /// <returns>bool; Was the animation type's data read?</returns>
 bool AnimTypeClass::Read_INI(CCINIClass const & ini)
 {
+	if (!ini.Section_Present(IniName)) {
+		return(false);
+	}
+
+	if (IsDemandLoad && ImageData != NULL) {
+		Free_Demand_Loaded_Shape(ImageData);
+	}
+
 	if (BASECLASS::Read_INI(ini)) {
 		if (!GraphicName.empty()) {
 			if (ImageData == NULL) {
@@ -407,6 +423,9 @@ bool AnimTypeClass::Read_INI(CCINIClass const & ini)
 
 		IsDemandLoad = ini.Get_Bool(Name(), "DemandLoad", IsDemandLoad);
 		IsFreeAfterPlaying = ini.Get_Bool(Name(), "FreeAfterPlaying", IsFreeAfterPlaying);
+		if (IsDemandLoad) {
+			ImageData = NULL;
+		}
 
 		Elasticity = ini.Get_Float(Name(), "Elasticity", Elasticity);
 		MaxXYVel = ini.Get_Float(Name(), "MaxXYVel", MaxXYVel);
@@ -472,9 +491,9 @@ void AnimTypeClass::Post_Load(void)
 	BASECLASS::Post_Load();
 
 	Fetch_Voxel_Image();
-	Fetch_Normal_Image();
 
 	if (!IsDemandLoad) {
+		Fetch_Normal_Image();
 		if (IsTheater) {
 			char fullname[_MAX_FNAME+_MAX_EXT];	// Fully constructed iconset name.
 			_makepath(fullname, NULL, NULL, Name(), Theaters[Scen->Theater].Suffix);
@@ -681,7 +700,6 @@ void AnimTypeClass::Free_Image(void)
 {
 	if (IsDemandLoad && ImageData != NULL && IsFreeAfterPlaying) {
 		DebugString("Freeing loaded image for %s\n", Full_Name());
-		delete [] (char*) ImageData;
-		ImageData = NULL;
+		Free_Demand_Loaded_Shape(ImageData);
 	}
 }

code/builtype.cpp

@@ -98,6 +98,15 @@ void const * BuildingTypeClass::WrenchShapes;
 
 BSurface * CloakingSurface;
 
+/// <summary>
+/// Releases shape data that this type loaded through the file layer.
+/// </summary>
+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 +375,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 +610,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 +623,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 +638,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 +1134,17 @@ bool BuildingTypeClass::Read_INI(CCINIClass const & ini)
 {
 	char buffer[128];
 
+	if (!ini.Section_Present(IniName)) {
+		return(false);
+	}
+
+	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 +1281,12 @@ 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;
+		}
+		if (IsDemandLoadBuildup) {
+			BuildupData = NULL;
+		}
 
 		OccupyList = OccupyLists[Size];
 		ExitList = ExitLists[Size];
@@ -1722,7 +1742,11 @@ void BuildingTypeClass::Post_Load(void)
 	BASECLASS::Post_Load();
 
 	Fetch_Building_Voxel_Image();
-	Fetch_Normal_Image();
+	if (IsDemandLoad) {
+		ImageData = NULL;
+	} else {
+		Fetch_Normal_Image();
+	}
 
 	ToTile = NULL;
 	OccupyList = OccupyLists[Size];
@@ -2047,10 +2071,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);
 		}
 	}
 }

code/overtype.cpp

@@ -77,6 +77,16 @@
 #include "tracker.h"
 
 
+/// <summary>
+/// Releases shape data that this type loaded through the file layer.
+/// </summary>
+static void Free_Demand_Loaded_Shape(void const *& data)
+{
+	delete [] (char *)data;
+	data = NULL;
+}
+
+
 /***********************************************************************************************
  * OverlayTypeClass::OverlayTypeClass -- Constructor for overlay type objects.                 *
  *                                                                                             *
@@ -133,8 +143,7 @@ OverlayTypeClass::OverlayTypeClass(char const * ininame) :
 OverlayTypeClass::~OverlayTypeClass(void)
 {
 	if (DemandLoad && ImageData != NULL) {
-		delete (ShapeSet *)ImageData;
-		ImageData = NULL;
+		Free_Demand_Loaded_Shape(ImageData);
 	}
 	Detach_This_From_All(this, true);
 	OverlayTypes.Delete(this);
@@ -301,8 +310,7 @@ void OverlayTypeClass::Init(TheaterType theater)
 		} else {
 			if (overlay.IsTheater || overlay.IsNewTheater) {
 				if (overlay.ImageData != NULL) {
-					delete [] (char*) overlay.ImageData;
-					overlay.ImageData = NULL;
+					Free_Demand_Loaded_Shape(overlay.ImageData);
 				}
 			}
 		}
@@ -321,6 +329,14 @@ bool OverlayTypeClass::Read_INI(CCINIClass const & ini)
 {
 	char fullname[_MAX_FNAME+_MAX_EXT];
 
+	if (!ini.Section_Present(IniName)) {
+		return(false);
+	}
+
+	if (DemandLoad && ImageData != NULL) {
+		Free_Demand_Loaded_Shape(ImageData);
+	}
+
 	if (BASECLASS::Read_INI(ini)) {
 		Land = ini.Get_LandType(IniName, "Land", Land);
 		DamagePoints = ini.Get_Int(IniName, "Strength", DamagePoints);
@@ -336,6 +352,9 @@ bool OverlayTypeClass::Read_INI(CCINIClass const & ini)
 
 		DamageLevels = ArtINI.Get_Int(GraphicName, "DamageLevels", DamageLevels);
 		DemandLoad = ArtINI.Get_Bool(GraphicName, "DemandLoad", DemandLoad);
+		if (DemandLoad) {
+			ImageData = NULL;
+		}
 
 		if (IsTiberium) {
 			Armor = ARMOR_WOOD;
@@ -417,9 +436,9 @@ void OverlayTypeClass::Post_Load(void)
 	BASECLASS::Post_Load();
 
 	Fetch_Voxel_Image();
-	Fetch_Normal_Image();
 
 	if (!DemandLoad) {
+		Fetch_Normal_Image();
 		char fullname[_MAX_FNAME+_MAX_EXT];
 		if (IsTheater) {
 			_makepath(fullname, NULL, NULL, GraphicName, Theaters[Scen->Theater].Suffix);
@@ -546,10 +565,9 @@ void const * OverlayTypeClass::Get_Image_Data(void) const
 	DebugString("Demand loading image for %s\n", (char const *)GivenName);
 	if (IsTheater) {
 		_makepath(fullname, NULL, NULL, GraphicName, Theaters[Scen->Theater].Suffix);
-
 	} else {
+		_makepath(fullname, NULL, NULL, GraphicName, ".SHP");
 		if (IsNewTheater) {
-			_makepath(fullname, NULL, NULL, GraphicName, ".SHP");
 			_this->Theater_Naming_Convention( fullname, Scen->Theater);
 		}
 	}

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

@@ -0,0 +1,39 @@
+---
+title: Keep demand-loaded artwork under its owner's lifetime
+category: fix
+release: 0.2.0
+targets:
+- type: key
+  id: DemandLoad
+  scope: buildingtype
+  effect: changed
+- type: key
+  id: DemandLoad
+  scope: animtype
+  effect: changed
+- type: key
+  id: DemandLoad
+  scope: overlaytype
+  effect: changed
+- type: key
+  id: DemandLoadBuildup
+  effect: changed
+- type: key
+  id: FreeBuildup
+  effect: changed
+credit: [Krisztiaan, ZivDero]
+---
+
+A structure with `DemandLoad=yes` now detaches archive-owned art after rules or save loading,
+then loads and releases its own copy on demand. Structure and construction shapes are also
+released as the byte arrays the file loader allocated. The previous code could free shared
+archive memory or use a mismatched scalar release, corrupting the heap during theater setup,
+construction-art cleanup or shutdown.
+
+`FreeBuildup=yes` now releases construction art only with `DemandLoadBuildup=yes`. Used alone,
+it leaves archive art attached, so later structures retain construction, deconstruction,
+sellability and technician conversion for nominal crew on destruction.
+
+Demand-loaded animations and overlays likewise detach archive shapes after settings or save
+loading and release only their on-demand copies. Ordinary overlays now build the deferred
+`.SHP` filename from their Image ID instead of an uninitialized buffer.

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.
+Direct pointers belong to the archive, remain valid only while it is cached and mounted, and must not be freed by their users. Demand-loaded structure, animation, overlay and construction shapes release only their file-layer copies.
 
 :::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--animtype.md

@@ -8,12 +8,8 @@ when_omitted:
   value: "no"
 ---
 
-The flag is read after the animation's shape has already been fetched from the archives under its [Image ID](/keys/image/#scope-animtype), so an animation whose artwork is present is holding it before the setting is consulted and nothing is deferred. What the flag then skips is the later work: the shape is not fetched again when the theater is set up, and the theater's own copy is not fetched again when a saved game is restored — though the restore does re-attach the archive's copy, which puts the release below back in reach.
+The flag detaches the archive shape already found under the animation's [Image ID](/keys/image/#scope-animtype), both after settings and save loading. The first draw loads a private copy and fills any unset frame count or loop end.
 
-An animation that reaches a draw with no shape reads one from disk at that moment, and the frame count and loop end the type left unset are taken from it then rather than at load time. The name built for that read is the Image ID if the animation has one and the AnimType ID otherwise, with a `.SHP` extension; a [`Theater=yes`](/keys/theater/#scope-animtype) animation instead uses the AnimType ID with the theater's own extension, dropping the Image ID, and a [`NewTheater=yes`](/keys/newtheater/#scope-animtype) one has the built name rewritten for the theater.
+That read uses the Image ID, or the AnimType ID when none is set, with a `.SHP` extension. [`Theater=yes`](/keys/theater/#scope-animtype) instead uses the AnimType ID and theater extension; [`NewTheater=yes`](/keys/newtheater/#scope-animtype) rewrites the ordinary name for the theater.
 
-On a `Theater=yes` or `NewTheater=yes` animation the shape the type is holding is released as the theater is set up rather than replaced. It is released with the type as well, and on a [`FreeAfterPlaying=yes`](/keys/freeafterplaying/) animation as soon as the animation finishes playing — the only combination that gives artwork back during a match.
-
-:::danger[The release hands back memory the type may never have allocated]
-Both releases run on whatever the shape pointer holds. Artwork the animation fetched for itself is a block it may give back; the pointer left by the earlier archive fetch is not, and handing that back corrupts the heap: the game may fault there or at a later, unrelated allocation.
-:::
+The private copy is released with the type, on theater changes for theater-aware animations, and after playback with [`FreeAfterPlaying=yes`](/keys/freeafterplaying/) — the only in-match release.

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 found in the archives under its [Image ID](/keys/image/) before this flag is read. The flag detaches that archive pointer, records the theater-resolved [main-shape basename](/keys/image/#scope-buildingtype), and leaves the type empty until its first draw loads a private copy.
 
-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 copy is released when the type's rules are reread, the type is destroyed, or theater setup revisits a `Theater=yes` or `NewTheater=yes` type. Unused types allocate nothing; a missing shape is not drawn.
 
 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/demandload--overlaytype.md

@@ -8,12 +8,8 @@ when_omitted:
   value: "no"
 ---
 
-The flag is read after the overlay's shape has already been fetched from the archives under its [Image ID](/keys/image/), so an overlay whose artwork is present is holding it before the setting is consulted. What the flag then skips is the later work: the shape is not fetched again as the theater is set up, and the theater-named copy is not fetched again when a saved game is restored — though the restore does re-attach the archive's copy, which puts the release below back in reach. An overlay that reaches a draw with no shape reads one from disk at that moment and holds it for the rest of the session.
+The flag detaches the archive shape already found under the overlay's [Image ID](/keys/image/), both after settings and save loading. The first draw loads a private copy.
 
-:::caution[Only a theater-aware overlay can be demand loaded]
-The deferred read builds its filename only for a [`Theater=yes`](/keys/theater/) or [`NewTheater=yes`](/keys/newtheater/) overlay. On any other overlay no name is built at all: the read is made against a name buffer nothing ever wrote, and the type is left with no artwork of its own to draw.
-:::
+An ordinary overlay loads its Image ID with a `.SHP` extension; [`Theater=yes`](/keys/theater/) uses the theater extension, while [`NewTheater=yes`](/keys/newtheater/) rewrites the ordinary name for the theater.
 
-:::danger[The release hands back memory the type may never have allocated]
-The type releases whatever its shape pointer holds when it is destroyed, and again as the theater is set up if it is [`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.
-:::
+The copy is released with the type and, for theater-aware overlays, on theater changes.

manual/content/keys/demandloadbuildup.md

@@ -11,4 +11,4 @@ By default the file [`Buildup=`](/keys/buildup/) names is fetched from the archi
 
 The deferred read builds the filename with a `.SHP` extension and the structure-art theater rewrite whatever the type's own theater settings say, so a `Theater=yes` structure that defers its construction artwork looks for the plain file rather than the theater-suffixed one. It gives the step count and the rate their ordinary treatment — half the frames in the file, divided into [`BuildupTime`](/keys/builduptime/) — where the theater fetch does not.
 
-This is also the setting that makes [`FreeBuildup=yes`](/keys/freebuildup/) safe to use, since only artwork read this way is a block the type may give back.
+Only artwork read this way is released by [`FreeBuildup=yes`](/keys/freebuildup/). Without `DemandLoadBuildup=yes`, `FreeBuildup=yes` has no effect.