Skip to content

keeper: fix memory leak in case of compression is used (default)#33840

Merged
alesapin merged 2 commits intoClickHouse:masterfrom
azat:keeper-zstd-fix-leak
Jan 21, 2022
Merged

keeper: fix memory leak in case of compression is used (default)#33840
alesapin merged 2 commits intoClickHouse:masterfrom
azat:keeper-zstd-fix-leak

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Jan 20, 2022

Changelog category (leave one):

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

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix memory leak in clickhouse-keeper in case of compression is used (default)

In case of compression is used, ZstdDeflatingAppendableWriteBuffer is
used, but it has a leak, since it frees ZSTD_CCtx only if there was
write while this is not correct, since it is created anyway.

This was found with jemalloc profile, keeper-bench and the following
keeper settings:

  • force_sync=false
  • snapshot_distance=100
  • reserved_log_items=0
  • rotate_log_storage_interval=100

And here is jemalloc profile with focus:

Details
$ jeprof clickhouse-keeper --focus 'ZstdDeflatingAppendableWriteBuffer' --text --base=$(ls -t jeprof.*.heap | tail -1) $(ls -t jeprof.*.heap | head -1)
Using local file clickhouse-keeper.
Using local file jeprof.29986.696.i696.heap.
addr2line: DWARF error: section .debug_info is larger than its filesize! (0x52f28a vs 0x3a9228)
Total: 7554.7 MB
  7395.3 100.0% 100.0%   7395.3 100.0% prof_backtrace
     0.0   0.0% 100.0%      7.0   0.1% ChangelogWriter
     0.0   0.0% 100.0%      7.0   0.1% DB::Changelog::appendEntry
     0.0   0.0% 100.0%      7.0   0.1% DB::Changelog::rotate
     0.0   0.0% 100.0%   7388.2  99.9% DB::ChangelogWriter::flush
     0.0   0.0% 100.0%   7395.3 100.0% DB::KeeperDispatcher::requestThread
     0.0   0.0% 100.0%      7.0   0.1% DB::KeeperLogStore::append
     0.0   0.0% 100.0%   7388.2  99.9% DB::KeeperLogStore::end_of_append_batch
     0.0   0.0% 100.0%   7395.3 100.0% DB::KeeperServer::putRequestBatch
     0.0   0.0% 100.0%   7388.2  99.9% DB::WriteBuffer::next (inline)
     0.0   0.0% 100.0%   7388.2  99.9% DB::ZstdDeflatingAppendableWriteBuffer::nextImpl
     0.0   0.0% 100.0%   7395.3 100.0% ThreadPoolImpl::worker
     0.0   0.0% 100.0%   7388.2  99.9% ZSTD_CCtx_init_compressStream2
     0.0   0.0% 100.0%   7388.2  99.9% ZSTD_compressBegin_internal
     0.0   0.0% 100.0%   7388.2  99.9% ZSTD_compressStream2
     0.0   0.0% 100.0%      7.0   0.1% ZSTD_createCCtx
     0.0   0.0% 100.0%      7.0   0.1% ZSTD_createCCtx_advanced
     0.0   0.0% 100.0%   7388.2  99.9% ZSTD_cwksp_create (inline)
     0.0   0.0% 100.0%   7388.2  99.9% ZSTD_resetCCtx_internal
     0.0   0.0% 100.0%      7.0   0.1% ZstdDeflatingAppendableWriteBuffer

Cc: @alesapin

In case of compression is used, ZstdDeflatingAppendableWriteBuffer is
used, but it has a leak, since it frees ZSTD_CCtx only if there was
write while this is not correct, since it is created anyway.

This was found with jemalloc profile, keeper-bench and the following
keeper settings:

- force_sync=false
- snapshot_distance=100
- reserved_log_items=0
- rotate_log_storage_interval=100

Signed-off-by: Azat Khuzhin <[email protected]>
@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Jan 20, 2022
@alesapin alesapin self-assigned this Jan 20, 2022
@alesapin
Copy link
Copy Markdown
Member

Unfortunately it's not this only "memory leak" we have :(

clang-tidy [1]:

    "Call to virtual method 'ZstdDeflatingAppendableWriteBuffer::finalizeAfter' during destruction bypasses virtual dispatch"

  [1]: https://s3.amazonaws.com/clickhouse-builds/33840/d021190b8c7ae0f5dc2decb953c471ea9d770981/binary_tidy/build_log.log

Signed-off-by: Azat Khuzhin <[email protected]>
@alesapin alesapin merged commit 25547de into ClickHouse:master Jan 21, 2022
@azat azat deleted the keeper-zstd-fix-leak branch January 24, 2022 10:02
azat added a commit to azat/ClickHouse that referenced this pull request Jan 26, 2022
getauxval() from glibc-compatibility did not work always correctly:

- it does not work after setenv(), and this breaks vsyscalls,
  like sched_getcpu() [1] (and BaseDaemon.cpp always set TZ if timezone
  is defined, which is true for CI [2]).

  [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1163404
  [2]: ClickHouse#32928 (comment)

- another think that is definitely broken is LSan (Leak Sanitizer), it
  relies on worked getauxval() but it does not work if __environ is not
  initialized yet (there is even a commit about this).

  And because of, at least, one leak had been introduced [3]:

    [3]: ClickHouse#33840

Fix this by using /proc/self/auxv.

And let's see how many issues will LSan find...

I've verified this patch manually by printing AT_BASE and compared it
with output of LD_SHOW_AUXV.

Signed-off-by: Azat Khuzhin <[email protected]>
azat referenced this pull request in azat/ClickHouse Jul 18, 2022
getauxval() from glibc-compatibility did not work always correctly:

- It does not work after setenv(), and this breaks vsyscalls,
  like sched_getcpu() [1] (and BaseDaemon.cpp always set TZ if timezone
  is defined, which is true for CI [2]).

  Also note, that fixing setenv() will not fix LSan,
  since the culprit is getauxval()

  [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1163404
  [2]: ClickHouse#32928 (comment)

- Another think that is definitely broken is LSan (Leak Sanitizer), it
  relies on worked getauxval() but it does not work if __environ is not
  initialized yet (there is even a commit about this).

  And because of, at least, one leak had been introduced [3]:

    [3]: ClickHouse#33840

Fix this by using /proc/self/auxv with fallback to environ solution to
make it compatible with environment that does not allow reading from
auxv (or no procfs).

v2: add fallback to environ solution
v3: fix return value for __auxv_init_procfs()
Refs: ClickHouse#33957
Signed-off-by: Azat Khuzhin <[email protected]>
azat referenced this pull request in azat/ClickHouse Jul 24, 2022
getauxval() from glibc-compatibility did not work always correctly:

- It does not work after setenv(), and this breaks vsyscalls,
  like sched_getcpu() [1] (and BaseDaemon.cpp always set TZ if timezone
  is defined, which is true for CI [2]).

  Also note, that fixing setenv() will not fix LSan,
  since the culprit is getauxval()

  [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1163404
  [2]: ClickHouse#32928 (comment)

- Another think that is definitely broken is LSan (Leak Sanitizer), it
  relies on worked getauxval() but it does not work if __environ is not
  initialized yet (there is even a commit about this).

  And because of, at least, one leak had been introduced [3]:

    [3]: ClickHouse#33840

Fix this by using /proc/self/auxv with fallback to environ solution to
make it compatible with environment that does not allow reading from
auxv (or no procfs).

v2: add fallback to environ solution
v3: fix return value for __auxv_init_procfs()
(cherry picked from commit f187c34)
v4: more verbose message on errors, CI founds [1]:
    AUXV already has value (529267711)
    [1]: https://s3.amazonaws.com/clickhouse-test-reports/39103/2325f7e8442d1672ce5fb43b11039b6a8937e298/stress_test__memory__actions_.html
v5: break at AT_NULL
v6: ignore AT_IGNORE
v7: suppress TSan and remove superior check to avoid abort() in case of race
v8: proper suppressions (not inner function but itself)
Refs: ClickHouse#33957
Signed-off-by: Azat Khuzhin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants