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

Fix off-by-one in char_buffer_length. #1060

Merged
merged 3 commits into from
Nov 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions include/highfive/bits/H5Converter_misc.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,14 @@ enum class BufferMode { Read, Write };
///
/// \brief String length in bytes excluding the `\0`.
///
inline size_t char_buffer_size(char const* const str, size_t max_string_length) {
for (size_t i = 0; i <= max_string_length; ++i) {
inline size_t char_buffer_length(char const* const str, size_t max_string_size) {
for (size_t i = 0; i < max_string_size; ++i) {
if (str[i] == '\0') {
return i;
}
}

return max_string_length;
return max_string_size;
}


Expand Down Expand Up @@ -229,7 +229,7 @@ struct StringBuffer {
/// `length() + 1` bytes long.
size_t length() const {
if (buffer.isNullTerminated()) {
return char_buffer_size(data(), buffer.string_size);
return char_buffer_length(data(), buffer.string_size);
} else {
return buffer.string_max_length;
}
Expand Down
35 changes: 25 additions & 10 deletions tests/unit/test_string.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -207,30 +207,45 @@ void check_multiple_string(File file, size_t string_length) {
}

template <class CreateTraits>
void check_supposedly_nullterm(HighFive::File& file) {
void check_supposedly_nullterm(HighFive::File& file, size_t string_length) {
auto dataspace = HighFive::DataSpace::Scalar();
auto datatype = HighFive::FixedLengthStringType(5, HighFive::StringPadding::NullTerminated);
auto obj = CreateTraits::create(file, "not_null_terminated", dataspace, datatype);

// Creates a 5 byte, "null-terminated", fixed-length string. The first five
// bytes are filled with "GROUP". Clearly, this isn't null-terminated. However,
// h5py will read it back as "GROUP", HDF5 allows us to create these; and they're
auto datatype = HighFive::FixedLengthStringType(string_length,
HighFive::StringPadding::NullTerminated);
auto obj = CreateTraits::create(file,
"not_null_terminated_" + std::to_string(string_length),
dataspace,
datatype);

// Creates an `string_length` byte, "null-terminated", fixed-length string. The first
// `string_length` bytes are filled with "a"s. Clearly, this isn't null-terminated. However,
// h5py will read it back, HDF5 allows us to create these; and they're
// found in the wild.
std::string value = "GROUP";
std::string value(string_length, 'a');
obj.write_raw(value.c_str(), datatype);

auto actual = obj.template read<std::string>();
REQUIRE(actual == value);
}

template <class CreateTraits>
void check_supposedly_nullterm_scan(HighFive::File& file) {
for (size_t n = 1; n < 256; ++n) {
check_supposedly_nullterm<CreateTraits>(file, n);
}

check_supposedly_nullterm<CreateTraits>(file, 4091);
check_supposedly_nullterm<CreateTraits>(file, 4092);
check_supposedly_nullterm<CreateTraits>(file, 4093);
}

TEST_CASE("HighFiveSTDString (attribute, nullterm cornercase)") {
auto file = HighFive::File("not_null_terminated_attribute.h5", HighFive::File::Truncate);
check_supposedly_nullterm<testing::AttributeCreateTraits>(file);
check_supposedly_nullterm_scan<testing::AttributeCreateTraits>(file);
}

TEST_CASE("HighFiveSTDString (dataset, nullterm cornercase)") {
auto file = HighFive::File("not_null_terminated_dataset.h5", HighFive::File::Truncate);
check_supposedly_nullterm<testing::DataSetCreateTraits>(file);
check_supposedly_nullterm_scan<testing::DataSetCreateTraits>(file);
}

TEST_CASE("HighFiveSTDString (dataset, single, short)") {
Expand Down
Loading