cpp/openttd-patchpack/source Changeset - r25365:e85b6eb3c9d9

Changeset - r25365:e85b6eb3c9d9

Parent rev.

Child rev.

[Not reviewed]

master

0 4 0

Peter Nelson - 4 years ago 2021-05-01 17:07:47
peter1138@openttd.org

Codechange: Use std::vector for NewGRF station platform layouts.

This avoids the need to custom memory management and additional members.

This also resolves use-after-free if modifying copied layouts, so presumably nobody has ever done that.

4 files changed with 44 insertions and 77 deletions:

src/newgrf.cpp

src/newgrf_station.h

src/station_cmd.cpp

src/waypoint_cmd.cpp

0 comments (0 inline, 0 general)

src/newgrf.cpp

➞

Show inline comments

@@ @@ -218,6 +218,19 @@ protected: @@
 public:
 	ByteReader(byte *data, byte *end) : data(data), end(end) { }
 	inline byte *ReadBytes(size_t size)
+	{
 		if (data + size >= end) {
 			/* Put data at the end, as would happen if every byte had been individually read. */
 			data = end;
 			throw OTTDByteReaderSignal();
+		}
 		byte *ret = data;
 		data += size;
 		return ret;
+	}
 	inline byte ReadByte()
+	{
 		if (data < end) return *(data)++;
@@ @@ -1883,7 +1896,7 @@ static ChangeInfoResult StationChangeInf @@
 				StationSpec **spec = &_cur.grffile->stations[stid + i];
 				/* Property 0x08 is special; it is where the station is allocated */
-				if (*spec == nullptr) *spec = CallocT<StationSpec>(1);
+				if (*spec == nullptr) *spec = new StationSpec();
 				/* Swap classid because we read it in BE meaning WAYP or DFLT */
 				uint32 classid = buf->ReadDWord();
@@ @@ -1966,54 +1979,17 @@ static ChangeInfoResult StationChangeInf @@
 				break;
 			case 0x0E: // Define custom layout
 				statspec->copied_layouts = false;
 				while (buf->HasData()) {
 					byte length = buf->ReadByte();
 					byte number = buf->ReadByte();
 					StationLayout layout;
 					uint l, p;
 					if (length == 0 || number == 0) break;
 					if (length > statspec->lengths) {
 						byte diff_length = length - statspec->lengths;
 						statspec->platforms = ReallocT(statspec->platforms, length);
 						memset(statspec->platforms + statspec->lengths, 0, diff_length);
 						statspec->layouts = ReallocT(statspec->layouts, length);
 						memset(statspec->layouts + statspec->lengths, 0, diff_length * sizeof(*statspec->layouts));
 						statspec->lengths = length;
+					}
 					l = length - 1; // index is zero-based
 					if (number > statspec->platforms[l]) {
 						statspec->layouts[l] = ReallocT(statspec->layouts[l], number);
 						/* We expect nullptr being 0 here, but C99 guarantees that. */
 						memset(statspec->layouts[l] + statspec->platforms[l], 0,
 						       (number - statspec->platforms[l]) * sizeof(**statspec->layouts));
 						statspec->platforms[l] = number;
+					}
 					p = 0;
 					layout = MallocT<byte>(length * number);
 					try {
 						for (l = 0; l < length; l++) {
 							for (p = 0; p < number; p++) {
 								layout[l * number + p] = buf->ReadByte();
+							}
+						}
 					} catch (...) {
 						free(layout);
 						throw;
+					}
 					l--;
 					p--;
 					free(statspec->layouts[l][p]);
 					statspec->layouts[l][p] = layout;
 					if (statspec->layouts.size() < length) statspec->layouts.resize(length);
 					if (statspec->layouts[length - 1].size() < number) statspec->layouts[length - 1].resize(number);
 					const byte *layout = buf->ReadBytes(length * number);
 					statspec->layouts[length - 1][number - 1].assign(layout, layout + length * number);
+				}
 				break;
@@ @@ -2026,10 +2002,7 @@ static ChangeInfoResult StationChangeInf @@
 					continue;
+				}
 				statspec->lengths   = srcstatspec->lengths;
 				statspec->platforms = srcstatspec->platforms;
 				statspec->layouts   = srcstatspec->layouts;
 				statspec->copied_layouts = true;
 				statspec->layouts = srcstatspec->layouts;
 				break;
+			}
@@ @@ -8399,20 +8372,8 @@ static void ResetCustomStations() @@
 			delete[] statspec->renderdata;
 			/* Release platforms and layouts */
 			if (!statspec->copied_layouts) {
 				for (uint l = 0; l < statspec->lengths; l++) {
 					for (uint p = 0; p < statspec->platforms[l]; p++) {
 						free(statspec->layouts[l][p]);
+					}
 					free(statspec->layouts[l]);
+				}
 				free(statspec->layouts);
 				free(statspec->platforms);
+			}
 			/* Release this station */
-			free(statspec);
+			delete statspec;
+		}
 		/* Free and reset the station data */

src/newgrf_station.h

➞

Show inline comments

@@ @@ -109,12 +109,13 @@ enum StationRandomTrigger { @@
 	SRT_PATH_RESERVATION, ///< Trigger platform when train reserves path.
 };
 /* Station layout for given dimensions - it is a two-dimensional array
  * where index is computed as (x * platforms) + platform. */
 typedef byte *StationLayout;
 /** Station specification. */
 struct StationSpec {
 	StationSpec() : cls_id(STAT_CLASS_DFLT), name(0),
 		disallowed_platforms(0), disallowed_lengths(0), tiles(0),
 		renderdata(nullptr), cargo_threshold(0), cargo_triggers(0),
 		callback_mask(0), flags(0), pylons(0), wires(0), blocked(0),
 		animation({0, 0, 0, 0}) {}
 	/**
 	 * Properties related the the grf file.
 	 * NUM_CARGO real cargo plus three pseudo cargo sprite groups.
@@ @@ -165,10 +166,15 @@ struct StationSpec { @@
 	AnimationInfo animation;
 	byte lengths;
 	byte *platforms;
 	StationLayout **layouts;
 	bool copied_layouts;
 	/**
 	 * Custom platform layouts.
 	 * This is a 2D array containing an array of tiles.
 	 * 1st layer is platform lengths.
 	 * 2nd layer is tracks (width).
 	 * These can be sparsely populated, and the upper limit is not defined but
 	 * limited to 255.
 	 */
 	std::vector<std::vector<std::vector<byte>>> layouts;
 };
 /** Struct containing information relating to station classes. */

src/station_cmd.cpp

➞

Show inline comments

@@ @@ -1109,13 +1109,13 @@ static inline byte *CreateMulti(byte *la @@
  * @param plat_len  The length of the platforms.
  * @param statspec  The specification of the station to (possibly) get the layout from.
  */
-void GetStationLayout(byte *layout, int numtracks, int plat_len, const StationSpec *statspec)
+void GetStationLayout(byte *layout, uint numtracks, uint plat_len, const StationSpec *statspec)
+{
 	if (statspec != nullptr && statspec->lengths >= plat_len &&
 			statspec->platforms[plat_len - 1] >= numtracks &&
 			statspec->layouts[plat_len - 1][numtracks - 1]) {
 	if (statspec != nullptr && statspec->layouts.size() >= plat_len &&
 			statspec->layouts[plat_len - 1].size() >= numtracks &&
 			!statspec->layouts[plat_len - 1][numtracks - 1].empty()) {
 		/* Custom layout defined, follow it. */
 		memcpy(layout, statspec->layouts[plat_len - 1][numtracks - 1],
+		memcpy(layout, statspec->layouts[plat_len - 1][numtracks - 1].data(),
 			plat_len * numtracks);
 		return;
+	}
@@ @@ -1124,9 +1124,9 @@ void GetStationLayout(byte *layout, int @@
 		CreateSingle(layout, numtracks);
 	} else {
 		if (numtracks & 1) layout = CreateSingle(layout, plat_len);
 		numtracks >>= 1;
 		while (--numtracks >= 0) {
 		int n = numtracks >> 1;
 		while (--n >= 0) {
 			layout = CreateMulti(layout, plat_len, 4);
 			layout = CreateMulti(layout, plat_len, 6);
+		}

src/waypoint_cmd.cpp

➞

Show inline comments

@@ @@ -153,7 +153,7 @@ static CommandCost IsValidTileForWaypoin @@
 	return CommandCost();
+}
-extern void GetStationLayout(byte *layout, int numtracks, int plat_len, const StationSpec *statspec);
+extern void GetStationLayout(byte *layout, uint numtracks, uint plat_len, const StationSpec *statspec);
 extern CommandCost FindJoiningWaypoint(StationID existing_station, StationID station_to_join, bool adjacent, TileArea ta, Waypoint **wp);
 extern CommandCost CanExpandRailStation(const BaseStation *st, TileArea &new_ta, Axis axis);

0 comments (0 inline, 0 general)