fix(query-cache): stop heap overflow when caching multi-result responses (#6229)

Merged
RE
1mo ago
+239-4

Fixes #6229.

Problem

For a text-protocol request that returns more than one result (a CALL, or a multi-statement COM_QUERY), MySQL_Result_to_MySQL_wire() runs once per result. When a result matched a rule with cache_ttl, the store copied every packet still queued in PSarrayOUT, starting at index 0, into a buffer sized for the current result only (MyRS->resultset_size):

client_myds->resultset->copy_add(client_myds->PSarrayOUT,0,client_myds->PSarrayOUT->len);
client_myds->resultset_length=MyRS->resultset_size;
unsigned char *aa=client_myds->resultset2buffer(false);

The earlier results of the same request were still queued, so:

  • resultset2buffer() wrote past the end of the buffer by the size of those results;
  • MySQL_Query_Cache::set() then parsed the buffer from the start and wrote out of bounds too.

The overflow silently corrupts neighbouring heap chunks. It then surfaces as crashes in unrelated code, which matches the varied backtraces reported in #6229. The code dates from 2018, which also fits the reporter seeing the same crashes on 2.7.1.

This is reachable from two places:

  • CALL: through the rc==2 branch, when the second result set is stored.
  • Multi-statement query: through the normal rc==0 branch, when the last result is stored.

Fix

  • Multi-result responses are not cached. A cache entry holds one result, so storing any single result of such a response would also give wrong answers on a hit. The rc==2 branch marks the request with the new Query_Info::multi_result. The store skips marked requests and any result with SERVER_MORE_RESULTS_EXIST. The marker is needed because the last result of a multi-statement query does not carry that flag.
  • Only the current result is copied. The store now copies from result_start, as the binary prepared-statement cache path already does.
  • resultset2buffer() asserts that every packet fits in the buffer, so a future mismatch fails in debug builds.

Caching multi-result responses properly is tracked in #6309. Until then, a cache_ttl rule matching a CALL or a multi-statement query always goes to the backend. Such responses were never cached correctly before this change either.

Tests

reg_test_6229_query_cache_multi_result-t (in legacy-g1, mysql84-g1, mysql90-g1, mysql95-g1) runs the following through a cache rule, with and without CLIENT_DEPRECATE_EOF:

  • a CALL to a procedure returning two result sets;
  • a two-statement query.

It checks that:

  • every execution returns both results with the right values;
  • the response is not stored in the cache;
  • a single-result query on the same connection is still cached;
  • ProxySQL is still serving clients at the end.

Validation, on an ASAN debug build (PROXYSQL31=1) against mysql84:

  • Without the fix: ASAN aborts ProxySQL on the first execution, for both the CALL form and the multi-statement form: 
    ERROR: AddressSanitizer: heap-buffer-overflow ... WRITE of size 89
  #1 MySQL_Data_Stream::resultset2buffer(bool)       mysql_data_stream.cpp:1730
  #2 MySQL_Session::MySQL_Result_to_MySQL_wire(...)  MySQL_Session.cpp:9405
  #3 MySQL_Session::handler()                        MySQL_Session.cpp:6719 (CALL) / :6625 (multi-statement)
0x... is located 0 bytes after 60-byte region
  • With the fix: all 21 checks pass, with no ASAN report.
  • Related tests, all passing with no ASAN report:
    • mysql84-g1: mysql-com_field_list-t, mysql-prepared_query_cache-t, mysql-reg_test_4723_query_cache_stores_empty_result-t;
    • mysql84-g4: all 8 deprecate_eof_support tests, test_ffto_mysql_multi_result-t, test_query_cache_soft_ttl_pct-t;
    • mysql84-g6: reg_test_3606-mysql_warnings-t.
  • Not run locally: the full TAP groups, and the Stable-tier build.

Summary by cubic

Fixes the heap-buffer-overflow that crashes ProxySQL when a query cache rule matches a request returning multiple result sets, like a CALL or a multi-statement query. Multi-result responses are now never cached, since a cache entry stores a single result and would otherwise answer hits incorrectly.

  • Multi-result requests are marked so their last result, which lacks SERVER_MORE_RESULTS_EXIST, is also excluded from caching.
  • The store copies only the current result's packets instead of everything still queued from earlier results.
  • resultset2buffer() asserts each packet fits in the remaining buffer space, so unsigned wraparound cannot defeat the check, and returns NULL instead of overflowing in release builds; callers skip the store on NULL.
  • Adds reg_test_6229_query_cache_multi_result-t covering CALL and multi-statement queries with and without CLIENT_DEPRECATE_EOF, including query-rule and backend cleanup.

Summary by CodeRabbit

  • Bug Fixes
    • Improved query-cache handling for stored procedures and multi-statement requests that return multiple result sets, preventing incorrect cached responses while preserving caching for eligible single-result queries.
    • Improved reliability when processing large query results and network traffic, including safer handling of oversized packets, incomplete decompression, and interrupted writes.