HIP: Switch to std::vector in rocblas version check #11820
Conversation
| int parsed_value = 0; | ||
| if (std::from_chars(version.c_str(), version.c_str() + version.length(), parsed_value).ec == std::errc()) { | ||
| if (std::from_chars(version.data(), version.data() + version.size(), parsed_value).ec == std::errc()) { |
There was a problem hiding this comment.
Is version.size() correct in terms of null termination? I think version.data() + version_length should definitely be correct though.
There was a problem hiding this comment.
strlen returns the length without the null terminator so length of the vector at that point is without the null terminator.
std::from_chars parses from version.data() to version.data()+version.size()-1 while version.data()+version.size() is one over the end (where the \0 was before the resize)
rocblas_get_version_string_size returns the length with the null pointer, not strlen. But i dident want to rely on that so theres a defensive +1 in there.
So i believe this is correct
There was a problem hiding this comment.
Sorry, I missed the resize one line up.
There was a problem hiding this comment.
@IMbackK @JohannesGaessler I have a concern with this line: version.resize(::strlen(version.data()));
When version was a string, it kept the null terminator as part of the data - resize automatically added +1. Now the null terminator will not be part of the data. If version is ever copied or passed to another function as a string pointer it may lead to a buffer overrun.
Please either add +1 to strlen(), remove the resize() - it was necessary only for string to update its internal length - you can just use strlen() instead of size() when calling from_chars(), or better yet undo this change as non-const data() became standard in C++17:
https://en.cppreference.com/w/cpp/string/basic_string/data
There was a problem hiding this comment.
the vector is temporary to this code block and will never be used anywhere else so i dont see the issue with this.
but we can still revert this pr as unnecessary.
There was a problem hiding this comment.
@IMbackK I agree that the code does not have any issues now. But it is breaking a C/C++ convention that all strings are to be null terminated. What if someone finds this code and copy-pastes then modifies it or trains an AI on it? :-)
Should fix the issue reported by #11080 (comment)