Skip to content

queryfrontend: reject lookback_delta values that overflow int64 - #9011

Open
youdie006 wants to merge 2 commits into
thanos-io:mainfrom
youdie006:queryfrontend-lookback-delta-overflow
Open

youdie006 wants to merge 2 commits into
thanos-io:mainfrom
youdie006:queryfrontend-lookback-delta-overflow

Conversation

@youdie006

Copy link
Copy Markdown
  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

parseDurationMillis (pkg/queryfrontend/queryrange_codec.go:214) guards its float64 to int64 conversion with ts > float64(math.MaxInt64). float64(math.MaxInt64) rounds up to exactly 2^63, which int64 cannot hold, so a value landing on 2^63 passes the guard and the conversion overflows. NaN passes too, since it compares false against every bound.

Three parameters go through this one parser, and two of them happen to catch the overflowed value downstream — step by result.Step <= 0 (:79-81), max_source_resolution by maxSourceResolution < 0 (:250-252). lookback_delta has no such check.

Decoded and re-encoded through the production codec on main:

param                  value                    verdict    LookbackDelta          re-encoded lookback_delta
lookback_delta         60                       ACCEPTED   60000                  60
lookback_delta         9223372036854775.808     ACCEPTED   -9223372036854775808   DROPPED
lookback_delta         NaN                      ACCEPTED   -9223372036854775808   DROPPED
step                   9223372036854775.808     REJECTED   (400 zero or negative query resolution step widths are not accepted)
step                   NaN                      REJECTED   (400 same)
max_source_resolution  9223372036854775.808     REJECTED   (400 negative max_source_resolution is not accepted)

So a user who sets lookback_delta to such a value gets HTTP 200 with no error, and the parameter is silently removed from the request forwarded to the querier — because re-encoding is gated on thanosReq.LookbackDelta > 0 (:197) — so the query runs with the server default instead. The bogus value also lands in the response cache key (pkg/queryfrontend/cache.go:112). Two sibling parameters parsed by the very same function return 400 for the same input.

parseLookbackDelta is shared with the instant-query codec (pkg/queryfrontend/queryinstant_codec.go:159, same > 0 re-encode gate at :219), so this one change covers both paths.

The lower bound deliberately stays exclusive. Unlike the upper one, float64(math.MinInt64) is exactly -2^63 and is representable, so -9223372036854775.808 is a valid duration; a test row pins that so the two bounds do not get "fixed" into symmetry.

Not touched here, mentioned as follow-ups: the identical guard at pkg/api/query/v1.go:1605 (open PR #8817 is already in that file) and at internal/cortex/querier/queryrange/query_range.go:866 (vendored Cortex fork).

Verification

Rows added to the existing TestQueryRangeCodec_DecodeRequest table.

  • The two error rows fail on main and pass here; the negative-bound row passes on both, guarding the asymmetry.
  • Mutation-checked, all three caught: >= weakened back to >; the math.IsNaN check removed; and the lower bound changed to <=.
  • gofmt clean, go vet ./pkg/queryfrontend/ clean.
  • go test ./pkg/queryfrontend/ -run TestQueryRangeCodec_DecodeRequest passes. The package as a whole has one failure, TestLabelsCodec_DecodeResponse, which panics with a slice bounds out of range inside encoding/json; it does the same on an unmodified tree here (Go 1.26.3 locally against the repo's declared 1.26.0), so it is unrelated to this change.

Go makes an out-of-range float to int conversion implementation-defined — amd64 gives MinInt64, arm64 saturates to MaxInt64 — so the added tests assert only that an error is returned.


AI assistance disclosure: this patch was found and written with Claude Code. The table above is verbatim from running the codec against main.

parseDurationMillis guarded its float64 to int64 conversion with
ts > float64(math.MaxInt64). float64(math.MaxInt64) rounds up to exactly 2^63,
which int64 cannot hold, so a value landing on 2^63 passed the guard and the
conversion overflowed to a negative duration. NaN passed as well, since it
compares false against every bound.

Three parameters go through this one parser, and two of them happen to catch the
overflowed value downstream: step is rejected by result.Step <= 0 and
max_source_resolution by maxSourceResolution < 0. lookback_delta has no such
check, so the negative value reached the request, and because re-encoding is
gated on LookbackDelta > 0 the parameter was then dropped from the request
forwarded to the querier. The user got 200 with no error and their query ran
with the default lookback instead.

The lower bound stays exclusive on purpose: float64(math.MinInt64) is exactly
-2^63 and is representable, so -9223372036854775.808 is a valid duration, and a
test pins it.

Signed-off-by: manon <youdie006@users.noreply.github.com>
Signed-off-by: manon <youdie006@users.noreply.github.com>
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.54%. Comparing base (d7833f6) to head (b1c2e2b).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9011      +/-   ##
==========================================
+ Coverage   64.52%   64.54%   +0.01%     
==========================================
  Files         289      289              
  Lines       37366    37366              
==========================================
+ Hits        24111    24117       +6     
+ Misses      11157    11156       -1     
+ Partials     2098     2093       -5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant