From 7f6e7fc0cb15d5367d48a3d8084d1a0b319575b7 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 5 Aug 2013 19:43:06 -0400
Subject: [PATCH] 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.
---
indra/newview/llappviewer.cpp | 28 +++--
indra/newview/llcommandlineparser.cpp | 154 +++++++++++++++++++++-----
indra/newview/llcommandlineparser.h | 2 +-
3 files changed, 145 insertions(+), 39 deletions(-)
diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp
index 6132e9b4663..b2aadf9cc09 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 7adc6b8c5ec..a6384ded125 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 44f2a268431..71388b8217e 100755
--- a/indra/newview/llcommandlineparser.h
+++ b/indra/newview/llcommandlineparser.h
@@ -86,7 +86,7 @@ class LLCommandLineParser
*
* Use this to handle the results of parsing.
*/
- void notify();
+ bool notify();
/** @brief Print a description of the configured options.
*
--
GitLab