From 37a05ec8d6cbf9ff4432225cffe78c16fdd0647d Mon Sep 17 00:00:00 2001
From: SmallJoker <mk939@ymail.com>
Date: Sun, 22 Nov 2020 17:49:30 +0100
Subject: Settings: Proper priority hierarchy

Remove old defaults system
Introduce priority-based fallback list
Use new functions for map_meta special functions
Change groups to use end tags

Unittest changes:
 * Adapt unittest to the new code
 * Compare Settings objects
---
 src/unittest/test_map_settings_manager.cpp | 86 +++++++++++++++++++-----------
 src/unittest/test_settings.cpp             | 73 +++++++++++++++++++++----
 2 files changed, 117 insertions(+), 42 deletions(-)

(limited to 'src/unittest')

diff --git a/src/unittest/test_map_settings_manager.cpp b/src/unittest/test_map_settings_manager.cpp
index 3d642a9b4..81ca68705 100644
--- a/src/unittest/test_map_settings_manager.cpp
+++ b/src/unittest/test_map_settings_manager.cpp
@@ -30,7 +30,7 @@ public:
 	TestMapSettingsManager() { TestManager::registerTestModule(this); }
 	const char *getName() { return "TestMapSettingsManager"; }
 
-	void makeUserConfig(Settings *conf);
+	void makeUserConfig();
 	std::string makeMetaFile(bool make_corrupt);
 
 	void runTests(IGameDef *gamedef);
@@ -65,8 +65,11 @@ void check_noise_params(const NoiseParams *np1, const NoiseParams *np2)
 }
 
 
-void TestMapSettingsManager::makeUserConfig(Settings *conf)
+void TestMapSettingsManager::makeUserConfig()
 {
+	delete Settings::getLayer(SL_GLOBAL);
+	Settings *conf = Settings::createLayer(SL_GLOBAL);
+
 	conf->set("mg_name", "v7");
 	conf->set("seed", "5678");
 	conf->set("water_level", "20");
@@ -103,12 +106,11 @@ std::string TestMapSettingsManager::makeMetaFile(bool make_corrupt)
 
 void TestMapSettingsManager::testMapSettingsManager()
 {
-	Settings user_settings;
-	makeUserConfig(&user_settings);
+	makeUserConfig();
 
 	std::string test_mapmeta_path = makeMetaFile(false);
 
-	MapSettingsManager mgr(&user_settings, test_mapmeta_path);
+	MapSettingsManager mgr(test_mapmeta_path);
 	std::string value;
 
 	UASSERT(mgr.getMapSetting("mg_name", &value));
@@ -140,6 +142,12 @@ void TestMapSettingsManager::testMapSettingsManager()
 	mgr.setMapSettingNoiseParams("mgv5_np_height", &script_np_height);
 	mgr.setMapSettingNoiseParams("mgv5_np_factor", &script_np_factor);
 
+	{
+		NoiseParams dummy;
+		mgr.getMapSettingNoiseParams("mgv5_np_factor", &dummy);
+		check_noise_params(&dummy, &script_np_factor);
+	}
+
 	// Now make our Params and see if the values are correctly sourced
 	MapgenParams *params = mgr.makeMapgenParams();
 	UASSERT(params->mgtype == MAPGEN_V5);
@@ -188,50 +196,66 @@ void TestMapSettingsManager::testMapSettingsManager()
 
 void TestMapSettingsManager::testMapMetaSaveLoad()
 {
-	Settings conf;
 	std::string path = getTestTempDirectory()
 		+ DIR_DELIM + "foobar" + DIR_DELIM + "map_meta.txt";
 
+	makeUserConfig();
+	Settings &conf = *Settings::getLayer(SL_GLOBAL);
+
+	// There cannot be two MapSettingsManager
+	// copy the mapgen params to compare them
+	MapgenParams params1, params2;
 	// Create a set of mapgen params and save them to map meta
-	conf.set("seed", "12345");
-	conf.set("water_level", "5");
-	MapSettingsManager mgr1(&conf, path);
-	MapgenParams *params1 = mgr1.makeMapgenParams();
-	UASSERT(params1);
-	UASSERT(mgr1.saveMapMeta());
+	{
+		conf.set("seed", "12345");
+		conf.set("water_level", "5");
+		MapSettingsManager mgr(path);
+		MapgenParams *params = mgr.makeMapgenParams();
+		UASSERT(params);
+		params1 = *params;
+		params1.bparams = nullptr; // No double-free
+		UASSERT(mgr.saveMapMeta());
+	}
 
 	// Now try loading the map meta to mapgen params
-	conf.set("seed", "67890");
-	conf.set("water_level", "32");
-	MapSettingsManager mgr2(&conf, path);
-	UASSERT(mgr2.loadMapMeta());
-	MapgenParams *params2 = mgr2.makeMapgenParams();
-	UASSERT(params2);
+	{
+		conf.set("seed", "67890");
+		conf.set("water_level", "32");
+		MapSettingsManager mgr(path);
+		UASSERT(mgr.loadMapMeta());
+		MapgenParams *params = mgr.makeMapgenParams();
+		UASSERT(params);
+		params2 = *params;
+		params2.bparams = nullptr; // No double-free
+	}
 
 	// Check that both results are correct
-	UASSERTEQ(u64, params1->seed, 12345);
-	UASSERTEQ(s16, params1->water_level, 5);
-	UASSERTEQ(u64, params2->seed, 12345);
-	UASSERTEQ(s16, params2->water_level, 5);
+	UASSERTEQ(u64, params1.seed, 12345);
+	UASSERTEQ(s16, params1.water_level, 5);
+	UASSERTEQ(u64, params2.seed, 12345);
+	UASSERTEQ(s16, params2.water_level, 5);
 }
 
 
 void TestMapSettingsManager::testMapMetaFailures()
 {
 	std::string test_mapmeta_path;
-	Settings conf;
 
 	// Check to see if it'll fail on a non-existent map meta file
-	test_mapmeta_path = "woobawooba/fgdfg/map_meta.txt";
-	UASSERT(!fs::PathExists(test_mapmeta_path));
+	{
+		test_mapmeta_path = "woobawooba/fgdfg/map_meta.txt";
+		UASSERT(!fs::PathExists(test_mapmeta_path));
 
-	MapSettingsManager mgr1(&conf, test_mapmeta_path);
-	UASSERT(!mgr1.loadMapMeta());
+		MapSettingsManager mgr1(test_mapmeta_path);
+		UASSERT(!mgr1.loadMapMeta());
+	}
 
 	// Check to see if it'll fail on a corrupt map meta file
-	test_mapmeta_path = makeMetaFile(true);
-	UASSERT(fs::PathExists(test_mapmeta_path));
+	{
+		test_mapmeta_path = makeMetaFile(true);
+		UASSERT(fs::PathExists(test_mapmeta_path));
 
-	MapSettingsManager mgr2(&conf, test_mapmeta_path);
-	UASSERT(!mgr2.loadMapMeta());
+		MapSettingsManager mgr2(test_mapmeta_path);
+		UASSERT(!mgr2.loadMapMeta());
+	}
 }
diff --git a/src/unittest/test_settings.cpp b/src/unittest/test_settings.cpp
index f91ba5b67..d2d35c357 100644
--- a/src/unittest/test_settings.cpp
+++ b/src/unittest/test_settings.cpp
@@ -21,6 +21,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 #include <cmath>
 #include "settings.h"
+#include "defaultsettings.h"
 #include "noise.h"
 
 class TestSettings : public TestBase {
@@ -31,6 +32,7 @@ public:
 	void runTests(IGameDef *gamedef);
 
 	void testAllSettings();
+	void testDefaults();
 	void testFlagDesc();
 
 	static const char *config_text_before;
@@ -42,6 +44,7 @@ static TestSettings g_test_instance;
 void TestSettings::runTests(IGameDef *gamedef)
 {
 	TEST(testAllSettings);
+	TEST(testDefaults);
 	TEST(testFlagDesc);
 }
 
@@ -70,7 +73,8 @@ const char *TestSettings::config_text_before =
 	"     with leading whitespace!\n"
 	"\"\"\"\n"
 	"np_terrain = 5, 40, (250, 250, 250), 12341, 5, 0.7, 2.4\n"
-	"zoop = true";
+	"zoop = true\n"
+	"[dummy_eof_end_tag]\n";
 
 const std::string TestSettings::config_text_after =
 	"leet = 1337\n"
@@ -111,12 +115,34 @@ const std::string TestSettings::config_text_after =
 	"	animals = cute\n"
 	"	num_apples = 4\n"
 	"	num_oranges = 53\n"
-	"}\n";
+	"}\n"
+	"[dummy_eof_end_tag]";
+
+void compare_settings(const std::string &name, Settings *a, Settings *b)
+{
+	auto keys = a->getNames();
+	Settings *group1, *group2;
+	std::string value1, value2;
+	for (auto &key : keys) {
+		if (a->getGroupNoEx(key, group1)) {
+			UASSERT(b->getGroupNoEx(key, group2));
+
+			compare_settings(name + "->" + key, group1, group2);
+			continue;
+		}
+
+		UASSERT(b->getNoEx(key, value1));
+		// For identification
+		value1 = name + "->" + key + "=" + value1;
+		value2 = name + "->" + key + "=" + a->get(key);
+		UASSERTCMP(std::string, ==, value2, value1);
+	}
+}
 
 void TestSettings::testAllSettings()
 {
 	try {
-	Settings s;
+	Settings s("[dummy_eof_end_tag]");
 
 	// Test reading of settings
 	std::istringstream is(config_text_before);
@@ -197,21 +223,44 @@ void TestSettings::testAllSettings()
 	is.clear();
 	is.seekg(0);
 
-	UASSERT(s.updateConfigObject(is, os, "", 0) == true);
-	//printf(">>>> expected config:\n%s\n", TEST_CONFIG_TEXT_AFTER);
-	//printf(">>>> actual config:\n%s\n", os.str().c_str());
-#if __cplusplus < 201103L
-	// This test only works in older C++ versions than C++11 because we use unordered_map
-	UASSERT(os.str() == config_text_after);
-#endif
+	UASSERT(s.updateConfigObject(is, os, 0) == true);
+
+	{
+		// Confirm settings
+		Settings s2("[dummy_eof_end_tag]");
+		std::istringstream is(config_text_after, std::ios_base::binary);
+		s2.parseConfigLines(is);
+
+		compare_settings("(main)", &s, &s2);
+	}
+
 	} catch (SettingNotFoundException &e) {
 		UASSERT(!"Setting not found!");
 	}
 }
 
+void TestSettings::testDefaults()
+{
+	Settings *game = Settings::createLayer(SL_GAME);
+	Settings *def = Settings::getLayer(SL_DEFAULTS);
+
+	def->set("name", "FooBar");
+	UASSERT(def->get("name") == "FooBar");
+	UASSERT(game->get("name") == "FooBar");
+
+	game->set("name", "Baz");
+	UASSERT(game->get("name") == "Baz");
+
+	delete game;
+
+	// Restore default settings
+	delete Settings::getLayer(SL_DEFAULTS);
+	set_default_settings();
+}
+
 void TestSettings::testFlagDesc()
 {
-	Settings s;
+	Settings &s = *Settings::createLayer(SL_GAME);
 	FlagDesc flagdesc[] = {
 		{ "biomes",  0x01 },
 		{ "trees",   0x02 },
@@ -242,4 +291,6 @@ void TestSettings::testFlagDesc()
 	// Enabled: tables
 	s.set("test_flags", "16");
 	UASSERT(s.getFlagStr("test_flags", flagdesc, nullptr) == 0x10);
+
+	delete &s;
 }
-- 
cgit v1.2.3


From 2760371d8e43327e94d1b4ecb79fbcb56278db90 Mon Sep 17 00:00:00 2001
From: SmallJoker <mk939@ymail.com>
Date: Sun, 29 Nov 2020 17:56:25 +0100
Subject: Settings: Purge getDefault, clean FontEngine

---
 src/client/clientlauncher.cpp     |  2 +-
 src/client/fontengine.cpp         | 65 +++++++++++++++++++--------------------
 src/client/fontengine.h           |  8 +----
 src/main.cpp                      |  1 -
 src/script/scripting_mainmenu.cpp |  1 -
 src/settings.cpp                  | 18 ++---------
 src/settings.h                    |  1 -
 src/unittest/test_settings.cpp    |  2 +-
 8 files changed, 38 insertions(+), 60 deletions(-)

(limited to 'src/unittest')

diff --git a/src/client/clientlauncher.cpp b/src/client/clientlauncher.cpp
index 7245f29f0..2bb0bc385 100644
--- a/src/client/clientlauncher.cpp
+++ b/src/client/clientlauncher.cpp
@@ -175,7 +175,7 @@ bool ClientLauncher::run(GameStartData &start_data, const Settings &cmd_args)
 		}
 	}
 #endif
-	g_fontengine = new FontEngine(g_settings, guienv);
+	g_fontengine = new FontEngine(guienv);
 	FATAL_ERROR_IF(g_fontengine == NULL, "Font engine creation failed.");
 
 #if (IRRLICHT_VERSION_MAJOR >= 1 && IRRLICHT_VERSION_MINOR >= 8) || IRRLICHT_VERSION_MAJOR >= 2
diff --git a/src/client/fontengine.cpp b/src/client/fontengine.cpp
index a55420846..47218c0d9 100644
--- a/src/client/fontengine.cpp
+++ b/src/client/fontengine.cpp
@@ -42,8 +42,7 @@ static void font_setting_changed(const std::string &name, void *userdata)
 }
 
 /******************************************************************************/
-FontEngine::FontEngine(Settings* main_settings, gui::IGUIEnvironment* env) :
-	m_settings(main_settings),
+FontEngine::FontEngine(gui::IGUIEnvironment* env) :
 	m_env(env)
 {
 
@@ -51,34 +50,34 @@ FontEngine::FontEngine(Settings* main_settings, gui::IGUIEnvironment* env) :
 		i = (FontMode) FONT_SIZE_UNSPECIFIED;
 	}
 
-	assert(m_settings != NULL); // pre-condition
+	assert(g_settings != NULL); // pre-condition
 	assert(m_env != NULL); // pre-condition
 	assert(m_env->getSkin() != NULL); // pre-condition
 
 	readSettings();
 
 	if (m_currentMode == FM_Standard) {
-		m_settings->registerChangedCallback("font_size", font_setting_changed, NULL);
-		m_settings->registerChangedCallback("font_bold", font_setting_changed, NULL);
-		m_settings->registerChangedCallback("font_italic", font_setting_changed, NULL);
-		m_settings->registerChangedCallback("font_path", font_setting_changed, NULL);
-		m_settings->registerChangedCallback("font_path_bold", font_setting_changed, NULL);
-		m_settings->registerChangedCallback("font_path_italic", font_setting_changed, NULL);
-		m_settings->registerChangedCallback("font_path_bolditalic", font_setting_changed, NULL);
-		m_settings->registerChangedCallback("font_shadow", font_setting_changed, NULL);
-		m_settings->registerChangedCallback("font_shadow_alpha", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("font_size", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("font_bold", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("font_italic", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("font_path", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("font_path_bold", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("font_path_italic", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("font_path_bolditalic", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("font_shadow", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("font_shadow_alpha", font_setting_changed, NULL);
 	}
 	else if (m_currentMode == FM_Fallback) {
-		m_settings->registerChangedCallback("fallback_font_size", font_setting_changed, NULL);
-		m_settings->registerChangedCallback("fallback_font_path", font_setting_changed, NULL);
-		m_settings->registerChangedCallback("fallback_font_shadow", font_setting_changed, NULL);
-		m_settings->registerChangedCallback("fallback_font_shadow_alpha", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("fallback_font_size", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("fallback_font_path", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("fallback_font_shadow", font_setting_changed, NULL);
+		g_settings->registerChangedCallback("fallback_font_shadow_alpha", font_setting_changed, NULL);
 	}
 
-	m_settings->registerChangedCallback("mono_font_path", font_setting_changed, NULL);
-	m_settings->registerChangedCallback("mono_font_size", font_setting_changed, NULL);
-	m_settings->registerChangedCallback("screen_dpi", font_setting_changed, NULL);
-	m_settings->registerChangedCallback("gui_scaling", font_setting_changed, NULL);
+	g_settings->registerChangedCallback("mono_font_path", font_setting_changed, NULL);
+	g_settings->registerChangedCallback("mono_font_size", font_setting_changed, NULL);
+	g_settings->registerChangedCallback("screen_dpi", font_setting_changed, NULL);
+	g_settings->registerChangedCallback("gui_scaling", font_setting_changed, NULL);
 }
 
 /******************************************************************************/
@@ -205,9 +204,9 @@ unsigned int FontEngine::getFontSize(FontMode mode)
 void FontEngine::readSettings()
 {
 	if (USE_FREETYPE && g_settings->getBool("freetype")) {
-		m_default_size[FM_Standard] = m_settings->getU16("font_size");
-		m_default_size[FM_Fallback] = m_settings->getU16("fallback_font_size");
-		m_default_size[FM_Mono]     = m_settings->getU16("mono_font_size");
+		m_default_size[FM_Standard] = g_settings->getU16("font_size");
+		m_default_size[FM_Fallback] = g_settings->getU16("fallback_font_size");
+		m_default_size[FM_Mono]     = g_settings->getU16("mono_font_size");
 
 		/*~ DO NOT TRANSLATE THIS LITERALLY!
 		This is a special string. Put either "no" or "yes"
@@ -220,15 +219,15 @@ void FontEngine::readSettings()
 		m_currentMode = is_yes(gettext("needs_fallback_font")) ?
 				FM_Fallback : FM_Standard;
 
-		m_default_bold = m_settings->getBool("font_bold");
-		m_default_italic = m_settings->getBool("font_italic");
+		m_default_bold = g_settings->getBool("font_bold");
+		m_default_italic = g_settings->getBool("font_italic");
 
 	} else {
 		m_currentMode = FM_Simple;
 	}
 
-	m_default_size[FM_Simple]       = m_settings->getU16("font_size");
-	m_default_size[FM_SimpleMono]   = m_settings->getU16("mono_font_size");
+	m_default_size[FM_Simple]       = g_settings->getU16("font_size");
+	m_default_size[FM_SimpleMono]   = g_settings->getU16("mono_font_size");
 
 	cleanCache();
 	updateFontCache();
@@ -244,7 +243,7 @@ void FontEngine::updateSkin()
 		m_env->getSkin()->setFont(font);
 	else
 		errorstream << "FontEngine: Default font file: " <<
-				"\n\t\"" << m_settings->get("font_path") << "\"" <<
+				"\n\t\"" << g_settings->get("font_path") << "\"" <<
 				"\n\trequired for current screen configuration was not found" <<
 				" or was invalid file format." <<
 				"\n\tUsing irrlicht default font." << std::endl;
@@ -292,7 +291,7 @@ gui::IGUIFont *FontEngine::initFont(const FontSpec &spec)
 		setting_suffix.append("_italic");
 
 	u32 size = std::floor(RenderingEngine::getDisplayDensity() *
-			m_settings->getFloat("gui_scaling") * spec.size);
+			g_settings->getFloat("gui_scaling") * spec.size);
 
 	if (size == 0) {
 		errorstream << "FontEngine: attempt to use font size 0" << std::endl;
@@ -311,8 +310,8 @@ gui::IGUIFont *FontEngine::initFont(const FontSpec &spec)
 
 	std::string fallback_settings[] = {
 		wanted_font_path,
-		m_settings->get("fallback_font_path"),
-		m_settings->getDefault(setting_prefix + "font_path")
+		g_settings->get("fallback_font_path"),
+		Settings::getLayer(SL_DEFAULTS)->get(setting_prefix + "font_path")
 	};
 
 #if USE_FREETYPE
@@ -346,7 +345,7 @@ gui::IGUIFont *FontEngine::initSimpleFont(const FontSpec &spec)
 	assert(spec.mode == FM_Simple || spec.mode == FM_SimpleMono);
 	assert(spec.size != FONT_SIZE_UNSPECIFIED);
 
-	const std::string &font_path = m_settings->get(
+	const std::string &font_path = g_settings->get(
 			(spec.mode == FM_SimpleMono) ? "mono_font_path" : "font_path");
 
 	size_t pos_dot = font_path.find_last_of('.');
@@ -364,7 +363,7 @@ gui::IGUIFont *FontEngine::initSimpleFont(const FontSpec &spec)
 
 	u32 size = std::floor(
 			RenderingEngine::getDisplayDensity() *
-			m_settings->getFloat("gui_scaling") *
+			g_settings->getFloat("gui_scaling") *
 			spec.size);
 
 	irr::gui::IGUIFont *font = nullptr;
diff --git a/src/client/fontengine.h b/src/client/fontengine.h
index d62e9b8ef..e27ef60e9 100644
--- a/src/client/fontengine.h
+++ b/src/client/fontengine.h
@@ -62,7 +62,7 @@ class FontEngine
 {
 public:
 
-	FontEngine(Settings* main_settings, gui::IGUIEnvironment* env);
+	FontEngine(gui::IGUIEnvironment* env);
 
 	~FontEngine();
 
@@ -128,9 +128,6 @@ public:
 	/** get font size for a specific mode */
 	unsigned int getFontSize(FontMode mode);
 
-	/** initialize font engine */
-	void initialize(Settings* main_settings, gui::IGUIEnvironment* env);
-
 	/** update internal parameters from settings */
 	void readSettings();
 
@@ -150,9 +147,6 @@ private:
 	/** clean cache */
 	void cleanCache();
 
-	/** pointer to settings for registering callbacks or reading config */
-	Settings* m_settings = nullptr;
-
 	/** pointer to irrlicht gui environment */
 	gui::IGUIEnvironment* m_env = nullptr;
 
diff --git a/src/main.cpp b/src/main.cpp
index 57768dbb2..39b441d2c 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -527,7 +527,6 @@ static bool read_config_file(const Settings &cmd_args)
 	// Path of configuration file in use
 	sanity_check(g_settings_path == "");	// Sanity check
 
-
 	if (cmd_args.exists("config")) {
 		bool r = g_settings->readConfigFile(cmd_args.get("config").c_str());
 		if (!r) {
diff --git a/src/script/scripting_mainmenu.cpp b/src/script/scripting_mainmenu.cpp
index 9b377366e..b102a66a1 100644
--- a/src/script/scripting_mainmenu.cpp
+++ b/src/script/scripting_mainmenu.cpp
@@ -31,7 +31,6 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 extern "C" {
 #include "lualib.h"
 }
-#include "settings.h"
 #define MAINMENU_NUM_ASYNC_THREADS 4
 
 
diff --git a/src/settings.cpp b/src/settings.cpp
index cf2a16aa6..3415ff818 100644
--- a/src/settings.cpp
+++ b/src/settings.cpp
@@ -33,7 +33,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include <cctype>
 #include <algorithm>
 
-Settings *g_settings = nullptr;
+Settings *g_settings = nullptr; // Populated in main()
 std::string g_settings_path;
 
 Settings *Settings::s_layers[SL_TOTAL_COUNT] = {0}; // Zeroed by compiler
@@ -85,6 +85,7 @@ Settings & Settings::operator = (const Settings &other)
 	if (&other == this)
 		return *this;
 
+	// TODO: Avoid copying Settings objects. Make this private.
 	FATAL_ERROR_IF(m_settingslayer != SL_TOTAL_COUNT && other.m_settingslayer != SL_TOTAL_COUNT,
 		("Tried to copy unique Setting layer " + std::to_string(m_settingslayer)).c_str());
 
@@ -208,6 +209,7 @@ void Settings::writeLines(std::ostream &os, u32 tab_depth) const
 	for (const auto &setting_it : m_settings)
 		printEntry(os, setting_it.first, setting_it.second, tab_depth);
 
+	// For groups this must be "}" !
 	if (!m_end_tag.empty()) {
 		for (u32 i = 0; i < tab_depth; i++)
 			os << "\t";
@@ -458,20 +460,6 @@ const std::string &Settings::get(const std::string &name) const
 }
 
 
-const std::string &Settings::getDefault(const std::string &name) const
-{
-	const SettingsEntry *entry;
-	if (auto parent = getParent())
-		entry = &parent->getEntry(name);
-	else
-		entry = &getEntry(name); // Bottom of the chain
-
-	if (entry->is_group)
-		throw SettingNotFoundException("Setting [" + name + "] is a group.");
-	return entry->value;
-}
-
-
 bool Settings::getBool(const std::string &name) const
 {
 	return is_yes(get(name));
diff --git a/src/settings.h b/src/settings.h
index af4cae3cd..b5e859ee0 100644
--- a/src/settings.h
+++ b/src/settings.h
@@ -132,7 +132,6 @@ public:
 
 	Settings *getGroup(const std::string &name) const;
 	const std::string &get(const std::string &name) const;
-	const std::string &getDefault(const std::string &name) const;
 	bool getBool(const std::string &name) const;
 	u16 getU16(const std::string &name) const;
 	s16 getS16(const std::string &name) const;
diff --git a/src/unittest/test_settings.cpp b/src/unittest/test_settings.cpp
index d2d35c357..6b493c9e4 100644
--- a/src/unittest/test_settings.cpp
+++ b/src/unittest/test_settings.cpp
@@ -229,7 +229,7 @@ void TestSettings::testAllSettings()
 		// Confirm settings
 		Settings s2("[dummy_eof_end_tag]");
 		std::istringstream is(config_text_after, std::ios_base::binary);
-		s2.parseConfigLines(is);
+		UASSERT(s2.parseConfigLines(is) == true);
 
 		compare_settings("(main)", &s, &s2);
 	}
-- 
cgit v1.2.3


From 83229921e5f378625d9ef63ede3dffbe778e1798 Mon Sep 17 00:00:00 2001
From: sfan5 <sfan5@live.de>
Date: Sun, 17 Jan 2021 01:56:50 +0100
Subject: Rework use_texture_alpha to provide three opaque/clip/blend modes

The change that turns nodeboxes and meshes opaque when possible is kept,
as is the compatibility code that warns modders to adjust their nodedefs.
---
 builtin/game/features.lua       |   1 +
 doc/lua_api.txt                 |  18 +++++--
 src/nodedef.cpp                 | 110 ++++++++++++++++++++++++++--------------
 src/nodedef.h                   |  56 +++++++++++++++-----
 src/script/common/c_content.cpp |  32 ++++++++----
 src/script/cpp_api/s_node.cpp   |   8 +++
 src/script/cpp_api/s_node.h     |   1 +
 src/unittest/test.cpp           |   4 +-
 8 files changed, 164 insertions(+), 66 deletions(-)

(limited to 'src/unittest')

diff --git a/builtin/game/features.lua b/builtin/game/features.lua
index 4d3c90ff0..36ff1f0b0 100644
--- a/builtin/game/features.lua
+++ b/builtin/game/features.lua
@@ -18,6 +18,7 @@ core.features = {
 	pathfinder_works = true,
 	object_step_has_moveresult = true,
 	direct_velocity_on_players = true,
+	use_texture_alpha_string_modes = true,
 }
 
 function core.has_feature(arg)
diff --git a/doc/lua_api.txt b/doc/lua_api.txt
index cfc150edc..8156f785a 100644
--- a/doc/lua_api.txt
+++ b/doc/lua_api.txt
@@ -4386,6 +4386,8 @@ Utilities
           object_step_has_moveresult = true,
           -- Whether get_velocity() and add_velocity() can be used on players (5.4.0)
           direct_velocity_on_players = true,
+          -- nodedef's use_texture_alpha accepts new string modes (5.4.0)
+          use_texture_alpha_string_modes = true,
       }
 
 * `minetest.has_feature(arg)`: returns `boolean, missing_features`
@@ -7340,10 +7342,18 @@ Used by `minetest.register_node`.
         -- If the node has a palette, then this setting only has an effect in
         -- the inventory and on the wield item.
 
-        use_texture_alpha = false,
-        -- Use texture's alpha channel
-        -- If this is set to false, the node will be rendered fully opaque
-        -- regardless of any texture transparency.
+        use_texture_alpha = ...,
+        -- Specifies how the texture's alpha channel will be used for rendering.
+        -- possible values:
+        -- * "opaque": Node is rendered opaque regardless of alpha channel
+        -- * "clip": A given pixel is either fully see-through or opaque
+        --           depending on the alpha channel being below/above 50% in value
+        -- * "blend": The alpha channel specifies how transparent a given pixel
+        --            of the rendered node is
+        -- The default is "opaque" for drawtypes normal, liquid and flowingliquid;
+        -- "clip" otherwise.
+        -- If set to a boolean value (deprecated): true either sets it to blend
+        -- or clip, false sets it to clip or opaque mode depending on the drawtype.
 
         palette = "palette.png",
         -- The node's `param2` is used to select a pixel from the image.
diff --git a/src/nodedef.cpp b/src/nodedef.cpp
index b2cfd1f87..57d4c008f 100644
--- a/src/nodedef.cpp
+++ b/src/nodedef.cpp
@@ -360,7 +360,7 @@ void ContentFeatures::reset()
 		i = TileDef();
 	for (auto &j : tiledef_special)
 		j = TileDef();
-	alpha = 255;
+	alpha = ALPHAMODE_OPAQUE;
 	post_effect_color = video::SColor(0, 0, 0, 0);
 	param_type = CPT_NONE;
 	param_type_2 = CPT2_NONE;
@@ -405,6 +405,31 @@ void ContentFeatures::reset()
 	node_dig_prediction = "air";
 }
 
+void ContentFeatures::setAlphaFromLegacy(u8 legacy_alpha)
+{
+	// No special handling for nodebox/mesh here as it doesn't make sense to
+	// throw warnings when the server is too old to support the "correct" way
+	switch (drawtype) {
+	case NDT_NORMAL:
+		alpha = legacy_alpha == 255 ? ALPHAMODE_OPAQUE : ALPHAMODE_CLIP;
+		break;
+	case NDT_LIQUID:
+	case NDT_FLOWINGLIQUID:
+		alpha = legacy_alpha == 255 ? ALPHAMODE_OPAQUE : ALPHAMODE_BLEND;
+		break;
+	default:
+		alpha = legacy_alpha == 255 ? ALPHAMODE_CLIP : ALPHAMODE_BLEND;
+		break;
+	}
+}
+
+u8 ContentFeatures::getAlphaForLegacy() const
+{
+	// This is so simple only because 255 and 0 mean wildly different things
+	// depending on drawtype...
+	return alpha == ALPHAMODE_OPAQUE ? 255 : 0;
+}
+
 void ContentFeatures::serialize(std::ostream &os, u16 protocol_version) const
 {
 	const u8 version = CONTENTFEATURES_VERSION;
@@ -433,7 +458,7 @@ void ContentFeatures::serialize(std::ostream &os, u16 protocol_version) const
 	for (const TileDef &td : tiledef_special) {
 		td.serialize(os, protocol_version);
 	}
-	writeU8(os, alpha);
+	writeU8(os, getAlphaForLegacy());
 	writeU8(os, color.getRed());
 	writeU8(os, color.getGreen());
 	writeU8(os, color.getBlue());
@@ -489,6 +514,7 @@ void ContentFeatures::serialize(std::ostream &os, u16 protocol_version) const
 
 	os << serializeString16(node_dig_prediction);
 	writeU8(os, leveled_max);
+	writeU8(os, alpha);
 }
 
 void ContentFeatures::deSerialize(std::istream &is)
@@ -524,7 +550,7 @@ void ContentFeatures::deSerialize(std::istream &is)
 		throw SerializationError("unsupported CF_SPECIAL_COUNT");
 	for (TileDef &td : tiledef_special)
 		td.deSerialize(is, version, drawtype);
-	alpha = readU8(is);
+	setAlphaFromLegacy(readU8(is));
 	color.setRed(readU8(is));
 	color.setGreen(readU8(is));
 	color.setBlue(readU8(is));
@@ -582,10 +608,16 @@ void ContentFeatures::deSerialize(std::istream &is)
 
 	try {
 		node_dig_prediction = deSerializeString16(is);
-		u8 tmp_leveled_max = readU8(is);
+
+		u8 tmp = readU8(is);
 		if (is.eof()) /* readU8 doesn't throw exceptions so we have to do this */
 			throw SerializationError("");
-		leveled_max = tmp_leveled_max;
+		leveled_max = tmp;
+
+		tmp = readU8(is);
+		if (is.eof())
+			throw SerializationError("");
+		alpha = static_cast<enum AlphaMode>(tmp);
 	} catch(SerializationError &e) {};
 }
 
@@ -677,6 +709,7 @@ bool ContentFeatures::textureAlphaCheck(ITextureSource *tsrc, const TileDef *til
 	video::IVideoDriver *driver = RenderingEngine::get_video_driver();
 	static thread_local bool long_warning_printed = false;
 	std::set<std::string> seen;
+
 	for (int i = 0; i < length; i++) {
 		if (seen.find(tiles[i].name) != seen.end())
 			continue;
@@ -701,20 +734,21 @@ bool ContentFeatures::textureAlphaCheck(ITextureSource *tsrc, const TileDef *til
 
 break_loop:
 		image->drop();
-		if (!ok) {
-			warningstream << "Texture \"" << tiles[i].name << "\" of "
-				<< name << " has transparent pixels, assuming "
-				"use_texture_alpha = true." << std::endl;
-			if (!long_warning_printed) {
-				warningstream << "  This warning can be a false-positive if "
-					"unused pixels in the texture are transparent. However if "
-					"it is meant to be transparent, you *MUST* update the "
-					"nodedef and set use_texture_alpha = true! This compatibility "
-					"code will be removed in a few releases." << std::endl;
-				long_warning_printed = true;
-			}
-			return true;
+		if (ok)
+			continue;
+		warningstream << "Texture \"" << tiles[i].name << "\" of "
+			<< name << " has transparency, assuming "
+			"use_texture_alpha = \"clip\"." << std::endl;
+		if (!long_warning_printed) {
+			warningstream << "  This warning can be a false-positive if "
+				"unused pixels in the texture are transparent. However if "
+				"it is meant to be transparent, you *MUST* update the "
+				"nodedef and set use_texture_alpha = \"clip\"! This "
+				"compatibility code will be removed in a few releases."
+				<< std::endl;
+			long_warning_printed = true;
 		}
+		return true;
 	}
 	return false;
 }
@@ -759,14 +793,18 @@ void ContentFeatures::updateTextures(ITextureSource *tsrc, IShaderSource *shdsrc
 
 	bool is_liquid = false;
 
-	MaterialType material_type = (alpha == 255) ?
-		TILE_MATERIAL_BASIC : TILE_MATERIAL_ALPHA;
+	if (alpha == ALPHAMODE_LEGACY_COMPAT) {
+		// Before working with the alpha mode, resolve any legacy kludges
+		alpha = textureAlphaCheck(tsrc, tdef, 6) ? ALPHAMODE_CLIP : ALPHAMODE_OPAQUE;
+	}
+
+	MaterialType material_type = alpha == ALPHAMODE_OPAQUE ?
+		TILE_MATERIAL_OPAQUE : (alpha == ALPHAMODE_CLIP ? TILE_MATERIAL_BASIC :
+		TILE_MATERIAL_ALPHA);
 
 	switch (drawtype) {
 	default:
 	case NDT_NORMAL:
-		material_type = (alpha == 255) ?
-			TILE_MATERIAL_OPAQUE : TILE_MATERIAL_ALPHA;
 		solidness = 2;
 		break;
 	case NDT_AIRLIKE:
@@ -774,14 +812,14 @@ void ContentFeatures::updateTextures(ITextureSource *tsrc, IShaderSource *shdsrc
 		break;
 	case NDT_LIQUID:
 		if (tsettings.opaque_water)
-			alpha = 255;
+			alpha = ALPHAMODE_OPAQUE;
 		solidness = 1;
 		is_liquid = true;
 		break;
 	case NDT_FLOWINGLIQUID:
 		solidness = 0;
 		if (tsettings.opaque_water)
-			alpha = 255;
+			alpha = ALPHAMODE_OPAQUE;
 		is_liquid = true;
 		break;
 	case NDT_GLASSLIKE:
@@ -833,19 +871,16 @@ void ContentFeatures::updateTextures(ITextureSource *tsrc, IShaderSource *shdsrc
 		break;
 	case NDT_MESH:
 	case NDT_NODEBOX:
-		if (alpha == 255 && textureAlphaCheck(tsrc, tdef, 6))
-			alpha = 0;
-
 		solidness = 0;
-		if (waving == 1)
+		if (waving == 1) {
 			material_type = TILE_MATERIAL_WAVING_PLANTS;
-		else if (waving == 2)
+		} else if (waving == 2) {
 			material_type = TILE_MATERIAL_WAVING_LEAVES;
-		else if (waving == 3)
-			material_type = (alpha == 255) ? TILE_MATERIAL_WAVING_LIQUID_OPAQUE :
-				TILE_MATERIAL_WAVING_LIQUID_BASIC;
-		else if (alpha == 255)
-			material_type = TILE_MATERIAL_OPAQUE;
+		} else if (waving == 3) {
+			material_type = alpha == ALPHAMODE_OPAQUE ?
+				TILE_MATERIAL_WAVING_LIQUID_OPAQUE : (alpha == ALPHAMODE_CLIP ?
+				TILE_MATERIAL_WAVING_LIQUID_BASIC : TILE_MATERIAL_WAVING_LIQUID_TRANSPARENT);
+		}
 		break;
 	case NDT_TORCHLIKE:
 	case NDT_SIGNLIKE:
@@ -860,10 +895,11 @@ void ContentFeatures::updateTextures(ITextureSource *tsrc, IShaderSource *shdsrc
 
 	if (is_liquid) {
 		if (waving == 3) {
-			material_type = (alpha == 255) ? TILE_MATERIAL_WAVING_LIQUID_OPAQUE :
-				TILE_MATERIAL_WAVING_LIQUID_TRANSPARENT;
+			material_type = alpha == ALPHAMODE_OPAQUE ?
+				TILE_MATERIAL_WAVING_LIQUID_OPAQUE : (alpha == ALPHAMODE_CLIP ?
+				TILE_MATERIAL_WAVING_LIQUID_BASIC : TILE_MATERIAL_WAVING_LIQUID_TRANSPARENT);
 		} else {
-			material_type = (alpha == 255) ? TILE_MATERIAL_LIQUID_OPAQUE :
+			material_type = alpha == ALPHAMODE_OPAQUE ? TILE_MATERIAL_LIQUID_OPAQUE :
 				TILE_MATERIAL_LIQUID_TRANSPARENT;
 		}
 	}
diff --git a/src/nodedef.h b/src/nodedef.h
index 63b9474b9..6fc20518d 100644
--- a/src/nodedef.h
+++ b/src/nodedef.h
@@ -231,6 +231,14 @@ enum AlignStyle : u8 {
 	ALIGN_STYLE_USER_DEFINED,
 };
 
+enum AlphaMode : u8 {
+	ALPHAMODE_BLEND,
+	ALPHAMODE_CLIP,
+	ALPHAMODE_OPAQUE,
+	ALPHAMODE_LEGACY_COMPAT, /* means either opaque or clip */
+};
+
+
 /*
 	Stand-alone definition of a TileSpec (basically a server-side TileSpec)
 */
@@ -315,9 +323,7 @@ struct ContentFeatures
 	// These will be drawn over the base tiles.
 	TileDef tiledef_overlay[6];
 	TileDef tiledef_special[CF_SPECIAL_COUNT]; // eg. flowing liquid
-	// If 255, the node is opaque.
-	// Otherwise it uses texture alpha.
-	u8 alpha;
+	AlphaMode alpha;
 	// The color of the node.
 	video::SColor color;
 	std::string palette_name;
@@ -418,20 +424,27 @@ struct ContentFeatures
 	void serialize(std::ostream &os, u16 protocol_version) const;
 	void deSerialize(std::istream &is);
 
-#ifndef SERVER
-	/*
-	 * Checks if any tile texture has any transparent pixels.
-	 * Prints a warning and returns true if that is the case, false otherwise.
-	 * This is supposed to be used for use_texture_alpha backwards compatibility.
-	 */
-	bool textureAlphaCheck(ITextureSource *tsrc, const TileDef *tiles,
-		int length);
-#endif
-	
-
 	/*
 		Some handy methods
 	*/
+	void setDefaultAlphaMode()
+	{
+		switch (drawtype) {
+		case NDT_NORMAL:
+		case NDT_LIQUID:
+		case NDT_FLOWINGLIQUID:
+			alpha = ALPHAMODE_OPAQUE;
+			break;
+		case NDT_NODEBOX:
+		case NDT_MESH:
+			alpha = ALPHAMODE_LEGACY_COMPAT; // this should eventually be OPAQUE
+			break;
+		default:
+			alpha = ALPHAMODE_CLIP;
+			break;
+		}
+	}
+
 	bool needsBackfaceCulling() const
 	{
 		switch (drawtype) {
@@ -465,6 +478,21 @@ struct ContentFeatures
 	void updateTextures(ITextureSource *tsrc, IShaderSource *shdsrc,
 		scene::IMeshManipulator *meshmanip, Client *client, const TextureSettings &tsettings);
 #endif
+
+private:
+#ifndef SERVER
+	/*
+	 * Checks if any tile texture has any transparent pixels.
+	 * Prints a warning and returns true if that is the case, false otherwise.
+	 * This is supposed to be used for use_texture_alpha backwards compatibility.
+	 */
+	bool textureAlphaCheck(ITextureSource *tsrc, const TileDef *tiles,
+		int length);
+#endif
+
+	void setAlphaFromLegacy(u8 legacy_alpha);
+
+	u8 getAlphaForLegacy() const;
 };
 
 /*!
diff --git a/src/script/common/c_content.cpp b/src/script/common/c_content.cpp
index 5d29422af..ecab7baa1 100644
--- a/src/script/common/c_content.cpp
+++ b/src/script/common/c_content.cpp
@@ -618,25 +618,39 @@ void read_content_features(lua_State *L, ContentFeatures &f, int index)
 	}
 	lua_pop(L, 1);
 
+	/* alpha & use_texture_alpha */
+	// This is a bit complicated due to compatibility
+
+	f.setDefaultAlphaMode();
+
 	warn_if_field_exists(L, index, "alpha",
-		"Obsolete, only limited compatibility provided");
+		"Obsolete, only limited compatibility provided; "
+		"replaced by \"use_texture_alpha\"");
 	if (getintfield_default(L, index, "alpha", 255) != 255)
-		f.alpha = 0;
+		f.alpha = ALPHAMODE_BLEND;
+
+	lua_getfield(L, index, "use_texture_alpha");
+	if (lua_isboolean(L, -1)) {
+		warn_if_field_exists(L, index, "use_texture_alpha",
+			"Boolean values are deprecated; use the new choices");
+		if (lua_toboolean(L, -1))
+			f.alpha = (f.drawtype == NDT_NORMAL) ? ALPHAMODE_CLIP : ALPHAMODE_BLEND;
+	} else if (check_field_or_nil(L, -1, LUA_TSTRING, "use_texture_alpha")) {
+		int result = f.alpha;
+		string_to_enum(ScriptApiNode::es_TextureAlphaMode, result,
+				std::string(lua_tostring(L, -1)));
+		f.alpha = static_cast<enum AlphaMode>(result);
+	}
+	lua_pop(L, 1);
 
-	bool usealpha = getboolfield_default(L, index,
-			"use_texture_alpha", false);
-	if (usealpha)
-		f.alpha = 0;
+	/* Other stuff */
 
-	// Read node color.
 	lua_getfield(L, index, "color");
 	read_color(L, -1, &f.color);
 	lua_pop(L, 1);
 
 	getstringfield(L, index, "palette", f.palette_name);
 
-	/* Other stuff */
-
 	lua_getfield(L, index, "post_effect_color");
 	read_color(L, -1, &f.post_effect_color);
 	lua_pop(L, 1);
diff --git a/src/script/cpp_api/s_node.cpp b/src/script/cpp_api/s_node.cpp
index e0f9bcd78..269ebacb2 100644
--- a/src/script/cpp_api/s_node.cpp
+++ b/src/script/cpp_api/s_node.cpp
@@ -93,6 +93,14 @@ struct EnumString ScriptApiNode::es_NodeBoxType[] =
 		{0, NULL},
 	};
 
+struct EnumString ScriptApiNode::es_TextureAlphaMode[] =
+	{
+		{ALPHAMODE_OPAQUE, "opaque"},
+		{ALPHAMODE_CLIP, "clip"},
+		{ALPHAMODE_BLEND, "blend"},
+		{0, NULL},
+	};
+
 bool ScriptApiNode::node_on_punch(v3s16 p, MapNode node,
 		ServerActiveObject *puncher, const PointedThing &pointed)
 {
diff --git a/src/script/cpp_api/s_node.h b/src/script/cpp_api/s_node.h
index 81b44f0f0..3f771c838 100644
--- a/src/script/cpp_api/s_node.h
+++ b/src/script/cpp_api/s_node.h
@@ -54,4 +54,5 @@ public:
 	static struct EnumString es_ContentParamType2[];
 	static struct EnumString es_LiquidType[];
 	static struct EnumString es_NodeBoxType[];
+	static struct EnumString es_TextureAlphaMode[];
 };
diff --git a/src/unittest/test.cpp b/src/unittest/test.cpp
index a783ccd32..af324e1b1 100644
--- a/src/unittest/test.cpp
+++ b/src/unittest/test.cpp
@@ -180,7 +180,7 @@ void TestGameDef::defineSomeNodes()
 		"{default_water.png";
 	f = ContentFeatures();
 	f.name = itemdef.name;
-	f.alpha = 128;
+	f.alpha = ALPHAMODE_BLEND;
 	f.liquid_type = LIQUID_SOURCE;
 	f.liquid_viscosity = 4;
 	f.is_ground_content = true;
@@ -201,7 +201,7 @@ void TestGameDef::defineSomeNodes()
 		"{default_lava.png";
 	f = ContentFeatures();
 	f.name = itemdef.name;
-	f.alpha = 128;
+	f.alpha = ALPHAMODE_OPAQUE;
 	f.liquid_type = LIQUID_SOURCE;
 	f.liquid_viscosity = 7;
 	f.light_source = LIGHT_MAX-1;
-- 
cgit v1.2.3


From 5e392cf34f8e062dd0533619921223656e32598a Mon Sep 17 00:00:00 2001
From: sfan5 <sfan5@live.de>
Date: Fri, 29 Jan 2021 13:09:17 +0100
Subject: Refactor utf8_to_wide/wide_to_utf8 functions

---
 src/unittest/test_utilities.cpp | 15 ++++++++---
 src/util/string.cpp             | 57 +++++++++++++++++------------------------
 src/util/string.h               |  6 +++--
 3 files changed, 40 insertions(+), 38 deletions(-)

(limited to 'src/unittest')

diff --git a/src/unittest/test_utilities.cpp b/src/unittest/test_utilities.cpp
index 447b591e1..5559cdbf2 100644
--- a/src/unittest/test_utilities.cpp
+++ b/src/unittest/test_utilities.cpp
@@ -302,9 +302,18 @@ void TestUtilities::testAsciiPrintableHelper()
 
 void TestUtilities::testUTF8()
 {
-	UASSERT(wide_to_utf8(utf8_to_wide("")) == "");
-	UASSERT(wide_to_utf8(utf8_to_wide("the shovel dug a crumbly node!"))
-		== "the shovel dug a crumbly node!");
+	UASSERT(utf8_to_wide("¤") == L"¤");
+
+	UASSERT(wide_to_utf8(L"¤") == "¤");
+
+	UASSERTEQ(std::string, wide_to_utf8(utf8_to_wide("")), "");
+	UASSERTEQ(std::string, wide_to_utf8(utf8_to_wide("the shovel dug a crumbly node!")),
+		"the shovel dug a crumbly node!");
+	UASSERTEQ(std::string, wide_to_utf8(utf8_to_wide("-ä-")),
+		"-ä-");
+	UASSERTEQ(std::string, wide_to_utf8(utf8_to_wide("-\xF0\xA0\x80\x8B-")),
+		"-\xF0\xA0\x80\x8B-");
+
 }
 
 void TestUtilities::testRemoveEscapes()
diff --git a/src/util/string.cpp b/src/util/string.cpp
index 3ac3b8cf0..7e6d6d3b3 100644
--- a/src/util/string.cpp
+++ b/src/util/string.cpp
@@ -50,8 +50,8 @@ static bool parseNamedColorString(const std::string &value, video::SColor &color
 
 #ifndef _WIN32
 
-bool convert(const char *to, const char *from, char *outbuf,
-		size_t outbuf_size, char *inbuf, size_t inbuf_size)
+static bool convert(const char *to, const char *from, char *outbuf,
+		size_t *outbuf_size, char *inbuf, size_t inbuf_size)
 {
 	iconv_t cd = iconv_open(to, from);
 
@@ -60,15 +60,14 @@ bool convert(const char *to, const char *from, char *outbuf,
 #else
 	char *inbuf_ptr = inbuf;
 #endif
-
 	char *outbuf_ptr = outbuf;
 
 	size_t *inbuf_left_ptr = &inbuf_size;
-	size_t *outbuf_left_ptr = &outbuf_size;
 
+	const size_t old_outbuf_size = *outbuf_size;
 	size_t old_size = inbuf_size;
 	while (inbuf_size > 0) {
-		iconv(cd, &inbuf_ptr, inbuf_left_ptr, &outbuf_ptr, outbuf_left_ptr);
+		iconv(cd, &inbuf_ptr, inbuf_left_ptr, &outbuf_ptr, outbuf_size);
 		if (inbuf_size == old_size) {
 			iconv_close(cd);
 			return false;
@@ -77,11 +76,12 @@ bool convert(const char *to, const char *from, char *outbuf,
 	}
 
 	iconv_close(cd);
+	*outbuf_size = old_outbuf_size - *outbuf_size;
 	return true;
 }
 
 #ifdef __ANDROID__
-// Android need manual caring to support the full character set possible with wchar_t
+// On Android iconv disagrees how big a wchar_t is for whatever reason
 const char *DEFAULT_ENCODING = "UTF-32LE";
 #else
 const char *DEFAULT_ENCODING = "WCHAR_T";
@@ -89,58 +89,52 @@ const char *DEFAULT_ENCODING = "WCHAR_T";
 
 std::wstring utf8_to_wide(const std::string &input)
 {
-	size_t inbuf_size = input.length() + 1;
+	const size_t inbuf_size = input.length();
 	// maximum possible size, every character is sizeof(wchar_t) bytes
-	size_t outbuf_size = (input.length() + 1) * sizeof(wchar_t);
+	size_t outbuf_size = input.length() * sizeof(wchar_t);
 
-	char *inbuf = new char[inbuf_size];
+	char *inbuf = new char[inbuf_size]; // intentionally NOT null-terminated
 	memcpy(inbuf, input.c_str(), inbuf_size);
-	char *outbuf = new char[outbuf_size];
-	memset(outbuf, 0, outbuf_size);
+	std::wstring out;
+	out.resize(outbuf_size / sizeof(wchar_t));
 
 #ifdef __ANDROID__
-	// Android need manual caring to support the full character set possible with wchar_t
 	SANITY_CHECK(sizeof(wchar_t) == 4);
 #endif
 
-	if (!convert(DEFAULT_ENCODING, "UTF-8", outbuf, outbuf_size, inbuf, inbuf_size)) {
+	char *outbuf = reinterpret_cast<char*>(&out[0]);
+	if (!convert(DEFAULT_ENCODING, "UTF-8", outbuf, &outbuf_size, inbuf, inbuf_size)) {
 		infostream << "Couldn't convert UTF-8 string 0x" << hex_encode(input)
 			<< " into wstring" << std::endl;
 		delete[] inbuf;
-		delete[] outbuf;
 		return L"<invalid UTF-8 string>";
 	}
-	std::wstring out((wchar_t *)outbuf);
-
 	delete[] inbuf;
-	delete[] outbuf;
 
+	out.resize(outbuf_size / sizeof(wchar_t));
 	return out;
 }
 
 std::string wide_to_utf8(const std::wstring &input)
 {
-	size_t inbuf_size = (input.length() + 1) * sizeof(wchar_t);
-	// maximum possible size: utf-8 encodes codepoints using 1 up to 6 bytes
-	size_t outbuf_size = (input.length() + 1) * 6;
+	const size_t inbuf_size = input.length() * sizeof(wchar_t);
+	// maximum possible size: utf-8 encodes codepoints using 1 up to 4 bytes
+	size_t outbuf_size = input.length() * 4;
 
-	char *inbuf = new char[inbuf_size];
+	char *inbuf = new char[inbuf_size]; // intentionally NOT null-terminated
 	memcpy(inbuf, input.c_str(), inbuf_size);
-	char *outbuf = new char[outbuf_size];
-	memset(outbuf, 0, outbuf_size);
+	std::string out;
+	out.resize(outbuf_size);
 
-	if (!convert("UTF-8", DEFAULT_ENCODING, outbuf, outbuf_size, inbuf, inbuf_size)) {
+	if (!convert("UTF-8", DEFAULT_ENCODING, &out[0], &outbuf_size, inbuf, inbuf_size)) {
 		infostream << "Couldn't convert wstring 0x" << hex_encode(inbuf, inbuf_size)
 			<< " into UTF-8 string" << std::endl;
 		delete[] inbuf;
-		delete[] outbuf;
-		return "<invalid wstring>";
+		return "<invalid wide string>";
 	}
-	std::string out(outbuf);
-
 	delete[] inbuf;
-	delete[] outbuf;
 
+	out.resize(outbuf_size);
 	return out;
 }
 
@@ -172,15 +166,12 @@ std::string wide_to_utf8(const std::wstring &input)
 
 #endif // _WIN32
 
-// You must free the returned string!
-// The returned string is allocated using new
 wchar_t *utf8_to_wide_c(const char *str)
 {
 	std::wstring ret = utf8_to_wide(std::string(str));
 	size_t len = ret.length();
 	wchar_t *ret_c = new wchar_t[len + 1];
-	memset(ret_c, 0, (len + 1) * sizeof(wchar_t));
-	memcpy(ret_c, ret.c_str(), len * sizeof(wchar_t));
+	memcpy(ret_c, ret.c_str(), (len + 1) * sizeof(wchar_t));
 	return ret_c;
 }
 
diff --git a/src/util/string.h b/src/util/string.h
index 6fd11fadc..ec14e9a2d 100644
--- a/src/util/string.h
+++ b/src/util/string.h
@@ -64,11 +64,13 @@ struct FlagDesc {
 	u32 flag;
 };
 
-// try not to convert between wide/utf8 encodings; this can result in data loss
-// try to only convert between them when you need to input/output stuff via Irrlicht
+// Try to avoid converting between wide and UTF-8 unless you need to
+// input/output stuff via Irrlicht
 std::wstring utf8_to_wide(const std::string &input);
 std::string wide_to_utf8(const std::wstring &input);
 
+// You must free the returned string!
+// The returned string is allocated using new[]
 wchar_t *utf8_to_wide_c(const char *str);
 
 // NEVER use those two functions unless you have a VERY GOOD reason to
-- 
cgit v1.2.3


From c834d2ab25694ef2d67dc24f85f304269d202c8e Mon Sep 17 00:00:00 2001
From: sfan5 <sfan5@live.de>
Date: Fri, 29 Jan 2021 14:03:27 +0100
Subject: Drop wide/narrow conversion functions

The only valid usecase for these is interfacing with OS APIs
that want a locale/OS-specific multibyte encoding.
But they weren't used for that anywhere, instead UTF-8 is pretty
much assumed when it comes to that.
Since these are only a potential source of bugs and do not fulfil
their purpose at all, drop them entirely.
---
 src/chat.cpp                        |  4 +--
 src/client/client.cpp               |  4 +--
 src/client/keycode.cpp              |  3 +-
 src/gui/guiConfirmRegistration.cpp  |  3 +-
 src/network/serverpackethandler.cpp |  8 ++---
 src/script/lua_api/l_server.cpp     |  2 +-
 src/server.cpp                      | 61 ++++++++++++++-----------------------
 src/server.h                        |  8 ++---
 src/unittest/test_utilities.cpp     |  4 +--
 src/util/string.cpp                 | 59 ++---------------------------------
 src/util/string.h                   | 28 -----------------
 11 files changed, 41 insertions(+), 143 deletions(-)

(limited to 'src/unittest')

diff --git a/src/chat.cpp b/src/chat.cpp
index 2f65e68b3..c9317a079 100644
--- a/src/chat.cpp
+++ b/src/chat.cpp
@@ -485,8 +485,8 @@ void ChatPrompt::nickCompletion(const std::list<std::string>& names, bool backwa
 	// find all names that start with the selected prefix
 	std::vector<std::wstring> completions;
 	for (const std::string &name : names) {
-		if (str_starts_with(narrow_to_wide(name), prefix, true)) {
-			std::wstring completion = narrow_to_wide(name);
+		std::wstring completion = utf8_to_wide(name);
+		if (str_starts_with(completion, prefix, true)) {
 			if (prefix_start == 0)
 				completion += L": ";
 			completions.push_back(completion);
diff --git a/src/client/client.cpp b/src/client/client.cpp
index 61888b913..ef4a3cdfc 100644
--- a/src/client/client.cpp
+++ b/src/client/client.cpp
@@ -1196,7 +1196,7 @@ void Client::sendChatMessage(const std::wstring &message)
 	if (canSendChatMessage()) {
 		u32 now = time(NULL);
 		float time_passed = now - m_last_chat_message_sent;
-		m_last_chat_message_sent = time(NULL);
+		m_last_chat_message_sent = now;
 
 		m_chat_message_allowance += time_passed * (CLIENT_CHAT_MESSAGE_LIMIT_PER_10S / 8.0f);
 		if (m_chat_message_allowance > CLIENT_CHAT_MESSAGE_LIMIT_PER_10S)
@@ -1832,7 +1832,7 @@ void Client::makeScreenshot()
 				sstr << "Failed to save screenshot '" << filename << "'";
 			}
 			pushToChatQueue(new ChatMessage(CHATMESSAGE_TYPE_SYSTEM,
-					narrow_to_wide(sstr.str())));
+					utf8_to_wide(sstr.str())));
 			infostream << sstr.str() << std::endl;
 			image->drop();
 		}
diff --git a/src/client/keycode.cpp b/src/client/keycode.cpp
index 6a0e9f569..ce5214f54 100644
--- a/src/client/keycode.cpp
+++ b/src/client/keycode.cpp
@@ -316,7 +316,8 @@ KeyPress::KeyPress(const char *name)
 	int chars_read = mbtowc(&Char, name, 1);
 	FATAL_ERROR_IF(chars_read != 1, "Unexpected multibyte character");
 	m_name = "";
-	warningstream << "KeyPress: Unknown key '" << name << "', falling back to first char.";
+	warningstream << "KeyPress: Unknown key '" << name
+		<< "', falling back to first char." << std::endl;
 }
 
 KeyPress::KeyPress(const irr::SEvent::SKeyInput &in, bool prefer_character)
diff --git a/src/gui/guiConfirmRegistration.cpp b/src/gui/guiConfirmRegistration.cpp
index 020a2796a..4a798c39b 100644
--- a/src/gui/guiConfirmRegistration.cpp
+++ b/src/gui/guiConfirmRegistration.cpp
@@ -192,8 +192,7 @@ void GUIConfirmRegistration::acceptInput()
 
 bool GUIConfirmRegistration::processInput()
 {
-	std::wstring m_password_ws = narrow_to_wide(m_password);
-	if (m_password_ws != m_pass_confirm) {
+	if (utf8_to_wide(m_password) != m_pass_confirm) {
 		gui::IGUIElement *e = getElementFromId(ID_message);
 		if (e)
 			e->setVisible(true);
diff --git a/src/network/serverpackethandler.cpp b/src/network/serverpackethandler.cpp
index 4d79f375c..02af06abc 100644
--- a/src/network/serverpackethandler.cpp
+++ b/src/network/serverpackethandler.cpp
@@ -778,15 +778,13 @@ void Server::handleCommand_ChatMessage(NetworkPacket* pkt)
 		return;
 	}
 
-	// Get player name of this client
 	std::string name = player->getName();
-	std::wstring wname = narrow_to_wide(name);
 
-	std::wstring answer_to_sender = handleChat(name, wname, message, true, player);
+	std::wstring answer_to_sender = handleChat(name, message, true, player);
 	if (!answer_to_sender.empty()) {
 		// Send the answer to sender
-		SendChatMessage(peer_id, ChatMessage(CHATMESSAGE_TYPE_NORMAL,
-			answer_to_sender, wname));
+		SendChatMessage(peer_id, ChatMessage(CHATMESSAGE_TYPE_SYSTEM,
+			answer_to_sender));
 	}
 }
 
diff --git a/src/script/lua_api/l_server.cpp b/src/script/lua_api/l_server.cpp
index 78cf4b403..bf5292521 100644
--- a/src/script/lua_api/l_server.cpp
+++ b/src/script/lua_api/l_server.cpp
@@ -44,7 +44,7 @@ int ModApiServer::l_request_shutdown(lua_State *L)
 int ModApiServer::l_get_server_status(lua_State *L)
 {
 	NO_MAP_LOCK_REQUIRED;
-	lua_pushstring(L, wide_to_narrow(getServer(L)->getStatusString()).c_str());
+	lua_pushstring(L, getServer(L)->getStatusString().c_str());
 	return 1;
 }
 
diff --git a/src/server.cpp b/src/server.cpp
index 90496129e..907bc6d24 100644
--- a/src/server.cpp
+++ b/src/server.cpp
@@ -2955,7 +2955,7 @@ void Server::handleChatInterfaceEvent(ChatEvent *evt)
 	}
 }
 
-std::wstring Server::handleChat(const std::string &name, const std::wstring &wname,
+std::wstring Server::handleChat(const std::string &name,
 	std::wstring wmessage, bool check_shout_priv, RemotePlayer *player)
 {
 	// If something goes wrong, this player is to blame
@@ -2993,7 +2993,7 @@ std::wstring Server::handleChat(const std::string &name, const std::wstring &wna
 
 	auto message = trim(wide_to_utf8(wmessage));
 	if (message.find_first_of("\n\r") != std::wstring::npos) {
-		return L"New lines are not permitted in chat messages";
+		return L"Newlines are not permitted in chat messages";
 	}
 
 	// Run script hook, exit if script ate the chat message
@@ -3014,10 +3014,10 @@ std::wstring Server::handleChat(const std::string &name, const std::wstring &wna
 			the Cyrillic alphabet and some characters on older Android devices
 		*/
 #ifdef __ANDROID__
-		line += L"<" + wname + L"> " + wmessage;
+		line += L"<" + utf8_to_wide(name) + L"> " + wmessage;
 #else
-		line += narrow_to_wide(m_script->formatChatMessage(name,
-				wide_to_narrow(wmessage)));
+		line += utf8_to_wide(m_script->formatChatMessage(name,
+				wide_to_utf8(wmessage)));
 #endif
 	}
 
@@ -3030,35 +3030,23 @@ std::wstring Server::handleChat(const std::string &name, const std::wstring &wna
 	/*
 		Send the message to others
 	*/
-	actionstream << "CHAT: " << wide_to_narrow(unescape_enriched(line)) << std::endl;
+	actionstream << "CHAT: " << wide_to_utf8(unescape_enriched(line)) << std::endl;
 
-	std::vector<session_t> clients = m_clients.getClientIDs();
-
-	/*
-		Send the message back to the inital sender
-		if they are using protocol version >= 29
-	*/
-
-	session_t peer_id_to_avoid_sending =
-		(player ? player->getPeerId() : PEER_ID_INEXISTENT);
+	ChatMessage chatmsg(line);
 
-	if (player && player->protocol_version >= 29)
-		peer_id_to_avoid_sending = PEER_ID_INEXISTENT;
+	std::vector<session_t> clients = m_clients.getClientIDs();
+	for (u16 cid : clients)
+		SendChatMessage(cid, chatmsg);
 
-	for (u16 cid : clients) {
-		if (cid != peer_id_to_avoid_sending)
-			SendChatMessage(cid, ChatMessage(line));
-	}
 	return L"";
 }
 
 void Server::handleAdminChat(const ChatEventChat *evt)
 {
 	std::string name = evt->nick;
-	std::wstring wname = utf8_to_wide(name);
 	std::wstring wmessage = evt->evt_msg;
 
-	std::wstring answer = handleChat(name, wname, wmessage);
+	std::wstring answer = handleChat(name, wmessage);
 
 	// If asked to send answer to sender
 	if (!answer.empty()) {
@@ -3095,46 +3083,43 @@ PlayerSAO *Server::getPlayerSAO(session_t peer_id)
 	return player->getPlayerSAO();
 }
 
-std::wstring Server::getStatusString()
+std::string Server::getStatusString()
 {
-	std::wostringstream os(std::ios_base::binary);
-	os << L"# Server: ";
+	std::ostringstream os(std::ios_base::binary);
+	os << "# Server: ";
 	// Version
-	os << L"version=" << narrow_to_wide(g_version_string);
+	os << "version=" << g_version_string;
 	// Uptime
-	os << L", uptime=" << m_uptime_counter->get();
+	os << ", uptime=" << m_uptime_counter->get();
 	// Max lag estimate
-	os << L", max_lag=" << (m_env ? m_env->getMaxLagEstimate() : 0);
+	os << ", max_lag=" << (m_env ? m_env->getMaxLagEstimate() : 0);
 
 	// Information about clients
 	bool first = true;
-	os << L", clients={";
+	os << ", clients={";
 	if (m_env) {
 		std::vector<session_t> clients = m_clients.getClientIDs();
 		for (session_t client_id : clients) {
 			RemotePlayer *player = m_env->getPlayer(client_id);
 
 			// Get name of player
-			std::wstring name = L"unknown";
-			if (player)
-				name = narrow_to_wide(player->getName());
+			const char *name = player ? player->getName() : "<unknown>";
 
 			// Add name to information string
 			if (!first)
-				os << L", ";
+				os << ", ";
 			else
 				first = false;
-
 			os << name;
 		}
 	}
-	os << L"}";
+	os << "}";
 
 	if (m_env && !((ServerMap*)(&m_env->getMap()))->isSavingEnabled())
-		os << std::endl << L"# Server: " << " WARNING: Map saving is disabled.";
+		os << std::endl << "# Server: " << " WARNING: Map saving is disabled.";
 
 	if (!g_settings->get("motd").empty())
-		os << std::endl << L"# Server: " << narrow_to_wide(g_settings->get("motd"));
+		os << std::endl << "# Server: " << g_settings->get("motd");
 
 	return os.str();
 }
diff --git a/src/server.h b/src/server.h
index 5c143a657..0b4084aa9 100644
--- a/src/server.h
+++ b/src/server.h
@@ -219,7 +219,7 @@ public:
 	void onMapEditEvent(const MapEditEvent &event);
 
 	// Connection must be locked when called
-	std::wstring getStatusString();
+	std::string getStatusString();
 	inline double getUptime() const { return m_uptime_counter->get(); }
 
 	// read shutdown state
@@ -495,10 +495,8 @@ private:
 	void handleChatInterfaceEvent(ChatEvent *evt);
 
 	// This returns the answer to the sender of wmessage, or "" if there is none
-	std::wstring handleChat(const std::string &name, const std::wstring &wname,
-		std::wstring wmessage_input,
-		bool check_shout_priv = false,
-		RemotePlayer *player = NULL);
+	std::wstring handleChat(const std::string &name, std::wstring wmessage_input,
+		bool check_shout_priv = false, RemotePlayer *player = nullptr);
 	void handleAdminChat(const ChatEventChat *evt);
 
 	// When called, connection mutex should be locked
diff --git a/src/unittest/test_utilities.cpp b/src/unittest/test_utilities.cpp
index 5559cdbf2..93ba3f844 100644
--- a/src/unittest/test_utilities.cpp
+++ b/src/unittest/test_utilities.cpp
@@ -247,8 +247,8 @@ void TestUtilities::testStartsWith()
 
 void TestUtilities::testStrEqual()
 {
-	UASSERT(str_equal(narrow_to_wide("abc"), narrow_to_wide("abc")));
-	UASSERT(str_equal(narrow_to_wide("ABC"), narrow_to_wide("abc"), true));
+	UASSERT(str_equal(utf8_to_wide("abc"), utf8_to_wide("abc")));
+	UASSERT(str_equal(utf8_to_wide("ABC"), utf8_to_wide("abc"), true));
 }
 
 
diff --git a/src/util/string.cpp b/src/util/string.cpp
index 7e6d6d3b3..611ad35cb 100644
--- a/src/util/string.cpp
+++ b/src/util/string.cpp
@@ -175,62 +175,6 @@ wchar_t *utf8_to_wide_c(const char *str)
 	return ret_c;
 }
 
-// You must free the returned string!
-// The returned string is allocated using new
-wchar_t *narrow_to_wide_c(const char *str)
-{
-	wchar_t *nstr = nullptr;
-#if defined(_WIN32)
-	int nResult = MultiByteToWideChar(CP_UTF8, 0, (LPCSTR) str, -1, 0, 0);
-	if (nResult == 0) {
-		errorstream<<"gettext: MultiByteToWideChar returned null"<<std::endl;
-	} else {
-		nstr = new wchar_t[nResult];
-		MultiByteToWideChar(CP_UTF8, 0, (LPCSTR) str, -1, (WCHAR *) nstr, nResult);
-	}
-#else
-	size_t len = strlen(str);
-	nstr = new wchar_t[len + 1];
-
-	std::wstring intermediate = narrow_to_wide(str);
-	memset(nstr, 0, (len + 1) * sizeof(wchar_t));
-	memcpy(nstr, intermediate.c_str(), len * sizeof(wchar_t));
-#endif
-
-	return nstr;
-}
-
-std::wstring narrow_to_wide(const std::string &mbs) {
-#ifdef __ANDROID__
-	return utf8_to_wide(mbs);
-#else
-	size_t wcl = mbs.size();
-	Buffer<wchar_t> wcs(wcl + 1);
-	size_t len = mbstowcs(*wcs, mbs.c_str(), wcl);
-	if (len == (size_t)(-1))
-		return L"<invalid multibyte string>";
-	wcs[len] = 0;
-	return *wcs;
-#endif
-}
-
-
-std::string wide_to_narrow(const std::wstring &wcs)
-{
-#ifdef __ANDROID__
-	return wide_to_utf8(wcs);
-#else
-	size_t mbl = wcs.size() * 4;
-	SharedBuffer<char> mbs(mbl+1);
-	size_t len = wcstombs(*mbs, wcs.c_str(), mbl);
-	if (len == (size_t)(-1))
-		return "Character conversion failed!";
-
-	mbs[len] = 0;
-	return *mbs;
-#endif
-}
-
 
 std::string urlencode(const std::string &str)
 {
@@ -757,7 +701,8 @@ void translate_string(const std::wstring &s, Translations *translations,
 		} else {
 			// This is an escape sequence *inside* the template string to translate itself.
 			// This should not happen, show an error message.
-			errorstream << "Ignoring escape sequence '" << wide_to_narrow(escape_sequence) << "' in translation" << std::endl;
+			errorstream << "Ignoring escape sequence '"
+				<< wide_to_utf8(escape_sequence) << "' in translation" << std::endl;
 		}
 	}
 
diff --git a/src/util/string.h b/src/util/string.h
index ec14e9a2d..d4afcaec8 100644
--- a/src/util/string.h
+++ b/src/util/string.h
@@ -73,16 +73,6 @@ std::string wide_to_utf8(const std::wstring &input);
 // The returned string is allocated using new[]
 wchar_t *utf8_to_wide_c(const char *str);
 
-// NEVER use those two functions unless you have a VERY GOOD reason to
-// they just convert between wide and multibyte encoding
-// multibyte encoding depends on current locale, this is no good, especially on Windows
-
-// You must free the returned string!
-// The returned string is allocated using new
-wchar_t *narrow_to_wide_c(const char *str);
-std::wstring narrow_to_wide(const std::string &mbs);
-std::string wide_to_narrow(const std::wstring &wcs);
-
 std::string urlencode(const std::string &str);
 std::string urldecode(const std::string &str);
 u32 readFlagString(std::string str, const FlagDesc *flagdesc, u32 *flagmask);
@@ -355,11 +345,6 @@ inline s32 mystoi(const std::string &str, s32 min, s32 max)
 	return i;
 }
 
-
-// MSVC2010 includes it's own versions of these
-//#if !defined(_MSC_VER) || _MSC_VER < 1600
-
-
 /**
  * Returns a 32-bit value reprensented by the string \p str (decimal).
  * @see atoi(3) for further limitations
@@ -369,17 +354,6 @@ inline s32 mystoi(const std::string &str)
 	return atoi(str.c_str());
 }
 
-
-/**
- * Returns s 32-bit value represented by the wide string \p str (decimal).
- * @see atoi(3) for further limitations
- */
-inline s32 mystoi(const std::wstring &str)
-{
-	return mystoi(wide_to_narrow(str));
-}
-
-
 /**
  * Returns a float reprensented by the string \p str (decimal).
  * @see atof(3)
@@ -389,8 +363,6 @@ inline float mystof(const std::string &str)
 	return atof(str.c_str());
 }
 
-//#endif
-
 #define stoi mystoi
 #define stof mystof
 
-- 
cgit v1.2.3


From 674d67f312c815e7f10dc00705e352bc392fc2af Mon Sep 17 00:00:00 2001
From: sfan5 <sfan5@live.de>
Date: Fri, 29 Jan 2021 15:24:07 +0100
Subject: Encode high codepoints as surrogates to safely transport wchar_t over
 network

fixes #7643
---
 src/network/networkpacket.cpp       | 49 ++++++++++++++++++++++++++++++-------
 src/network/networkpacket.h         |  2 +-
 src/network/serverpackethandler.cpp | 15 +-----------
 src/server.cpp                      |  3 ++-
 src/unittest/test_connection.cpp    | 35 ++++++++++++++++++++++++++
 5 files changed, 79 insertions(+), 25 deletions(-)

(limited to 'src/unittest')

diff --git a/src/network/networkpacket.cpp b/src/network/networkpacket.cpp
index 6d0abb12c..a71e26572 100644
--- a/src/network/networkpacket.cpp
+++ b/src/network/networkpacket.cpp
@@ -50,7 +50,7 @@ void NetworkPacket::checkReadOffset(u32 from_offset, u32 field_size)
 	}
 }
 
-void NetworkPacket::putRawPacket(u8 *data, u32 datasize, session_t peer_id)
+void NetworkPacket::putRawPacket(const u8 *data, u32 datasize, session_t peer_id)
 {
 	// If a m_command is already set, we are rewriting on same packet
 	// This is not permitted
@@ -145,6 +145,8 @@ void NetworkPacket::putLongString(const std::string &src)
 	putRawString(src.c_str(), msgsize);
 }
 
+static constexpr bool NEED_SURROGATE_CODING = sizeof(wchar_t) > 2;
+
 NetworkPacket& NetworkPacket::operator>>(std::wstring& dst)
 {
 	checkReadOffset(m_read_offset, 2);
@@ -160,9 +162,16 @@ NetworkPacket& NetworkPacket::operator>>(std::wstring& dst)
 	checkReadOffset(m_read_offset, strLen * 2);
 
 	dst.reserve(strLen);
-	for(u16 i=0; i<strLen; i++) {
-		wchar_t c16 = readU16(&m_data[m_read_offset]);
-		dst.append(&c16, 1);
+	for (u16 i = 0; i < strLen; i++) {
+		wchar_t c = readU16(&m_data[m_read_offset]);
+		if (NEED_SURROGATE_CODING && c >= 0xD800 && c < 0xDC00 && i+1 < strLen) {
+			i++;
+			m_read_offset += sizeof(u16);
+
+			wchar_t c2 = readU16(&m_data[m_read_offset]);
+			c = 0x10000 + ( ((c & 0x3ff) << 10) | (c2 & 0x3ff) );
+		}
+		dst.push_back(c);
 		m_read_offset += sizeof(u16);
 	}
 
@@ -175,15 +184,37 @@ NetworkPacket& NetworkPacket::operator<<(const std::wstring &src)
 		throw PacketError("String too long");
 	}
 
-	u16 msgsize = src.size();
+	if (!NEED_SURROGATE_CODING || src.size() == 0) {
+		*this << static_cast<u16>(src.size());
+		for (u16 i = 0; i < src.size(); i++)
+			*this << static_cast<u16>(src[i]);
 
-	*this << msgsize;
+		return *this;
+	}
 
-	// Write string
-	for (u16 i=0; i<msgsize; i++) {
-		*this << (u16) src[i];
+	// write dummy value, to be overwritten later
+	const u32 len_offset = m_read_offset;
+	u32 written = 0;
+	*this << static_cast<u16>(0xfff0);
+
+	for (u16 i = 0; i < src.size(); i++) {
+		wchar_t c = src[i];
+		if (c > 0xffff) {
+			// Encode high code-points as surrogate pairs
+			u32 n = c - 0x10000;
+			*this << static_cast<u16>(0xD800 | (n >> 10))
+				<< static_cast<u16>(0xDC00 | (n & 0x3ff));
+			written += 2;
+		} else {
+			*this << static_cast<u16>(c);
+			written++;
+		}
 	}
 
+	if (written > WIDE_STRING_MAX_LEN)
+		throw PacketError("String too long");
+	writeU16(&m_data[len_offset], written);
+
 	return *this;
 }
 
diff --git a/src/network/networkpacket.h b/src/network/networkpacket.h
index e77bfb744..c7ff03b8e 100644
--- a/src/network/networkpacket.h
+++ b/src/network/networkpacket.h
@@ -34,7 +34,7 @@ public:
 
 	~NetworkPacket();
 
-	void putRawPacket(u8 *data, u32 datasize, session_t peer_id);
+	void putRawPacket(const u8 *data, u32 datasize, session_t peer_id);
 	void clear();
 
 	// Getters
diff --git a/src/network/serverpackethandler.cpp b/src/network/serverpackethandler.cpp
index 02af06abc..270b8e01f 100644
--- a/src/network/serverpackethandler.cpp
+++ b/src/network/serverpackethandler.cpp
@@ -752,21 +752,8 @@ void Server::handleCommand_InventoryAction(NetworkPacket* pkt)
 
 void Server::handleCommand_ChatMessage(NetworkPacket* pkt)
 {
-	/*
-		u16 command
-		u16 length
-		wstring message
-	*/
-	u16 len;
-	*pkt >> len;
-
 	std::wstring message;
-	for (u16 i = 0; i < len; i++) {
-		u16 tmp_wchar;
-		*pkt >> tmp_wchar;
-
-		message += (wchar_t)tmp_wchar;
-	}
+	*pkt >> message;
 
 	session_t peer_id = pkt->getPeerId();
 	RemotePlayer *player = m_env->getPlayer(peer_id);
diff --git a/src/server.cpp b/src/server.cpp
index 907bc6d24..b815558fb 100644
--- a/src/server.cpp
+++ b/src/server.cpp
@@ -1482,7 +1482,8 @@ void Server::SendChatMessage(session_t peer_id, const ChatMessage &message)
 	NetworkPacket pkt(TOCLIENT_CHAT_MESSAGE, 0, peer_id);
 	u8 version = 1;
 	u8 type = message.type;
-	pkt << version << type << std::wstring(L"") << message.message << (u64)message.timestamp;
+	pkt << version << type << message.sender << message.message
+		<< static_cast<u64>(message.timestamp);
 
 	if (peer_id != PEER_ID_INEXISTENT) {
 		RemotePlayer *player = m_env->getPlayer(peer_id);
diff --git a/src/unittest/test_connection.cpp b/src/unittest/test_connection.cpp
index c5e4085e1..c3aacc536 100644
--- a/src/unittest/test_connection.cpp
+++ b/src/unittest/test_connection.cpp
@@ -39,6 +39,7 @@ public:
 
 	void runTests(IGameDef *gamedef);
 
+	void testNetworkPacketSerialize();
 	void testHelpers();
 	void testConnectSendReceive();
 };
@@ -47,6 +48,7 @@ static TestConnection g_test_instance;
 
 void TestConnection::runTests(IGameDef *gamedef)
 {
+	TEST(testNetworkPacketSerialize);
 	TEST(testHelpers);
 	TEST(testConnectSendReceive);
 }
@@ -78,6 +80,39 @@ struct Handler : public con::PeerHandler
 	const char *name;
 };
 
+void TestConnection::testNetworkPacketSerialize()
+{
+	const static u8 expected[] = {
+		0x00, 0x7b,
+		0x00, 0x02, 0xd8, 0x42, 0xdf, 0x9a
+	};
+
+	if (sizeof(wchar_t) == 2)
+		warningstream << __func__ << " may fail on this platform." << std::endl;
+
+	{
+		NetworkPacket pkt(123, 0);
+
+		// serializing wide strings should do surrogate encoding, we test that here
+		pkt << std::wstring(L"\U00020b9a");
+
+		SharedBuffer<u8> buf = pkt.oldForgePacket();
+		UASSERTEQ(int, buf.getSize(), sizeof(expected));
+		UASSERT(!memcmp(expected, &buf[0], buf.getSize()));
+	}
+
+	{
+		NetworkPacket pkt;
+		pkt.putRawPacket(expected, sizeof(expected), 0);
+
+		// same for decoding
+		std::wstring pkt_s;
+		pkt >> pkt_s;
+
+		UASSERT(pkt_s == L"\U00020b9a");
+	}
+}
+
 void TestConnection::testHelpers()
 {
 	// Some constants for testing
-- 
cgit v1.2.3