Skip to content

Fix null pointer dereference in low cardinality data#33021

Merged
evillique merged 1 commit intoClickHouse:masterfrom
ClibMouse:Issue135
Dec 22, 2021
Merged

Fix null pointer dereference in low cardinality data#33021
evillique merged 1 commit intoClickHouse:masterfrom
ClibMouse:Issue135

Conversation

@HarryLeeIBM
Copy link
Copy Markdown
Contributor

@HarryLeeIBM HarryLeeIBM commented Dec 21, 2021

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category:

  • Bug Fix

Changelog entry:

Fix null pointer dereference in low cardinality data when deserializing LowCardinality data in the Native format.

Detailed description / Documentation draft:

When deserializing LowCardinality data in the Native format it is possible for an attacker to cause ClickHouse to crash with a null pointer dereference.

In original code, if the flag index_type.has_additional_keys is false(which can be set to false in binary file or when sending data through native protocol by an attacker) and the code is expecting additional keys to be used in normal cases, the null pointer dereferencing happens since the false flag will cause addtional_keys to be nullptr by following code:

if (low_cardinality_state->index_type.has_additional_keys)
            read_additional_keys();
else
            low_cardinality_state->additional_keys = nullptr;

The fix is to add code to check if it is nullptr before the usage and throw exceptions if it is the case.

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Dec 21, 2021
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Dec 21, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@evillique evillique self-assigned this Dec 21, 2021
@evillique evillique added the can be tested Allows running workflows for external contributors label Dec 21, 2021
@ClickHouse ClickHouse deleted a comment from mergify bot Dec 21, 2021
@alesapin alesapin added can be tested Allows running workflows for external contributors and removed can be tested Allows running workflows for external contributors labels Dec 22, 2021
@alesapin
Copy link
Copy Markdown
Member

alesapin commented Dec 22, 2021

Please, merge this pr with master.

@evillique evillique merged commit e1a58d0 into ClickHouse:master Dec 22, 2021
@tavplubix
Copy link
Copy Markdown
Member

tavplubix commented Dec 22, 2021

@evillique, seems like it broke Style Check: https://s3.amazonaws.com/clickhouse-test-reports/0/85d49bf557eed696d2ea65f7ad1ac27b7b4df1c6/style_check__actions_.html

@evillique evillique mentioned this pull request Dec 22, 2021
Merged
evillique added a commit that referenced this pull request Dec 22, 2021
@evillique
Merge pull request #33049 from evillique/fix_pr
3d34f74 
Fix merge #33021
@alexey-milovidov alexey-milovidov added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Jan 11, 2022
This was referenced Jan 12, 2022
Merged
Merged
Merged
Merged
Merged
robot-clickhouse pushed a commit that referenced this pull request Jan 12, 2022
Backport #33021 to 21.11: Fix null pointer dereference in low cardina…
ff8f280 
…lity data
@robot-clickhouse robot-clickhouse mentioned this pull request Jan 12, 2022
Closed
robot-clickhouse pushed a commit that referenced this pull request Jan 12, 2022
Backport #33021 to 21.12: Fix null pointer dereference in low cardina…
053cfa0 
…lity data
@robot-clickhouse robot-clickhouse mentioned this pull request Jan 12, 2022
Merged
robot-clickhouse pushed a commit that referenced this pull request Jan 18, 2022
Backport #33021 to 21.8: Fix null pointer dereference in low cardinal…
143f9e0 
…ity data
@robot-clickhouse robot-clickhouse mentioned this pull request Jan 18, 2022
Merged
robot-clickhouse pushed a commit that referenced this pull request Jan 18, 2022
Backport #33021 to 21.3: Fix null pointer dereference in low cardinal…
c6e87c8 
…ity data
@robot-clickhouse robot-clickhouse mentioned this pull request Jan 18, 2022
Merged
robot-clickhouse pushed a commit that referenced this pull request Jan 18, 2022
Backport #33021 to 21.10: Fix null pointer dereference in low cardina…
7370a63 
…lity data
@robot-clickhouse robot-clickhouse mentioned this pull request Jan 18, 2022
Closed
evillique added a commit that referenced this pull request Jan 19, 2022
@evillique
Merge pull request #33727 from ClickHouse/backport/21.3/33021
6ac500b 
Backport #33021 to 21.3: Fix null pointer dereference in low cardinality data
evillique added a commit that referenced this pull request Jan 19, 2022
@evillique
Merge pull request #33551 from ClickHouse/backport/21.12/33021
1431cb1 
Backport #33021 to 21.12: Fix null pointer dereference in low cardinality data
@nikhars
Copy link
Copy Markdown

nikhars commented Feb 25, 2022 •
edited
Loading

We have a stack trace which looks like the one below. Does this fix address the following stack trace? I see the following in the stack trace which leads me to believe that this PR might fix it

DB::SerializationLowCardinality::serializeBinaryBulkWithMultipleStreams

2022.02.24 22:14:28.207074 [ 16974 ] {} <Fatal> Application: Child process was terminated by signal 11.2022.02.24 22:14:28.207074 [ 16974 ] {} <Fatal> Application: Child process was terminated by signal 11."2022.02.24 22:14:08.262929 [ 15483 ] {} <Fatal> BaseDaemon: Checksum of the binary: 8144291EC9956EC5A663DEFDB69A4258"2022.02.24 22:14:08.262929 [ 15483 ] {} <Fatal> BaseDaemon: Checksum of the binary: 8144291EC9956EC5A663DEFDB69A42582022.02.24 22:14:08.166713 [ 15483 ] {} <Fatal> BaseDaemon: 16. __clone @ 0xf94cf in /usr/lib/x86_64-linux-gnu/libc-2.28.so2022.02.24 22:14:08.166713 [ 15483 ] {} <Fatal> BaseDaemon: 16. __clone @ 0xf94cf in /usr/lib/x86_64-linux-gnu/libc-2.28.so2022.02.24 22:14:08.166701 [ 15483 ] {} <Fatal> BaseDaemon: 15. start_thread @ 0x7fa3 in /usr/lib/x86_64-linux-gnu/libpthread-2.28.so2022.02.24 22:14:08.166688 [ 15483 ] {} <Fatal> BaseDaemon: 14. ? @ 0x902c203 in /usr/bin/clickhouse"2022.02.24 22:14:08.166681 [ 15483 ] {} <Fatal> BaseDaemon: 13. ThreadPoolImpl[std::__1::thread]()::worker(std::__1::__list_iterator<std::__1::thread"2022.02.24 22:14:08.166669 [ 15483 ] {} <Fatal> BaseDaemon: 12. ThreadFromGlobalPool::ThreadFromGlobalPool<void ThreadPoolImpl<ThreadFromGlobalPool>::scheduleImpl<void>(std::__1::function<void ()>"2022.02.24 22:14:08.166652 [ 15483 ] {} <Fatal> BaseDaemon: 11. ThreadPoolImpl<ThreadFromGlobalPool>::worker(std::__1::__list_iterator<ThreadFromGlobalPool2022.02.24 22:14:08.166642 [ 15483 ] {} <Fatal> BaseDaemon: 10. ? @ 0x10d08e18 in /usr/bin/clickhouse2022.02.24 22:14:08.166634 [ 15483 ] {} <Fatal> BaseDaemon: 9. DB::RemoteBlockOutputStream::write(DB::Block const&) @ 0x10cfc2c6 in /usr/bin/clickhouse"2022.02.24 22:14:08.166624 [ 15483 ] {} <Fatal> BaseDaemon: 8. DB::Connection::sendData(DB::Block const&2022.02.24 22:14:08.166613 [ 15483 ] {} <Fatal> BaseDaemon: 7. DB::NativeBlockOutputStream::write(DB::Block const&) @ 0x1065e955 in /usr/bin/clickhouse"2022.02.24 22:14:08.166600 [ 15483 ] {} <Fatal> BaseDaemon: 6. DB::SerializationLowCardinality::serializeBinaryBulkWithMultipleStreams([DB::IColumn]() const&"2022.02.24 22:14:08.166589 [ 15483 ] {} <Fatal> BaseDaemon: 5. DB::ColumnLowCardinality::cutAndCompact(unsigned long2022.02.24 22:14:08.166577 [ 15483 ] {} <Fatal> BaseDaemon: 4. DB::ColumnLowCardinality::compactInplace() @ 0x108f7389 in /usr/bin/clickhouse2022.02.24 22:14:08.166569 [ 15483 ] {} <Fatal> BaseDaemon: 3. DB::ColumnLowCardinality::Dictionary::compact(COW[DB::IColumn]()::immutable_ptr[DB::IColumn]()&) @ 0x108f7499 in /usr/bin/clickhouse"2022.02.24 22:14:08.166559 [ 15483 ] {} <Fatal> BaseDaemon: 2. DB::ColumnNullable::index([DB::IColumn]() const&"2022.02.24 22:14:08.166530 [ 15483 ] {} <Fatal> BaseDaemon: 1. COWDB::IColumn::immutable_ptrDB::IColumn DB::selectIndexImpl<DB::ColumnVector<char8_t> >(DB::ColumnVector<char8_t> const&2022.02.24 22:14:08.166701 [ 15483 ] {} <Fatal> BaseDaemon: 15. start_thread @ 0x7fa3 in /usr/lib/x86_64-linux-gnu/libpthread-2.28.so2022.02.24 22:14:08.166688 [ 15483 ] {} <Fatal> BaseDaemon: 14. ? @ 0x902c203 in /usr/bin/clickhouse"2022.02.24 22:14:08.166681 [ 15483 ] {} <Fatal> BaseDaemon: 13. ThreadPoolImpl[std::__1::thread]()::worker(std::__1::__list_iterator<std::__1::thread"2022.02.24 22:14:08.166669 [ 15483 ] {} <Fatal> BaseDaemon: 12. ThreadFromGlobalPool::ThreadFromGlobalPool<void ThreadPoolImpl<ThreadFromGlobalPool>::scheduleImpl<void>(std::__1::function<void ()>"2022.02.24 22:14:08.166652 [ 15483 ] {} <Fatal> BaseDaemon: 11. ThreadPoolImpl<ThreadFromGlobalPool>::worker(std::__1::__list_iterator<ThreadFromGlobalPool2022.02.24 22:14:08.166642 [ 15483 ] {} <Fatal> BaseDaemon: 10. ? @ 0x10d08e18 in /usr/bin/clickhouse2022.02.24 22:14:08.166634 [ 15483 ] {} <Fatal> BaseDaemon: 9. DB::RemoteBlockOutputStream::write(DB::Block const&) @ 0x10cfc2c6 in /usr/bin/clickhouse"2022.02.24 22:14:08.166624 [ 15483 ] {} <Fatal> BaseDaemon: 8. DB::Connection::sendData(DB::Block const&2022.02.24 22:14:08.166613 [ 15483 ] {} <Fatal> BaseDaemon: 7. DB::NativeBlockOutputStream::write(DB::Block const&) @ 0x1065e955 in /usr/bin/clickhouse"2022.02.24 22:14:08.166600 [ 15483 ] {} <Fatal> BaseDaemon: 6. DB::SerializationLowCardinality::serializeBinaryBulkWithMultipleStreams([DB::IColumn]() const&"2022.02.24 22:14:08.166589 [ 15483 ] {} <Fatal> BaseDaemon: 5. DB::ColumnLowCardinality::cutAndCompact(unsigned long2022.02.24 22:14:08.166577 [ 15483 ] {} <Fatal> BaseDaemon: 4. DB::ColumnLowCardinality::compactInplace() @ 0x108f7389 in /usr/bin/clickhouse2022.02.24 22:14:08.166569 [ 15483 ] {} <Fatal> BaseDaemon: 3. DB::ColumnLowCardinality::Dictionary::compact(COW[DB::IColumn]()::immutable_ptr[DB::IColumn]()&) @ 0x108f7499 in /usr/bin/clickhouse"2022.02.24 22:14:08.166559 [ 15483 ] {} <Fatal> BaseDaemon: 2. DB::ColumnNullable::index([DB::IColumn]() const&"2022.02.24 22:14:08.166530 [ 15483 ] {} <Fatal> BaseDaemon: 1. COWDB::IColumn::immutable_ptrDB::IColumn DB::selectIndexImpl<DB::ColumnVector<char8_t> >(DB::ColumnVector<char8_t> const&2022.02.24 22:14:08.166479 [ 15483 ] {} <Fatal> BaseDaemon: Stack trace: 0x1092feb4 0x109066db 0x108f7499 0x108f7389 0x108f7274 0xff4d272 0x1065e955 0x10ff1406 0x10cfc2c6 0x10d08e18 0x902b638 0x902d1df 0x902891f 0x902c203 0x7f3cd2da0fa3 0x7f3cd2cc24cf2022.02.24 22:14:08.166469 [ 15483 ] {} <Fatal> BaseDaemon: Address: 0x7f3caaf7d002 Access: read. Attempted access has violated the permissions assigned to the memory area."2022.02.24 22:14:08.166451 [ 15483 ] {} <Fatal> BaseDaemon: (version 21.8.13.1.altinitystable (altinity build)2022.02.24 22:14:08.166421 [ 15483 ] {} <Fatal> BaseDaemon: ########################################2022.02.24 22:14:08.166479 [ 15483 ] {} <Fatal> BaseDaemon: Stack trace: 0x1092feb4 0x109066db 0x108f7499 0x108f7389 0x108f7274 0xff4d272 0x1065e955 0x10ff1406 0x10cfc2c6 0x10d08e18 0x902b638 0x902d1df 0x902891f 0x902c203 0x7f3cd2da0fa3 0x7f3cd2cc24cf2022.02.24 22:14:08.166469 [ 15483 ] {} <Fatal> BaseDaemon: Address: 0x7f3caaf7d002 Access: read. Attempted access has violated the permissions assigned to the memory area."2022.02.24 22:14:08.166451 [ 15483 ] {} <Fatal> BaseDaemon: (version 21.8.13.1.altinitystable (altinity build)2022.02.24 22:14:08.166421 [ 15483 ] {} <Fatal> BaseDaemon: ########################################

evillique added a commit that referenced this pull request Mar 3, 2022
@evillique
Merge pull request #33726 from ClickHouse/backport/21.8/33021
f9468b3 
Backport #33021 to 21.8: Fix null pointer dereference in low cardinality data
@Felixoid Felixoid added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evillique evillique evillique approved these changes

Assignees

@evillique evillique

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@HarryLeeIBM @CLAassistant @alesapin @tavplubix @nikhars @evillique @Felixoid @alexey-milovidov @robot-clickhouse