Implementation of gwp-asan technique

[l1tsolaiki]

Changelog category (leave one):

• New Feature

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

Implementation of GWP-Asan (https://www.chromium.org/Home/chromium-security/articles/gwp-asan/) in ClickHouse.

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

Checklist:

• guarded allocator (implement guarded allocator + use it from new/delete)
• record allocation metadata for diagnostics on crash (record stack traces on allocation and deallallocation)
• provide report on failure (implement a function to check if error is related to memory allocated with GWP)
• make GWP configurable (i.e pass parameters to init)
• add build option to not include GWP-Asan in build altoughether – we discussed @alexey-milovidov that we do not need that option, it will always be enabled.
• tests – we will just run regular tests with GWP enabled
• remove debug stuff
• code review changes

[CLAassistant]

All committers have signed the CLA.

[l1tsolaiki]

@azat You agreed to be tagged in PR. This is a draft PR, but basic allocate/deallocate seem to work locally on my machine.

[qoega]

How to enable/use it?

[l1tsolaiki]

@qoega

How to enable/use it?

Right now it is enabled by default. It is initialized at.

It is automatically used for allocations with new/delete. For every allocation GWP-Asan's shouldAllocate method is called which is used to randomly sample allocatioins (although currently in this draft it always returns true). If it returns true, newImpl allocates memory with GWP-Asan's method allocate here.

So overall it is automatically used for every allocation made through new/delete, for which shouldSample returns true.
GWP-Asan also has a fixed number of slots which it allocates on program start. This number is configured with maxSimultaneousAllocations parameter.

At this moment threre are also a number of debug printf outputs, which give an insight into everything going on inside of GWP-Asan.

I should note that as of right now linux build seems to produce segmentation fault on start, because for some reason (I am in process of figuring this out) GWP is initialized too late: new/delete are called before its initialization. Although on my machine (Apple M1) everything seems to be working correctly.

There is also a simple commented out test in main.cpp which results in segmentation fault (as expected) upon reaching guard page.

[l1tsolaiki]

Ok, looks like I fixed it for linux: had to set __attribute__((init_priority(102))) and __attribute__((init_priority(103))) to __attribute__((init_priority(101))).

[azat]

I see that this is a draft, so just a few brief comments.

Also can you add a checklist into the PR description, to see the progress, something like:

• guard allocator
• server settings (like pool size and so on)
• provide report on failure
• tests
• remove debug stuff

[l1tsolaiki]

Updated PR. Addressed several comments above in it + added allocation metadata collecting (stack traces on alloc/dealloc).
Note: this code is only for debug/testing purposes – to make sure that stack traces are correctly collected. However, printing stack trace here causes SIGSEGV on server shutdown for an unknown to me reason. Of course this code will not be present in the final version, but I am little worried that I will have the same issue when printing stack trace in signal handler.

[l1tsolaiki]

This PR is now ready for review for the most part. There is still work to be done to make it configurable, which I would consider to be a minor task. Backbone of GWP is done.
Note: I recommend building it with clang-13, because clang-14 uses DWARF-5 format and AFAIK ClickHouse uses DWARF-4. This causes problems when trying to symbolize (and subsequently output) stack trace in report.

As for tests, I am not sure how exactly to test this component. I am open to advice on this part.

Note: this code contains intentional memory error in programs/Server.cpp to test report. If your local build does not produce SIGSEGV and subsequent report, I would recommend increasing maxSimultaneousAllocations parameter.

CC @azat @qoega

[azat]

@l1tsolaiki can you please rebase on top upstream/master? (there was some changes in some basic headers, and no CI fails to compile your code, because it uses merge head).

[l1tsolaiki]

@l1tsolaiki can you please rebase on top upstream/master? (there was some changes in some basic headers, and no CI fails to compile your code, because it uses merge head).

Yes, I did that, but apparently I did not push it =/

Just now pushed rebased version + added tuning GWP with options passed through env variable CLICKHOUSE_GWP_ASAN_OPTIONS . This marks completion of this PR as I see it.
You can launch ClickHouse like so: CLICKHOUSE_GWP_ASAN_OPTIONS =help=true ./programs/clickhouse-server to see all available options. Other options are passed in the same way (example: CLICKHOUSE_GWP_ASAN_OPTIONS =sample_rate=8,max_simultaneous_allocations=15000 ./programs/clickhouse-server)
There are also warnings printed if you input some garbage in these options. However, those will probably be removed, because we should probably not used printf this early in execution. In the final version there will probably only be a generic "Error while parsing GWP-ASan options\n".

Also we discussed with @alexey-milovidov that no specific tests are expected here. We'll just run all existing tests and make sure that none of them fail with GWP enabled.

UPD: updated env variable name

[azat]

You can launch ClickHouse like so: GWP_ASAN_OPTIONS=help=true ./programs/clickhouse-server to see all available options. Other options are passed in the same way (example: GWP_ASAN_OPTIONS=sample_rate=8,max_simultaneous_allocations=15000 ./programs/clickhouse-server)

Name of the environment variable conflicts with GWP ASan from LLVM, maybe it should be changed to something like CLICKHOUSE_GWP_ASAN_OPTIONS ?

[l1tsolaiki]

You can launch ClickHouse like so: GWP_ASAN_OPTIONS=help=true ./programs/clickhouse-server to see all available options. Other options are passed in the same way (example: GWP_ASAN_OPTIONS=sample_rate=8,max_simultaneous_allocations=15000 ./programs/clickhouse-server)

Name of the environment variable conflicts with GWP ASan from LLVM, maybe it should be changed to something like CLICKHOUSE_GWP_ASAN_OPTIONS ?

Agreed. Changed that in the most recent commit.

[azat]

In general LGTM, apart from my comments.

Also I still think that it worth adding some tests, i.e. few examples with different errors (double free, overflow, underflow, ...), later/separately they can be integrated to CI.

[azat]

Let's also add profile events for guarded allocations, this way we can measure the difference between queries with them and w/o (this will help ensuring that this pool does not affects performance tests for instance).

For this you need to add new events to

ClickHouse/src/Common/ProfileEvents.cpp

Line 261 in ef35fa6

M(QueryMemoryLimitExceeded, "Number of times when memory limit exceeded for query.") \

    M(GuardedPoolAllocations, "Number of times when allocation was satisfied from guarded pool.") \
    M(GuardedPoolDeallocations, "Number of times when deallocation was from guarded pool.") \

And them use them here like (somewhere in the beginning of this file):

#include <Common/ProfileEvents.h>

namespace ProfileEvents
{
    extern const Event GuardedPoolAllocations;
    extern const Event GuardedPoolDeallocations;
}

And in this function:

ProfileEvents::increment(ProfileEvents::GuardedPoolAllocations);

And don't forget to do the same for deallocations.

[l1tsolaiki]

Done this, now waiting for result.

[azat]

Also to measure the effect of this in performance tests you can tune guard allocator, by adding export CLICKHOUSE_GWP_ASAN_OPTIONS=sample_rate=100:slot_size:512 here

ClickHouse/docker/test/performance-comparison/compare.sh

Line 114 in ef35fa6

export MALLOC_CONF="confirm_conf:true"

After I can help with interpreting the results of performance tests.

NOTE: I've set slot_size to 512 to cover allocations up to 2MB, since they should be pretty hot (#12142 (comment))

[azat]

Also it will be great if you will add layout in the comment, that will include:

• user pointers
• guard pages for that user pointers
• metadata

[azat]

Performance Comparison (actions) [1/4] — Failed to parse the report.

2022-05-17 03:30:43      right/scripts/compare.sh: line 19:   536 Segmentation fault      (core dumped) clickhouse-client --port "$1" --query "select 1"

[l1tsolaiki]

Performance Comparison (actions) [1/4] — Failed to parse the report.

2022-05-17 03:30:43      right/scripts/compare.sh: line 19:   536 Segmentation fault      (core dumped) clickhouse-client --port "$1" --query "select 1"

Yes, I noticed that too. Will try to investigate tomorrow.

[l1tsolaiki]

Also to measure the effect of this in performance tests you can tune guard allocator, by adding export CLICKHOUSE_GWP_ASAN_OPTIONS=sample_rate=100:slot_size:512 here

ClickHouse/docker/test/performance-comparison/compare.sh

Line 114 in ef35fa6

export MALLOC_CONF="confirm_conf:true"

After I can help with interpreting the results of performance tests.

NOTE: I've set slot_size to 512 to cover allocations up to 2MB, since they should be pretty hot (#12142 (comment))

@azat I've added ProfileEvents, where do we look at results now?
P.S. I see errors in tests, but those are not connected to GWP as far as I can see.
P.S.S. Previous segmentation fault error was due to the fact that GWP was initialized before ProfileEvents, so ProfileEvents::increment() caused an error.

[azat]

Here are some numbers with different options (CLICKHOUSE_GWP_ASAN_OPTIONS)
I see ~2% overhead for default setup, it does not looks significant, but I did not expect that with default sampling settings it should give >1% overhead.
So maybe it should be turned OFF by default for now until performance will be investigated.

TL;DR;

quantile	server	env	avg (sec)	dev
90.000%	upstream		3.21	0.000036
90.000%	pr	enabled=0	3.07	0.000274
90.000%	pr	sample_rate=100,slot_size=512,max_simultaneous_allocations=512,enabled=1	3.36	0.003614
90.000%	pr	sample_rate=100000,slot_size=1,max_simultaneous_allocations=32,enabled=1	3.12	0.000469
95.000%	upstream		3.23	0.000149
95.000%	pr	enabled=0	3.09	0.000352
95.000%	pr	sample_rate=100,slot_size=512,max_simultaneous_allocations=512,enabled=1	3.4	0.002172
95.000%	pr	sample_rate=100000,slot_size=1,max_simultaneous_allocations=32,enabled=1	3.16	0.000397

• Memory pattern of used query - https://gist.github.com/azat/76bceb23b051a24fa6d3e06cb113f680#file-memory-pattern-sql
• Benchmark - https://gist.github.com/azat/76bceb23b051a24fa6d3e06cb113f680#file-clickhouse-gwp-asan-bench-sh

Also interesting moment that upstream is slower here, maybe my environment was not stable, need to verify.

[azat]

I did not looked into performance tests deeply (will do this later), but seems that they are fine even with more higher sample rate.
Let's remove this line for now them and see how they will differs.

[Enna1]

Hi, what's the status about this PR? Is there any plan or progress to enable this gwp-asan technique? :)

[alexey-milovidov]

This PR is unfinished, but it is on the list of our tasks for hardening and reliability - we want to continue and merge it.

[hanfei1991]

I will continue the work in a few days. Thanks for patience!

[l1tsolaiki]

As far as I remember, the only thing left to do here was to fix style

[alexey-milovidov]

Superseded by #45226.