QueryKnobSnapshotCache _version can overflow

ExportXMLWordPrintableJSON

    • Type: Bug
    • Resolution: Fixed
    • Priority: Major - P3
    • 9.0.0-rc1
    • Affects Version/s: None
    • Component/s: None
    • None
    • Query Execution
    • Fully Compatible
    • ALL
    • Hide 
      # add the following to /src/mongo/db/query/query_knobs/query_knob_snapshot_test.cpp

#include "mongo/util/scopeguard.h"

// TODO(SERVER-130362): repro attempt for a suspected 8-bit version wraparound bug. '_version' is
// an AtomicWord<uint8_t>, so exactly 256 knob updates bring it back to the same value. If a
// thread's cached snapshot survives the wraparound, getThreadLocalSnapshot() will incorrectly
// report a cache hit and return stale data even though the real snapshot has moved on.
TEST(QueryKnobSnapshotCacheTest, ThreadLocalCacheSurvivesVersionWraparound) {
    const auto& cache = QueryKnobSnapshotCache::instance();
    const int initialValue = cache.getSnapshot().get<int>(test_knobs::testIntKnob.id);
    auto before = cache.getThreadLocalSnapshot();
    ASSERT_EQ(before.get<int>(test_knobs::testIntKnob.id), initialValue);
    auto* serverParam = ServerParameterSet::getNodeParameterSet()->getIfExists("testIntKnob");
    ASSERT(serverParam);
    auto setValue = [&](int value) {
        BSONObjBuilder b;
        b.append("testIntKnob", value);
        uassertStatusOK(serverParam->set(b.obj().firstElement(), boost::none));
    };
    ON_BLOCK_EXIT([&] { setValue(initialValue); });
    // Exactly 256 updates wrap the uint8_t version counter back to the value 'before' observed,
    // while genuinely changing the knob's value. testIntKnob is validated to (0, 1000).
    for (int i = 0; i < 256; i++) {
        setValue(100 + i);
    }
    const int expectedCurrentValue = 100 + 255;
    ASSERT_EQ(cache.getSnapshot().get<int>(test_knobs::testIntKnob.id), expectedCurrentValue);
    auto after = cache.getThreadLocalSnapshot();
    ASSERT_EQ(after.get<int>(test_knobs::testIntKnob.id), expectedCurrentValue);
}
      Show
      # add the following to /src/mongo/db/query/query_knobs/query_knob_snapshot_test.cpp #include "mongo/util/scopeguard.h" // TODO(SERVER-130362): repro attempt for a suspected 8-bit version wraparound bug. '_version' is // an AtomicWord<uint8_t>, so exactly 256 knob updates bring it back to the same value. If a // thread's cached snapshot survives the wraparound, getThreadLocalSnapshot() will incorrectly // report a cache hit and return stale data even though the real snapshot has moved on. TEST(QueryKnobSnapshotCacheTest, ThreadLocalCacheSurvivesVersionWraparound) {     const auto& cache = QueryKnobSnapshotCache::instance();     const int initialValue = cache.getSnapshot().get< int >(test_knobs::testIntKnob.id);     auto before = cache.getThreadLocalSnapshot();     ASSERT_EQ(before.get< int >(test_knobs::testIntKnob.id), initialValue);     auto* serverParam = ServerParameterSet::getNodeParameterSet()->getIfExists( "testIntKnob" );     ASSERT(serverParam);     auto setValue = [&]( int value) {         BSONObjBuilder b;         b.append( "testIntKnob" , value);         uassertStatusOK(serverParam->set(b.obj().firstElement(), boost::none));     };     ON_BLOCK_EXIT([&] { setValue(initialValue); });     // Exactly 256 updates wrap the uint8_t version counter back to the value 'before' observed,     // while genuinely changing the knob's value. testIntKnob is validated to (0, 1000).     for ( int i = 0; i < 256; i++) {         setValue(100 + i);     }     const int expectedCurrentValue = 100 + 255;     ASSERT_EQ(cache.getSnapshot().get< int >(test_knobs::testIntKnob.id), expectedCurrentValue);     auto after = cache.getThreadLocalSnapshot();     ASSERT_EQ(after.get< int >(test_knobs::testIntKnob.id), expectedCurrentValue); }
    • 0
    • None
    • None
    • None
    • None
    • None
    • None
    • None

      QueryKnobSnapshotCache._version is a 8 bit integer, but it needs to be large enough to handle reasonable numbers of query knob updates.

      If you update the query knobs more than 256 times the cache version will wrap around and cause a logic error in the cache validity checking logic here. getThreadLocalSnapshot() will report a cache hit and hand back a stale snapshot — even though the real global knob state has since changed multiple times.

      I saw this as a flaky CI failure on the all-feature-flags variant, because those pass 60+ setParameters at startup, bringing us closer to the limit.

      This bug was introduced 3 weeks ago, so it was never released.

            Assignee:
            Steve Tarzia
            Reporter:
            Steve Tarzia
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved: