Possible to corrupt minetest.conf by setting font_path to nonsense value in advanced settings #4095

Wuzzy2 · 2016-05-05T23:52:01Z

Here's how to break Minetest:

Start Minetest
Go to Advanced settings
Edit font_path (Client setting), remove the string from the input field and hit “save”

Minetest immediately crashes. Even worse, Minetest fails to start afterwards since font_path = (without value) is written into minetest.conf.
The problem seems to be that Minetest requires a valid font_path no matter what. There is no fallback. There also does not seem to be any sanity check in the main menu. So one typo means breakage as well.

I suspect similar issues with other font-related settings.

The text was updated successfully, but these errors were encountered:

Wuzzy2 · 2016-12-14T03:05:10Z

Can you please add this to the 0.4.15 milestone? IMO players being able to completely lock themselves out of Minetest is pretty serious.

srifqi · 2017-05-30T08:50:53Z

I do some tests and here is the result:

`font_path`	`mono_font_path`	`fallback_font_path`	Result
✔️	✔️	✔️	🆗
✔️	✔️	❌	🆗
✔️	❌	✔️	🆗
✔️	❌	❌	🆗
❌	✔️	✔️	🆗 with different look
❌	✔️	❌	❌ crashed
❌	❌	✔️	🆗 with different look
❌	❌	❌	❌ crashed

Tested with Indonesian locale on Minetest 0.4.15-dev.

✔️ means default setting.
❌ means setting set empty.

EDIT: This test is starting Minetest until Main Menu shown, I haven't test it to play the game.

srifqi · 2017-05-30T08:56:56Z

What if we check for custom path? When it is not found or can't be loaded, just use default and then show dialog that the custom font path is not exist.

Wuzzy2 · 2017-05-30T13:54:28Z

Would it be possible to hardcode a font path in case everythng fails?

When you launch Minetest from console, Minetest is begging the player to fix the font_path. :D
That's not very useful, as players normally do not start Minetest from console.

srifqi · 2017-05-30T21:16:26Z

We have default values where points to default fonts from Minetest, which is hard-coded.

https://github.com/minetest/minetest/blob/master/src/defaultsettings.cpp#L228-L248

But still it can be overridden. Should we add a constant value which can't get overridden?

srifqi · 2017-05-31T06:27:46Z

Just tried to fix, I until now just fixing so that it won't crash (look up my table above).

I check whether fallback_font_path is different than its default. If different, try to use default value.

It works! It won't crash even when those three values are set to empty.

font_path = 
mono_font_path = 
fallback_font_path =

This is the diff:

diff --git a/src/fontengine.cpp b/src/fontengine.cpp
index da327c3..a01e4cf 100644
--- a/src/fontengine.cpp
+++ b/src/fontengine.cpp
@@ -22,6 +22,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include "porting.h"
 #include "constants.h"
 #include "filesys.h"
+#include "defaultsettings.h"
 
 #if USE_FREETYPE
 #include "gettext.h"
@@ -43,6 +44,7 @@ static void font_setting_changed(const std::string &name, void *userdata)
 /******************************************************************************/
 FontEngine::FontEngine(Settings* main_settings, gui::IGUIEnvironment* env) :
 	m_settings(main_settings),
+	m_default_settings(),
 	m_env(env),
 	m_font_cache(),
 	m_currentMode(FM_Standard),
@@ -59,6 +61,7 @@ FontEngine::FontEngine(Settings* main_settings, gui::IGUIEnvironment* env) :
 	assert(m_env != NULL); // pre-condition
 	assert(m_env->getSkin() != NULL); // pre-condition
 
+	set_default_settings(&m_default_settings);
 	m_currentMode = FM_Simple;
 
 #if USE_FREETYPE
@@ -361,6 +364,23 @@ void FontEngine::initFont(unsigned int basesize, FontMode mode)
 			return;
 		}
 
+		std::string fallback_font_path = m_default_settings.get(font_config_prefix + "fallback_font_path");
+
+		if (font_path != fallback_font_path) {
+			// try original fallback font
+			errorstream << "FontEngine: failed to load custom fallback font: " << font_path << ", "
+					"trying to fall back to original fallback font" << std::endl;
+
+			font = gui::CGUITTFont::createTTFont(m_env,
+				fallback_font_path.c_str(), size, true, true,
+				font_shadow, font_shadow_alpha);
+
+			if (font != NULL) {
+				m_font_cache[mode][basesize] = font;
+				return;
+			}
+		}
+
 		// give up
 		errorstream << "FontEngine: failed to load freetype font: "
 				<< font_path << std::endl;
diff --git a/src/fontengine.h b/src/fontengine.h
index 9f72667..648f344 100644
--- a/src/fontengine.h
+++ b/src/fontengine.h
@@ -110,6 +110,9 @@ class FontEngine
 	/** pointer to settings for registering callbacks or reading config */
 	Settings* m_settings;
 
+	/** settings to store default settings */
+	Settings m_default_settings;
+
 	/** pointer to irrlicht gui environment */
 	gui::IGUIEnvironment* m_env;

(Should I make PR?)

Wuzzy2 · 2017-05-31T11:02:14Z

Your patch does not work when font_path, mono_font_path and fallback_font_path are empty.
Minetest segfaults as soon as I start an actual game.

I think the mono font is only used in-game (for the chat console, I guess).

nerzhul · 2017-05-31T12:50:18Z

The diff is crappy, you instanciate a useless path, it's better to prevent wrong value at setting read

srifqi · 2017-05-31T13:50:27Z

@Wuzzy2 TBH, I haven't tried to start the game. (^_^")\

@nerzhul I was just trying to do as what Wuzzy2 suggested: use default as fall back. But, that's nice idea, too.

srifqi · 2017-05-31T15:45:20Z

I've tried to fix crash when the game starts. Still haven't prevent wrong value at setting read.

This is the diff:

diff --git a/src/fontengine.cpp b/src/fontengine.cpp
index da327c3..60e6f44 100644
--- a/src/fontengine.cpp
+++ b/src/fontengine.cpp
@@ -22,6 +22,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include "porting.h"
 #include "constants.h"
 #include "filesys.h"
+#include "defaultsettings.h"
 
 #if USE_FREETYPE
 #include "gettext.h"
@@ -43,6 +44,7 @@ static void font_setting_changed(const std::string &name, void *userdata)
 /******************************************************************************/
 FontEngine::FontEngine(Settings* main_settings, gui::IGUIEnvironment* env) :
 	m_settings(main_settings),
+	m_default_settings(),
 	m_env(env),
 	m_font_cache(),
 	m_currentMode(FM_Standard),
@@ -59,6 +61,7 @@ FontEngine::FontEngine(Settings* main_settings, gui::IGUIEnvironment* env) :
 	assert(m_env != NULL); // pre-condition
 	assert(m_env->getSkin() != NULL); // pre-condition
 
+	set_default_settings(&m_default_settings);
 	m_currentMode = FM_Simple;
 
 #if USE_FREETYPE
@@ -346,19 +349,55 @@ void FontEngine::initFont(unsigned int basesize, FontMode mode)
 			return;
 		}
 
-		// try fallback font
-		errorstream << "FontEngine: failed to load: " << font_path << ", trying to fall back "
-				"to fallback font" << std::endl;
+		if (font_config_prefix == "mono_") {
+			std::string mono_font_path = m_default_settings.get("mono_font_path");
 
-		font_path = g_settings->get(font_config_prefix + "fallback_font_path");
+			if (font_path != mono_font_path) {
+				// try original mono font
+				errorstream << "FontEngine: failed to load custom mono font: " << font_path << ", "
+						"trying to fall back to original mono font" << std::endl;
 
-		font = gui::CGUITTFont::createTTFont(m_env,
-			font_path.c_str(), size, true, true, font_shadow,
-			font_shadow_alpha);
+				font = gui::CGUITTFont::createTTFont(m_env,
+					mono_font_path.c_str(), size, true, true,
+					font_shadow, font_shadow_alpha);
 
-		if (font != NULL) {
-			m_font_cache[mode][basesize] = font;
-			return;
+				if (font != NULL) {
+					m_font_cache[mode][basesize] = font;
+					return;
+				}
+			}
+		} else {
+			// try fallback font
+			errorstream << "FontEngine: failed to load: " << font_path << ", trying to fall back "
+					"to fallback font" << std::endl;
+
+			font_path = g_settings->get(font_config_prefix + "fallback_font_path");
+
+			font = gui::CGUITTFont::createTTFont(m_env,
+				font_path.c_str(), size, true, true, font_shadow,
+				font_shadow_alpha);
+
+			if (font != NULL) {
+				m_font_cache[mode][basesize] = font;
+				return;
+			}
+
+			std::string fallback_font_path = m_default_settings.get(font_config_prefix + "fallback_font_path");
+
+			if (font_path != fallback_font_path) {
+				// try original fallback font
+				errorstream << "FontEngine: failed to load custom fallback font: " << font_path << ", "
+						"trying to fall back to original fallback font" << std::endl;
+
+				font = gui::CGUITTFont::createTTFont(m_env,
+					fallback_font_path.c_str(), size, true, true,
+					font_shadow, font_shadow_alpha);
+
+				if (font != NULL) {
+					m_font_cache[mode][basesize] = font;
+					return;
+				}
+			}
 		}
 
 		// give up
diff --git a/src/fontengine.h b/src/fontengine.h
index 9f72667..648f344 100644
--- a/src/fontengine.h
+++ b/src/fontengine.h
@@ -110,6 +110,9 @@ class FontEngine
 	/** pointer to settings for registering callbacks or reading config */
 	Settings* m_settings;
 
+	/** settings to store default settings */
+	Settings m_default_settings;
+
 	/** pointer to irrlicht gui environment */
 	gui::IGUIEnvironment* m_env;

nerzhul · 2017-06-01T06:02:45Z

you are always storing a useless default settings instance and copy the whole class for ever in MT memory

srifqi · 2017-06-01T06:06:00Z

Any idea on how to just pick some default values? Should we add #define DEFAULT_FONT_PATH "path".ttf" and just use them?

Zeno- · 2017-06-01T10:08:58Z

Nah, just make a copy of whatever the default string is and not the whole object

e.g. m_blah = m_settings.get(font_config_prefix + "fallback_font_path");

srifqi · 2017-06-01T14:43:46Z

@Zeno- Wouldn't that will give modified value instead of default?

srifqi · 2017-06-03T03:02:40Z

Okay, I have another idea. Check my PR #5886.

Wuzzy2 · 2018-05-01T11:38:22Z

I just re-tested this in 55bb193.

The crashes do not occour anymore. The bug is fixed.

Ekdohibs added Bug Issues that were confirmed to be a bug @ Client / Audiovisuals labels May 6, 2016

Wuzzy2 closed this as completed May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Possible to corrupt minetest.conf by setting font_path to nonsense value in advanced settings #4095

Possible to corrupt minetest.conf by setting font_path to nonsense value in advanced settings #4095

Wuzzy2 commented May 5, 2016 •

edited

Wuzzy2 commented Dec 14, 2016

srifqi commented May 30, 2017 •

edited

srifqi commented May 30, 2017

Wuzzy2 commented May 30, 2017

srifqi commented May 30, 2017

srifqi commented May 31, 2017 •

edited

Wuzzy2 commented May 31, 2017

nerzhul commented May 31, 2017

srifqi commented May 31, 2017 •

edited

srifqi commented May 31, 2017

nerzhul commented Jun 1, 2017

srifqi commented Jun 1, 2017

Zeno- commented Jun 1, 2017 •

edited

srifqi commented Jun 1, 2017

srifqi commented Jun 3, 2017

Wuzzy2 commented May 1, 2018

Possible to corrupt minetest.conf by setting font_path to nonsense value in advanced settings #4095

Possible to corrupt minetest.conf by setting font_path to nonsense value in advanced settings #4095

Comments

Wuzzy2 commented May 5, 2016 • edited

Wuzzy2 commented Dec 14, 2016

srifqi commented May 30, 2017 • edited

srifqi commented May 30, 2017

Wuzzy2 commented May 30, 2017

srifqi commented May 30, 2017

srifqi commented May 31, 2017 • edited

Wuzzy2 commented May 31, 2017

nerzhul commented May 31, 2017

srifqi commented May 31, 2017 • edited

srifqi commented May 31, 2017

nerzhul commented Jun 1, 2017

srifqi commented Jun 1, 2017

Zeno- commented Jun 1, 2017 • edited

srifqi commented Jun 1, 2017

srifqi commented Jun 3, 2017

Wuzzy2 commented May 1, 2018

Wuzzy2 commented May 5, 2016 •

edited

srifqi commented May 30, 2017 •

edited

srifqi commented May 31, 2017 •

edited

srifqi commented May 31, 2017 •

edited

Zeno- commented Jun 1, 2017 •

edited