Merge pull request #22583 from azat/log-rwlock-ub-fix

Avoid UB in *Log engines for rwlock unlock due to unlock from another thread

src/Storages/StorageLog.cpp

@@ -357,6 +357,11 @@ void LogBlockOutputStream::writeSuffix()
 
     streams.clear();
     done = true;
+
+    /// unlock should be done from the same thread as lock, and dtor may be
+    /// called from different thread, so it should be done here (at least in
+    /// case of no exceptions occurred)
+    lock.unlock();
 }
 
 

src/Storages/StorageLog.h

@@ -86,7 +86,7 @@ private:
     DiskPtr disk;
     String table_path;
 
-    mutable std::shared_timed_mutex rwlock;
+    std::shared_timed_mutex rwlock;
 
     Files files;
 

src/Storages/StorageStripeLog.cpp

@@ -228,6 +228,11 @@ public:
         storage.file_checker.save();
 
         done = true;
+
+        /// unlock should be done from the same thread as lock, and dtor may be
+        /// called from different thread, so it should be done here (at least in
+        /// case of no exceptions occurred)
+        lock.unlock();
     }
 
 private:

src/Storages/StorageStripeLog.h

@@ -68,7 +68,7 @@ private:
     size_t max_compress_block_size;
 
     FileChecker file_checker;
-    mutable std::shared_timed_mutex rwlock;
+    std::shared_timed_mutex rwlock;
 
     Poco::Logger * log;
 };

src/Storages/StorageTinyLog.cpp

@@ -358,6 +358,9 @@ void TinyLogBlockOutputStream::writeSuffix()
         storage.file_checker.update(file);
     storage.file_checker.save();
 
+    /// unlock should be done from the same thread as lock, and dtor may be
+    /// called from different thread, so it should be done here (at least in
+    /// case of no exceptions occurred)
     lock.unlock();
 }
 

tests/queries/0_stateless/01795_TinyLog_rwlock_ub.sql

@@ -0,0 +1,5 @@
+DROP TABLE IF EXISTS underlying_01795;
+CREATE TABLE underlying_01795 (key UInt64) Engine=TinyLog();
+INSERT INTO FUNCTION remote('127.1', currentDatabase(), underlying_01795) SELECT toUInt64(number) FROM system.numbers LIMIT 1;
+SELECT * FROM underlying_01795 FORMAT Null;
+DROP TABLE underlying_01795;

tests/queries/0_stateless/01796_Log_rwlock_ub.sql

@@ -0,0 +1,5 @@
+DROP TABLE IF EXISTS underlying_01796;
+CREATE TABLE underlying_01796 (key UInt64) Engine=Log();
+INSERT INTO FUNCTION remote('127.1', currentDatabase(), underlying_01796) SELECT toUInt64(number) FROM system.numbers LIMIT 1;
+SELECT * FROM underlying_01796 FORMAT Null;
+DROP TABLE underlying_01796;

tests/queries/0_stateless/01797_StripeLog_rwlock_ub.sql

@@ -0,0 +1,5 @@
+DROP TABLE IF EXISTS underlying_01797;
+CREATE TABLE underlying_01797 (key UInt64) Engine=StripeLog();
+INSERT INTO FUNCTION remote('127.1', currentDatabase(), underlying_01797) SELECT toUInt64(number) FROM system.numbers LIMIT 1;
+SELECT * FROM underlying_01797 FORMAT Null;
+DROP TABLE underlying_01797;