libchromeos: Make KeyValueStore support multiline values.
Make the KeyValueStore class support using trailing
backslashes to extend values across multiple lines, e.g.
key=here is a really long value. there's really no good \
reason for it to be this long.
Also tighten validation:
- report failure for lines with empty keys
- report failure for lines without equals signs
- trim leading and trailing whitespace around keys
- permit leading whitespace before comments
BUG=chromium:452520
TEST=added unit tests
Change-Id: I6320901aa1f258c5fd9cf2740b871368981d5b5a
Reviewed-on: https://chromium-review.googlesource.com/243561
Reviewed-by: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
diff --git a/chromeos/key_value_store.cc b/chromeos/key_value_store.cc
index 8914229..77da87c 100644
--- a/chromeos/key_value_store.cc
+++ b/chromeos/key_value_store.cc
@@ -12,6 +12,7 @@
#include <base/files/important_file_writer.h>
#include <base/strings/string_split.h>
#include <base/strings/string_util.h>
+#include <chromeos/strings/string_utils.h>
using std::map;
using std::string;
@@ -19,21 +20,56 @@
namespace chromeos {
+namespace {
+
+// Values used for booleans.
+const char kTrueValue[] = "true";
+const char kFalseValue[] = "false";
+
+// Returns a copy of |key| with leading and trailing whitespace removed.
+string TrimKey(const string& key) {
+ string trimmed_key;
+ base::TrimWhitespace(key, base::TRIM_ALL, &trimmed_key);
+ CHECK(!trimmed_key.empty());
+ return trimmed_key;
+}
+
+} // namespace
+
bool KeyValueStore::Load(const base::FilePath& path) {
string file_data;
if (!base::ReadFileToString(path, &file_data))
return false;
- // Split along '\n', then along '='
+ // Split along '\n', then along '='.
vector<string> lines;
base::SplitStringDontTrim(file_data, '\n', &lines);
- for (const auto& line : lines) {
+ for (auto it = lines.begin(); it != lines.end(); ++it) {
+ std::string line;
+ base::TrimWhitespace(*it, base::TRIM_LEADING, &line);
if (line.empty() || line.front() == '#')
continue;
- string::size_type pos = line.find('=');
- if (pos == string::npos)
- continue;
- store_[line.substr(0, pos)] = line.substr(pos + 1);
+
+ std::string key;
+ std::string value;
+ if (!string_utils::SplitAtFirst(line, '=', &key, &value, false))
+ return false;
+
+ base::TrimWhitespace(key, base::TRIM_TRAILING, &key);
+ if (key.empty())
+ return false;
+
+ // Append additional lines to the value as long as we see trailing
+ // backslashes.
+ while (!value.empty() && value.back() == '\\') {
+ ++it;
+ if (it == lines.end() || it->empty())
+ return false;
+ value.pop_back();
+ value += *it;
+ }
+
+ store_[key] = value;
}
return true;
}
@@ -47,7 +83,7 @@
}
bool KeyValueStore::GetString(const string& key, string* value) const {
- const auto key_value = store_.find(key);
+ const auto key_value = store_.find(TrimKey(key));
if (key_value == store_.end())
return false;
*value = key_value->second;
@@ -55,17 +91,18 @@
}
void KeyValueStore::SetString(const string& key, const string& value) {
- store_[key] = value;
+ store_[TrimKey(key)] = value;
}
bool KeyValueStore::GetBoolean(const string& key, bool* value) const {
- const auto key_value = store_.find(key);
- if (key_value == store_.end())
+ string string_value;
+ if (!GetString(key, &string_value))
return false;
- if (key_value->second == "true") {
+
+ if (string_value == kTrueValue) {
*value = true;
return true;
- } else if (key_value->second == "false") {
+ } else if (string_value == kFalseValue) {
*value = false;
return true;
}
@@ -73,7 +110,7 @@
}
void KeyValueStore::SetBoolean(const string& key, bool value) {
- store_[key] = value ? "true" : "false";
+ SetString(key, value ? kTrueValue : kFalseValue);
}
} // namespace chromeos
diff --git a/chromeos/key_value_store.h b/chromeos/key_value_store.h
index 9e8e343..bbc9963 100644
--- a/chromeos/key_value_store.h
+++ b/chromeos/key_value_store.h
@@ -22,15 +22,18 @@
// Creates an empty KeyValueStore.
KeyValueStore() = default;
- // Loads the key=value pairs from the given |path|. Lines starting with
- // '#' and empty lines are ignored. Adds all the readed key=values to the
- // store, overriding those already defined but persisting the ones that
- // aren't present on the passed file.
- // Returns whether reading the file succeeded.
+ // Loads the key=value pairs from the given |path|. Lines starting with '#'
+ // and empty lines are ignored, and whitespace around keys is trimmed.
+ // Trailing backslashes may be used to extend values across multiple lines.
+ // Adds all the read key=values to the store, overriding those already defined
+ // but persisting the ones that aren't present on the passed file. Returns
+ // whether reading the file succeeded.
bool Load(const base::FilePath& path);
- // Saves the current store to the given |path| file. Returns whether the
- // file creation succeeded.
+ // Saves the current store to the given |path| file. Returns whether the file
+ // creation succeeded. Calling Load() and then Save() may result in different
+ // data being written if the original file contained backslash-terminated
+ // lines (i.e. these values will be rewritten on single lines).
bool Save(const base::FilePath& path) const;
// Getter for the given key. Returns whether the key was found on the store.
diff --git a/chromeos/key_value_store_unittest.cc b/chromeos/key_value_store_unittest.cc
index 668cdff..702953d 100644
--- a/chromeos/key_value_store_unittest.cc
+++ b/chromeos/key_value_store_unittest.cc
@@ -9,6 +9,7 @@
#include <base/files/file_util.h>
#include <base/files/scoped_temp_dir.h>
+#include <base/logging.h>
#include <base/strings/string_util.h>
#include <gtest/gtest.h>
@@ -27,17 +28,26 @@
}
protected:
+ // Returns the value from |store_| corresponding to |key|, or an empty string
+ // if the key is not present. Crashes if the store returns an empty value.
+ string GetNonemptyStringValue(const string& key) {
+ string value;
+ if (store_.GetString(key, &value))
+ CHECK(!value.empty());
+ return value;
+ }
+
base::FilePath temp_file_;
base::ScopedTempDir temp_dir_;
KeyValueStore store_; // KeyValueStore under test.
};
TEST_F(KeyValueStoreTest, CommentsAreIgnored) {
- string blob = "# comment\nA=B\n\n\n#another=comment\n\n";
+ string blob = "# comment\nA=B\n\n\n#another=comment\n # leading spaces\n";
ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
- EXPECT_TRUE(store_.Load(temp_file_));
+ ASSERT_TRUE(store_.Load(temp_file_));
- EXPECT_TRUE(store_.Save(temp_file_));
+ ASSERT_TRUE(store_.Save(temp_file_));
string read_blob;
ASSERT_TRUE(ReadFileToString(FilePath(temp_file_), &read_blob));
EXPECT_EQ("A=B\n", read_blob);
@@ -45,27 +55,26 @@
TEST_F(KeyValueStoreTest, EmptyTest) {
ASSERT_EQ(0, base::WriteFile(temp_file_, "", 0));
- EXPECT_TRUE(store_.Load(temp_file_));
+ ASSERT_TRUE(store_.Load(temp_file_));
- EXPECT_TRUE(store_.Save(temp_file_));
+ ASSERT_TRUE(store_.Save(temp_file_));
string read_blob;
ASSERT_TRUE(ReadFileToString(FilePath(temp_file_), &read_blob));
EXPECT_EQ("", read_blob);
}
TEST_F(KeyValueStoreTest, LoadAndReloadTest) {
- string blob = "A=B\nC=\n=\nFOO=BAR=BAZ\nBAR=BAX\nMISSING=NEWLINE";
+ string blob = "A=B\nC=\nFOO=BAR=BAZ\nBAR=BAX\nMISSING=NEWLINE";
ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
- EXPECT_TRUE(store_.Load(temp_file_));
+ ASSERT_TRUE(store_.Load(temp_file_));
map<string, string> expected = {{"A", "B"},
{"C", ""},
- {"", ""},
{"FOO", "BAR=BAZ"},
{"BAR", "BAX"},
{"MISSING", "NEWLINE"}};
- // Test expected values
+ // Test expected values.
string value;
for (const auto& it : expected) {
EXPECT_TRUE(store_.GetString(it.first, &value));
@@ -73,9 +82,9 @@
}
// Save, load and test again.
- EXPECT_TRUE(store_.Save(temp_file_));
+ ASSERT_TRUE(store_.Save(temp_file_));
KeyValueStore new_store;
- EXPECT_TRUE(new_store.Load(temp_file_));
+ ASSERT_TRUE(new_store.Load(temp_file_));
for (const auto& it : expected) {
EXPECT_TRUE(new_store.GetString(it.first, &value)) << "key: " << it.first;
@@ -99,7 +108,7 @@
TEST_F(KeyValueStoreTest, BooleanParsingTest) {
string blob = "TRUE=true\nfalse=false\nvar=false\nDONT_SHOUT=TRUE\n";
ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
- EXPECT_TRUE(store_.Load(temp_file_));
+ ASSERT_TRUE(store_.Load(temp_file_));
map<string, bool> expected = {
{"TRUE", true}, {"false", false}, {"var", false}};
@@ -108,11 +117,83 @@
string str_value;
EXPECT_TRUE(store_.GetString("DONT_SHOUT", &str_value));
- // Test expected values
+ // Test expected values.
for (const auto& it : expected) {
EXPECT_TRUE(store_.GetBoolean(it.first, &value)) << "key: " << it.first;
EXPECT_EQ(it.second, value) << "key: " << it.first;
}
}
+TEST_F(KeyValueStoreTest, TrimWhitespaceAroundKey) {
+ string blob = " a=1\nb =2\n c =3\n";
+ ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
+ ASSERT_TRUE(store_.Load(temp_file_));
+
+ EXPECT_EQ("1", GetNonemptyStringValue("a"));
+ EXPECT_EQ("2", GetNonemptyStringValue("b"));
+ EXPECT_EQ("3", GetNonemptyStringValue("c"));
+
+ // Keys should also be trimmed when setting new values.
+ store_.SetString(" foo ", "4");
+ EXPECT_EQ("4", GetNonemptyStringValue("foo"));
+
+ store_.SetBoolean(" bar ", true);
+ bool value = false;
+ ASSERT_TRUE(store_.GetBoolean("bar", &value));
+ EXPECT_TRUE(value);
+}
+
+TEST_F(KeyValueStoreTest, IgnoreWhitespaceLine) {
+ string blob = "a=1\n \t \nb=2";
+ ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
+ ASSERT_TRUE(store_.Load(temp_file_));
+
+ EXPECT_EQ("1", GetNonemptyStringValue("a"));
+ EXPECT_EQ("2", GetNonemptyStringValue("b"));
+}
+
+TEST_F(KeyValueStoreTest, RejectEmptyKeys) {
+ string blob = "=1";
+ ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
+ EXPECT_FALSE(store_.Load(temp_file_));
+
+ blob = " =2";
+ ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
+ EXPECT_FALSE(store_.Load(temp_file_));
+
+ // Trying to set an empty (after trimming) key should fail an assert.
+ EXPECT_DEATH(store_.SetString(" ", "3"), "");
+ EXPECT_DEATH(store_.SetBoolean(" ", "4"), "");
+}
+
+TEST_F(KeyValueStoreTest, RejectBogusLines) {
+ string blob = "a=1\nbogus\nb=2";
+ ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
+ EXPECT_FALSE(store_.Load(temp_file_));
+}
+
+TEST_F(KeyValueStoreTest, MultilineValue) {
+ string blob = "a=foo\nb=bar\\\n baz \\ \nc=3\n";
+ ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
+ ASSERT_TRUE(store_.Load(temp_file_));
+
+ EXPECT_EQ("foo", GetNonemptyStringValue("a"));
+ EXPECT_EQ("bar baz \\ ", GetNonemptyStringValue("b"));
+ EXPECT_EQ("3", GetNonemptyStringValue("c"));
+}
+
+TEST_F(KeyValueStoreTest, UnterminatedMultilineValue) {
+ string blob = "a=foo\\";
+ ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
+ EXPECT_FALSE(store_.Load(temp_file_));
+
+ blob = "a=foo\\\n";
+ ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
+ EXPECT_FALSE(store_.Load(temp_file_));
+
+ blob = "a=foo\\\n\n# blah\n";
+ ASSERT_EQ(blob.size(), base::WriteFile(temp_file_, blob.data(), blob.size()));
+ EXPECT_FALSE(store_.Load(temp_file_));
+}
+
} // namespace chromeos