Investigate implicit double->int narrowing in WindowFunctionPercentileCommon::computePercentile

XMLWordPrintableJSON

    • Query Integration
    • None
    • None
    • None
    • None
    • None
    • None
    • None

      Context

      Copilot flagged the following on a PR review comment in WindowFunctionPercentileCommon::computePercentile:

      Value WindowFunctionPercentileCommon::computePercentile(double p) const {
          const double n = _values.size();
          ...
          if (_method != PercentileMethodEnum::kContinuous) {
              const double rank = DiscretePercentile::computeTrueRank(n, p);
              ...
      

      n is derived from _values.size() (a size_t) but stored as double and passed to computeTrueRank(int, ...), requiring an implicit double -> int narrowing conversion. If the window ever contained more than INT_MAX elements, the conversion is UB.

      Copilot's suggestion: store n as int (with a checked conversion) since the rank APIs already take int.

      Impact Estimate

      sroche noted in the PR thread:

      bummer. I think technically the memory tracker/limits would cap out before we would reach this condition, but maybe it's worth fixing?

      * $setWindowFields default memory limit (internalDocumentSourceSetWindowFieldsMaxMemoryBytes): 100 MiB (see src/mongo/db/query/stage_memory_limit_knobs/stage_memory_limit_knobs.idl).

      • INT_MAX ≈ 2.15 × 10^9.
      • Value is at least ~16 bytes; a window holding INT_MAX Value s would consume >= ~34 GiB even ignoring container overhead — far above the 100 MiB default and above the SimpleMemoryUsageTracker that $setWindowFields plumbs through its exec classes.

      At the default knob, the memory tracker should trip long before n approaches INT_MAX. However:

      • The knob is settable at startup and runtime, so users can raise it arbitrarily.
      • We should confirm this specific code path is actually behind that memory tracker (i.e., that _values growth is reported to the tracker before we call computePercentile). If it isn't, the "memory tracker saves us" argument doesn't apply.

      Proposed fix (if we decide to)

      Change n's type from double to int (or size_t with a checked narrowing conversion / tassert) so we don't rely on an implicit double -> int conversion. Match whatever computeTrueRank and computeTrueRank's continuous counterpart take.

      Acceptance

      • Confirm (or refute) the memory-tracker-saves-us claim by tracing whether _values contributes to the SimpleMemoryUsageTracker on the $percentile window function path.
      • If confirmed and the fix is deemed unnecessary: close with a comment documenting the finding.
      • If not confirmed, or if we still want defense-in-depth: apply the narrowing fix and add a tassert.

            Assignee:
            Unassigned
            Reporter:
            Charlie Swanson
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated: