Skip to content

Commit

Permalink
fix: Summarizer::VariationKey operator< was unsound (#412)
Browse files Browse the repository at this point in the history
This is a soundness bug that was detected by MSVC in debug
configuration.

The `Summarizer::VariationKey` has `==` and `<` operators. 

In the case of `<`, the comparison wasn't properly handling the case
where `this->variation` was greater than the parameter. In that case, it
would fall back to comparing `this->version` which is incorrect (should
only happen if the variations are equal.)

This could have affected the JSON serialization of the summary counters,
mainly the order in which the array was serialized. I'm wondering if it
could also caused bugs in actual counts. `std::map` uses the `<`
operator to determine key equality.
  • Loading branch information
cwaldren-ld authored Jun 11, 2024
1 parent af0333f commit 57f45b2
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,22 @@ class Summarizer {
std::optional<VariationIndex> variation);

bool operator==(VariationKey const& k) const {
return k.variation == variation && k.version == version;
return !(*this < k) && !(k < *this);
}

bool operator!=(VariationKey const& k) const { return !(*this == k); }

bool operator<(VariationKey const& k) const {
if (variation < k.variation) {
return true;
}
if (variation > k.variation) {
return false;
}
return version < k.version;
}

bool operator>(VariationKey const& k) const { return k < *this; }
};

struct State {
Expand Down
46 changes: 46 additions & 0 deletions libs/internal/tests/event_summarizer_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,52 @@ TEST(SummarizerTests, ExplicitStartTimeIsCorrect) {
ASSERT_EQ(summarizer.StartTime(), start);
}

TEST(SummarizerTests, VariationKeyOrdering) {
Summarizer::VariationKey a(0, 1);

Check warning on line 39 in libs/internal/tests/event_summarizer_test.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/tests/event_summarizer_test.cpp:39:30 [readability-identifier-length]

variable name 'a' is too short, expected at least 3 characters
Summarizer::VariationKey b(1, 0);

Check warning on line 40 in libs/internal/tests/event_summarizer_test.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/tests/event_summarizer_test.cpp:40:30 [readability-identifier-length]

variable name 'b' is too short, expected at least 3 characters

ASSERT_FALSE(a < b);
ASSERT_TRUE(a > b);

ASSERT_FALSE(b > a);
ASSERT_TRUE(b < a);

ASSERT_TRUE(a != b);
ASSERT_FALSE(a == b);
}

TEST(SummarizerTests, VariationKeyEquality) {

Check warning on line 52 in libs/internal/tests/event_summarizer_test.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/tests/event_summarizer_test.cpp:52:1 [readability-function-cognitive-complexity]

function 'TestBody' has cognitive complexity of 61 (threshold 25)
Summarizer::VariationKey different(2, 2);

std::vector<Summarizer::VariationKey> keys = {
Summarizer::VariationKey(0, 1),
Summarizer::VariationKey(1, 0),
Summarizer::VariationKey(0, 0),
Summarizer::VariationKey(1, 1),
Summarizer::VariationKey(12412, 123),

Check warning on line 60 in libs/internal/tests/event_summarizer_test.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/tests/event_summarizer_test.cpp:60:34 [cppcoreguidelines-avoid-magic-numbers

12412 is a magic number; consider replacing it with a named constant

Check warning on line 60 in libs/internal/tests/event_summarizer_test.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/tests/event_summarizer_test.cpp:60:41 [cppcoreguidelines-avoid-magic-numbers

123 is a magic number; consider replacing it with a named constant
Summarizer::VariationKey(324, 5323332)};

Check warning on line 61 in libs/internal/tests/event_summarizer_test.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/tests/event_summarizer_test.cpp:61:34 [cppcoreguidelines-avoid-magic-numbers

324 is a magic number; consider replacing it with a named constant

Check warning on line 61 in libs/internal/tests/event_summarizer_test.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/tests/event_summarizer_test.cpp:61:39 [cppcoreguidelines-avoid-magic-numbers

5323332 is a magic number; consider replacing it with a named constant

for (auto const& key : keys) {
Summarizer::VariationKey a = key;

Check warning on line 64 in libs/internal/tests/event_summarizer_test.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/tests/event_summarizer_test.cpp:64:34 [readability-identifier-length]

variable name 'a' is too short, expected at least 3 characters
Summarizer::VariationKey b = key;

Check warning on line 65 in libs/internal/tests/event_summarizer_test.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

/libs/internal/tests/event_summarizer_test.cpp:65:34 [readability-identifier-length]

variable name 'b' is too short, expected at least 3 characters

ASSERT_EQ(a, b);
ASSERT_EQ(b, a);

ASSERT_FALSE(a != b);
ASSERT_FALSE(b != a);

ASSERT_FALSE(a < b);
ASSERT_FALSE(b < a);

ASSERT_FALSE(a > b);
ASSERT_FALSE(b > a);

ASSERT_NE(key, different);
ASSERT_FALSE(key == different);
}
}

struct EvaluationParams {
std::string feature_key;
Version feature_version;
Expand Down

0 comments on commit 57f45b2

Please sign in to comment.