Skip to content

Commit

Permalink
Fix a bunch of undefined behaviour detected by the static analyzer in…
Browse files Browse the repository at this point in the history 
… the saved game format.
  • Loading branch information
@CelticMinstrel
CelticMinstrel committed Aug 10, 2024
1 parent 4391d76 commit a52ce51
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 61 deletions.
25 changes: 14 additions & 11 deletions src/fileio/tagfile.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class cTagFile_Tag {
template<typename T>
typename std::enable_if<!detail::is_container<T>::value, size_t>::type
extract(size_t i, T& to) const {
if(i >= values.size()) return 1;
if(i >= values.size()) return 0;
// This bizarre construct simply allows us to not include <sstream> here
using stream = typename std::conditional<std::is_same<T, T>::value, std::istringstream, void>::type;
stream file(values[i]);
Expand Down Expand Up @@ -229,14 +229,15 @@ class cTagFile_TagExtractProxy {
if(tag == nullptr) return *this;
size_t j = i;
j += tag->extract(i, value);
if(i == j) return cTagFile_TagExtractProxy();
return cTagFile_TagExtractProxy(*tag, j);
}
template<typename T>
cTagFile_TagExtractProxy operator>>(T&& value) {
cTagFile_TagExtractProxy operator>>(const T& value) {
if(tag == nullptr) return *this;
size_t j = i;
T check = value;
j += tag->extract(i, value);
j += tag->extract(i, check);
if(check != value) {
return cTagFile_TagExtractProxy();
}
Expand Down Expand Up @@ -284,7 +285,7 @@ class cTagFile_TagList {
cTagFile_TagExtractProxy operator>>(T&& value) const {
if(i >= tags.size()) {
i = 0;
return cTagFile_TagExtractProxy() >> value;
return cTagFile_TagExtractProxy() >> std::forward<T>(value);
} else {
return cTagFile_TagExtractProxy(tags[i++]) >> std::forward<T>(value);
}
Expand Down Expand Up @@ -315,9 +316,10 @@ class cTagFile_TagList {
for(size_t n = 0; n < tags.size(); n++) {
size_t i = 0;
T value;
*this >> i >> value;
if(i >= values.size()) values.resize(i + 1, def);
values[i] = value;
if(*this >> i >> value) {
if(i >= values.size()) values.resize(i + 1, def);
values[i] = value;
}
}
}
template<typename Container>
Expand Down Expand Up @@ -387,11 +389,12 @@ class cTagFile_TagList {
for(size_t n = 0; n < tags.size(); n++) {
size_t x = 0, y = 0;
T value;
*this >> x >> y >> value;
if(x >= values.width() || y >= values.height()) {
values.resize(std::max(x + 1, values.width()), std::max(y + 1, values.height()), def);
if(*this >> x >> y >> value) {
if(x >= values.width() || y >= values.height()) {
values.resize(std::max(x + 1, values.width()), std::max(y + 1, values.height()), def);
}
values[x][y] = value;
}
values[x][y] = value;
}
}
template<typename T>
Expand Down
4 changes: 2 additions & 2 deletions src/scenario/monster.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,7 @@ void cMonster::readFrom(const cTagFile_Page& page) {
guard = page.contains("GUARD");
amorphous = page.contains("AMORPHOUS");
for(int i = 0; i < page["ATTACK"].size(); i++) {
size_t which_atk;
size_t which_atk = 0;
auto tmp = page["ATTACK"] >> which_atk;
if(which_atk > 0 && which_atk <= a.size()) {
which_atk--;
Expand All @@ -884,7 +884,7 @@ void cMonster::readFrom(const cTagFile_Page& page) {
}

eMonstAbil uAbility::readFrom(const cTagFile_Page& page) {
eMonstAbil key;
eMonstAbil key = eMonstAbil::NO_ABIL;
page["ABIL"] >> key;
eMonstAbilCat cat = getMonstAbilCategory(key);
switch(cat) {
Expand Down
75 changes: 41 additions & 34 deletions src/universe/party.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,7 @@ void cParty::readFrom(const cTagFile& file) {

wipe_sdfs();
for(size_t i = 0; i < page["SDF"].size(); i++) {
size_t x, y, val;
size_t x = sdx_max, y = sdy_max, val = 0;
page["SDF"] >> x >> y >> val;
if(x <= sdx_max && y <= sdy_max) {
stuff_done[x][y] = val;
Expand All @@ -916,11 +916,11 @@ void cParty::readFrom(const cTagFile& file) {
pointers.clear();
magic_ptrs.fill(0);
for(size_t n = 0; n < page["POINTER"].size(); n++) {
int i, j, k;
int i = 0, j = 0, k = 0;
auto tmp = page["POINTER"] >> i >> j;
if(i >= 10 && i < 100) {
magic_ptrs[i - 10] = j;
} else {
} else if(i >= 100 && i < 200) {
tmp >> k;
pointers[i] = std::make_pair(j, k);
}
Expand Down Expand Up @@ -949,7 +949,7 @@ void cParty::readFrom(const cTagFile& file) {
for(size_t n = 0; n < page["TOWNSAVE"].size(); n++) {
size_t i;
auto tmp = page["TOWNSAVE"] >> i;
if(i >= creature_save.size()) continue;
if(!tmp || i >= creature_save.size()) continue;
std::string hostile;
tmp >> creature_save[i].which_town >> hostile;
creature_save[i].hostile = hostile == "HOSTILE";
Expand Down Expand Up @@ -987,25 +987,29 @@ void cParty::readFrom(const cTagFile& file) {
}
} else if(page.getFirstKey() == "BOAT") {
size_t i;
page["BOAT"] >> i;
if(i >= boats.size()) boats.resize(i + 1);
boats[i].exists = true;
boats[i].readFrom(page);
if(page["BOAT"] >> i) {
if(i >= boats.size()) boats.resize(i + 1);
boats[i].exists = true;
boats[i].readFrom(page);
}
} else if(page.getFirstKey() == "HORSE") {
size_t i;
page["HORSE"] >> i;
if(i >= horses.size()) horses.resize(i + 1);
horses[i].exists = true;
horses[i].readFrom(page);
if(page["HORSE"] >> i) {
if(i >= horses.size()) horses.resize(i + 1);
horses[i].exists = true;
horses[i].readFrom(page);
}
} else if(page.getFirstKey() == "MAGICSTORE") {
size_t i, j;
page["MAGICSTORE"] >> i >> j;
magic_store_items[i][j].readFrom(page);
if(page["MAGICSTORE"] >> i >> j) {
magic_store_items[i][j].readFrom(page);
}
} else if(page.getFirstKey() == "ENCOUNTER") {
int i;
page["ENCOUNTER"] >> i;
out_c[i].exists = true;
out_c[i].readFrom(page);
if(page["ENCOUNTER"] >> i) {
out_c[i].exists = true;
out_c[i].readFrom(page);
}
} else if(page.getFirstKey() == "TIMER") {
size_t i, j;
cTimer timer;
Expand All @@ -1016,18 +1020,20 @@ void cParty::readFrom(const cTagFile& file) {
}
} else if(page.getFirstKey() == "CREATURE") {
size_t i, j;
page["CREATURE"] >> i >> j;
if(i < 0 || i >= creature_save.size()) continue;
creature_save[i].init(j);
creature_save[i][j].readFrom(page);
if(page["CREATURE"] >> i >> j) {
if(i < 0 || i >= creature_save.size()) continue;
creature_save[i].init(j);
creature_save[i][j].readFrom(page);
}
} else if(page.getFirstKey() == "STORED") {
size_t i, j;
page["STORED"] >> i >> j;
if(i >= 3) continue;
if(j >= stored_items[i].size()) {
stored_items[i].resize(j + 1);
if(page["STORED"] >> i >> j) {
if(i >= 3) continue;
if(j >= stored_items[i].size()) {
stored_items[i].resize(j + 1);
}
stored_items[i][j].readFrom(page);
}
stored_items[i][j].readFrom(page);
} else if(page.getFirstKey() == "SUMMON") {
page["SUMMON"] >> monst_i;
if(monst_i >= summons.size()) {
Expand Down Expand Up @@ -1062,15 +1068,16 @@ void cParty::readFrom(const cTagFile& file) {
} else if(page.getFirstKey() == "JOBBANK") {
size_t i;
job_bank_t bank;
page["JOBBANK"] >> i >> bank.anger;
if(i >= job_banks.size()) {
job_banks.resize(i + 1);
if(page["JOBBANK"] >> i >> bank.anger) {
if(i >= job_banks.size()) {
job_banks.resize(i + 1);
}
std::vector<int> jobs;
page["JOB"].extractSparse(jobs);
bank.inited = !jobs.empty();
std::copy_n(jobs.begin(), std::min(jobs.size(), bank.jobs.size()), bank.jobs.begin());
job_banks[i] = bank;
}
std::vector<int> jobs;
page["JOB"].extractSparse(jobs);
bank.inited = !jobs.empty();
std::copy_n(jobs.begin(), std::min(jobs.size(), bank.jobs.size()), bank.jobs.begin());
job_banks[i] = bank;
}
}
}
Expand Down
14 changes: 7 additions & 7 deletions src/universe/pc.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1300,23 +1300,23 @@ void cPlayer::readFrom(const cTagFile& file) {

equip.reset();
for(size_t n = 0; n < page["EQUIP"].size(); n++) {
size_t i;
size_t i = equip.size();
page["EQUIP"] >> i;
equip[i] = true;
if(i < equip.size()) equip[i] = true;
}

mage_spells.reset();
for(size_t n = 0; n < page["MAGE"].size(); n++) {
size_t i;
size_t i = mage_spells.size();
page["MAGE"] >> i;
mage_spells[i] = true;
if(i < mage_spells.size()) mage_spells[i] = true;
}

priest_spells.reset();
for(size_t n = 0; n < page["PRIEST"].size(); n++) {
size_t i;
size_t i = priest_spells.size();
page["PRIEST"] >> i;
priest_spells[i] = true;
if(i < priest_spells.size()) priest_spells[i] = true;
}

traits.clear();
Expand All @@ -1336,7 +1336,7 @@ void cPlayer::readFrom(const cTagFile& file) {
party->next_pc_id = max(unique_id + 1, party->next_pc_id);
}
} else if(page.getFirstKey() == "ITEM") {
size_t i;
size_t i = items.size();
page["ITEM"] >> i;
if(i >= items.size()) continue;
items[i].readFrom(page);
Expand Down
16 changes: 9 additions & 7 deletions src/universe/universe.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -910,15 +910,17 @@ void cCurTown::readFrom(const cTagFile& file){
}
} else if(page.getFirstKey() == "ITEM") {
size_t i;
page["ITEM"] >> i;
if(i >= items.size()) items.resize(i + 1);
items[i].readFrom(page);
if(page["ITEM"] >> i) {
if(i >= items.size()) items.resize(i + 1);
items[i].readFrom(page);
}
} else if(page.getFirstKey() == "CREATURE") {
size_t i;
page["CREATURE"] >> i;
monst.init(i);
monst[i].readFrom(page);
monst[i].active = true;
if(page["CREATURE"] >> i) {
monst.init(i);
monst[i].readFrom(page);
monst[i].active = true;
}
}
}
}
Expand Down

0 comments on commit a52ce51

Please sign in to comment.