Backport #72870 to 24.11: Fix use-after-free in loadPathPrefixMap on thread pool exception

robot-clickhouse · robot-clickhouse · commit 6ee5bbadf433 · 2024-12-26T19:08:14.000Z
diff --git a/src/Disks/ObjectStorages/MetadataStorageFromPlainRewritableObjectStorage.cpp b/src/Disks/ObjectStorages/MetadataStorageFromPlainRewritableObjectStorage.cpp
@@ -126,72 +126,80 @@ std::shared_ptr<InMemoryDirectoryPathMap> loadPathPrefixMap(const std::string &
 
     std::mutex mutex;
     InMemoryDirectoryPathMap::Map map;
-    for (auto iterator = object_storage->iterate(metadata_key_prefix, 0); iterator->isValid(); iterator->next())
+    try
     {
-        ++num_files;
-        auto file = iterator->current();
-        String path = file->getPath();
-        auto remote_metadata_path = std::filesystem::path(path);
-        if (remote_metadata_path.filename() != PREFIX_PATH_FILE_NAME)
-            continue;
-
-        runner(
-            [remote_metadata_path, path, &object_storage, &mutex, &map, &log, &settings, &metadata_key_prefix]
-            {
-                setThreadName("PlainRWMetaLoad");
+        for (auto iterator = object_storage->iterate(metadata_key_prefix, 0); iterator->isValid(); iterator->next())
+        {
+            ++num_files;
+            auto file = iterator->current();
+            String path = file->getPath();
+            auto remote_metadata_path = std::filesystem::path(path);
+            if (remote_metadata_path.filename() != PREFIX_PATH_FILE_NAME)
+                continue;
+
+            runner(
+                [remote_metadata_path, path, &object_storage, &mutex, &map, &log, &settings, &metadata_key_prefix]
+                {
+                    setThreadName("PlainRWMetaLoad");
 
-                StoredObject object{path};
-                String local_path;
-                Poco::Timestamp last_modified{};
+                    StoredObject object{path};
+                    String local_path;
+                    Poco::Timestamp last_modified{};
 
-                try
-                {
-                    auto read_buf = object_storage->readObject(object, settings);
-                    readStringUntilEOF(local_path, *read_buf);
-                    auto object_metadata = object_storage->tryGetObjectMetadata(path);
-                    /// It ok if a directory was removed just now.
-                    /// We support attaching a filesystem that is concurrently modified by someone else.
-                    if (!object_metadata)
-                        return;
-                    /// Assuming that local and the object storage clocks are synchronized.
-                    last_modified = object_metadata->last_modified;
-                }
-#if USE_AWS_S3
-                catch (const S3Exception & e)
-                {
-                    /// It is ok if a directory was removed just now.
-                    if (e.getS3ErrorCode() == Aws::S3::S3Errors::NO_SUCH_KEY)
-                        return;
-                    throw;
-                }
-#endif
-                catch (...)
-                {
-                    throw;
-                }
+                    try
+                    {
+                        auto read_buf = object_storage->readObject(object, settings);
+                        readStringUntilEOF(local_path, *read_buf);
+                        auto object_metadata = object_storage->tryGetObjectMetadata(path);
+                        /// It ok if a directory was removed just now.
+                        /// We support attaching a filesystem that is concurrently modified by someone else.
+                        if (!object_metadata)
+                            return;
+                        /// Assuming that local and the object storage clocks are synchronized.
+                        last_modified = object_metadata->last_modified;
+                    }
+    #if USE_AWS_S3
+                    catch (const S3Exception & e)
+                    {
+                        /// It is ok if a directory was removed just now.
+                        if (e.getS3ErrorCode() == Aws::S3::S3Errors::NO_SUCH_KEY)
+                            return;
+                        throw;
+                    }
+    #endif
+                    catch (...)
+                    {
+                        throw;
+                    }
 
-                chassert(remote_metadata_path.has_parent_path());
-                chassert(remote_metadata_path.string().starts_with(metadata_key_prefix));
-                auto suffix = remote_metadata_path.string().substr(metadata_key_prefix.size());
-                auto rel_path = std::filesystem::path(std::move(suffix));
-                std::pair<Map::iterator, bool> res;
-                {
-                    std::lock_guard lock(mutex);
-                    res = map.emplace(
-                        std::filesystem::path(local_path).parent_path(),
-                        InMemoryDirectoryPathMap::RemotePathInfo{rel_path.parent_path(), last_modified.epochTime(), {}});
-                }
+                    chassert(remote_metadata_path.has_parent_path());
+                    chassert(remote_metadata_path.string().starts_with(metadata_key_prefix));
+                    auto suffix = remote_metadata_path.string().substr(metadata_key_prefix.size());
+                    auto rel_path = std::filesystem::path(std::move(suffix));
+                    std::pair<Map::iterator, bool> res;
+                    {
+                        std::lock_guard lock(mutex);
+                        res = map.emplace(
+                            std::filesystem::path(local_path).parent_path(),
+                            InMemoryDirectoryPathMap::RemotePathInfo{rel_path.parent_path(), last_modified.epochTime(), {}});
+                    }
 
-                /// This can happen if table replication is enabled, then the same local path is written
-                /// in `prefix.path` of each replica.
-                if (!res.second)
-                    LOG_WARNING(
-                        log,
-                        "The local path '{}' is already mapped to a remote path '{}', ignoring: '{}'",
-                        local_path,
-                        res.first->second.path,
-                        rel_path.parent_path().string());
-            });
+                    /// This can happen if table replication is enabled, then the same local path is written
+                    /// in `prefix.path` of each replica.
+                    if (!res.second)
+                        LOG_WARNING(
+                            log,
+                            "The local path '{}' is already mapped to a remote path '{}', ignoring: '{}'",
+                            local_path,
+                            res.first->second.path,
+                            rel_path.parent_path().string());
+                });
+        }
+    }
+    catch (...)
+    {
+        runner.waitForAllToFinish();
+        throw;
     }
 
     runner.waitForAllToFinishAndRethrowFirstError();
