Moved all sanity checking for OptionRowHandlerLua to a separate function so that a malformed row does not crash StepMania, and instead prints an error to the log file, and creates a row that does nothing. Added AllowAnything arg to CheckEnum.

Docs/Themerdocs/Examples/OptionRowHandlerLua.lua

@@ -46,6 +46,8 @@ function FooMods()
 		-- PlayerNumbers that are allowed to use the row.
 		-- A row that can't be used by one player can be confusing for the players
 		-- so consider carefully before using this.
+		-- This function will be called an extra time during loading to ensure
+		-- it returns a table.
 		EnabledForPlayers= function(self)
 			Trace("FooMods:EnabledForPlayers() called.")
 			-- Leave out PLAYER_1 just for example.

Themes/_fallback/Languages/en.ini

@@ -401,6 +401,7 @@ Hide=Hide
 Holds=Holds
 Insert=Insert
 Insert Credit=Insert Credit
+Invalid=Invalid row definition.
 Key Joy Mappings=Map keys and joystick buttons to game functions.
 Language=Choose your language.
 LifeDifficulty=Increase this value to cause your life meter to drain faster and refill slower.
@@ -975,6 +976,7 @@ Insert=Insert
 Insert Credit=Insert Credit
 Insert beat and shift down=Insert beat and shift down
 Insert Entry=Insert Entry
+Invalid=Invalid
 Invert notes' player=Invert player notes (routine only)
 Jumps=Jumps
 Key Joy Mappings=Config Key/Joy Mappings

src/EnumHelper.cpp

@@ -2,8 +2,9 @@
 #include "EnumHelper.h"
 #include "LuaManager.h"
 #include "RageUtil.h"
+#include "RageLog.h"
 
-int CheckEnum( lua_State *L, LuaReference &table, int iPos, int iInvalid, const char *szType, bool bAllowInvalid )
+int CheckEnum( lua_State *L, LuaReference &table, int iPos, int iInvalid, const char *szType, bool bAllowInvalid, bool bAllowAnything )
 {
 	luaL_checkany( L, iPos );
 
@@ -61,6 +62,18 @@ int CheckEnum( lua_State *L, LuaReference &table, int iPos, int iInvalid, const
 			LuaHelpers::Pop( L, sGot );
 		}
 		LuaHelpers::Push( L, ssprintf("Expected %s; got %s", szType, sGot.c_str() ) );
+		// There are a couple places where CheckEnum is used outside of a
+		// function called from lua.  If we use lua_error from one of them,
+		// StepMania crashes out completely.  bAllowAnything allows those places
+		// to avoid crashing over theme mistakes.
+		if(bAllowAnything)
+		{
+			RString errmsg;
+			LuaHelpers::Pop(L, errmsg);
+			LOG->Warn(errmsg.c_str());
+			lua_pop(L, 2);
+			return iInvalid;
+		}
 		lua_error( L );
 	}
 	int iRet = lua_tointeger( L, -1 );

src/EnumHelper.h

@@ -20,7 +20,8 @@ int CheckEnum(lua_State *L,
 	      int iPos,
 	      int iInvalid,
 	      const char *szType,
-	      bool bAllowInvalid);
+	      bool bAllowInvalid,
+	      bool bAllowAnything= false);
 
 template<typename T>
 struct EnumTraits
@@ -36,14 +37,15 @@ template<typename T> LuaReference EnumTraits<T>::EnumToString;
 namespace Enum
 {
 	template<typename T>
-	static T Check( lua_State *L, int iPos, bool bAllowInvalid = false )
+	static T Check( lua_State *L, int iPos, bool bAllowInvalid = false, bool bAllowAnything= false )
 	{
 		return (T) CheckEnum(L,
 				     EnumTraits<T>::StringToEnum,
 				     iPos,
 				     EnumTraits<T>::Invalid,
 				     EnumTraits<T>::szName,
-				     bAllowInvalid);
+				     bAllowInvalid,
+				     bAllowAnything);
 	}
 	template<typename T>
 	static void Push( lua_State *L, T iVal )

src/LuaManager.h

@@ -213,6 +213,26 @@ inline bool MyLua_checkintboolean( lua_State *L, int iArg )
 	return MyLua_checkboolean( L, iArg );
 }
 
+// Checks the table at index to verify that it contains strings.
+inline bool TableContainsOnlyStrings(lua_State* L, int index)
+{
+	bool passed= true;
+	lua_pushnil(L);
+	while(lua_next(L, index) != 0)
+	{
+		// `key' is at index -2 and `value' at index -1
+		const char *pValue = lua_tostring(L, -1);
+		if(pValue == NULL)
+		{
+			// Was going to print an error to the log with the key that failed,
+			// but didn't want to pull in RageLog. -Kyz
+			passed= false;
+		}
+		lua_pop(L, 1);
+	}
+	return passed;
+}
+
 #define SArg(n) (luaL_checkstring(L,(n)))
 #define BIArg(n) (MyLua_checkintboolean(L,(n)))
 #define IArg(n) (luaL_checkint(L,(n)))

src/OptionRowHandler.cpp

@@ -809,7 +809,10 @@ public:
 	LuaReference *m_pLuaTable;
 	LuaReference m_EnabledForPlayersFunc;
 
-	OptionRowHandlerLua() { m_pLuaTable = new LuaReference; Init(); }
+	bool m_TableIsSane;
+
+	OptionRowHandlerLua(): m_TableIsSane(false)
+	{ m_pLuaTable = new LuaReference; Init(); }
 	virtual ~OptionRowHandlerLua() { delete m_pLuaTable; }
 	void Init()
 	{
@@ -817,8 +820,133 @@ public:
 		m_pLuaTable->Unset();
 	}
 
+	bool SanityCheckTable(lua_State* L, RString& RowName)
+	{
+		if(m_pLuaTable->GetLuaType() != LUA_TTABLE)
+		{
+			LOG->Warn("LUA_ERROR:  Result of \"%s\" is not a table.", RowName.c_str());
+			return false;
+		}
+		m_pLuaTable->PushSelf(L);
+		lua_getfield(L, -1, "Name");
+		const char *pStr = lua_tostring(L, -1);
+		if( pStr == NULL )
+		{
+			LOG->Warn("LUA_ERROR:  \"%s\" \"Name\" entry is not a string.", RowName.c_str());
+			return false;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, -1, "LayoutType");
+		pStr = lua_tostring(L, -1);
+		if(pStr == NULL || StringToLayoutType(pStr) == LayoutType_Invalid)
+		{
+			LOG->Warn("LUA_ERROR:  \"%s\" \"LayoutType\" entry is not a string.", RowName.c_str());
+			return false;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, -1, "SelectType");
+		pStr = lua_tostring(L, -1);
+		if(pStr == NULL || StringToSelectType(pStr) == SelectType_Invalid)
+		{
+			LOG->Warn("LUA_ERROR:  \"%s\" \"SelectType\" entry is not a string.", RowName.c_str());
+			return false;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, -1, "Choices");
+		if(!lua_istable(L, -1))
+		{
+			LOG->Warn("LUA_ERROR:  \"%s\" \"Choices\" is not a table.", RowName.c_str());
+			return false;
+		}
+		if(!TableContainsOnlyStrings(L, lua_gettop(L)))
+		{
+			LOG->Warn("LUA_ERROR:  \"%s\" \"Choices\" table contains a non-string.", RowName.c_str());
+			return false;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, -1, "EnabledForPlayers");
+		if(!lua_isnil(L, -1))
+		{
+			if(!lua_isfunction(L, -1))
+			{
+				LOG->Warn("LUA_ERROR:  \"%s\" \"EnabledForPlayers\" is not a function.", RowName.c_str());
+				return false;
+			}
+			m_pLuaTable->PushSelf( L );
+			lua_call( L, 1, 1 ); // call function with 1 argument and 1 result
+			if(!lua_istable(L, -1))
+			{
+				LOG->Warn("LUA_ERROR:  \"%s\" \"EnabledForPlayers\" did not return a table.", RowName.c_str());
+				return false;
+			}
+			lua_pushnil(L);
+			while(lua_next(L, -2) != 0)
+			{
+				PlayerNumber pn= Enum::Check<PlayerNumber>(L, -1, true, true);
+				if(pn == PlayerNumber_Invalid)
+				{
+					LOG->Warn("LUA_ERROR:  \"%s\" \"EnabledForPlayers\" contains a non-PlayerNumber.", RowName.c_str());
+					return false;
+				}
+				lua_pop(L, 1);
+			}
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, -1, "ReloadRowMessages");
+		if(!lua_isnil(L, -1))
+		{
+			if(!lua_istable(L, -1))
+			{
+				LOG->Warn("LUA_ERROR:  \"%s\" \"ReloadRowMessages\" is not a table.", RowName.c_str());
+				return false;
+			}
+			if(!TableContainsOnlyStrings(L, lua_gettop(L)))
+			{
+				LOG->Warn("LUA_ERROR:  \"%s\" \"ReloadRowMessages\" table contains a non-string.", RowName.c_str());
+				return false;
+			}
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, -1, "LoadSelections");
+		if(!lua_isfunction(L, -1))
+		{
+			LOG->Warn("LUA_ERROR:  \"%s\" \"LoadSelections\" entry is not a function.", RowName.c_str());
+			return false;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, -1, "SaveSelections");
+		if(!lua_isfunction(L, -1))
+		{
+			LOG->Warn("LUA_ERROR:  \"%s\" \"SaveSelections\" entry is not a function.", RowName.c_str());
+			return false;
+		}
+		lua_pop(L, 1);
+
+		lua_getfield(L, -1, "NotifyOfSelection");
+		if(!lua_isnil(L, -1) && !lua_isfunction(L, -1))
+		{
+			LOG->Warn("LUA_ERROR:  \"%s\" \"NotifyOfSelection\" entry is not a function.", RowName.c_str());
+			return false;
+		}
+		lua_pop(L, 1);
+
+		lua_pop(L, 1);
+		return true;
+	}
+
 	void SetEnabledForPlayers()
 	{
+		if(!m_TableIsSane)
+		{
+			return;
+		}
 		Lua *L = LUA->Get();
 
 		if( m_EnabledForPlayersFunc.IsNil() )
@@ -833,11 +961,6 @@ public:
 		m_pLuaTable->PushSelf( L );
 
 		lua_call( L, 1, 1 ); // call function with 1 argument and 1 result
-		if( !lua_istable(L, -1) )
-		{
-			LOG->Warn("LUA_ERROR:  \"EnabledForPlayers\" did not return a table." );
-		}
-
 		m_Def.m_vEnabledForPlayers.clear();	// and fill in with supplied PlayerNumbers below
 
 		lua_pushnil( L );
@@ -869,113 +992,89 @@ public:
 
 		// Run the Lua expression.  It should return a table.
 		m_pLuaTable->SetFromExpression( sLuaFunction );
-
+		m_TableIsSane= SanityCheckTable(L, sLuaFunction);
-		if( m_pLuaTable->GetLuaType() != LUA_TTABLE )
+		if(!m_TableIsSane)
 		{
-			LOG->Warn("LUA_ERROR:  Result of \"%s\" is not a table.", sLuaFunction.c_str());
+			m_pLuaTable->PushSelf(L);
+			lua_getfield(L, -1, "Name");
+			const char *pStr = lua_tostring( L, -1 );
+			if(pStr == NULL)
+			{
+				m_Def.m_sName = "Invalid";
+			}
+			else
+			{
+				m_Def.m_sName = pStr;
+			}
+			lua_pop( L, 1 );
+			// Add a fake choice so that there won't be a crash.
+			// This is so that a themer that makes a mistake doesn't have to
+			// completely restart and can just reload scripts.
+			m_Def.m_vsChoices.push_back("Error in row.");
+			// Set m_selectType to SELECT_MULTIPLE so we won't hit the assert in
+			// VerifySelected.
+			m_Def.m_selectType= SELECT_MULTIPLE;
+			lua_settop(L, 0); // Release has an assert that forces a clear stack.
+			LUA->Release(L);
+			return;
 		}
+		m_pLuaTable->PushSelf(L);
 
-		m_pLuaTable->PushSelf( L );
+		lua_getfield(L, -1, "Name");
-
-		lua_pushstring( L, "Name" );
-		lua_gettable( L, -2 );
 		const char *pStr = lua_tostring( L, -1 );
-		if( pStr == NULL )
-		{
-			LOG->Warn("LUA_ERROR:  \"%s\" \"Name\" entry is not a string.", sLuaFunction.c_str());
-		}
 		m_Def.m_sName = pStr;
 		lua_pop( L, 1 );
 
-		lua_pushstring( L, "OneChoiceForAllPlayers" );
+		lua_getfield(L, -1, "OneChoiceForAllPlayers"); 
-		lua_gettable( L, -2 );
 		m_Def.m_bOneChoiceForAllPlayers = lua_toboolean( L, -1 );
 		lua_pop( L, 1 );
 
-		lua_pushstring( L, "ExportOnChange" );
+		lua_getfield(L, -1, "ExportOnChange");
-		lua_gettable( L, -2 );
 		m_Def.m_bExportOnChange = lua_toboolean( L, -1 );
 		lua_pop( L, 1 );
 
-		lua_pushstring( L, "LayoutType" );
+		// TODO:  Change these to use the proper enum strings like everything
-		lua_gettable( L, -2 );
+		// else.  This will break theme compatibility, so it has to wait until
+		// after SM5.  -Kyz
+		lua_getfield(L, -1, "LayoutType");
 		pStr = lua_tostring( L, -1 );
-		if( pStr == NULL )
-		{
-			LOG->Warn("LUA_ERROR:  \"%s\" \"LayoutType\" entry is not a string.", sLuaFunction.c_str());
-		}
 		m_Def.m_layoutType = StringToLayoutType( pStr );
-		ASSERT( m_Def.m_layoutType != LayoutType_Invalid );
 		lua_pop( L, 1 );
 
-		lua_pushstring( L, "SelectType" );
+		lua_getfield(L, -1, "SelectType");
-		lua_gettable( L, -2 );
 		pStr = lua_tostring( L, -1 );
-		if( pStr == NULL )
-		{
-			LOG->Warn("LUA_ERROR:  \"%s\" \"SelectType\" entry is not a string.", sLuaFunction.c_str());
-		}
 		m_Def.m_selectType = StringToSelectType( pStr );
-		ASSERT( m_Def.m_selectType != SelectType_Invalid );
 		lua_pop( L, 1 );
 
 		// Iterate over the "Choices" table.
-		lua_pushstring( L, "Choices" );
+		lua_getfield(L, -1, "Choices");
-		lua_gettable( L, -2 );
-		if( !lua_istable( L, -1 ) )
-		{
-			LOG->Warn("LUA_ERROR:  \"%s\" \"Choices\" is not a table.", sLuaFunction.c_str());
-		}
-
 		lua_pushnil( L );
 		while( lua_next(L, -2) != 0 )
 		{
 			// `key' is at index -2 and `value' at index -1
 			const char *pValue = lua_tostring( L, -1 );
-			if( pValue == NULL )
-			{
-				LOG->Warn("LUA_ERROR:  \"%s\" Column entry is not a string.", sLuaFunction.c_str());
-			}
 			//LOG->Trace( "choice: '%s'", pValue);
-
 			m_Def.m_vsChoices.push_back( pValue );
-
 			lua_pop( L, 1 ); // removes `value'; keeps `key' for next iteration
 		}
-
 		lua_pop( L, 1 ); // pop choices table
 
 		// Set the EnabledForPlayers function.
-		lua_pushstring( L, "EnabledForPlayers" );
+		lua_getfield(L, -1, "EnabledForPlayers");
-		lua_gettable( L, -2 );
-		if( !lua_isfunction( L, -1 ) && !lua_isnil( L, -1 ) )
-		{
-			LOG->Warn("LUA_ERROR:  \"%s\" \"EnabledForPlayers\" is not a function.", sLuaFunction.c_str());
-		}
 		m_EnabledForPlayersFunc.SetFromStack( L );
 		SetEnabledForPlayers();
 
 		// Iterate over the "ReloadRowMessages" table.
-		lua_pushstring( L, "ReloadRowMessages" );
+		lua_getfield(L, -1, "ReloadRowMessages");
-		lua_gettable( L, -2 );
 		if( !lua_isnil( L, -1 ) )
 		{
-			if( !lua_istable( L, -1 ) )
-				RageException::Throw( "\"%s\" \"ReloadRowMessages\" is not a table.", sLuaFunction.c_str() );
-
 			lua_pushnil( L );
 			while( lua_next(L, -2) != 0 )
 			{
 				// `key' is at index -2 and `value' at index -1
 				const char *pValue = lua_tostring( L, -1 );
-				if( pValue == NULL )
-				{
-					LOG->Warn("LUA_ERROR:  \"%s\" Column entry is not a string.", sLuaFunction.c_str());
-				}
 				//LOG->Trace( "Found ReloadRowMessage '%s'", pValue);
-
 				m_vsReloadRowMessages.push_back( pValue );
-
 				lua_pop( L, 1 ); // removes `value'; keeps `key' for next iteration
 			}
 		}
@@ -995,6 +1094,10 @@ public:
 
 	virtual void ImportOption( OptionRow *pRow, const vector<PlayerNumber> &vpns, vector<bool> vbSelectedOut[NUM_PLAYERS] ) const
 	{
+		if(!m_TableIsSane)
+		{
+			return;
+		}
 		Lua *L = LUA->Get();
 
 		ASSERT( lua_gettop(L) == 0 );
@@ -1019,12 +1122,7 @@ public:
 			m_pLuaTable->PushSelf( L );
 			ASSERT( lua_istable( L, -1 ) );
 
-			lua_pushstring( L, "LoadSelections" );
+			lua_getfield(L, -1, "LoadSelections");
-			lua_gettable( L, -2 );
-			if( !lua_isfunction( L, -1 ) )
-			{
-				LOG->Warn("LUA_ERROR:  \"%s\" \"LoadSelections\" entry is not a function.", m_Def.m_sName.c_str());
-			}
 
 			// Argument 1 (self):
 			m_pLuaTable->PushSelf( L );
@@ -1053,6 +1151,10 @@ public:
 	}
 	virtual int ExportOption( const vector<PlayerNumber> &vpns, const vector<bool> vbSelected[NUM_PLAYERS] ) const
 	{
+		if(!m_TableIsSane)
+		{
+			return 0;
+		}
 		Lua *L = LUA->Get();
 
 		ASSERT( lua_gettop(L) == 0 );
@@ -1075,12 +1177,7 @@ public:
 			m_pLuaTable->PushSelf( L );
 			ASSERT( lua_istable( L, -1 ) );
 
-			lua_pushstring( L, "SaveSelections" );
+			lua_getfield(L, -1, "SaveSelections");
-			lua_gettable( L, -2 );
-			if( !lua_isfunction( L, -1 ) )
-			{
-				LOG->Warn("LUA_ERROR:  \"%s\" \"SaveSelections\" entry is not a function.", m_Def.m_sName.c_str());
-			}
 
 			// Argument 1 (self):
 			m_pLuaTable->PushSelf( L );
@@ -1109,11 +1206,14 @@ public:
 	}
 	virtual bool NotifyOfSelection(PlayerNumber pn, int choice)
 	{
+		if(!m_TableIsSane)
+		{
+			return false;
+		}
 		Lua *L= LUA->Get();
 		m_pLuaTable->PushSelf(L);
 
-		lua_pushstring(L, "NotifyOfSelection");
+		lua_getfield(L, -1, "NotifyOfSelection");
-		lua_gettable(L, -2);
 		bool changed= false;
 		if(lua_isfunction(L, -1))
 		{
@@ -1128,25 +1228,14 @@ public:
 				changed= true;
 				m_Def.m_vsChoices.clear();
 				// Iterate over the "Choices" table.
-				lua_pushstring( L, "Choices" );
+				lua_getfield(L, -1, "Choices");
-				lua_gettable( L, -2 );
-				if(!lua_istable(L, -1))
-				{
-					LOG->Warn("\"%s\" \"Choices\" is not a table.", m_Def.m_sName.c_str());
-				}
 				lua_pushnil( L );
 				while( lua_next(L, -2) != 0 )
 				{
 					// `key' is at index -2 and `value' at index -1
 					const char *pValue = lua_tostring( L, -1 );
-					if(pValue == NULL)
-					{
-						LOG->Warn("\"%s\" Column entry is not a string.", m_Def.m_sName.c_str());
-					}
 					//LOG->Trace( "choice: '%s'", pValue);
-
 					m_Def.m_vsChoices.push_back( pValue );
-
 					lua_pop( L, 1 ); // removes `value'; keeps `key' for next iteration
 				}
 			}