Experimental KQL: Clickhouse server v24.1.1.1 SEGV inside DB::ParserKQLQuery::parseImpl
#59036
Comments
alexey-milovidov
added
the
experimental feature
|
The deadline to fix is one month (Mar 1st, 2024) - if it will not be fixed, we will remove the experimental KQL altogether. |
alexey-milovidov
changed the title
SEGV inside DB::ParserKQLQuery::parseImplSEGV inside DB::ParserKQLQuery::parseImpl
|
@kashwy is working on it |
|
@hnlcf I am working on this issue, I cannot get segment fault, only got assertion failure BTW, if you don't mind , can you add me to your wechat ? that will be easier to communicate |
This commit fix the issues: ClickHouse#59036 ClickHouse#59037 both issues are same reason, the input query exceed the max_query_size, so the condition isEnd() of token is not meet and cause the assertion failure
|
can you do the WINGFUZZ again ? thanks |
Depending on which asssertion it is, it might be possible that the assertion is only present in debug builds but not in release builds. |
|
I cannot reproduce any assertion/crash with the specified queries. @kashwy how did you manage to get assertion failure? |
|
start clickhouse_local or clickhouse_client without "--multiquery" SET max_query_size=29; |
|
Okay, this is something I can confirm failed before your fix and it is working after your fix. I guess the provided sql is not correct. I found the following issues:
|
|
This is generated by a fuzzer, so don't worry about incorrect queries - we only need to ensure that it does not crash. |
|
no crash can be reproduced again, so can we close these issues ? |
|
Yes, almost done - but let's make a few more efforts - maybe we can reproduce it. |
|
Hi, do you have the result of test? thanks |
|
@yakov-olkhovskiy, this code looks suspicious for me: |
this code is to update the subquery inside the AST |
|
@alexey-milovidov except it's not pretty I think it's ok - the whole idea of KQL->SQL translation on AST level is not pretty by itself :) |
|
It is suspicious because:
|
|
frankly I don't see a point to check anything at all - it uses hardcoded |
updated with .at(0) the AST of select passed in , it check if a node exist at each level of the AST, is that the NULL check you mean ? |
because each pipe (operator) is parsed independently, so we need to update it's input from the previous operator |
… `max_query_size` (#59626) * Fix_kql_issue_found_by_wingfuzz This commit fix the issues: #59036 #59037 both issues are same reason, the input query exceed the max_query_size, so the condition isEnd() of token is not meet and cause the assertion failure * fix_kql_issue_found_by_wingfuzz: use isValid instead of TokenType::EndOfStream * fix_kql_issue_found_by_wingfuzz: make functional test result consist * fix_kql_issue_found_by_wingfuzz: update test case for makeseries * fix_kql_issue_found_by_wingfuzz: disable makeseries * fix_kql_issue_found_by_wingfuzz: use isvalid() function to replace isEnd() function of TokenIterator to check the end of stream * fix_kql_issue_found_by_wingfuzz: add test case for max_query_size * fix_kql_issue_found_by_wingfuzz: fix AST structure * fix_kql_issue_found_by_wingfuzz: make sure the max query size test is in the dialect of kusto * fix_kql_issue_found_by_wingfuzz : restore max query size after test * fix_kql_issue_found_by_wingfuzz : fix typo --------- Co-authored-by: János Benjamin Antal <benjamin.antal@clickhouse.com>
Describe the bug
ClickHouse server version 24.1.1.1 received SIGSEGV signal
It was found by an in-development fuzzer of WINGFUZZ.
How to reproduce
The SQL statement to reproduce (it is need to execute line by line): poc-1.sql
Stacktrace
Server log
The text was updated successfully, but these errors were encountered: