Merge pull request #2845 from rouault/fix_2843

Fix database access across fork() when SQLite3 doesn't use pread[64]() (fixes #2843)

4 files changed, 227 insertions, 16 deletions

diff --git a/src/iso19111/factory.cpp b/src/iso19111/factory.cpp
index 8b54160d..4ea9d2ad 100644
--- a/src/iso19111/factory.cpp
+++ b/src/iso19111/factory.cpp
@@ -78,6 +78,20 @@
 // parallel. This is slightly faster
 #define ENABLE_CUSTOM_LOCKLESS_VFS
 
+#if defined(_WIN32) && defined(MUTEX_pthread)
+#undef MUTEX_pthread
+#endif
+
+/* SQLite3 might use seak()+read() or pread[64]() to read data */
+/* The later allows the same SQLite handle to be safely used in forked */
+/* children of a parent process, while the former doesn't. */
+/* So we use pthread_atfork() to set a flag in forked children, to ask them */
+/* to close and reopen their database handle. */
+#if defined(MUTEX_pthread) && !defined(SQLITE_USE_PREAD)
+#include <pthread.h>
+#define REOPEN_SQLITE_DB_AFTER_FORK
+#endif
+
 using namespace NS_PROJ::internal;
 using namespace NS_PROJ::common;
 
@@ -217,6 +231,10 @@ class SQLiteHandle {
     sqlite3 *sqlite_handle_ = nullptr;
     bool close_handle_ = true;
 
+#ifdef REOPEN_SQLITE_DB_AFTER_FORK
+    bool is_valid_ = true;
+#endif
+
     int nLayoutVersionMajor_ = 0;
     int nLayoutVersionMinor_ = 0;
 
@@ -244,6 +262,12 @@ class SQLiteHandle {
 
     sqlite3 *handle() { return sqlite_handle_; }
 
+#ifdef REOPEN_SQLITE_DB_AFTER_FORK
+    bool isValid() const { return is_valid_; }
+
+    void invalidate() { is_valid_ = false; }
+#endif
+
     static std::shared_ptr<SQLiteHandle> open(PJ_CONTEXT *ctx,
                                               const std::string &path);
 
@@ -559,6 +583,10 @@ void SQLiteHandle::registerFunctions() {
 // ---------------------------------------------------------------------------
 
 class SQLiteHandleCache {
+#ifdef REOPEN_SQLITE_DB_AFTER_FORK
+    bool firstTime_ = true;
+#endif
+
     NS_PROJ::mutex sMutex_{};
 
     // Map dbname to SQLiteHandle
@@ -571,6 +599,10 @@ class SQLiteHandleCache {
                                             PJ_CONTEXT *ctx);
 
     void clear();
+
+#ifdef REOPEN_SQLITE_DB_AFTER_FORK
+    void invalidateHandles();
+#endif
 };
 
 // ---------------------------------------------------------------------------
@@ -593,6 +625,15 @@ void SQLiteHandleCache::clear() {
 std::shared_ptr<SQLiteHandle>
 SQLiteHandleCache::getHandle(const std::string &path, PJ_CONTEXT *ctx) {
     NS_PROJ::lock_guard<NS_PROJ::mutex> lock(sMutex_);
+
+#ifdef REOPEN_SQLITE_DB_AFTER_FORK
+    if (firstTime_) {
+        firstTime_ = false;
+        pthread_atfork(nullptr, nullptr,
+                       []() { SQLiteHandleCache::get().invalidateHandles(); });
+    }
+#endif
+
     std::shared_ptr<SQLiteHandle> handle;
     std::string key = path + ctx->custom_sqlite3_vfs_name;
     if (!cache_.tryGet(key, handle)) {
@@ -602,6 +643,19 @@ SQLiteHandleCache::getHandle(const std::string &path, PJ_CONTEXT *ctx) {
     return handle;
 }
 
+#ifdef REOPEN_SQLITE_DB_AFTER_FORK
+// ---------------------------------------------------------------------------
+
+void SQLiteHandleCache::invalidateHandles() {
+    NS_PROJ::lock_guard<NS_PROJ::mutex> lock(sMutex_);
+    const auto lambda =
+        [](const lru11::KeyValuePair<std::string, std::shared_ptr<SQLiteHandle>>
+               &kvp) { kvp.value->invalidate(); };
+    cache_.cwalk(lambda);
+    cache_.clear();
+}
+#endif
+
 // ---------------------------------------------------------------------------
 
 struct DatabaseContext::Private {
@@ -611,9 +665,7 @@ struct DatabaseContext::Private {
     void open(const std::string &databasePath, PJ_CONTEXT *ctx);
     void setHandle(sqlite3 *sqlite_handle);
 
-    sqlite3 *handle() const {
-        return sqlite_handle_ ? sqlite_handle_->handle() : nullptr;
-    }
+    const std::shared_ptr<SQLiteHandle> &handle();
 
     PJ_CONTEXT *pjCtxt() const { return pjCtxt_; }
     void setPjCtxt(PJ_CONTEXT *ctxt) { pjCtxt_ = ctxt; }
@@ -931,6 +983,21 @@ void DatabaseContext::Private::clearCaches() {
 
 // ---------------------------------------------------------------------------
 
+const std::shared_ptr<SQLiteHandle> &DatabaseContext::Private::handle() {
+#ifdef REOPEN_SQLITE_DB_AFTER_FORK
+    if (sqlite_handle_ && !sqlite_handle_->isValid()) {
+        closeDB();
+        open(databasePath_, pjCtxt_);
+        if (!auxiliaryDatabasePaths_.empty()) {
+            attachExtraDatabases(auxiliaryDatabasePaths_);
+        }
+    }
+#endif
+    return sqlite_handle_;
+}
+
+// ---------------------------------------------------------------------------
+
 void DatabaseContext::Private::insertIntoCache(LRUCacheOfObjects &cache,
                                                const std::string &code,
                                                const util::BaseObjectPtr &obj) {
@@ -1170,7 +1237,8 @@ std::vector<std::string> DatabaseContext::Private::getDatabaseStructure() {
 void DatabaseContext::Private::attachExtraDatabases(
     const std::vector<std::string> &auxiliaryDatabasePaths) {
 
-    assert(sqlite_handle_);
+    auto l_handle = handle();
+    assert(l_handle);
 
     auto tables =
         run("SELECT name FROM sqlite_master WHERE type IN ('table', 'view') "
@@ -1186,8 +1254,8 @@ void DatabaseContext::Private::attachExtraDatabases(
         }
     }
 
-    const int nLayoutVersionMajor = sqlite_handle_->getLayoutVersionMajor();
-    const int nLayoutVersionMinor = sqlite_handle_->getLayoutVersionMinor();
+    const int nLayoutVersionMajor = l_handle->getLayoutVersionMajor();
+    const int nLayoutVersionMinor = l_handle->getLayoutVersionMinor();
 
     closeDB();
     if (auxiliaryDatabasePaths.empty()) {
@@ -1204,6 +1272,7 @@ void DatabaseContext::Private::attachExtraDatabases(
     }
     sqlite_handle_ = SQLiteHandle::initFromExisting(
         sqlite_handle, true, nLayoutVersionMajor, nLayoutVersionMinor);
+    l_handle = sqlite_handle_;
 
     run("ATTACH DATABASE '" + replaceAll(databasePath_, "'", "''") +
         "' AS db_0");
@@ -1218,8 +1287,8 @@ void DatabaseContext::Private::attachExtraDatabases(
         count++;
         run(sql);
 
-        sqlite_handle_->checkDatabaseLayout(databasePath_, otherDbPath,
-                                            attachedDbName + '.');
+        l_handle->checkDatabaseLayout(databasePath_, otherDbPath,
+                                      attachedDbName + '.');
     }
 
     for (const auto &pair : tableStructure) {
@@ -1263,23 +1332,26 @@ SQLResultSet DatabaseContext::Private::run(const std::string &sql,
                                            const ListOfParams &parameters,
                                            bool useMaxFloatPrecision) {
 
+    auto l_handle = handle();
+    assert(l_handle);
+
     sqlite3_stmt *stmt = nullptr;
     auto iter = mapSqlToStatement_.find(sql);
     if (iter != mapSqlToStatement_.end()) {
         stmt = iter->second;
         sqlite3_reset(stmt);
     } else {
-        if (sqlite3_prepare_v2(handle(), sql.c_str(),
+        if (sqlite3_prepare_v2(l_handle->handle(), sql.c_str(),
                                static_cast<int>(sql.size()), &stmt,
                                nullptr) != SQLITE_OK) {
             throw FactoryException("SQLite error on " + sql + ": " +
-                                   sqlite3_errmsg(handle()));
+                                   sqlite3_errmsg(l_handle->handle()));
         }
         mapSqlToStatement_.insert(
             std::pair<std::string, sqlite3_stmt *>(sql, stmt));
     }
 
-    return sqlite_handle_->run(stmt, sql, parameters, useMaxFloatPrecision);
+    return l_handle->run(stmt, sql, parameters, useMaxFloatPrecision);
 }
 
 // ---------------------------------------------------------------------------
@@ -3117,9 +3189,7 @@ DatabaseContextNNPtr DatabaseContext::create(void *sqlite_handle) {
 
 // ---------------------------------------------------------------------------
 
-void *DatabaseContext::getSqliteHandle() const {
-    return getPrivate()->handle();
-}
+void *DatabaseContext::getSqliteHandle() const { return d->handle()->handle(); }
 
 // ---------------------------------------------------------------------------
 
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 1a080ac5..cc5d18b7 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -189,3 +189,15 @@ target_link_libraries(test_misc
 add_test(NAME test_misc COMMAND test_misc)
 set_property(TEST test_misc
   PROPERTY ENVIRONMENT ${PROJ_TEST_ENVIRONMENT})
+
+if (USE_THREAD AND Threads_FOUND AND CMAKE_USE_PTHREADS_INIT)
+add_definitions(-DMUTEX_pthread)
+add_executable(test_fork
+  test_fork.c)
+target_link_libraries(test_fork
+  PRIVATE ${PROJ_LIBRARIES}
+  PRIVATE ${CMAKE_THREAD_LIBS_INIT})
+add_test(NAME test_fork COMMAND test_fork)
+set_property(TEST test_fork
+  PROPERTY ENVIRONMENT ${PROJ_TEST_ENVIRONMENT})
+endif()
diff --git a/test/unit/Makefile.am b/test/unit/Makefile.am
index 1326332b..9ca3a723 100644
--- a/test/unit/Makefile.am
+++ b/test/unit/Makefile.am
@@ -4,7 +4,7 @@ EXTRA_DIST = CMakeLists.txt
 
 noinst_HEADERS = gtest_include.h test_primitives.hpp
 
-AM_CPPFLAGS = -I$(top_srcdir)/src -I$(top_srcdir)/include -I$(top_srcdir)/test @GTEST_CFLAGS@ @SQLITE3_CFLAGS@
+AM_CPPFLAGS = -I$(top_srcdir)/src -I$(top_srcdir)/include -I$(top_srcdir)/test @GTEST_CFLAGS@ @SQLITE3_CFLAGS@ -DMUTEX_@MUTEX_SETTING@
 AM_CXXFLAGS = @CXX_WFLAGS@ @NO_ZERO_AS_NULL_POINTER_CONSTANT_FLAG@
 
 PROJ_LIB ?= ../../data/for_tests
@@ -20,6 +20,7 @@ noinst_PROGRAMS += test_network
 noinst_PROGRAMS += test_defmodel
 noinst_PROGRAMS += test_tinshift
 noinst_PROGRAMS += test_misc
+noinst_PROGRAMS += test_fork
 
 pj_phi2_test_SOURCES = pj_phi2_test.cpp main.cpp
 pj_phi2_test_LDADD = ../../src/libproj.la @GTEST_LIBS@
@@ -95,7 +96,13 @@ test_misc_LDADD = ../../src/libproj.la @GTEST_LIBS@
 test_misc-check: test_misc
 	PROJ_LIB=$(PROJ_LIB) ./test_misc
 
+test_fork_SOURCES = test_fork.c
+test_fork_LDADD = ../../src/libproj.la @THREAD_LIB@
+
+test_fork-check: test_fork
+	PROJ_LIB=$(PROJ_LIB) ./test_fork
+
 check-local: pj_phi2_test-check proj_errno_string_test-check \
 		proj_angular_io_test-check proj_context_test-check test_cpp_api-check \
 		gie_self_tests-check test_network-check test_defmodel-check test_tinshift-check \
-		test_misc-check
+		test_misc-check test_fork-check
diff --git a/test/unit/test_fork.c b/test/unit/test_fork.c
new file mode 100644
index 00000000..181c1121
--- /dev/null
+++ b/test/unit/test_fork.c
@@ -0,0 +1,122 @@
+/******************************************************************************
+ *
+ * Project:  PROJ
+ * Purpose:  Test behavior of database access across fork()
+ * Author:   Even Rouault <even dot rouault at spatialys dot com>
+ *
+ ******************************************************************************
+ * Copyright (c) 2021, Even Rouault <even dot rouault at spatialys dot com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included
+ * in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ ****************************************************************************/
+
+#if defined(_WIN32) && defined(MUTEX_pthread)
+#undef MUTEX_pthread
+#endif
+
+#ifdef MUTEX_pthread
+
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "proj.h"
+
+int main()
+{
+    PJ_CONTEXT* ctxt = proj_context_create();
+    PJ_CONTEXT* ctxt2 = proj_context_create();
+
+    /* Cause database initialization */
+    {
+        PJ* P = proj_create(ctxt, "EPSG:4326");
+        if( P == NULL )
+        {
+            proj_context_destroy(ctxt);
+            return 1;
+        }
+        proj_destroy(P);
+    }
+    {
+        PJ* P = proj_create(ctxt2, "EPSG:4326");
+        if( P == NULL )
+        {
+            proj_context_destroy(ctxt);
+            proj_context_destroy(ctxt2);
+            return 1;
+        }
+        proj_destroy(P);
+    }
+
+    for(int iters = 0; iters < 100; ++iters )
+    {
+        pid_t children[4];
+        for( int i = 0; i< 4; i++ )
+        {
+            children[i] = fork();
+            if( children[i] < 0 )
+            {
+                fprintf(stderr, "fork() failed\n");
+                return 1;
+            }
+            if( children[i] == 0 )
+            {
+                {
+                    PJ* P = proj_create(ctxt, "EPSG:3067");
+                    if( P == NULL )
+                        _exit(1);
+                    proj_destroy(P);
+                }
+                {
+                    PJ* P = proj_create(ctxt2, "EPSG:32631");
+                    if( P == NULL )
+                        _exit(1);
+                    proj_destroy(P);
+                }
+                _exit(0);
+            }
+        }
+        for( int i = 0; i< 4; i++ )
+        {
+            int status = 0;
+            waitpid(children[i], &status, 0);
+            if( status != 0 )
+            {
+                fprintf(stderr, "Error in child\n");
+                return 1;
+            }
+        }
+    }
+
+    proj_context_destroy(ctxt);
+    proj_context_destroy(ctxt2);
+
+    return 0;
+}
+
+#else
+
+int main()
+{
+    return 0;
+}
+
+#endif