Skip to content

Commit f785cc2

Browse files
lgritzlgritz
authored
Fixes to ShadingSystemImpl::decode_connected_param (#805)
@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.
1 parent 0c6e557 commit f785cc2

File tree

8 files changed

+69
-74
lines changed

8 files changed

+69
-74
lines changed

‎src/liboslcomp/osllex.l

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ void preprocess (const char *yytext);
223223

224224

225225
{FLT} {
226-
yylval.f = atof (yytext);
226+
yylval.f = OIIO::Strutil::from_string<float>(yytext);
227227
SETLINE;
228228
return FLOAT_LITERAL;
229229
}

‎src/liboslexec/constfold.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1239,7 +1239,7 @@ DECLFOLDER(constfold_stoi)
12391239
if (S.is_constant()) {
12401240
ASSERT (S.typespec().is_string());
12411241
ustring s = *(ustring *)S.data();
1242-
int cind = rop.add_constant ((int) strtol(s.c_str(), NULL, 10));
1242+
int cind = rop.add_constant (Strutil::from_string<int>(s));
12431243
rop.turn_into_assign (op, cind, "const fold stoi");
12441244
return 1;
12451245
}
@@ -1256,7 +1256,7 @@ DECLFOLDER(constfold_stof)
12561256
if (S.is_constant()) {
12571257
ASSERT (S.typespec().is_string());
12581258
ustring s = *(ustring *)S.data();
1259-
int cind = rop.add_constant ((float) strtod(s.c_str(), NULL));
1259+
int cind = rop.add_constant (Strutil::from_string<float>(s));
12601260
rop.turn_into_assign (op, cind, "const fold stof");
12611261
return 1;
12621262
}

‎src/liboslexec/dictionary.cpp

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -372,10 +372,11 @@ Dictionary::dict_value (int nodeID, ustring attribname,
372372
}
373373
if (type.basetype == TypeDesc::INT) {
374374
r.valueoffset = (int) m_intdata.size();
375+
string_view valstr (val);
375376
for (int i = 0; i < n; ++i) {
376-
int v = (int) strtol (val, (char **)&val, 10);
377-
while (isspace(*val) || *val == ',')
378-
++val;
377+
int v;
378+
OIIO::Strutil::parse_int (valstr, v);
379+
OIIO::Strutil::parse_char (valstr, ',');
379380
m_intdata.push_back (v);
380381
((int *)data)[i] = v;
381382
}
@@ -384,10 +385,11 @@ Dictionary::dict_value (int nodeID, ustring attribname,
384385
}
385386
if (type.basetype == TypeDesc::FLOAT) {
386387
r.valueoffset = (int) m_floatdata.size();
388+
string_view valstr (val);
387389
for (int i = 0; i < n; ++i) {
388-
float v = (float) strtod (val, (char **)&val);
389-
while (isspace(*val) || *val == ',')
390-
++val;
390+
float v;
391+
OIIO::Strutil::parse_float (valstr, v);
392+
OIIO::Strutil::parse_char (valstr, ',');
391393
m_floatdata.push_back (v);
392394
((float *)data)[i] = v;
393395
}

‎src/liboslexec/opstring.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,13 @@ osl_endswith_iss (const char *s_, const char *substr_)
105105
OSL_SHADEOP int
106106
osl_stoi_is (const char *str)
107107
{
108-
return str ? strtol(str, NULL, 10) : 0;
108+
return str ? Strutil::from_string<int>(str) : 0;
109109
}
110110

111111
OSL_SHADEOP float
112112
osl_stof_fs (const char *str)
113113
{
114-
return str ? (float)strtod(str, NULL) : 0.0f;
114+
return str ? Strutil::from_string<float>(str) : 0.0f;
115115
}
116116

117117
OSL_SHADEOP const char *

‎src/liboslexec/osolex.l

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,13 +194,13 @@ using namespace OSL::pvt;
194194
195195
/* Literal values */
196196
{INTEGER} {
197-
yylval.i = atoi (yytext);
197+
yylval.i = OIIO::Strutil::from_string<int>(yytext);
198198
// std::cerr << "lex int " << yylval.i << "\n";
199199
return INT_LITERAL;
200200
}
201201
202202
{FLT} {
203-
yylval.f = atof (yytext);
203+
yylval.f = OIIO::Strutil::from_string<float>(yytext);
204204
// std::cerr << "lex float " << yylval.f << "\n";
205205
return FLOAT_LITERAL;
206206
}

‎src/liboslexec/shadingsys.cpp

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2482,10 +2482,9 @@ ShadingSystemImpl::decode_connected_param (string_view connectionname,
24822482

24832483
// Look for a bracket in the "parameter name"
24842484
size_t bracketpos = connectionname.find ('[');
2485-
const char *bracket = bracketpos == string_view::npos ? NULL
2486-
: connectionname.data()+bracketpos;
24872485
// Grab just the part of the param name up to the bracket
24882486
ustring param (connectionname, 0, bracketpos);
2487+
string_view cname_remaining = connectionname.substr (bracketpos);
24892488

24902489
// Search for the param with that name, fail if not found
24912490
c.param = inst->findsymbol (param);
@@ -2514,36 +2513,52 @@ ShadingSystemImpl::decode_connected_param (string_view connectionname,
25142513

25152514
c.type = sym->typespec();
25162515

2517-
if (bracket && c.type.is_array()) {
2516+
if (! cname_remaining.empty() && c.type.is_array()) {
25182517
// There was at least one set of brackets that appears to be
25192518
// selecting an array element.
2520-
c.arrayindex = atoi (bracket+1);
2519+
int index = 0;
2520+
if (! (Strutil::parse_char (cname_remaining, '[') &&
2521+
Strutil::parse_int (cname_remaining, index) &&
2522+
Strutil::parse_char (cname_remaining, ']'))) {
2523+
error ("ConnectShaders: malformed parameter \"%s\"", connectionname);
2524+
c.param = -1; // mark as invalid
2525+
return c;
2526+
}
2527+
c.arrayindex = index;
25212528
if (c.arrayindex >= c.type.arraylength()) {
25222529
error ("ConnectShaders: cannot request array element %s from a %s",
25232530
connectionname, c.type.c_str());
25242531
c.arrayindex = c.type.arraylength() - 1; // clamp it
25252532
}
25262533
c.type.make_array (0); // chop to the element type
2527-
bracket = strchr (bracket+1, '['); // skip to next bracket
2534+
Strutil::skip_whitespace (cname_remaining); // skip to next bracket
25282535
}
25292536

2530-
if (bracket && ! c.type.is_closure() &&
2531-
c.type.aggregate() != TypeDesc::SCALAR) {
2537+
if (! cname_remaining.empty() && cname_remaining.front() == '[' &&
2538+
! c.type.is_closure() && c.type.aggregate() != TypeDesc::SCALAR) {
25322539
// There was at least one set of brackets that appears to be
25332540
// selecting a color/vector component.
2534-
c.channel = atoi (bracket+1);
2541+
int index = 0;
2542+
if (! (Strutil::parse_char (cname_remaining, '[') &&
2543+
Strutil::parse_int (cname_remaining, index) &&
2544+
Strutil::parse_char (cname_remaining, ']'))) {
2545+
error ("ConnectShaders: malformed parameter \"%s\"", connectionname);
2546+
c.param = -1; // mark as invalid
2547+
return c;
2548+
}
2549+
c.channel = index;
25352550
if (c.channel >= (int)c.type.aggregate()) {
25362551
error ("ConnectShaders: cannot request component %s from a %s",
25372552
connectionname, c.type.c_str());
25382553
c.channel = (int)c.type.aggregate() - 1; // clamp it
25392554
}
25402555
// chop to just the scalar part
25412556
c.type = TypeSpec ((TypeDesc::BASETYPE)c.type.simpletype().basetype);
2542-
bracket = strchr (bracket+1, '['); // skip to next bracket
2557+
Strutil::skip_whitespace (cname_remaining);
25432558
}
25442559

2545-
// Deal with left over brackets
2546-
if (bracket) {
2560+
// Deal with left over nonsense or unsupported param designations
2561+
if (! cname_remaining.empty()) {
25472562
// Still a leftover bracket, no idea what to do about that
25482563
error ("ConnectShaders: don't know how to connect '%s' when \"%s\" is a \"%s\"",
25492564
connectionname, param.c_str(), c.type.c_str());

‎src/testrender/testrender.cpp

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -152,24 +152,16 @@ void getargs(int argc, const char *argv[])
152152
errhandler.verbosity (ErrorHandler::VERBOSE);
153153
}
154154

155-
Vec3 strtovec(const char* str) {
155+
Vec3 strtovec(string_view str) {
156156
Vec3 v(0, 0, 0);
157-
sscanf(str, " %f , %f , %f", &v.x, &v.y, &v.z);
157+
OIIO::Strutil::parse_float (str, v[0]);
158+
OIIO::Strutil::parse_char (str, ',');
159+
OIIO::Strutil::parse_float (str, v[1]);
160+
OIIO::Strutil::parse_char (str, ',');
161+
OIIO::Strutil::parse_float (str, v[2]);
158162
return v;
159163
}
160164

161-
int strtoint(const char* str) {
162-
int i = 0;
163-
sscanf(str, " %d", &i);
164-
return i;
165-
}
166-
167-
float strtoflt(const char* str) {
168-
float f = 0;
169-
sscanf(str, " %f", &f);
170-
return f;
171-
}
172-
173165
bool strtobool(const char* str) {
174166
return strcmp(str, "1") == 0 ||
175167
strcmp(str, "on") == 0 ||
@@ -281,7 +273,7 @@ void parse_scene() {
281273
if (dir_attr) dir = strtovec(dir_attr.value()); else
282274
if ( at_attr) dir = strtovec( at_attr.value()) - eye;
283275
if ( up_attr) up = strtovec( up_attr.value());
284-
if (fov_attr) fov = strtoflt(fov_attr.value());
276+
if (fov_attr) fov = OIIO::Strutil::from_string<float>(fov_attr.value());
285277

286278
// create actual camera
287279
camera = Camera(eye, dir, up, fov, xres, yres);
@@ -291,7 +283,7 @@ void parse_scene() {
291283
pugi::xml_attribute radius_attr = node.attribute("radius");
292284
if (center_attr && radius_attr) {
293285
Vec3 center = strtovec(center_attr.value());
294-
float radius = strtoflt(radius_attr.value());
286+
float radius = OIIO::Strutil::from_string<float>(radius_attr.value());
295287
if (radius > 0) {
296288
pugi::xml_attribute light_attr = node.attribute("is_light");
297289
bool is_light = light_attr ? strtobool(light_attr.value()) : false;
@@ -314,7 +306,7 @@ void parse_scene() {
314306
} else if (strcmp(node.name(), "Background") == 0) {
315307
pugi::xml_attribute res_attr = node.attribute("resolution");
316308
if (res_attr)
317-
backgroundResolution = strtoint(res_attr.value());
309+
backgroundResolution = OIIO::Strutil::from_string<int>(res_attr.value());
318310
backgroundShaderID = int(shaders.size()) - 1;
319311
} else if (strcmp(node.name(), "ShaderGroup") == 0) {
320312
ShaderGroupRef group;

‎src/testshade/testshade.cpp

Lines changed: 19 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,7 @@ action_param (int argc, const char *argv[])
303303
else if (OIIO::Strutil::istarts_with(splits[0],"type="))
304304
type.fromstring (splits[0].c_str()+5);
305305
else if (OIIO::Strutil::istarts_with(splits[0],"lockgeom="))
306-
unlockgeom = (strtol (splits[0].c_str()+9, NULL, 10) == 0);
306+
unlockgeom = (OIIO::Strutil::from_string<int> (splits[0]) == 0);
307307
}
308308

309309
// 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[])
313313
&f[0], &f[1], &f[2], &f[3],
314314
&f[4], &f[5], &f[6], &f[7], &f[8], &f[9], &f[10], &f[11],
315315
&f[12], &f[13], &f[14], &f[15]) == 16) {
316-
params.push_back (ParamValue());
317-
params.back().init (paramname, TypeDesc::TypeMatrix, 1, f);
316+
params.emplace_back (paramname, TypeDesc::TypeMatrix, 1, f);
318317
if (unlockgeom)
319318
params.back().interp (ParamValue::INTERP_VERTEX);
320319
return;
@@ -324,37 +323,28 @@ action_param (int argc, const char *argv[])
324323
&& sscanf (stringval.c_str(), "%g, %g, %g", &f[0], &f[1], &f[2]) == 3) {
325324
if (type == TypeDesc::UNKNOWN)
326325
type = TypeDesc::TypeVector;
327-
params.push_back (ParamValue());
328-
params.back().init (paramname, type, 1, f);
326+
params.emplace_back (paramname, type, 1, f);
329327
if (unlockgeom)
330328
params.back().interp (ParamValue::INTERP_VERTEX);
331329
return;
332330
}
333331
// If it is or might be an int, look for an int that takes up the whole
334332
// string.
335-
if ((type == TypeDesc::UNKNOWN || type == TypeDesc::TypeInt)) {
336-
char *endptr = NULL;
337-
int ival = strtol(stringval.c_str(),&endptr,10);
338-
if (endptr && *endptr == 0) {
339-
params.push_back (ParamValue());
340-
params.back().init (paramname, TypeDesc::TypeInt, 1, &ival);
341-
if (unlockgeom)
342-
params.back().interp (ParamValue::INTERP_VERTEX);
343-
return;
344-
}
333+
if ((type == TypeDesc::UNKNOWN || type == TypeDesc::TypeInt)
334+
&& OIIO::Strutil::string_is<int>(stringval)) {
335+
params.emplace_back (paramname, OIIO::Strutil::from_string<int>(stringval));
336+
if (unlockgeom)
337+
params.back().interp (ParamValue::INTERP_VERTEX);
338+
return;
345339
}
346340
// If it is or might be an float, look for a float that takes up the
347341
// whole string.
348-
if ((type == TypeDesc::UNKNOWN || type == TypeDesc::TypeFloat)) {
349-
char *endptr = NULL;
350-
float fval = (float) strtod(stringval.c_str(),&endptr);
351-
if (endptr && *endptr == 0) {
352-
params.push_back (ParamValue());
353-
params.back().init (paramname, TypeDesc::TypeFloat, 1, &fval);
354-
if (unlockgeom)
355-
params.back().interp (ParamValue::INTERP_VERTEX);
356-
return;
357-
}
342+
if ((type == TypeDesc::UNKNOWN || type == TypeDesc::TypeFloat)
343+
&& OIIO::Strutil::string_is<float>(stringval)) {
344+
params.emplace_back (paramname, OIIO::Strutil::from_string<float>(stringval));
345+
if (unlockgeom)
346+
params.back().interp (ParamValue::INTERP_VERTEX);
347+
return;
358348
}
359349

360350
// Catch-all for float types and arrays
@@ -365,8 +355,7 @@ action_param (int argc, const char *argv[])
365355
OIIO::Strutil::parse_float (stringval, vals[i]);
366356
OIIO::Strutil::parse_char (stringval, ',');
367357
}
368-
params.push_back (ParamValue());
369-
params.back().init (paramname, type, 1, &vals[0]);
358+
params.emplace_back (paramname, type, 1, &vals[0]);
370359
if (unlockgeom)
371360
params.back().interp (ParamValue::INTERP_VERTEX);
372361
return;
@@ -380,8 +369,7 @@ action_param (int argc, const char *argv[])
380369
OIIO::Strutil::parse_int (stringval, vals[i]);
381370
OIIO::Strutil::parse_char (stringval, ',');
382371
}
383-
params.push_back (ParamValue());
384-
params.back().init (paramname, type, 1, &vals[0]);
372+
params.emplace_back (paramname, type, 1, &vals[0]);
385373
if (unlockgeom)
386374
params.back().interp (ParamValue::INTERP_VERTEX);
387375
return;
@@ -395,17 +383,15 @@ action_param (int argc, const char *argv[])
395383
std::vector<ustring> strelements;
396384
for (auto&& s : splitelements)
397385
strelements.push_back (ustring(s));
398-
params.push_back (ParamValue());
399-
params.back().init (paramname, type, 1, &strelements[0]);
386+
params.emplace_back (paramname, type, 1, &strelements[0]);
400387
if (unlockgeom)
401388
params.back().interp (ParamValue::INTERP_VERTEX);
402389
return;
403390
}
404391

405392
// All remaining cases -- it's a string
406393
const char *s = stringval.c_str();
407-
params.push_back (ParamValue());
408-
params.back().init (paramname, TypeDesc::TypeString, 1, &s);
394+
params.emplace_back (paramname, TypeDesc::TypeString, 1, &s);
409395
if (unlockgeom)
410396
params.back().interp (ParamValue::INTERP_VERTEX);
411397
}

0 commit comments

Comments
 (0)