[MDEV-29447] SIGSEGV in spider_db_open_item_field and SIGSEGV in spider_db_print_item_type, on SELECT Created: 2022-09-02 Updated: 2023-09-22 Resolved: 2023-06-27 |
|
| Status: | Closed |
| Project: | MariaDB Server |
| Component/s: | Storage Engine - Spider |
| Affects Version/s: | 10.3, 10.4, 10.5, 10.6, 10.7, 10.8, 10.9, 10.10, 10.11, 11.0 |
| Fix Version/s: | 10.4.31, 10.5.22, 10.6.15, 10.9.8, 10.10.6, 10.11.5, 11.0.3, 11.1.2 |
| Type: | Bug | Priority: | Critical |
| Reporter: | Roel Van de Paar | Assignee: | Yuchen Pei |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | spider-gbh | ||
| Issue Links: |
|
||||||||||||||||||||||||
| Description |
|
Leads to:
Bug confirmed present in: Bug (or feature/syntax) confirmed not present in: |
| Comments |
| Comment by Roel Van de Paar [ 2022-10-01 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
Additional testcase which leads to a slightly different stack/UniqueID (10.5+ only, table creation fails in 10.4+, possible regression):
Leads to:
Bug confirmed present in: Bug (or feature/syntax) confirmed not present in: | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Roel Van de Paar [ 2022-11-29 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
Neither testcase produces any errors on an UB+ASAN build, nor do these crash on the given testcases. | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Roel Van de Paar [ 2022-11-29 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
MTR testcase, also updated to work with latest 10.11 (re user privileges)
This crashes 10.11.2 936436ef437c73911c18854a8ce8dad1216331b8 debug as of build from today. | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Nayuta Yanagisawa (Inactive) [ 2022-11-29 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
A MTR test case with cleanup:
The bug is also reproducible on 10.3. | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2022-12-23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
The function spider_db_print_item_type walks the AST of an item, and may rebuild the query string str somehow. It is called in multiple occasions, including when the spider group by handler is created (spider_create_group_by_handler), and indirectly when it builds and executes the query in spider_group_by_handler::init_scan, through spider_db_handler::append_list_item_select_part. The problem is that the function seems to serve very different purposes depending on whether str is null. In the call of spider_db_print_item_type during handler creation, str is null, and in the call during init_scan, str is the query string. When the item is of ITEM_FUNC type, spider_db_print_item_type delegates to spider_db_mbase_util::open_item_func(), which is a monster function[1]. It may further walk the item down to its arguments. [1] https://jira.mariadb.org/browse/MDEV-26285 When the item is of ITEM_FIELD type, it delegates to spider_db_open_item_field. What happens in this testcase is, the function is ltrim, with a string ('c') and a field ('tbl_a.a') arguments. When spider_db_print_item_type is called during handler creation, it is supposed to populate the fields, which happens when spider_db_open_item_field is called with a null str. But because str is null, spider_db_mbase_util::open_item_func() decides not to descend into the arguments of the func item and call spider_db_open_item_field, so the fields are empty. Then, when spider_db_print_item_type is called again during init_scan, str is not null, and spider_db_mbase_util::open_item_func() walks down to the field argument, calling spider_db_open_item_field with a non-null str, causing the latter to access the invalid fields. Why does spider_db_mbase_util::open_item_func() not continue walking the AST of the item if str is null? Shall we invert this behaviour? That requires further investigation... The string variable with the poor name str acting as a flag makes the code rather unreadable, thus some refactoring in this regard would be nice. | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2022-12-23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
The commit 3836098c29ef1b7ff9d5fbde99b690eab73a0df1 in
It nicely handles null and non-null str}}s in two separate functions. Where it is null, inside {{check_item_func, it descends into the arguments, which fixes the problem in this ticket. Indeed, the testcase passes at commit 3836098c29ef1b7ff9d5fbde99b690eab73a0df1 and fails at its parent commit. So the problem is reduced to adapting that commit (based on version 10.6.4) to the current 10.3 onwards. | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-01-03 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
serg Assigning you to review because you seemed to be the reviewer of 10.3: https://github.com/MariaDB/server/commit/dcd69cbd19a
| |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-01-24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
The review is taking place on the maria-developers mailing list: | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-01-24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
As I was going through the review comments, I noticed that the commit breaks spider.udf_pushdown... I thought I tested the commit against all spider tests but I don't remember as it was three weeks ago. It would be nice if buildbot could run all spider tests when any spider code is touched. So I need to update the commit both according to the comments and to fix the testcase. Update: That turned out to be a silly mistake: I forgot to check for Item_func::FUNC_SP in check_item_func(). | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-01-30 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
serg Thanks for the comments - I have replied in the thread in maria-developers[1]. The updated commit is https://github.com/MariaDB/server/commit/3ea2f6a5cd7. PTAL thanks. [1] https://lists.launchpad.net/maria-developers/msg13287.html | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Roel Van de Paar [ 2023-03-14 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
I found an additional testcase with a similar but slightly different stack on optimized builds.
MTR:
Leads to:
| |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-06-14 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
I have rebased the patch on the "current" 10.4-11.1 and included the test for
Hi serg, ptal thanks: https://github.com/MariaDB/server/commit/f3aa4d0a215599344d6660878f963c75b4598f6b | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-06-21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
Thanks for the comments serg, ptal: | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-06-22 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
New patch addressing the comment in https://lists.mariadb.org/hyperkitty/list/developers@lists.mariadb.org/message/BDN3FCX4KYO2BHTHZO24NBKXVSNRRTRY/ | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Sergei Golubchik [ 2023-06-22 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
c438f70d7b7 is ok to push | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-06-27 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
Thanks for the review. Pushed to 10.4. Marking as fixed. |