Skip to content

Conversation

@lgritz
Copy link
Collaborator

@lgritz lgritz commented Oct 25, 2017

@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.

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 25, 2017

This is a counter-proposal to #800

@fpsunflower
Copy link
Contributor

LGTM

@marsupial
Copy link
Contributor

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.
line 689: p.remove_prefix (end-p.begin()); can throw with a std::string_view.

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 25, 2017

Oh, let me have a look at parse_int, too, then.

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 25, 2017

Ow, you are quite right. Will make a fix there...

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 25, 2017

parse_float as well...

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 25, 2017

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.

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 25, 2017

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.

@marsupial
Copy link
Contributor

marsupial commented Oct 25, 2017

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.

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 25, 2017

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.
@lgritz lgritz merged commit f785cc2 into AcademySoftwareFoundation:master Oct 27, 2017
@lgritz lgritz deleted the lg-safestring branch October 27, 2017 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants