MDEV-40739 Server crashes in spider_db_open_item_field

[arcivanov]

Fixes MDEV-40739, reported by
Elena Stepanova as Critical.

SELECT 1 WHERE (1,1) IN (SELECT a, b FROM t1_s);

on a Spider table crashes the server with SIGSEGV in
spider_db_open_item_field().

Cause

spider_db_open_item_field() looks a field's table up among the Spider tables
of the query whenever the field does not belong to an internal temporary table:

if (field->table->s->tmp_table != INTERNAL_TMP_TABLE)

That is a hand-rolled copy of the server's own
TABLE_SHARE::is_optimizer_tmp_table(). Temporary tables created by
Create_tmp_table are marked RESULT_TMP_TABLE rather than
INTERNAL_TMP_TABLE, so a field of such a table passes the test,
spider_fields::find_table() finds no holder for it, and the returned NULL
is dereferenced.

Only the second pass crashes. The first pass, which decides whether the group
by handler can be created at all, does guard against a NULL holder. The two
passes do not resolve to the same items, though: an Item_direct_ref is
followed through real_item(), and between optimization and execution it is
re-pointed at a field of the optimizer's result temporary table.

Confirmed under a debug build. In the crash frame the field's table has
tmp_table RESULT_TMP_TABLE, table_name "(temporary)" and an empty db,
which is what Create_tmp_table::start() passes to init_tmp_table_share(),
while the sole entry in spider_fields is test.t1_s with NO_TMP_TABLE.

Fix

Ask the server's accessor instead of restating it, so that the predicate keeps
following the server's definition of an optimizer temporary table.

Testing

New test spider/bugfix.gbh_row_in_subquery, written before the fix. It
crashes the server without the fix, with a stack identical to the one reported,
and passes with it under both the group_by_handler and usual_handler
combinations, which share one result file.

The generated remote query was inspected separately to confirm the group by
handler still pushes the whole condition down rather than silently declining
to build the query:

select `a`,`b` from `test`.`t1` where ((1 = `a`) and (1 = `b`))

spider, spider/bg, spider/bugfix, spider/feature,
spider/regression/e1121 and spider/regression/e112122 are green: 507/507
pass. Two tests skip because they require the wsrep plugin, which this build
does not include.

[mariadb-YuchenPei]

claude:

---

Review: MDEV-40739 — spider_db_open_item_field crash fix

The change (spd_db_conn.cc:7263): adds && !field->table->s->is_optimizer_tmp_table() to the guard, so fields of an optimizer-created RESULT_TMP_TABLE skip the spider-table lookup and print as a plain identifier instead of NULL-dereferencing the missing holder. Plus a new MTR test, gbh_row_in_subquery. The core fix is correct and tested for the row-IN-subquery case in the bug report.

Three residual findings, all PLAUSIBLE (no crashing input was constructed to confirm any of them):

#	Location	Concern
1	spd_db_conn.cc:7300 (fall-through)	Relocated NULL-deref. For an optimizer-tmp-table field that also carries cached_field_index/cached_table, execution now flows into spider_db_open_item_ident, whose use_fields && str branch does spider = table->spider with no NULL check (unlike the two other find_table sites which guard it). The tested case has no cached_table, so it prints cleanly — but a subquery field resolving with cached_table set could hit the same crash one function over.
2	spd_db_conn.cc:7263	Behavior change from safe-skip to real pushdown. Before, the first pass hit find_table→NULL and returned ER_SPIDER_COND_SKIP_NUM, safely falling back to local execution. Now GBH creation proceeds and emits a bare alias.field_name. For subquery shapes beyond the trivial 2-INT-column test (aliased/expression select-list items, multiple spider tables), that identifier may not resolve correctly on the remote — wrong results instead of a safe fallback. No test covers these.
3	spd_db_conn.cc:7264	Blacklist, not whitelist. The guard now reads "not INTERNAL and not optimizer-tmp", still admitting SYSTEM_TMP_TABLE and any future tmp_table_type into the same unchecked find_table path. A positive whitelist of genuine spider base / user-TEMPORARY tables would fix the class rather than one enum value.

Finding 1 is the one worth the author's attention — it's the same crash signature the patch fixes, potentially just moved. Findings 2 and 3 are judgment calls about coverage and defensive coding altitude, not demonstrated defects.

Nothing here blocks the fix; they're the questions I'd raise on the PR before considering the crash class fully closed.