From bd1a20f6eee6758f6a812af18fbe5b41b491b5c4 Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Sat, 1 May 2021 18:07:47 +0100 Subject: [PATCH] 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. --- src/newgrf.cpp | 79 +++++++++++--------------------------------- src/newgrf_station.h | 22 +++++++----- src/station_cmd.cpp | 14 ++++---- src/waypoint_cmd.cpp | 2 +- 4 files changed, 42 insertions(+), 75 deletions(-) diff --git a/src/newgrf.cpp b/src/newgrf.cpp index a9727d38de..9e8bbda190 100644 --- a/src/newgrf.cpp +++ b/src/newgrf.cpp @@ -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 StationChangeInfo(uint stid, int numinfo, int prop, Byte StationSpec **spec = &_cur.grffile->stations[stid + i]; /* Property 0x08 is special; it is where the station is allocated */ - if (*spec == nullptr) *spec = CallocT(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 StationChangeInfo(uint stid, int numinfo, int prop, Byte 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); + if (statspec->layouts.size() < length) statspec->layouts.resize(length); + if (statspec->layouts[length - 1].size() < number) statspec->layouts[length - 1].resize(number); - 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(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; + 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 StationChangeInfo(uint stid, int numinfo, int prop, Byte 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 */ diff --git a/src/newgrf_station.h b/src/newgrf_station.h index fac5d64ddd..4c4a5831be 100644 --- a/src/newgrf_station.h +++ b/src/newgrf_station.h @@ -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>> layouts; }; /** Struct containing information relating to station classes. */ diff --git a/src/station_cmd.cpp b/src/station_cmd.cpp index 482b954625..ac78064d49 100644 --- a/src/station_cmd.cpp +++ b/src/station_cmd.cpp @@ -1109,13 +1109,13 @@ static inline byte *CreateMulti(byte *layout, int n, byte b) * @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 numtracks, int plat_len, const StationSp CreateSingle(layout, numtracks); } else { if (numtracks & 1) layout = CreateSingle(layout, plat_len); - numtracks >>= 1; + int n = numtracks >> 1; - while (--numtracks >= 0) { + while (--n >= 0) { layout = CreateMulti(layout, plat_len, 4); layout = CreateMulti(layout, plat_len, 6); } diff --git a/src/waypoint_cmd.cpp b/src/waypoint_cmd.cpp index 01cdbc16e0..e8e9e69455 100644 --- a/src/waypoint_cmd.cpp +++ b/src/waypoint_cmd.cpp @@ -153,7 +153,7 @@ static CommandCost IsValidTileForWaypoint(TileIndex tile, Axis axis, StationID * 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);