Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix columnObject crash with array of variadic dimension elems #40483

Merged
merged 4 commits into from
Aug 31, 2022

Conversation

canhld94
Copy link
Contributor

@canhld94 canhld94 commented Aug 22, 2022

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

  • Fix crash while parsing values of type Object that contains arrays of variadic dimension.

Close #40333

Following queries will crash the server without need to SET allow_experimental_object_type=1

SELECT * FROM url('https://datahub.io/core/geo-countries/r/0.geojson', JSONAsObject); -- select from url
-- test.json: {"array" : [ {"val" : 1} , {"val" : [ 1 ] } ] }
SELECT * FROM file('test.json', JSONAsObject); -- select from file
SELECT CAST('{"x" : [ 1 , [ 1 , 2] ]}', 'Object(\'json\')'); -- cast to object type

The crash reason is column object assume all elements in array have same dimension and lead to segfault in serialization. The solution can be either (1) disallow variadic dimension in array, or (2) fold elements in array to same dimensions. This MR implements approach (2).
I personally think approach (2) is consistent to #23516 as we can consider high dimension arrays as super-type of lower dimension arrays of the same type.

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

@robot-ch-test-poll robot-ch-test-poll added the pr-bugfix Pull request with bugfix, not backported by default label Aug 22, 2022
@Avogar Avogar added the can be tested Allows running workflows for external contributors label Aug 22, 2022
@canhld94
Copy link
Contributor Author

Test fail reason:

SELECT CAST('{"x" : [ {} , [ 1 , 2] ]}', 'Object(\'json\')');
Expect: {"x" : [ [] , [ 1 , 2] ]}'
Return: {"x" : [ [0] , [ 1 , 2] ]}'

Finding the cause.

@canhld94
Copy link
Contributor Author

Integration case fail not relates. ASTFuzzer fail because it generate a query with ORDER BY <JSON>, no idea how to fix..

@CurtizJ CurtizJ self-assigned this Aug 29, 2022
@canhld94
Copy link
Contributor Author

Wait for #40754

@CurtizJ
Copy link
Member

CurtizJ commented Aug 31, 2022

Fuzzer failures will be fixed by #40754 and are not related to changes in this PR, so let's merge now.

@CurtizJ CurtizJ merged commit 5a3e24c into ClickHouse:master Aug 31, 2022
@CurtizJ CurtizJ added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Aug 31, 2022
robot-clickhouse pushed a commit that referenced this pull request Aug 31, 2022
robot-clickhouse pushed a commit that referenced this pull request Aug 31, 2022
robot-clickhouse pushed a commit that referenced this pull request Aug 31, 2022
CurtizJ added a commit that referenced this pull request Sep 1, 2022
Backport #40483 to 22.8: Fix columnObject crash with array of variadic dimension elems
CurtizJ added a commit that referenced this pull request Sep 1, 2022
Backport #40483 to 22.7: Fix columnObject crash with array of variadic dimension elems
CurtizJ added a commit that referenced this pull request Sep 1, 2022
Backport #40483 to 22.6: Fix columnObject crash with array of variadic dimension elems
@robot-ch-test-poll robot-ch-test-poll added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-bugfix Pull request with bugfix, not backported by default pr-must-backport Pull request should be backported intentionally. Use this label with great care!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClickHouse terminates on JSONAsObject
4 participants