From c0d7b6efc388f10388b4cb6fcbbf5dc40c70f980 Mon Sep 17 00:00:00 2001
From: Alexey Milovidov <milovidov@clickhouse.com>
Date: Sat, 30 Jul 2022 01:45:06 +0200
Subject: [PATCH] Suggestions from @tavplubix

---
 programs/server/config.xml |  5 ++++-
 src/Common/StackTrace.cpp  | 30 ++++++++++++++++++++----------
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/programs/server/config.xml b/programs/server/config.xml
index c49d2a83499..1060cb3db0a 100644
--- a/programs/server/config.xml
+++ b/programs/server/config.xml
@@ -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>
diff --git a/src/Common/StackTrace.cpp b/src/Common/StackTrace.cpp
index 01d8bccc1f5..70f80b62868 100644
--- a/src/Common/StackTrace.cpp
+++ b/src/Common/StackTrace.cpp
@@ -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
 }