From c8155d48d433d2db2a4526970b73780fd3cc590b Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Tue, 24 Oct 2017 17:12:14 -0700 Subject: [PATCH] Fixes to ShadingSystemImpl::decode_connected_param @marsupial correctly points out that we are unsafely passing a string_view.data() into both atoi and strchr, which is unsafe because there's no guarantee that a string_view is null-terminated. Ugh! He proposed a fix here: https://github.com/imageworks/OpenShadingLanguage/pull/800 and there's nothing wrong with it per se, but I suspected it could be done more simply, so this PR is my stab at it, using the `OIIO::Strutil::parse_*` functions. --- src/liboslcomp/osllex.l | 2 +- src/liboslexec/constfold.cpp | 4 +-- src/liboslexec/dictionary.cpp | 14 ++++++---- src/liboslexec/opstring.cpp | 4 +-- src/liboslexec/osolex.l | 4 +-- src/liboslexec/shadingsys.cpp | 37 +++++++++++++++++-------- src/testrender/testrender.cpp | 26 ++++++------------ src/testshade/testshade.cpp | 52 +++++++++++++---------------------- 8 files changed, 69 insertions(+), 74 deletions(-) diff --git a/src/liboslcomp/osllex.l b/src/liboslcomp/osllex.l index d0deee345..cbd8c9815 100644 --- a/src/liboslcomp/osllex.l +++ b/src/liboslcomp/osllex.l @@ -223,7 +223,7 @@ void preprocess (const char *yytext); {FLT} { - yylval.f = atof (yytext); + yylval.f = OIIO::Strutil::from_string(yytext); SETLINE; return FLOAT_LITERAL; } diff --git a/src/liboslexec/constfold.cpp b/src/liboslexec/constfold.cpp index db929820b..fa0813717 100644 --- a/src/liboslexec/constfold.cpp +++ b/src/liboslexec/constfold.cpp @@ -1239,7 +1239,7 @@ DECLFOLDER(constfold_stoi) if (S.is_constant()) { ASSERT (S.typespec().is_string()); ustring s = *(ustring *)S.data(); - int cind = rop.add_constant ((int) strtol(s.c_str(), NULL, 10)); + int cind = rop.add_constant (Strutil::from_string(s)); rop.turn_into_assign (op, cind, "const fold stoi"); return 1; } @@ -1256,7 +1256,7 @@ DECLFOLDER(constfold_stof) if (S.is_constant()) { ASSERT (S.typespec().is_string()); ustring s = *(ustring *)S.data(); - int cind = rop.add_constant ((float) strtod(s.c_str(), NULL)); + int cind = rop.add_constant (Strutil::from_string(s)); rop.turn_into_assign (op, cind, "const fold stof"); return 1; } diff --git a/src/liboslexec/dictionary.cpp b/src/liboslexec/dictionary.cpp index 37c9d90dd..284e34a57 100644 --- a/src/liboslexec/dictionary.cpp +++ b/src/liboslexec/dictionary.cpp @@ -372,10 +372,11 @@ Dictionary::dict_value (int nodeID, ustring attribname, } if (type.basetype == TypeDesc::INT) { r.valueoffset = (int) m_intdata.size(); + string_view valstr (val); for (int i = 0; i < n; ++i) { - int v = (int) strtol (val, (char **)&val, 10); - while (isspace(*val) || *val == ',') - ++val; + int v; + OIIO::Strutil::parse_int (valstr, v); + OIIO::Strutil::parse_char (valstr, ','); m_intdata.push_back (v); ((int *)data)[i] = v; } @@ -384,10 +385,11 @@ Dictionary::dict_value (int nodeID, ustring attribname, } if (type.basetype == TypeDesc::FLOAT) { r.valueoffset = (int) m_floatdata.size(); + string_view valstr (val); for (int i = 0; i < n; ++i) { - float v = (float) strtod (val, (char **)&val); - while (isspace(*val) || *val == ',') - ++val; + float v; + OIIO::Strutil::parse_float (valstr, v); + OIIO::Strutil::parse_char (valstr, ','); m_floatdata.push_back (v); ((float *)data)[i] = v; } diff --git a/src/liboslexec/opstring.cpp b/src/liboslexec/opstring.cpp index 6651c3915..6b488f971 100644 --- a/src/liboslexec/opstring.cpp +++ b/src/liboslexec/opstring.cpp @@ -105,13 +105,13 @@ osl_endswith_iss (const char *s_, const char *substr_) OSL_SHADEOP int osl_stoi_is (const char *str) { - return str ? strtol(str, NULL, 10) : 0; + return str ? Strutil::from_string(str) : 0; } OSL_SHADEOP float osl_stof_fs (const char *str) { - return str ? (float)strtod(str, NULL) : 0.0f; + return str ? Strutil::from_string(str) : 0.0f; } OSL_SHADEOP const char * diff --git a/src/liboslexec/osolex.l b/src/liboslexec/osolex.l index 798b7cb30..ea1504e1c 100644 --- a/src/liboslexec/osolex.l +++ b/src/liboslexec/osolex.l @@ -194,13 +194,13 @@ using namespace OSL::pvt; /* Literal values */ {INTEGER} { - yylval.i = atoi (yytext); + yylval.i = OIIO::Strutil::from_string(yytext); // std::cerr << "lex int " << yylval.i << "\n"; return INT_LITERAL; } {FLT} { - yylval.f = atof (yytext); + yylval.f = OIIO::Strutil::from_string(yytext); // std::cerr << "lex float " << yylval.f << "\n"; return FLOAT_LITERAL; } diff --git a/src/liboslexec/shadingsys.cpp b/src/liboslexec/shadingsys.cpp index 3c1a69bcd..36f330556 100644 --- a/src/liboslexec/shadingsys.cpp +++ b/src/liboslexec/shadingsys.cpp @@ -2482,10 +2482,9 @@ ShadingSystemImpl::decode_connected_param (string_view connectionname, // Look for a bracket in the "parameter name" size_t bracketpos = connectionname.find ('['); - const char *bracket = bracketpos == string_view::npos ? NULL - : connectionname.data()+bracketpos; // Grab just the part of the param name up to the bracket ustring param (connectionname, 0, bracketpos); + string_view cname_remaining = connectionname.substr (bracketpos); // Search for the param with that name, fail if not found c.param = inst->findsymbol (param); @@ -2514,24 +2513,40 @@ ShadingSystemImpl::decode_connected_param (string_view connectionname, c.type = sym->typespec(); - if (bracket && c.type.is_array()) { + if (! cname_remaining.empty() && c.type.is_array()) { // There was at least one set of brackets that appears to be // selecting an array element. - c.arrayindex = atoi (bracket+1); + int index = 0; + if (! (Strutil::parse_char (cname_remaining, '[') && + Strutil::parse_int (cname_remaining, index) && + Strutil::parse_char (cname_remaining, ']'))) { + error ("ConnectShaders: malformed parameter \"%s\"", connectionname); + c.param = -1; // mark as invalid + return c; + } + c.arrayindex = index; if (c.arrayindex >= c.type.arraylength()) { error ("ConnectShaders: cannot request array element %s from a %s", connectionname, c.type.c_str()); c.arrayindex = c.type.arraylength() - 1; // clamp it } c.type.make_array (0); // chop to the element type - bracket = strchr (bracket+1, '['); // skip to next bracket + Strutil::skip_whitespace (cname_remaining); // skip to next bracket } - if (bracket && ! c.type.is_closure() && - c.type.aggregate() != TypeDesc::SCALAR) { + if (! cname_remaining.empty() && cname_remaining.front() == '[' && + ! c.type.is_closure() && c.type.aggregate() != TypeDesc::SCALAR) { // There was at least one set of brackets that appears to be // selecting a color/vector component. - c.channel = atoi (bracket+1); + int index = 0; + if (! (Strutil::parse_char (cname_remaining, '[') && + Strutil::parse_int (cname_remaining, index) && + Strutil::parse_char (cname_remaining, ']'))) { + error ("ConnectShaders: malformed parameter \"%s\"", connectionname); + c.param = -1; // mark as invalid + return c; + } + c.channel = index; if (c.channel >= (int)c.type.aggregate()) { error ("ConnectShaders: cannot request component %s from a %s", connectionname, c.type.c_str()); @@ -2539,11 +2554,11 @@ ShadingSystemImpl::decode_connected_param (string_view connectionname, } // chop to just the scalar part c.type = TypeSpec ((TypeDesc::BASETYPE)c.type.simpletype().basetype); - bracket = strchr (bracket+1, '['); // skip to next bracket + Strutil::skip_whitespace (cname_remaining); } - // Deal with left over brackets - if (bracket) { + // Deal with left over nonsense or unsupported param designations + if (! cname_remaining.empty()) { // Still a leftover bracket, no idea what to do about that error ("ConnectShaders: don't know how to connect '%s' when \"%s\" is a \"%s\"", connectionname, param.c_str(), c.type.c_str()); diff --git a/src/testrender/testrender.cpp b/src/testrender/testrender.cpp index 33f23153f..338f40b5a 100644 --- a/src/testrender/testrender.cpp +++ b/src/testrender/testrender.cpp @@ -152,24 +152,16 @@ void getargs(int argc, const char *argv[]) errhandler.verbosity (ErrorHandler::VERBOSE); } -Vec3 strtovec(const char* str) { +Vec3 strtovec(string_view str) { Vec3 v(0, 0, 0); - sscanf(str, " %f , %f , %f", &v.x, &v.y, &v.z); + OIIO::Strutil::parse_float (str, v[0]); + OIIO::Strutil::parse_char (str, ','); + OIIO::Strutil::parse_float (str, v[1]); + OIIO::Strutil::parse_char (str, ','); + OIIO::Strutil::parse_float (str, v[2]); return v; } -int strtoint(const char* str) { - int i = 0; - sscanf(str, " %d", &i); - return i; -} - -float strtoflt(const char* str) { - float f = 0; - sscanf(str, " %f", &f); - return f; -} - bool strtobool(const char* str) { return strcmp(str, "1") == 0 || strcmp(str, "on") == 0 || @@ -281,7 +273,7 @@ void parse_scene() { if (dir_attr) dir = strtovec(dir_attr.value()); else if ( at_attr) dir = strtovec( at_attr.value()) - eye; if ( up_attr) up = strtovec( up_attr.value()); - if (fov_attr) fov = strtoflt(fov_attr.value()); + if (fov_attr) fov = OIIO::Strutil::from_string(fov_attr.value()); // create actual camera camera = Camera(eye, dir, up, fov, xres, yres); @@ -291,7 +283,7 @@ void parse_scene() { pugi::xml_attribute radius_attr = node.attribute("radius"); if (center_attr && radius_attr) { Vec3 center = strtovec(center_attr.value()); - float radius = strtoflt(radius_attr.value()); + float radius = OIIO::Strutil::from_string(radius_attr.value()); if (radius > 0) { pugi::xml_attribute light_attr = node.attribute("is_light"); bool is_light = light_attr ? strtobool(light_attr.value()) : false; @@ -314,7 +306,7 @@ void parse_scene() { } else if (strcmp(node.name(), "Background") == 0) { pugi::xml_attribute res_attr = node.attribute("resolution"); if (res_attr) - backgroundResolution = strtoint(res_attr.value()); + backgroundResolution = OIIO::Strutil::from_string(res_attr.value()); backgroundShaderID = int(shaders.size()) - 1; } else if (strcmp(node.name(), "ShaderGroup") == 0) { ShaderGroupRef group; diff --git a/src/testshade/testshade.cpp b/src/testshade/testshade.cpp index dc6eaac30..d9898ea02 100644 --- a/src/testshade/testshade.cpp +++ b/src/testshade/testshade.cpp @@ -303,7 +303,7 @@ action_param (int argc, const char *argv[]) else if (OIIO::Strutil::istarts_with(splits[0],"type=")) type.fromstring (splits[0].c_str()+5); else if (OIIO::Strutil::istarts_with(splits[0],"lockgeom=")) - unlockgeom = (strtol (splits[0].c_str()+9, NULL, 10) == 0); + unlockgeom = (OIIO::Strutil::from_string (splits[0]) == 0); } // If it is or might be a matrix, look for 16 comma-separated floats @@ -313,8 +313,7 @@ action_param (int argc, const char *argv[]) &f[0], &f[1], &f[2], &f[3], &f[4], &f[5], &f[6], &f[7], &f[8], &f[9], &f[10], &f[11], &f[12], &f[13], &f[14], &f[15]) == 16) { - params.push_back (ParamValue()); - params.back().init (paramname, TypeDesc::TypeMatrix, 1, f); + params.emplace_back (paramname, TypeDesc::TypeMatrix, 1, f); if (unlockgeom) params.back().interp (ParamValue::INTERP_VERTEX); return; @@ -324,37 +323,28 @@ action_param (int argc, const char *argv[]) && sscanf (stringval.c_str(), "%g, %g, %g", &f[0], &f[1], &f[2]) == 3) { if (type == TypeDesc::UNKNOWN) type = TypeDesc::TypeVector; - params.push_back (ParamValue()); - params.back().init (paramname, type, 1, f); + params.emplace_back (paramname, type, 1, f); if (unlockgeom) params.back().interp (ParamValue::INTERP_VERTEX); return; } // If it is or might be an int, look for an int that takes up the whole // string. - if ((type == TypeDesc::UNKNOWN || type == TypeDesc::TypeInt)) { - char *endptr = NULL; - int ival = strtol(stringval.c_str(),&endptr,10); - if (endptr && *endptr == 0) { - params.push_back (ParamValue()); - params.back().init (paramname, TypeDesc::TypeInt, 1, &ival); - if (unlockgeom) - params.back().interp (ParamValue::INTERP_VERTEX); - return; - } + if ((type == TypeDesc::UNKNOWN || type == TypeDesc::TypeInt) + && OIIO::Strutil::string_is(stringval)) { + params.emplace_back (paramname, OIIO::Strutil::from_string(stringval)); + if (unlockgeom) + params.back().interp (ParamValue::INTERP_VERTEX); + return; } // If it is or might be an float, look for a float that takes up the // whole string. - if ((type == TypeDesc::UNKNOWN || type == TypeDesc::TypeFloat)) { - char *endptr = NULL; - float fval = (float) strtod(stringval.c_str(),&endptr); - if (endptr && *endptr == 0) { - params.push_back (ParamValue()); - params.back().init (paramname, TypeDesc::TypeFloat, 1, &fval); - if (unlockgeom) - params.back().interp (ParamValue::INTERP_VERTEX); - return; - } + if ((type == TypeDesc::UNKNOWN || type == TypeDesc::TypeFloat) + && OIIO::Strutil::string_is(stringval)) { + params.emplace_back (paramname, OIIO::Strutil::from_string(stringval)); + if (unlockgeom) + params.back().interp (ParamValue::INTERP_VERTEX); + return; } // Catch-all for float types and arrays @@ -365,8 +355,7 @@ action_param (int argc, const char *argv[]) OIIO::Strutil::parse_float (stringval, vals[i]); OIIO::Strutil::parse_char (stringval, ','); } - params.push_back (ParamValue()); - params.back().init (paramname, type, 1, &vals[0]); + params.emplace_back (paramname, type, 1, &vals[0]); if (unlockgeom) params.back().interp (ParamValue::INTERP_VERTEX); return; @@ -380,8 +369,7 @@ action_param (int argc, const char *argv[]) OIIO::Strutil::parse_int (stringval, vals[i]); OIIO::Strutil::parse_char (stringval, ','); } - params.push_back (ParamValue()); - params.back().init (paramname, type, 1, &vals[0]); + params.emplace_back (paramname, type, 1, &vals[0]); if (unlockgeom) params.back().interp (ParamValue::INTERP_VERTEX); return; @@ -395,8 +383,7 @@ action_param (int argc, const char *argv[]) std::vector strelements; for (auto&& s : splitelements) strelements.push_back (ustring(s)); - params.push_back (ParamValue()); - params.back().init (paramname, type, 1, &strelements[0]); + params.emplace_back (paramname, type, 1, &strelements[0]); if (unlockgeom) params.back().interp (ParamValue::INTERP_VERTEX); return; @@ -404,8 +391,7 @@ action_param (int argc, const char *argv[]) // All remaining cases -- it's a string const char *s = stringval.c_str(); - params.push_back (ParamValue()); - params.back().init (paramname, TypeDesc::TypeString, 1, &s); + params.emplace_back (paramname, TypeDesc::TypeString, 1, &s); if (unlockgeom) params.back().interp (ParamValue::INTERP_VERTEX); }