Skip to content

Add explicit interface for saving history with or without updating current session (backport)#10

Merged
alexey-milovidov merged 1 commit intoClickHouse:masterfrom
azat-ch:history-save-reload-flag
Oct 12, 2020
Merged

Add explicit interface for saving history with or without updating current session (backport)#10
alexey-milovidov merged 1 commit intoClickHouse:masterfrom
azat-ch:history-save-reload-flag

Conversation

@azat
Copy link
Member

@azat azat commented Aug 2, 2020 •
edited
Loading

Before this patch replxx, while saving to the history file always reload
the history from the disk and add new items from that file into the
current session. This looks incovenient, if user code calls
history_save() after each command (to save history ASAP, example of such
program is clickhouse-client) since if you run two programs with replxx
completion the completion will overlaps after each command.

Looks like at least this should be configurable.

Refs: ClickHouse/ClickHouse#13086
Upstream PR: AmokHuginnsson#83

@azat azat force-pushed the history-save-reload-flag branch from 62fd6e6 to ac3894a Compare August 2, 2020 21:38
@azat azat mentioned this pull request Aug 2, 2020
Merged
@alexey-milovidov
Copy link
Member

alexey-milovidov commented Aug 2, 2020 •
edited
Loading

But actually it is very convenient.
In other programs that are using readline two programs run in parallel will rewrite history of each other (data loss).

It was in the list of demanded features:
https://clickhouse.tech/docs/ru/whats-new/extended-roadmap/#uluchshenie-vozmozhnostei-interaktivnogo-rezhima-clickhouse-client

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Aug 2, 2020

If history update is implemented as append under flock, it's Ok.

@azat
Copy link
Member Author

azat commented Aug 2, 2020

In other programs that are using readline two programs run in parallel will rewrite history of each other (data loss).

You are right, it should do reload from file, but do not add new items from file to the completion list in the current client.
At least this is what I'm interested in, thoughts?

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Aug 2, 2020

You are right, it should do reload from file, but do not add new items from file to the completion list in the current client.

Why need to reload? Just append to history file...

@azat
Copy link
Member Author

azat commented Aug 2, 2020 •
edited
Loading

Why need to reload? Just append to history file...

But in this case you may have duplicates in file (and besides it requires tracking of which was written, or saving some offset)

@azat azat force-pushed the history-save-reload-flag branch from ac3894a to 43e7d6e Compare August 2, 2020 23:22
@azat azat changed the title Add reload flag for saving history Add ability to avoid updating current session history after dumping to file Aug 2, 2020
@azat
Copy link
Member Author

azat commented Aug 2, 2020

Make it always reads from disk, w/o updating current client history (this PR and upstream)

@azat
Copy link
Member Author

azat commented Aug 2, 2020 •
edited
Loading

Plus, adds .hist_autosave command for replxx-example-cxx-api that behaves as clickhouse-client, i.e. calls history_save after each command

@azat
Copy link
Member Author

azat commented Aug 13, 2020

@alexey-milovidov thoughts on this?

@azat
Copy link
Member Author

azat commented Oct 11, 2020

This has been implemented in upstream (in a little bit different way - abe46ea), so after it history_save() will only write to the file, w/o updating current session, but that patch requires backporting since this fork has some modifications, I will replace this patch with backporting upstream commit

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Oct 11, 2020

Ok.

@alexey-milovidov thoughts on this?

No thoughts, I forgot about this PR.

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Oct 11, 2020

I don't remember, why it's not good to have single persistent shared history?
So, when you work with many clickhouse-clients, they will see history of each other.
It's very convenient when you just switch to some random tab and want to get a query from the history.

@AmokHuginnsson @azat
Add explicit interface for saving history with or without updating
04bea6a 
current session.

Before this patch replxx, while saving to the history file always reload
the history from the disk and add new items from that file into the
current session. This could be incovenient, if user code calls
history_save() after each command (to save history ASAP, example of such
program is clickhouse-client) since if you run two programs with replxx
completion the completion will overlaps after each command.

Hence an introduction of additional interface:
Replxx::history_save() - just save merged history without changing
current session
Replxx::history_sync() - save merged history and update current session

Upstream commit is abe46ea ("Add
explicit interface for saving history with or without updating current
session.")

And this also partially backports
737c8e8 ("History load/save methods
return success status.")
@azat azat force-pushed the history-save-reload-flag branch from 43e7d6e to 04bea6a Compare October 11, 2020 19:40
@azat azat changed the title Add ability to avoid updating current session history after dumping to file Add explicit interface for saving history with or without updating current session (backport) Oct 11, 2020
@azat
Copy link
Member Author

azat commented Oct 11, 2020

I don't remember, why it's not good to have single persistent shared history?

To me different clients to do different things and it is kind of unexpected to see history that you don't see before (so for example bash will not do this, after conifguring it to dump history on each line as described here)

So, when you work with many clickhouse-clients, they will see history of each other.

I suggested to make it under option, but as you said it is too complex (and I agree, since then this option should be covered)

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Oct 11, 2020

Ok, reasonable.

@azat
Copy link
Member Author

azat commented Oct 12, 2020

@alexey-milovidov can you merge then?

@alexey-milovidov alexey-milovidov merged commit 8cf626c into ClickHouse:master Oct 12, 2020
azat added a commit to azat/ClickHouse that referenced this pull request Oct 12, 2020
@azat
Avoid creating separate replxx instance
f211d33 
This replxx object is pretty heavy and in debug build may slow down [1]
(although I cannot confirm 0.5s delay for each query in debug build) the
client and besides it is not required since ClickHouse/replxx#10,
which changes the behaviour of history_save(), and now it will not
update current session anymore, only save the history to the disk.

  [1]: ClickHouse#13086 (comment)
@azat azat mentioned this pull request Oct 12, 2020
Merged
@azat azat deleted the history-save-reload-flag branch August 23, 2021 18:10
azat added a commit to azat-ch/replxx that referenced this pull request Sep 4, 2022
@azat
Fix uncaught exception from ReplxxImpl dtor
920e009 
You may get uncaugh exception (in case of i.e. broken pipe):

    terminating with uncaught exception of type std::runtime_error: write failed

On destroy:

    (lldb) target create "clickhouse-22.8-release" --core "core.clickhouse-clie.402986-642410"
    bt
    Core file '/wrk/core.clickhouse-clie.402986-642410' (x86_64) was loaded.
    (lldb) bt
    * thread ClickHouse#1, name = 'clickhouse-clie', stop reason = signal SIGABRT
      * frame #0: 0x00007f03fb5c900b libc.so.6`raise + 203
        frame ClickHouse#1: 0x00007f03fb5a8859 libc.so.6`abort + 299
        frame ClickHouse#2: 0x000000001b703f44 clickhouse-22.8-release`::abort_message(format=<unavailable>) at abort_message.cpp:78:5
        frame ClickHouse#3: 0x000000001b703dd4 clickhouse-22.8-release`demangling_terminate_handler() at cxa_default_handlers.cpp:67:21
        frame ClickHouse#4: 0x000000001b721063 clickhouse-22.8-release`std::__terminate(func=<unavailable>)()) at cxa_handlers.cpp:59:9
        frame ClickHouse#5: 0x000000001b720fce clickhouse-22.8-release`std::terminate() at cxa_handlers.cpp:88:17
        frame ClickHouse#6: 0x000000000a3b21db clickhouse-22.8-release`__clang_call_terminate + 11
        frame ClickHouse#7: 0x00000000189b1bfc clickhouse-22.8-release`replxx::Replxx::ReplxxImpl::~ReplxxImpl(this=0x00007f03fa945308) at replxx_impl.cxx:336:1
        frame ClickHouse#8: 0x00000000189b1ce9 clickhouse-22.8-release`replxx::Replxx::ReplxxImpl::~ReplxxImpl(this=0x00007f03fa945300) at replxx_impl.cxx:334:41
        frame ClickHouse#9: 0x00000000188b0644 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader() [inlined] std::__1::unique_ptr<replxx::Replxx::ReplxxImpl, void (*)(replxx::Replxx::ReplxxImpl*)>::reset(this=<unavailable>, __p=<unavailable>) at unique_ptr.h:315:7
        frame ClickHouse#10: 0x00000000188b0626 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader() [inlined] std::__1::unique_ptr<replxx::Replxx::ReplxxImpl, void (*)(replxx::Replxx::ReplxxImpl*)>::~unique_ptr(this=<unavailable>) at unique_ptr.h:269
        frame ClickHouse#11: 0x00000000188b0626 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader() [inlined] replxx::Replxx::~Replxx(this=<unavailable>) at replxx.hxx:76
        frame ClickHouse#12: 0x00000000188b0626 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader(this=0x00007ffd10038440) at ReplxxLineReader.cpp:229
        frame ClickHouse#13: 0x00000000158b2100 clickhouse-22.8-release`DB::ClientBase::runInteractive(this=0x00007ffd10038620) at ClientBase.cpp:2067:1
        frame ClickHouse#14: 0x000000000a4de372 clickhouse-22.8-release`DB::Client::main(this=0x00007ffd10038620, (null)=<unavailable>) at Client.cpp:261:9
        frame ClickHouse#15: 0x0000000018923a86 clickhouse-22.8-release`Poco::Util::Application::run(this=0x00007ffd10038620) at Application.cpp:334:8
        frame ClickHouse#16: 0x000000000a4ed341 clickhouse-22.8-release`mainEntryClickHouseClient(argc=39, argv=0x00007f03fa8201c0) at Client.cpp:1220:23
        frame ClickHouse#17: 0x000000000a3b17ab clickhouse-22.8-release`main(argc_=<unavailable>, argv_=<unavailable>) at main.cpp:449:12
        frame ClickHouse#18: 0x00007f03fb5aa083 libc.so.6`__libc_start_main + 243
        frame ClickHouse#19: 0x000000000a17032e clickhouse-22.8-release`_start + 46
azat added a commit to azat-ch/replxx that referenced this pull request Sep 4, 2022
@azat
Fix uncaught exception from ReplxxImpl dtor
b93cf69 
You may get uncaugh exception (in case of i.e. broken pipe):

    terminating with uncaught exception of type std::runtime_error: write failed

On destroy:

    (lldb) target create "clickhouse-22.8-release" --core "core.clickhouse-clie.402986-642410"
    bt
    Core file '/wrk/core.clickhouse-clie.402986-642410' (x86_64) was loaded.
    (lldb) bt
    * thread ClickHouse#1, name = 'clickhouse-clie', stop reason = signal SIGABRT
      * frame #0: 0x00007f03fb5c900b libc.so.6`raise + 203
        frame ClickHouse#1: 0x00007f03fb5a8859 libc.so.6`abort + 299
        frame ClickHouse#2: 0x000000001b703f44 clickhouse-22.8-release`::abort_message(format=<unavailable>) at abort_message.cpp:78:5
        frame ClickHouse#3: 0x000000001b703dd4 clickhouse-22.8-release`demangling_terminate_handler() at cxa_default_handlers.cpp:67:21
        frame ClickHouse#4: 0x000000001b721063 clickhouse-22.8-release`std::__terminate(func=<unavailable>)()) at cxa_handlers.cpp:59:9
        frame ClickHouse#5: 0x000000001b720fce clickhouse-22.8-release`std::terminate() at cxa_handlers.cpp:88:17
        frame ClickHouse#6: 0x000000000a3b21db clickhouse-22.8-release`__clang_call_terminate + 11
        frame ClickHouse#7: 0x00000000189b1bfc clickhouse-22.8-release`replxx::Replxx::ReplxxImpl::~ReplxxImpl(this=0x00007f03fa945308) at replxx_impl.cxx:336:1
        frame ClickHouse#8: 0x00000000189b1ce9 clickhouse-22.8-release`replxx::Replxx::ReplxxImpl::~ReplxxImpl(this=0x00007f03fa945300) at replxx_impl.cxx:334:41
        frame ClickHouse#9: 0x00000000188b0644 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader() [inlined] std::__1::unique_ptr<replxx::Replxx::ReplxxImpl, void (*)(replxx::Replxx::ReplxxImpl*)>::reset(this=<unavailable>, __p=<unavailable>) at unique_ptr.h:315:7
        frame ClickHouse#10: 0x00000000188b0626 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader() [inlined] std::__1::unique_ptr<replxx::Replxx::ReplxxImpl, void (*)(replxx::Replxx::ReplxxImpl*)>::~unique_ptr(this=<unavailable>) at unique_ptr.h:269
        frame ClickHouse#11: 0x00000000188b0626 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader() [inlined] replxx::Replxx::~Replxx(this=<unavailable>) at replxx.hxx:76
        frame ClickHouse#12: 0x00000000188b0626 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader(this=0x00007ffd10038440) at ReplxxLineReader.cpp:229
        frame ClickHouse#13: 0x00000000158b2100 clickhouse-22.8-release`DB::ClientBase::runInteractive(this=0x00007ffd10038620) at ClientBase.cpp:2067:1
        frame ClickHouse#14: 0x000000000a4de372 clickhouse-22.8-release`DB::Client::main(this=0x00007ffd10038620, (null)=<unavailable>) at Client.cpp:261:9
        frame ClickHouse#15: 0x0000000018923a86 clickhouse-22.8-release`Poco::Util::Application::run(this=0x00007ffd10038620) at Application.cpp:334:8
        frame ClickHouse#16: 0x000000000a4ed341 clickhouse-22.8-release`mainEntryClickHouseClient(argc=39, argv=0x00007f03fa8201c0) at Client.cpp:1220:23
        frame ClickHouse#17: 0x000000000a3b17ab clickhouse-22.8-release`main(argc_=<unavailable>, argv_=<unavailable>) at main.cpp:449:12
        frame ClickHouse#18: 0x00007f03fb5aa083 libc.so.6`__libc_start_main + 243
        frame ClickHouse#19: 0x000000000a17032e clickhouse-22.8-release`_start + 46
nikitamikhaylov pushed a commit that referenced this pull request Aug 23, 2024
@azat @nikitamikhaylov
Fix uncaught exception from ReplxxImpl dtor
73dad09 
You may get uncaugh exception (in case of i.e. broken pipe):

    terminating with uncaught exception of type std::runtime_error: write failed

On destroy:

    (lldb) target create "clickhouse-22.8-release" --core "core.clickhouse-clie.402986-642410"
    bt
    Core file '/wrk/core.clickhouse-clie.402986-642410' (x86_64) was loaded.
    (lldb) bt
    * thread #1, name = 'clickhouse-clie', stop reason = signal SIGABRT
      * frame #0: 0x00007f03fb5c900b libc.so.6`raise + 203
        frame #1: 0x00007f03fb5a8859 libc.so.6`abort + 299
        frame #2: 0x000000001b703f44 clickhouse-22.8-release`::abort_message(format=<unavailable>) at abort_message.cpp:78:5
        frame #3: 0x000000001b703dd4 clickhouse-22.8-release`demangling_terminate_handler() at cxa_default_handlers.cpp:67:21
        frame #4: 0x000000001b721063 clickhouse-22.8-release`std::__terminate(func=<unavailable>)()) at cxa_handlers.cpp:59:9
        frame #5: 0x000000001b720fce clickhouse-22.8-release`std::terminate() at cxa_handlers.cpp:88:17
        frame #6: 0x000000000a3b21db clickhouse-22.8-release`__clang_call_terminate + 11
        frame #7: 0x00000000189b1bfc clickhouse-22.8-release`replxx::Replxx::ReplxxImpl::~ReplxxImpl(this=0x00007f03fa945308) at replxx_impl.cxx:336:1
        frame #8: 0x00000000189b1ce9 clickhouse-22.8-release`replxx::Replxx::ReplxxImpl::~ReplxxImpl(this=0x00007f03fa945300) at replxx_impl.cxx:334:41
        frame #9: 0x00000000188b0644 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader() [inlined] std::__1::unique_ptr<replxx::Replxx::ReplxxImpl, void (*)(replxx::Replxx::ReplxxImpl*)>::reset(this=<unavailable>, __p=<unavailable>) at unique_ptr.h:315:7
        frame #10: 0x00000000188b0626 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader() [inlined] std::__1::unique_ptr<replxx::Replxx::ReplxxImpl, void (*)(replxx::Replxx::ReplxxImpl*)>::~unique_ptr(this=<unavailable>) at unique_ptr.h:269
        frame #11: 0x00000000188b0626 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader() [inlined] replxx::Replxx::~Replxx(this=<unavailable>) at replxx.hxx:76
        frame #12: 0x00000000188b0626 clickhouse-22.8-release`ReplxxLineReader::~ReplxxLineReader(this=0x00007ffd10038440) at ReplxxLineReader.cpp:229
        frame #13: 0x00000000158b2100 clickhouse-22.8-release`DB::ClientBase::runInteractive(this=0x00007ffd10038620) at ClientBase.cpp:2067:1
        frame #14: 0x000000000a4de372 clickhouse-22.8-release`DB::Client::main(this=0x00007ffd10038620, (null)=<unavailable>) at Client.cpp:261:9
        frame #15: 0x0000000018923a86 clickhouse-22.8-release`Poco::Util::Application::run(this=0x00007ffd10038620) at Application.cpp:334:8
        frame #16: 0x000000000a4ed341 clickhouse-22.8-release`mainEntryClickHouseClient(argc=39, argv=0x00007f03fa8201c0) at Client.cpp:1220:23
        frame #17: 0x000000000a3b17ab clickhouse-22.8-release`main(argc_=<unavailable>, argv_=<unavailable>) at main.cpp:449:12
        frame #18: 0x00007f03fb5aa083 libc.so.6`__libc_start_main + 243
        frame #19: 0x000000000a17032e clickhouse-22.8-release`_start + 46
azat added a commit that referenced this pull request May 24, 2025
@azat
Fix possible crash in History::erase()
4906367 
The problem was that you can remove different entries from _locations
and _entries in case of Entry::_scratch does not match Entry::_text
(i.e. after some modifications of existing entries in history)

And you will get heap-use-after-free in this case:

    =================================================================
    ==21751==ERROR: AddressSanitizer: heap-use-after-free on address 0x508000077be0 at pc 0x5555908ad5e9 bp 0x7fffffffa2c0 sp 0x7fffffffa2b8
    READ of size 8 at 0x508000077be0 thread T0
        #0 0x5555908ad5e8 in std::__1::vector<>::data[abi:ne190107]() const ci/tmp/build/./contrib/llvm-project/libcxx/include/vector:674:36
        #1 0x5555908ad5e8 in replxx::UnicodeString::get() const ci/tmp/build/./contrib/replxx/src/unicodestring.hxx:148:16
        #2 0x5555908ad5e8 in std::__1::hash<replxx::UnicodeString>::operator()(replxx::UnicodeString const&) const ci/tmp/build/./contrib/replxx/src/history.hxx:17:26
        #3 0x5555908ad5e8 in ci/tmp/build/./contrib/llvm-project/libcxx/include/unordered_map:646:75
        #4 0x5555908ad5e8 in ci/tmp/build/./contrib/llvm-project/libcxx/include/__hash_table:1748:20
        #5 0x55559089c55b in ci/tmp/build/./contrib/llvm-project/libcxx/include/__hash_table:1851:18
        #6 0x55559089c55b in ci/tmp/build/./contrib/llvm-project/libcxx/include/unordered_map:1322:80
        #7 0x55559089c55b in replxx::History::erase(std::__1::__list_iterator<replxx::History::Entry, void*>) ci/tmp/build/./contrib/replxx/src/history.cxx:355:13
        #8 0x55559089c55b in replxx::History::remove_duplicate(replxx::UnicodeString const&) ci/tmp/build/./contrib/replxx/src/history.cxx:381:2
        #9 0x55559089b9ab in replxx::History::add(replxx::UnicodeString const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./contrib/replxx/src/history.cxx:94:2
        #10 0x5555908e9d70 in replxx::Replxx::ReplxxImpl::history_add(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./contrib/replxx/src/replxx_impl.cxx:2467:11
        #11 0x555586b4dc4f in DB::ReplxxLineReader::addToHistory(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./src/Client/ReplxxLineReader.cpp:527:8
        #12 0x555586b11f7f in DB::LineReader::readLine(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./src/Client/LineReader.cpp:217:9
        #13 0x555586a4445e in DB::ClientBase::runInteractive() ci/tmp/build/./src/Client/ClientBase.cpp:3335:25

    0x508000077be0 is located 64 bytes inside of 88-byte region [0x508000077ba0,0x508000077bf8)
    freed by thread T0 here:
        #0 0x55556193a622 in operator delete(void*, unsigned long) (/src/ch/tmp/ci/clickhouse+0xc3e6622) (BuildId: 6b920302e2b03137aca21bee17dec6310aedbafa)
        #1 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/new:274:3
        #2 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/new:298:10
        #3 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/new:311:12
        #4 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/__memory/allocator.h:132:7
        #5 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/__memory/allocator_traits.h:312:9
        #6 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/list:574:5
        #7 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/list:1341:9
        #8 0x55559089c60c in replxx::History::erase(std::__1::__list_iterator<replxx::History::Entry, void*>) ci/tmp/build/./contrib/replxx/src/history.cxx:356:17
        #9 0x55559089c60c in replxx::History::remove_duplicate(replxx::UnicodeString const&) ci/tmp/build/./contrib/replxx/src/history.cxx:381:2
        #10 0x55559089b9ab in replxx::History::add(replxx::UnicodeString const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./contrib/replxx/src/history.cxx:94:2
        #11 0x5555908e9d70 in replxx::Replxx::ReplxxImpl::history_add(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./contrib/replxx/src/replxx_impl.cxx:2467:11
        #12 0x555586b4dc4f in DB::ReplxxLineReader::addToHistory(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./src/Client/ReplxxLineReader.cpp:527:8
        #13 0x555586b11f7f in DB::LineReader::readLine(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./src/Client/LineReader.cpp:217:9
        #14 0x555586a4445e in DB::ClientBase::runInteractive() ci/tmp/build/./src/Client/ClientBase.cpp:3335:25

    SUMMARY: AddressSanitizer: heap-use-after-free ci/tmp/build/./contrib/llvm-project/libcxx/include/vector:674:36 in std::__1::vector<char32_t, std::__1::allocator<char32_t>>::data[abi:ne190107]() const
azat added a commit to azat-ch/replxx that referenced this pull request May 24, 2025
@azat
Fix possible crash in History::erase()
a33d909 
The problem was that you can remove different entries from _locations
and _entries in case of Entry::_scratch does not match Entry::_text
(i.e. after some modifications of existing entries in history)

And you will get heap-use-after-free in this case:

    =================================================================
    ==21751==ERROR: AddressSanitizer: heap-use-after-free on address 0x508000077be0 at pc 0x5555908ad5e9 bp 0x7fffffffa2c0 sp 0x7fffffffa2b8
    READ of size 8 at 0x508000077be0 thread T0
        #0 0x5555908ad5e8 in std::__1::vector<>::data[abi:ne190107]() const ci/tmp/build/./contrib/llvm-project/libcxx/include/vector:674:36
        ClickHouse#1 0x5555908ad5e8 in replxx::UnicodeString::get() const ci/tmp/build/./contrib/replxx/src/unicodestring.hxx:148:16
        ClickHouse#2 0x5555908ad5e8 in std::__1::hash<replxx::UnicodeString>::operator()(replxx::UnicodeString const&) const ci/tmp/build/./contrib/replxx/src/history.hxx:17:26
        ClickHouse#3 0x5555908ad5e8 in ci/tmp/build/./contrib/llvm-project/libcxx/include/unordered_map:646:75
        ClickHouse#4 0x5555908ad5e8 in ci/tmp/build/./contrib/llvm-project/libcxx/include/__hash_table:1748:20
        ClickHouse#5 0x55559089c55b in ci/tmp/build/./contrib/llvm-project/libcxx/include/__hash_table:1851:18
        ClickHouse#6 0x55559089c55b in ci/tmp/build/./contrib/llvm-project/libcxx/include/unordered_map:1322:80
        ClickHouse#7 0x55559089c55b in replxx::History::erase(std::__1::__list_iterator<replxx::History::Entry, void*>) ci/tmp/build/./contrib/replxx/src/history.cxx:355:13
        ClickHouse#8 0x55559089c55b in replxx::History::remove_duplicate(replxx::UnicodeString const&) ci/tmp/build/./contrib/replxx/src/history.cxx:381:2
        ClickHouse#9 0x55559089b9ab in replxx::History::add(replxx::UnicodeString const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./contrib/replxx/src/history.cxx:94:2
        ClickHouse#10 0x5555908e9d70 in replxx::Replxx::ReplxxImpl::history_add(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./contrib/replxx/src/replxx_impl.cxx:2467:11
        ClickHouse#11 0x555586b4dc4f in DB::ReplxxLineReader::addToHistory(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./src/Client/ReplxxLineReader.cpp:527:8
        ClickHouse#12 0x555586b11f7f in DB::LineReader::readLine(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./src/Client/LineReader.cpp:217:9
        ClickHouse#13 0x555586a4445e in DB::ClientBase::runInteractive() ci/tmp/build/./src/Client/ClientBase.cpp:3335:25

    0x508000077be0 is located 64 bytes inside of 88-byte region [0x508000077ba0,0x508000077bf8)
    freed by thread T0 here:
        #0 0x55556193a622 in operator delete(void*, unsigned long) (/src/ch/tmp/ci/clickhouse+0xc3e6622) (BuildId: 6b920302e2b03137aca21bee17dec6310aedbafa)
        ClickHouse#1 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/new:274:3
        ClickHouse#2 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/new:298:10
        ClickHouse#3 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/new:311:12
        ClickHouse#4 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/__memory/allocator.h:132:7
        ClickHouse#5 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/__memory/allocator_traits.h:312:9
        ClickHouse#6 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/list:574:5
        ClickHouse#7 0x55559089c60c in ci/tmp/build/./contrib/llvm-project/libcxx/include/list:1341:9
        ClickHouse#8 0x55559089c60c in replxx::History::erase(std::__1::__list_iterator<replxx::History::Entry, void*>) ci/tmp/build/./contrib/replxx/src/history.cxx:356:17
        ClickHouse#9 0x55559089c60c in replxx::History::remove_duplicate(replxx::UnicodeString const&) ci/tmp/build/./contrib/replxx/src/history.cxx:381:2
        ClickHouse#10 0x55559089b9ab in replxx::History::add(replxx::UnicodeString const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./contrib/replxx/src/history.cxx:94:2
        ClickHouse#11 0x5555908e9d70 in replxx::Replxx::ReplxxImpl::history_add(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./contrib/replxx/src/replxx_impl.cxx:2467:11
        ClickHouse#12 0x555586b4dc4f in DB::ReplxxLineReader::addToHistory(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./src/Client/ReplxxLineReader.cpp:527:8
        ClickHouse#13 0x555586b11f7f in DB::LineReader::readLine(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) ci/tmp/build/./src/Client/LineReader.cpp:217:9
        ClickHouse#14 0x555586a4445e in DB::ClientBase::runInteractive() ci/tmp/build/./src/Client/ClientBase.cpp:3335:25

    SUMMARY: AddressSanitizer: heap-use-after-free ci/tmp/build/./contrib/llvm-project/libcxx/include/vector:674:36 in std::__1::vector<char32_t, std::__1::allocator<char32_t>>::data[abi:ne190107]() const
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@azat @alexey-milovidov @AmokHuginnsson