Fixes to ShadingSystemImpl::decode_connected_param (#805)

lgritz · web-flow · commit f785cc28e0ff · 2017-10-27T13:48:19.000-07:00
@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: #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.
diff --git 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<float>(yytext);
                             SETLINE;
                             return FLOAT_LITERAL;
                         }
diff --git 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<int>(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<float>(s));
         rop.turn_into_assign (op, cind, "const fold stof");
         return 1;
     }
diff --git 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
@@ -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<int>(str) : 0;
 }
 
 OSL_SHADEOP float
 osl_stof_fs (const char *str)
 {
-    return str ? (float)strtod(str, NULL) : 0.0f;
+    return str ? Strutil::from_string<float>(str) : 0.0f;
 }
 
 OSL_SHADEOP const char *
diff --git 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<int>(yytext);
                             // std::cerr << "lex int " << yylval.i << "\n";
                             return INT_LITERAL;
                         }
 
 {FLT}                   {
-                            yylval.f = atof (yytext);
+                            yylval.f = OIIO::Strutil::from_string<float>(yytext);
                             // std::cerr << "lex float " << yylval.f << "\n";
                             return FLOAT_LITERAL;
                         }
diff --git 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,36 +2513,52 @@ 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());
             c.channel = (int)c.type.aggregate() - 1;  // clamp it
         }
         // 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
@@ -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<float>(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<float>(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<int>(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
@@ -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<int> (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<int>(stringval)) {
+        params.emplace_back (paramname, OIIO::Strutil::from_string<int>(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<float>(stringval)) {
+        params.emplace_back (paramname, OIIO::Strutil::from_string<float>(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,17 +383,15 @@ action_param (int argc, const char *argv[])
         std::vector<ustring> 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;
     }
 
     // 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);
 }

Original file line number	Diff line number	Diff line change
`@@ -223,7 +223,7 @@ void preprocess (const char *yytext);`
`223`	`223`
`224`	`224`
`225`	`225`	`{FLT} {`
`226`		`- yylval.f = atof (yytext);`
	`226`	`+ yylval.f = OIIO::Strutil::from_string<float>(yytext);`
`227`	`227`	`SETLINE;`
`228`	`228`	`return FLOAT_LITERAL;`
`229`	`229`	`}`
Original file line number	Diff line number	Diff line change
`@@ -105,13 +105,13 @@ osl_endswith_iss (const char s_, const char substr_)`
`105`	`105`	`OSL_SHADEOP int`
`106`	`106`	`osl_stoi_is (const char *str)`
`107`	`107`	`{`
`108`		`- return str ? strtol(str, NULL, 10) : 0;`
	`108`	`+ return str ? Strutil::from_string<int>(str) : 0;`
`109`	`109`	`}`
`110`	`110`
`111`	`111`	`OSL_SHADEOP float`
`112`	`112`	`osl_stof_fs (const char *str)`
`113`	`113`	`{`
`114`		`- return str ? (float)strtod(str, NULL) : 0.0f;`
	`114`	`+ return str ? Strutil::from_string<float>(str) : 0.0f;`
`115`	`115`	`}`
`116`	`116`
`117`	`117`	`OSL_SHADEOP const char *`
Original file line number	Diff line number	Diff line change
`@@ -194,13 +194,13 @@ using namespace OSL::pvt;`
`194`	`194`
`195`	`195`	`/* Literal values */`
`196`	`196`	`{INTEGER} {`
`197`		`- yylval.i = atoi (yytext);`
	`197`	`+ yylval.i = OIIO::Strutil::from_string<int>(yytext);`
`198`	`198`	`// std::cerr << "lex int " << yylval.i << "\n";`
`199`	`199`	`return INT_LITERAL;`
`200`	`200`	`}`
`201`	`201`
`202`	`202`	`{FLT} {`
`203`		`- yylval.f = atof (yytext);`
	`203`	`+ yylval.f = OIIO::Strutil::from_string<float>(yytext);`
`204`	`204`	`// std::cerr << "lex float " << yylval.f << "\n";`
`205`	`205`	`return FLOAT_LITERAL;`
`206`	`206`	`}`