Skip to content

Fix unit test UB resulting from incorrect initialization#751

Merged
rapids-bot[bot] merged 1 commit intorapidsai:branch-25.08from
kingcrimsontianyu:fix-test-ub
Jun 13, 2025
Merged

Fix unit test UB resulting from incorrect initialization#751
rapids-bot[bot] merged 1 commit intorapidsai:branch-25.08from
kingcrimsontianyu:fix-test-ub

Conversation

@kingcrimsontianyu
Copy link
Contributor

@kingcrimsontianyu kingcrimsontianyu commented Jun 9, 2025

KvikIO unit test has a utility class EnvVarContext class, introduced in #700, and slightly improved in #735. It has been identified that this class was incorrectly initialized, resulting in UB: it causes the unit test failure in C++20, which by fluke was not observed in C++17. This PR fixes this error. Specifically, the constructor of EnvVarContext is:

EnvVarContext(std::initializer_list<std::pair<std::string_view, std::string_view>> env_var_entries);

There are several ways of instantiation:

// Direct initialization
EnvVarContext env_var_ctx({{"env_1", "v1"}, {"env_2", "v2"}});

// Direct list initialization
EnvVarContext env_var_ctx{{"env_1", "v1"}, {"env_2", "v2"}};

// Copy list initialization
EnvVarContext env_var_ctx = {{"env_1", "v1"}, {"env_2", "v2"}};

The erroneous instantiation performed is:

// Extra pair of braces
// {}: brace-enclosed initializer list
// {{"env_1", "v1"}, {"env_2", "v2"}}: one element of type pair<std::string_view, std::string_view>
// {"env_1", "v1"}: first
// {"env_2", "v2"}: second
EnvVarContext env_var_ctx{{{"env_1", "v1"}, {"env_2", "v2"}}};

As a result, the initializer list only has 1 pair, with the key being {"env_1", "v1"} and value being {"env_2", "v2"}. For the key, for instance, the 5-th overload (https://en.cppreference.com/w/cpp/string/basic_string_view/basic_string_view.html) of the constructor was used, where first points to "env_1" and last points to "v1". Since the two iterators do not form a valid range, UB ensues.

@kingcrimsontianyu kingcrimsontianyu added bug Something isn't working non-breaking Introduces a non-breaking change c++ Affects the C++ API of KvikIO labels Jun 9, 2025
@copy-pr-bot
Copy link

copy-pr-bot bot commented Jun 9, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@kingcrimsontianyu kingcrimsontianyu marked this pull request as ready for review June 9, 2025 06:12
@kingcrimsontianyu kingcrimsontianyu requested a review from a team as a code owner June 9, 2025 06:12
@kingcrimsontianyu kingcrimsontianyu requested a review from vuule June 9, 2025 06:13
Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

@vuule
Copy link
Contributor

vuule commented Jun 13, 2025

/merge

@rapids-bot rapids-bot bot merged commit 03351cf into rapidsai:branch-25.08 Jun 13, 2025
52 checks passed
rapids-bot bot pushed a commit that referenced this pull request Jul 25, 2025
This PR changes KvikIO C++ standard from 17 to 20.

Depends on #751

Authors:
  - Tianyu Liu (https://github.com/kingcrimsontianyu)
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Mads R. B. Kristensen (https://github.com/madsbk)
  - Bradley Dice (https://github.com/bdice)

URL: #749
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working c++ Affects the C++ API of KvikIO non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants