Annotate the borrowed lifetime of CollectionStatistics::getHistogram()

ExportXMLWordPrintableJSON

    • Type: Bug
    • Resolution: Unresolved
    • Priority: Major - P3
    • None
    • Affects Version/s: None
    • Component/s: None
    • None
    • Query Optimization
    • ALL
    • Hide

      Tested on MongoDB 8.0.0 revision b41cda4fe697dce6fd9b83b3805362ccc02fbeb3.

      Add two focused tests to src/mongo/db/query/stats/stats_cache_test.cpp:

      1. BorrowOutlivesStatisticsOwner

      ```cpp
      const ArrayHistogram* borrowed = nullptr;

      { CollectionStatisticsImpl statistics(1.0, nss); statistics.addHistogram("a", ArrayHistogram::make()); borrowed = statistics.getHistogram("a"); }

      borrowed->getSampleSize();
      ```

      2. BorrowOutlivesSamePathReplacement

      ```cpp
      CollectionStatisticsImpl statistics(1.0, nss);
      statistics.addHistogram("a", ArrayHistogram::make());
      const auto* borrowed = statistics.getHistogram("a");
      statistics.addHistogram("a", ArrayHistogram::make());
      borrowed->getSampleSize();
      ```

      Build with:

      ```sh
      python buildscripts/scons.py \
      --dbg=on --opt=on \
      --allocator=system \
      --sanitize=address,undefined \
      --link-model=dynamic \
      build/optdebug/mongo/db/query/stats/stats_cache_test
      ```

      Run the two tests separately. Each exits with status 134 and reports an AddressSanitizer heap-use-after-free.

      Show
      Tested on MongoDB 8.0.0 revision b41cda4fe697dce6fd9b83b3805362ccc02fbeb3. Add two focused tests to src/mongo/db/query/stats/stats_cache_test.cpp: 1. BorrowOutlivesStatisticsOwner ```cpp const ArrayHistogram* borrowed = nullptr; { CollectionStatisticsImpl statistics(1.0, nss); statistics.addHistogram("a", ArrayHistogram::make()); borrowed = statistics.getHistogram("a"); } borrowed->getSampleSize(); ``` 2. BorrowOutlivesSamePathReplacement ```cpp CollectionStatisticsImpl statistics(1.0, nss); statistics.addHistogram("a", ArrayHistogram::make()); const auto* borrowed = statistics.getHistogram("a"); statistics.addHistogram("a", ArrayHistogram::make()); borrowed->getSampleSize(); ``` Build with: ```sh python buildscripts/scons.py \ --dbg=on --opt=on \ --allocator=system \ --sanitize=address,undefined \ --link-model=dynamic \ build/optdebug/mongo/db/query/stats/stats_cache_test ``` Run the two tests separately. Each exits with status 134 and reports an AddressSanitizer heap-use-after-free.
    • None
    • None
    • None
    • None
    • None
    • None
    • None

      CollectionStatistics::getHistogram() returns a non-owning pointer to a histogram stored in the receiver's mutable _histograms map:

      ```cpp
      void addHistogram(const std::string& path,
      std::shared_ptr<const ArrayHistogram> histogram) const

      { _histograms[path] = histogram; }

      const ArrayHistogram* getHistogram(const std::string& path) const

      { if (auto it = _histograms.find(path); it != _histograms.end()) return it->second.get(); const auto histogram = std::move(swHistogram.getValue()); addHistogram(path, histogram); return histogram.get(); }

      ```

      The local shared_ptr is safe because the map retains an owner. However, the returned pointer dangles if:

      1. the CollectionStatistics object is destroyed; or
      2. addHistogram() replaces the same-path entry and releases the old histogram's last owner.

      Both cases were reproduced on MongoDB 8.0.0, revision b41cda4fe697dce6fd9b83b3805362ccc02fbeb3, using Clang 18.1.8 with ASan/UBSan and the system allocator. Both tests reported a symbolized heap-use-after-free.

      The same unannotated ownership pattern remains on master revision a4aede23f74385fbce982896d8e19c9081e1dab2. ArrayHistogram has been renamed to CEHistogram, and the code is now in:

      • src/mongo/db/query/compiler/stats/collection_statistics.h
      • src/mongo/db/query/compiler/stats/collection_statistics_impl.cpp

      I did not find a current production caller that violates this implicit contract.

      Suggested fix:

      • Add MONGO_COMPILER_LIFETIME_BOUND to getHistogram() to annotate the lifetime dependency.
      • If callers need to retain the result across mutations, return a shared_ptr instead.

            Assignee:
            Unassigned
            Reporter:
            Shuhao N/A (EXT)
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Created:
              Updated: