CHOP-951, IQA-1477: Validate args for numeric command-line switches.

The logic in llcommandlineparser.cpp's setControlValueCB() callback function
for converting command-line switch argument values from string to the actual
type of the map-to settings variable had a couple special cases for boolean
and LLSD array. But for S32, U32 and F32, it simply used default LLSD
string-to-numeric conversion. LLSD's string-to-numeric conversion is a bit
lame: for non-numeric strings, it shrugs and returns 0.
Introduce onevalue() and badvalue() helper functions that, like certain errors
during command-line parsing, throw LLCLPError. Use them to streamline certain
redundancies in setControlValueCB(). Introduce convertTo<T>() helper function
that uses boost::lexical_cast() for slightly more stringent conversions. Add
cases for U32, S32 and F32 targets.
setControlValueCB() is actually called only by LLControlGroupCLP::notify(),
not during actual command-line parsing.
Make LLControlGroupCLP::notify() return bool. Make it catch LLCLPError, set
the error for getErrorMessage() and return false on that exception.
Package LLAppViewer::initConfiguration()'s response to initParseCommandLine()
returning false as a new handleCommandLineError() function; invoke it both
there and when LLControlGroupCLP::notify() returns false.

3 files changed, 145 insertions, 39 deletions

diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp
index 6132e9b466..b2aadf9cc0 100755
--- a/indra/newview/llappviewer.cpp
+++ b/indra/newview/llappviewer.cpp
@@ -2306,6 +2306,20 @@ void LLAppViewer::loadColorSettings()
 	LLUIColorTable::instance().loadFromSettings();
 }
 
+namespace
+{
+    void handleCommandLineError(LLControlGroupCLP& clp)
+    {
+		llwarns << "Error parsing command line options. Command Line options ignored."  << llendl;
+
+		llinfos << "Command line usage:\n" << clp << llendl;
+
+		OSMessageBox(STRINGIZE(LLTrans::getString("MBCmdLineError") << clp.getErrorMessage()),
+					 LLStringUtil::null,
+					 OSMB_OK);
+    }
+} // anonymous namespace
+
 bool LLAppViewer::initConfiguration()
 {	
 	//Load settings files list
@@ -2404,13 +2418,7 @@ bool LLAppViewer::initConfiguration()
 
 	if(!initParseCommandLine(clp))
 	{
-		llwarns	<< "Error parsing command line options.	Command	Line options ignored."  << llendl;
-		
-		llinfos	<< "Command	line usage:\n" << clp << llendl;
-
-		std::ostringstream msg;
-		msg << LLTrans::getString("MBCmdLineError") << clp.getErrorMessage();
-		OSMessageBox(msg.str(),LLStringUtil::null,OSMB_OK);
+		handleCommandLineError(clp);
 		return false;
 	}
 	
@@ -2457,7 +2465,11 @@ bool LLAppViewer::initConfiguration()
 	loadSettingsFromDirectory("UserSession");
 
 	// - apply command line settings 
-	clp.notify(); 
+	if (! clp.notify())
+	{
+		handleCommandLineError(clp);
+		return false;
+	}
 
 	// Register the core crash option as soon as we can
 	// if we want gdb post-mortem on cores we need to be up and running
diff --git a/indra/newview/llcommandlineparser.cpp b/indra/newview/llcommandlineparser.cpp
index 7adc6b8c5e..a6384ded12 100755
--- a/indra/newview/llcommandlineparser.cpp
+++ b/indra/newview/llcommandlineparser.cpp
@@ -38,6 +38,7 @@
 #endif
 
 #include <boost/program_options.hpp>
+#include <boost/lexical_cast.hpp>
 #include <boost/bind.hpp>
 #include <boost/tokenizer.hpp>
 #include <boost/assign/list_of.hpp>
@@ -48,10 +49,12 @@
 
 #include "llsdserialize.h"
 #include "llerror.h"
+#include "stringize.h"
 #include <string>
 #include <set>
 #include <iostream>
 #include <sstream>
+#include <typeinfo>
 
 #include "llcontrol.h"
 
@@ -82,7 +85,7 @@ namespace
     po::options_description gOptionsDesc;
     po::positional_options_description gPositionalOptions;
 	po::variables_map gVariableMap;
-    
+
     const LLCommandLineParser::token_vector_t gEmptyValue;
 
     void read_file_into_string(std::string& str, const std::basic_istream < char >& file)
@@ -400,9 +403,19 @@ bool LLCommandLineParser::parseCommandLineFile(const std::basic_istream < char >
     return parseCommandLineString(args);
 }
 
-void LLCommandLineParser::notify()
+bool LLCommandLineParser::notify()
 {
-    po::notify(gVariableMap);    
+    try
+    {
+        po::notify(gVariableMap);
+        return true;
+    }
+    catch (const LLCLPError& e)
+    {
+        llwarns << "Caught Error: " << e.what() << llendl;
+        mErrorMsg = e.what();
+        return false;
+    }
 }
 
 void LLCommandLineParser::printOptions() const
@@ -444,43 +457,129 @@ const LLCommandLineParser::token_vector_t& LLCommandLineParser::getOption(const
 //----------------------------------------------------------------------------
 // LLControlGroupCLP defintions
 //----------------------------------------------------------------------------
+namespace {
+LLCommandLineParser::token_vector_t::value_type
+onevalue(const std::string& option,
+         const LLCommandLineParser::token_vector_t& value)
+{
+    if (value.empty())
+    {
+        // What does it mean when the user specifies a command-line switch
+        // that requires a value, but omits the value? Complain.
+        throw LLCLPError(STRINGIZE("No value specified for --" << option << "!"));
+    }
+    else if (value.size() > 1)
+    {
+        llwarns << "Ignoring extra tokens specified for --"
+                << option << "." << llendl; 
+    }
+    return value[0];
+}
+
+void badvalue(const std::string& option,
+              const std::string& varname,
+              const std::string& type,
+              const std::string& value)
+{
+    // If the user passes an unusable value for a command-line switch, it
+    // seems like a really bad idea to just ignore it, even with a log
+    // warning.
+    throw LLCLPError(STRINGIZE("Invalid value specified by command-line switch '" << option
+                               << "' for variable '" << varname << "' of type " << type
+                               << ": '" << value << "'"));
+}
+
+template <typename T>
+T convertTo(const std::string& option,
+            const std::string& varname,
+            const LLCommandLineParser::token_vector_t::value_type& value)
+{
+    try
+    {
+        return boost::lexical_cast<T>(value);
+    }
+    catch (const boost::bad_lexical_cast&)
+    {
+        badvalue(option, varname, typeid(T).name(), value);
+        // bogus return; compiler unaware that badvalue() won't return
+        return T();
+    }
+}
+
 void setControlValueCB(const LLCommandLineParser::token_vector_t& value, 
-                       const std::string& opt_name, 
-                       LLControlGroup* ctrlGroup)
+                       const std::string& option, 
+                       LLControlVariable* ctrl)
 {
-    // *FIX: Do sematic conversion here.
+    // *FIX: Do semantic conversion here.
     // LLSD (ImplString) Is no good for doing string to type conversion for...
     // booleans
     // compound types
     // ?...
 
-    LLControlVariable* ctrl = ctrlGroup->getControl(opt_name);
     if(NULL != ctrl)
     {
         switch(ctrl->type())
         {
         case TYPE_BOOLEAN:
-            if(value.size() > 1)
+            if (value.empty())
             {
-                llwarns << "Ignoring extra tokens." << llendl; 
+                // Boolean-valued command-line switches are unusual. If you
+                // simply specify the switch without an explicit value, we can
+                // infer you mean 'true'.
+                ctrl->setValue(LLSD(true), false);
             }
-              
-            if(value.size() > 0)
+            else
             {
+                // Only call onevalue() AFTER handling value.empty() case!
+                std::string token(onevalue(option, value));
+            
                 // There's a token. check the string for true/false/1/0 etc.
                 BOOL result = false;
-                BOOL gotSet = LLStringUtil::convertToBOOL(value[0], result);
-                if(gotSet)
+                BOOL gotSet = LLStringUtil::convertToBOOL(token, result);
+                if (gotSet)
                 {
                     ctrl->setValue(LLSD(result), false);
                 }
+                else
+                {
+                    badvalue(option, ctrl->getName(), "bool", token);
+                }
+            }
+            break;
+
+        case TYPE_U32:
+        {
+            std::string token(onevalue(option, value));
+            // To my surprise, for an unsigned target, lexical_cast() doesn't
+            // complain about an input string such as "-17". In that case, you
+            // get a very large positive result. So for U32, make sure there's
+            // no minus sign!
+            if (token.find('-') == std::string::npos)
+            {
+                ctrl->setValue(LLSD::Integer(convertTo<U32>(option, ctrl->getName(), token)),
+                               false);
             }
             else
             {
-                ctrl->setValue(LLSD(true), false);
+                badvalue(option, ctrl->getName(), "unsigned", token);
             }
             break;
+        }
+
+        case TYPE_S32:
+            ctrl->setValue(convertTo<S32>(option, ctrl->getName(),
+                                          onevalue(option, value)), false);
+            break;
+
+        case TYPE_F32:
+            ctrl->setValue(convertTo<F32>(option, ctrl->getName(),
+                                          onevalue(option, value)), false);
+            break;
 
+        // It appears that no one has yet tried to define a command-line
+        // switch mapped to a settings variable of TYPE_VEC3, TYPE_VEC3D,
+        // TYPE_RECT, TYPE_COL4, TYPE_COL3. Such types would certainly seem to
+        // call for a bit of special handling here...
         default:
             {
                 // For the default types, let llsd do the conversion.
@@ -497,16 +596,9 @@ void setControlValueCB(const LLCommandLineParser::token_vector_t& value,
 
                     ctrl->setValue(llsdArray, false);
                 }
-                else if(value.size() > 0)
+                else
                 {
-					if(value.size() > 1)
-					{
-						llwarns << "Ignoring extra tokens mapped to the setting: " << opt_name << "." << llendl; 
-					}
-
-                    LLSD llsdValue;
-                    llsdValue.assign(LLSD::String(value[0]));
-                    ctrl->setValue(llsdValue, false);
+                    ctrl->setValue(onevalue(option, value), false);
                 }
             }
             break;
@@ -514,12 +606,14 @@ void setControlValueCB(const LLCommandLineParser::token_vector_t& value,
     }
     else
     {
-        llwarns << "Command Line option mapping '" 
-            << opt_name 
-            << "' not found! Ignoring." 
-            << llendl;
+        // This isn't anything a user can affect -- it's a misconfiguration on
+        // the part of the coder. Rub the coder's nose in the problem right
+        // away so even preliminary testing will surface it.
+        llerrs << "Command Line option --" << option
+               << " maps to unknown setting!" << llendl;
     }
 }
+} // anonymous namespace
 
 void LLControlGroupCLP::configure(const std::string& config_filename, LLControlGroup* controlGroup)
 {
@@ -600,14 +694,14 @@ void LLControlGroupCLP::configure(const std::string& config_filename, LLControlG
                            << long_name << " (map-to " << controlName << ")" << llendl;
                 }
 
-                if (! controlGroup->getControl(controlName))
+                LLControlVariable* ctrl = controlGroup->getControl(controlName);
+                if (! ctrl)
                 {
                     llerrs << "Option " << long_name << " specifies map-to " << controlName
                            << " which does not exist" << llendl;
                 }
 
-                callback = boost::bind(setControlValueCB, _1, 
-                                       controlName, controlGroup);
+                callback = boost::bind(setControlValueCB, _1, long_name, ctrl);
             }
 
             this->addOptionDesc(
diff --git a/indra/newview/llcommandlineparser.h b/indra/newview/llcommandlineparser.h
index 44f2a26843..71388b8217 100755
--- a/indra/newview/llcommandlineparser.h
+++ b/indra/newview/llcommandlineparser.h
@@ -86,7 +86,7 @@ public:
 	 * 
 	 * Use this to handle the results of parsing. 
 	 */
-	void notify();
+	bool notify();
 	
 	/** @brief Print a description of the configured options.
 	 *