Skip to content

HIP: Switch to std::vector in rocblas version check #11820

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 12, 2025
Merged

HIP: Switch to std::vector in rocblas version check #11820

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ggml/src/ggml-cuda/ggml-cuda.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,11 +178,11 @@ static ggml_cuda_device_info ggml_cuda_init() {
int major_version = 0;
size_t version_length = 0;
if (rocblas_get_version_string_size(&version_length) == rocblas_status_success) {
std::string version(version_length, '\0');
std::vector<char> version(version_length+1, '\0');
if (rocblas_get_version_string(version.data(), version.size()) == rocblas_status_success) {
version.resize(::strlen(version.c_str()));
version.resize(::strlen(version.data()));
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()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is version.size() correct in terms of null termination? I think version.data() + version_length should definitely be correct though.

Copy link
Collaborator Author

@IMbackK IMbackK Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed the resize one line up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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? :-)

major_version = parsed_value;
}
}
Expand Down
Loading