Suggestions from @tavplubix

This commit is contained in:
Alexey Milovidov 2022-07-30 01:45:06 +02:00
parent 4aa96767d5
commit c0d7b6efc3
2 changed files with 24 additions and 11 deletions

View File

@ -1389,7 +1389,10 @@
<!-- This allows to disable exposing addresses in stack traces for security reasons.
Please be aware that it does not improve security much, but makes debugging much harder.
The addresses that are small offsets from zero will be displayed nevertheless to show nullptr dereferences.
Regardless of this configuration, the addresses are visible in the system.stack_trace and system.trace_log tables
if the user has access to these tables.
I don't recommend to change this setting.
-->
<show_addresses_in_stack_traces>true</show_addresses_in_stack_traces>
<show_addresses_in_stack_traces>false</show_addresses_in_stack_traces>
</clickhouse>

View File

@ -25,6 +25,18 @@ namespace
/// Currently this variable is set up once on server startup.
/// But we use atomic just in case, so it is possible to be modified at runtime.
std::atomic<bool> show_addresses = true;
bool shouldShowAddress(const void * addr)
{
/// If the address is less than 4096, most likely it is a nullptr dereference with offset,
/// and showing this offset is secure nevertheless.
/// NOTE: 4096 is the page size on x86 and it can be different on other systems,
/// but for the purpose of this branch, it does not matter.
if (reinterpret_cast<uintptr_t>(addr) < 4096)
return true;
return show_addresses.load(std::memory_order_relaxed);
}
}
void StackTrace::setShowAddresses(bool show)
@ -44,7 +56,7 @@ std::string signalToErrorMessage(int sig, const siginfo_t & info, [[maybe_unused
/// Print info about address and reason.
if (nullptr == info.si_addr)
error << "Address: NULL pointer.";
else if (show_addresses.load(std::memory_order_relaxed))
else if (shouldShowAddress(info.si_addr))
error << "Address: " << info.si_addr;
#if defined(__x86_64__) && !defined(OS_FREEBSD) && !defined(OS_DARWIN) && !defined(__arm__) && !defined(__powerpc__)
@ -386,7 +398,7 @@ static void toStringEveryLineImpl(
else
out << "?";
if (show_addresses.load(std::memory_order_relaxed))
if (shouldShowAddress(physical_addr))
out << " @ " << physical_addr;
out << " in " << (object ? object->name : "?");
@ -403,22 +415,20 @@ static void toStringEveryLineImpl(
out.str({});
}
#else
if (show_addresses.load(std::memory_order_relaxed))
{
std::stringstream out; // STYLE_CHECK_ALLOW_STD_STRING_STREAM
out.exceptions(std::ios::failbit);
std::stringstream out; // STYLE_CHECK_ALLOW_STD_STRING_STREAM
out.exceptions(std::ios::failbit);
for (size_t i = offset; i < size; ++i)
for (size_t i = offset; i < size; ++i)
{
const void * addr = frame_pointers[i];
if (shouldShowAddress(addr))
{
const void * addr = frame_pointers[i];
out << i << ". " << addr;
callback(out.str());
out.str({});
}
}
else
callback("Addresses are hidden for security reasons.");
#endif
}