-
Notifications
You must be signed in to change notification settings - Fork 395
Fixes to ShadingSystemImpl::decode_connected_param #805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This is a counter-proposal to #800 |
|
LGTM |
|
Seems to play nicely here with #801. FWIW OIIO::Strutil::parse_int also looks to have the same issue and may parse beyond the string_view's length. |
|
Oh, let me have a look at parse_int, too, then. |
|
Ow, you are quite right. Will make a fix there... |
|
parse_float as well... |
|
I want to point out that this is a legit bug according to spec, but in practice in our code base the string_view's are always generated from a std::string or ustring or null-terminated char* (or a substring of one of those), so it's not a huge emergency "how did this ever work" kind of bug -- in our code, every string_view does point to (or within) a null-terminated string. So it can't crash, but it can generate wrong results if there's a string like "1234" and the string_view is referring to just the first 2 characters (should parse to 12, but will erroneously parse to 1234). I don't think we actually do this anywhere, but it's a nasty bug just waiting to happen, so I want to fix it. |
|
I feel like this issue is very much related to the changes in AcademySoftwareFoundation/OpenImageIO#1785, so I think I'll address the primary problems with the parse functionality there. |
|
The strchr bug was definitely creating bad results for me. I agree the rest is a bit pedantic from what I can see in the context of OSL, but it's not too hard to imagine an exploit where OIIO (or some client of) tries parsing a well crafted string in some image header/metadata. |
|
I agree totally! I'm working on a robust fix for it all. |
@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: AcademySoftwareFoundation#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.
@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.