Skip to content
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

pbrt::SplitString triggers dereferencing asserts #108

Closed
pierremoreau opened this issue Feb 15, 2021 · 2 comments
Closed

pbrt::SplitString triggers dereferencing asserts #108

pierremoreau opened this issue Feb 15, 2021 · 2 comments

Comments

@pierremoreau
Copy link
Contributor

Visual Studio (2019, 16.8.5) has asserts checking that dereferencing a string-view iterator occurs within the range of the string-view.

pbrt::SplitString() expects the string-view to end with a '\0', however when constructing std::string the '\0' character does not count as being part of the string even if space is allocated for it.
For example,

std::string str = "Geometry/Disks";
std::cout << str.size() << '\n'; // Outputs: 14, i.e. the length of the string without the final '\0'.
assert(str.c_str()[str.size()] == '\0'); // Valid since C++11, and is true.

So once pbrt::SplitString("Geometry/Disks", '/') has pushed the string "Disks", end and begin now both point to the 15th character of str (i.e. the '\0') which also corresponds to str.end(), so when running if (*begin == ch) it tries to get the character at index 14 which is no longer < str.size() as str.size() == 14 and the assert fails.
Since the value at that position is guaranteed to be '\0' since C++11 (unless the user decided to modify it), the program will not end up reading undefined memory and will perform as expected, assuming the string_view was constructed from std::string and not something else.

It would probably be safer to change if (*begin == ch) to if (end != str.end() && *begin == ch).

@mmp
Copy link
Owner

mmp commented Feb 15, 2021

Thanks for reporting this! I've rewritten the logic (and slightly simplified it) in a way that I think should fix this. Please let me know if it takes care of it for you, though.

@pierremoreau
Copy link
Contributor Author

I can finally confirm that the rework fixed it. Thank you for the quick fix!

Dolkar pushed a commit to Dolkar/pbrt-v4-myod-integration that referenced this issue May 8, 2023
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

No branches or pull requests

2 participants