[Scummvm-git-logs] scummvm master -> 2345a0d3a6b2e9b809f59f6340ddc0efc8b89e75
mduggan
noreply at scummvm.org
Fri Mar 25 07:50:10 UTC 2022
This automated email contains information about 1 new commit which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .
Summary:
2345a0d3a6 ULTIMA8: Add error checking for treasure loader
Commit: 2345a0d3a6b2e9b809f59f6340ddc0efc8b89e75
https://github.com/scummvm/scummvm/commit/2345a0d3a6b2e9b809f59f6340ddc0efc8b89e75
Author: Matthew Duggan (mgithub at guarana.org)
Date: 2022-03-25T16:44:59+09:00
Commit Message:
ULTIMA8: Add error checking for treasure loader
This might help with bug #12182 where the comment suggests there is a problem
with loading the treasure data. To make sure it loads properly, added a lot
more error checking and error messages, and created unit tests for the same.
Changed paths:
A test/engines/ultima/ultima8/games/treasure_loader.h
engines/ultima/ultima8/games/treasure_loader.cpp
engines/ultima/ultima8/games/treasure_loader.h
engines/ultima/ultima8/world/actors/treasure_info.h
diff --git a/engines/ultima/ultima8/games/treasure_loader.cpp b/engines/ultima/ultima8/games/treasure_loader.cpp
index 3b305d1cb1e..3d9848d0aeb 100644
--- a/engines/ultima/ultima8/games/treasure_loader.cpp
+++ b/engines/ultima/ultima8/games/treasure_loader.cpp
@@ -41,22 +41,23 @@ void TreasureLoader::loadDefaults() {
// load default treasure types
lootkeyvals = config->listKeyValues("game", "treasure");
KeyMap::const_iterator defaultiter;
+
for (defaultiter = lootkeyvals.begin();
defaultiter != lootkeyvals.end(); ++defaultiter) {
TreasureInfo ti;
- bool ok = internalParse(defaultiter->_value, ti, true);
+ const istring &key = defaultiter->_key;
+ const Std::string &val = defaultiter->_value;
+ bool ok = internalParse(val, ti, true);
if (ok) {
- _defaultTreasure[defaultiter->_key] = ti;
+ _defaultTreasure[key] = ti;
} else {
- perr << "Failed to parse treasure type '" << defaultiter->_key
- << "': " << defaultiter->_value << Std::endl;
+ warning("Failed to parse treasure type '%s': %s", key.c_str(), val.c_str());
}
}
-
}
bool TreasureLoader::parse(const Std::string &desc,
- Std::vector<TreasureInfo> &treasure) {
+ Std::vector<TreasureInfo> &treasure) const {
treasure.clear();
Std::vector<Std::string> tr;
@@ -64,7 +65,6 @@ bool TreasureLoader::parse(const Std::string &desc,
TreasureInfo ti;
for (unsigned int i = 0; i < tr.size(); ++i) {
-// pout << "parse: item=" << tr[i] << Std::endl;
if (internalParse(tr[i], ti, false)) {
treasure.push_back(ti);
} else {
@@ -76,40 +76,55 @@ bool TreasureLoader::parse(const Std::string &desc,
}
bool TreasureLoader::internalParse(const Std::string &desc, TreasureInfo &ti,
- bool loadingDefault) {
- ti._special = "";
- ti._chance = 1;
- ti._map = 0;
- ti._shapes.clear();
- ti._frames.clear();
- ti._minCount = ti._maxCount = 1;
-
+ bool loadingDefault) const {
+ ti.clear();
bool loadedDefault = false;
Std::vector<Std::pair<Std::string, Std::string> > kv;
SplitStringKV(desc, ' ', kv);
for (unsigned int i = 0; i < kv.size(); ++i) {
- Std::string key = kv[i].first;
+ const Std::string &key = kv[i].first;
Std::string val = kv[i].second;
-// pout << "internalParse: key=" << key << " val=" << val << Std::endl;
if (key == "shape") {
- if (!parseUInt32Vector(val, ti._shapes))
+ if (!parseUInt32Vector(val, ti._shapes)) {
+ warning("Failed to parse treasure shape list '%s'", val.c_str());
return false;
+ }
+ // validate the shapes are > 0 and < max shape
+ for (unsigned int j = 0; j < ti._shapes.size(); j++) {
+ if (ti._shapes[j] <= 0 || ti._shapes[j] > 65535) {
+ warning("Invalid treasure shape in list '%s'", val.c_str());
+ return false;
+ }
+ }
} else if (key == "frame") {
- if (!parseUInt32Vector(val, ti._frames))
+ if (!parseUInt32Vector(val, ti._frames)) {
+ warning("Failed to parse treasure frame list '%s'", val.c_str());
return false;
+ }
+ // validate the frames are > 0 and < max frame
+ for (unsigned int j = 0; j < ti._frames.size(); j++) {
+ if (ti._frames[j] < 0 || ti._frames[j] > 65535) {
+ warning("Invalid treasure frame in list '%s'", val.c_str());
+ return false;
+ }
+ }
} else if (key == "count") {
if (!parseUIntRange(val, ti._minCount, ti._maxCount)) {
int x;
- if (!parseInt(val, x))
+ if (!parseInt(val, x) || x <= 0) {
+ warning("Invalid treasure count '%s'", val.c_str());
return false;
+ }
ti._minCount = ti._maxCount = x;
}
} else if (key == "chance") {
- if (!parseDouble(val, ti._chance))
+ if (!parseDouble(val, ti._chance) || ti._chance <= 0) {
+ warning("Invalid treasure chance '%s'", val.c_str());
return false;
+ }
} else if (key == "map") {
if (val.size() > 1 && val[0] == '!')
val[0] = '-'; // HACK: invert map for 'not this map'
@@ -128,17 +143,23 @@ bool TreasureLoader::internalParse(const Std::string &desc, TreasureInfo &ti,
return false;
loadedDefault = true;
} else if (key == "mult" && !loadingDefault) {
- if (!loadedDefault) return false;
+ if (!loadedDefault) {
+ warning("Need defaults before applying multiplier in treasure data '%s'", val.c_str());
+ return false;
+ }
unsigned int minmult, maxmult;
if (!parseUIntRange(val, minmult, maxmult)) {
int x;
- if (!parseInt(val, x))
+ if (!parseInt(val, x) || x <= 0) {
+ warning("Invalid treasure multiplier '%s'", val.c_str());
return false;
+ }
minmult = maxmult = x;
}
ti._minCount *= minmult;
ti._maxCount *= maxmult;
} else {
+ warning("Unknown key parsing treasure '%s'", key.c_str());
return false;
}
}
@@ -147,14 +168,16 @@ bool TreasureLoader::internalParse(const Std::string &desc, TreasureInfo &ti,
}
bool TreasureLoader::parseUInt32Vector(const Std::string &val_,
- Std::vector<uint32> &vec) {
+ Std::vector<uint32> &vec) const {
Std::string val = val_;
vec.clear();
- Std::string::size_type pos;
+ if (val.empty())
+ return false;
+
while (!val.empty()) {
- pos = val.find(',');
- Std::string item = val.substr(0, pos);
+ Std::string::size_type pos = val.find(',');
+ const Std::string item = val.substr(0, pos);
Std::string::size_type itempos = val.find('-');
if (itempos != Std::string::npos) {
@@ -165,7 +188,7 @@ bool TreasureLoader::parseUInt32Vector(const Std::string &val_,
vec.push_back(i);
} else {
int x;
- if (!parseInt(item, x))
+ if (!parseInt(item, x) || x < 0)
return false;
vec.push_back(x);
}
@@ -178,7 +201,7 @@ bool TreasureLoader::parseUInt32Vector(const Std::string &val_,
}
bool TreasureLoader::parseUIntRange(const Std::string &val,
- unsigned int &min, unsigned int &max) {
+ unsigned int &min, unsigned int &max) const {
Std::string::size_type pos = val.find('-');
if (pos == 0 || pos == Std::string::npos || pos + 1 >= val.size())
return false;
@@ -186,20 +209,24 @@ bool TreasureLoader::parseUIntRange(const Std::string &val,
bool ok = true;
ok &= parseInt(val.substr(0, pos), t1);
ok &= parseInt(val.substr(pos + 1), t2);
- if (ok) {
+ if (ok && t1 <= t2 && t1 >= 0 && t2 >= 0) {
min = t1;
max = t2;
}
return ok;
}
-bool TreasureLoader::parseDouble(const Std::string &val, double &d) {
+bool TreasureLoader::parseDouble(const Std::string &val, double &d) const {
+ if (val.empty())
+ return false;
// TODO: error checking
d = atof(val.c_str());
return true;
}
-bool TreasureLoader::parseInt(const Std::string &val, int &i) {
+bool TreasureLoader::parseInt(const Std::string &val, int &i) const {
+ if (val.empty())
+ return false;
// TODO: error checking
i = strtol(val.c_str(), 0, 0);
return true;
diff --git a/engines/ultima/ultima8/games/treasure_loader.h b/engines/ultima/ultima8/games/treasure_loader.h
index f9ffa8289c7..d077ca8d2ff 100644
--- a/engines/ultima/ultima8/games/treasure_loader.h
+++ b/engines/ultima/ultima8/games/treasure_loader.h
@@ -23,6 +23,7 @@
#define ULTIMA8_GAMES_TREASURELOADER_H
#include "ultima/ultima8/world/actors/treasure_info.h"
+#include "ultima/ultima8/misc/istring.h"
#include "ultima/shared/std/containers.h"
namespace Ultima {
@@ -39,17 +40,17 @@ public:
void loadDefaults();
//! parse treasure string into vector of TreasureInfo objects
- bool parse(const Std::string &, Std::vector<TreasureInfo> &treasure);
+ bool parse(const Std::string &, Std::vector<TreasureInfo> &treasure) const;
private:
TreasureMap _defaultTreasure;
- bool internalParse(const Std::string &desc, TreasureInfo &ti, bool loadingDefault);
+ bool internalParse(const Std::string &desc, TreasureInfo &ti, bool loadingDefault) const;
- bool parseUInt32Vector(const Std::string &val, Std::vector<uint32> &vec);
- bool parseUIntRange(const Std::string &val, unsigned int &min, unsigned int &max);
- bool parseDouble(const Std::string &val, double &d);
- bool parseInt(const Std::string &val, int &i);
+ bool parseUInt32Vector(const Std::string &val, Std::vector<uint32> &vec) const;
+ bool parseUIntRange(const Std::string &val, unsigned int &min, unsigned int &max) const;
+ bool parseDouble(const Std::string &val, double &d) const;
+ bool parseInt(const Std::string &val, int &i) const;
};
} // End of namespace Ultima8
diff --git a/engines/ultima/ultima8/world/actors/treasure_info.h b/engines/ultima/ultima8/world/actors/treasure_info.h
index 4f53d11bd66..2db781c17fa 100644
--- a/engines/ultima/ultima8/world/actors/treasure_info.h
+++ b/engines/ultima/ultima8/world/actors/treasure_info.h
@@ -37,6 +37,15 @@ struct TreasureInfo {
unsigned int _minCount, _maxCount;
TreasureInfo() : _chance(1), _map(0), _minCount(1), _maxCount(1) {}
+
+ void clear() {
+ _special.clear();
+ _chance = 1;
+ _map = 0;
+ _shapes.clear();
+ _frames.clear();
+ _minCount = _maxCount = 1;
+ }
};
} // End of namespace Ultima8
diff --git a/test/engines/ultima/ultima8/games/treasure_loader.h b/test/engines/ultima/ultima8/games/treasure_loader.h
new file mode 100644
index 00000000000..15546f7b5bc
--- /dev/null
+++ b/test/engines/ultima/ultima8/games/treasure_loader.h
@@ -0,0 +1,112 @@
+#include <cxxtest/TestSuite.h>
+#include "engines/ultima/ultima8/games/treasure_loader.h"
+
+/**
+ * Test suite for the functions in engines/ultima/ultima8/games/treasure_loader.h
+ *
+ * TODO: We should test type= values, but they are loaded from the config file
+ * so we need a way to add those.
+ *
+ * That would also allow testing "type", "special", and "mult" values which
+ * need defaults.
+ */
+class U8TreasureLoaderTestSuite : public CxxTest::TestSuite {
+ public:
+ U8TreasureLoaderTestSuite() {
+ }
+ Ultima::Ultima8::TreasureLoader loader;
+
+ /* Parse nothing -> should return nothing */
+ void test_parse_empty() {
+ Ultima::Std::vector<Ultima::Ultima8::TreasureInfo> t;
+ bool result = loader.parse("", t);
+ TS_ASSERT(result);
+ TS_ASSERT(t.empty());
+ }
+
+ /* Parse a single treasure type */
+ void test_parse_basic() {
+ Ultima::Std::vector<Ultima::Ultima8::TreasureInfo> t;
+
+ bool result = loader.parse("shape=123,456 frame=2,3 count=4-20 map=23 chance=0.234", t);
+ TS_ASSERT(result);
+
+ TS_ASSERT_EQUALS(t.size(), 1);
+ const Ultima::Ultima8::TreasureInfo ti = t[0];
+ TS_ASSERT_EQUALS(ti._shapes.size(), 2);
+ TS_ASSERT_EQUALS(ti._shapes[0], 123);
+ TS_ASSERT_EQUALS(ti._shapes[1], 456);
+ TS_ASSERT_EQUALS(ti._frames.size(), 2);
+ TS_ASSERT_EQUALS(ti._frames[0], 2);
+ TS_ASSERT_EQUALS(ti._frames[1], 3);
+ TS_ASSERT_EQUALS(ti._minCount, 4);
+ TS_ASSERT_EQUALS(ti._maxCount, 20);
+ TS_ASSERT_EQUALS(ti._special, "");
+ TS_ASSERT_EQUALS(ti._map, 23);
+ TS_ASSERT_EQUALS(ti._chance, 0.234);
+ }
+
+ /* Parse multiple treasure types */
+ void test_parse_multi() {
+ Ultima::Std::vector<Ultima::Ultima8::TreasureInfo> t;
+
+ bool result = loader.parse("shape=123;shape=456 frame=2-5;shape=888 map=-12", t);
+ TS_ASSERT(result);
+
+ TS_ASSERT_EQUALS(t.size(), 3);
+
+ TS_ASSERT_EQUALS(t[0]._shapes.size(), 1);
+ TS_ASSERT_EQUALS(t[0]._shapes[0], 123);
+ TS_ASSERT_EQUALS(t[0]._frames.size(), 0);
+ TS_ASSERT_EQUALS(t[0]._minCount, 1);
+ TS_ASSERT_EQUALS(t[0]._maxCount, 1);
+ TS_ASSERT_EQUALS(t[0]._special, "");
+ TS_ASSERT_EQUALS(t[0]._map, 0);
+ TS_ASSERT_EQUALS(t[0]._chance, 1);
+
+ TS_ASSERT_EQUALS(t[1]._shapes.size(), 1);
+ TS_ASSERT_EQUALS(t[1]._shapes[0], 456);
+ TS_ASSERT_EQUALS(t[1]._frames.size(), 4);
+ TS_ASSERT_EQUALS(t[1]._frames[0], 2);
+ TS_ASSERT_EQUALS(t[1]._frames[1], 3);
+ TS_ASSERT_EQUALS(t[1]._frames[2], 4);
+ TS_ASSERT_EQUALS(t[1]._frames[3], 5);
+
+ TS_ASSERT_EQUALS(t[2]._shapes.size(), 1);
+ TS_ASSERT_EQUALS(t[2]._shapes[0], 888);
+ TS_ASSERT_EQUALS(t[2]._map, -12);
+ }
+
+ /* Check that various invalid strings don't parse */
+ void test_parse_invalid() {
+ Ultima::Std::vector<Ultima::Ultima8::TreasureInfo> t;
+
+ bool result;
+
+ result = loader.parse("shape=", t);
+ TS_ASSERT(!result);
+
+ result = loader.parse("what", t);
+ TS_ASSERT(!result);
+
+ result = loader.parse("shape=abc", t);
+ TS_ASSERT(!result);
+
+ result = loader.parse("shape=123,123456789", t);
+ TS_ASSERT(!result);
+
+ result = loader.parse("shape=-123", t);
+ TS_ASSERT(!result);
+
+ result = loader.parse("frame=-1,5", t);
+ TS_ASSERT(!result);
+
+ /* TODO: This case falls back to parsing the 10, not great.
+ result = loader.parse("count=10-1", t);
+ TS_ASSERT(!result);
+ */
+
+ result = loader.parse("chance=-1", t);
+ TS_ASSERT(!result);
+ }
+};
More information about the Scummvm-git-logs
mailing list