kicad-developers team mailing list archive

[Simon Richter <Simon.Richter@xxxxxxxxxx>]

This allows compiling KiCad without OpenSSL. If CURL uses OpenSSL in the
background, we serialize requests to avoid a bug there, which degrades
performance. Given that no one should link KiCad against OpenSSL really
because of licence conflicts, this is not so bad.
---
 common/CMakeLists.txt                 |   5 --
 common/kicad_curl/kicad_curl.cpp      | 161 ----------------------------------
 common/kicad_curl/kicad_curl_easy.cpp |  23 +++--
 include/kicad_curl/kicad_curl.h       |  17 ----
 include/kicad_curl/kicad_curl_easy.h  |   5 ++
 5 files changed, 22 insertions(+), 189 deletions(-)

diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt
index 61255cf..c064a2d 100644
--- a/common/CMakeLists.txt
+++ b/common/CMakeLists.txt
@@ -14,11 +14,6 @@ include_directories(
     )
 
 
-if( NOT APPLE )     # windows and linux use openssl under curl
-    find_package( OpenSSL REQUIRED )
-endif()
-
-
 # Generate header files containing shader programs
 # Order of input files is significant
 add_custom_command(
diff --git a/common/kicad_curl/kicad_curl.cpp b/common/kicad_curl/kicad_curl.cpp
index fb4413c..0b25360 100644
--- a/common/kicad_curl/kicad_curl.cpp
+++ b/common/kicad_curl/kicad_curl.cpp
@@ -32,172 +32,11 @@
 
 #include <macros.h>
 #include <fctsys.h>
-#include <ki_mutex.h>       // MUTEX and MUTLOCK
 #include <richio.h>
 
 
-
-// These are even more private than class members, and since there is only
-// one instance of KICAD_CURL ever, these statics are hidden here to simplify the
-// client (API) header file.
-static volatile bool s_initialized;
-
-static MUTEX s_lock;        // for s_initialized
-
-// Assume that on these platforms libcurl uses OpenSSL
-#if defined(__linux__) || defined(__MINGW32__)
-
-#include <openssl/crypto.h>
-
-static MUTEX* s_crypto_locks;
-
-static void lock_callback( int mode, int type, const char* file, int line )
-{
-    (void)file;
-    (void)line;
-
-    wxASSERT( s_crypto_locks && unsigned( type ) < unsigned( CRYPTO_num_locks() ) );
-
-    //DBG( printf( "%s: mode=0x%x type=%d file=%s line=%d\n", __func__, mode, type, file, line );)
-
-    if( mode & CRYPTO_LOCK )
-    {
-        s_crypto_locks[ type ].lock();
-    }
-    else
-    {
-        s_crypto_locks[ type ].unlock();
-    }
-}
-
-
-static void init_locks()
-{
-    s_crypto_locks = new MUTEX[ CRYPTO_num_locks() ];
-
-    // From http://linux.die.net/man/3/crypto_set_id_callback:
-
-    /*
-
-    OpenSSL can safely be used in multi-threaded applications provided that at
-    least two callback functions are set, locking_function and threadid_func.
-
-    locking_function(int mode, int n, const char *file, int line) is needed to
-    perform locking on shared data structures. (Note that OpenSSL uses a number
-    of global data structures that will be implicitly shared whenever multiple
-    threads use OpenSSL.) Multi-threaded applications will crash at random if it
-    is not set.
-
-    threadid_func( CRYPTO_THREADID *id) is needed to record the
-    currently-executing thread's identifier into id. The implementation of this
-    callback should not fill in id directly, but should use
-    CRYPTO_THREADID_set_numeric() if thread IDs are numeric, or
-    CRYPTO_THREADID_set_pointer() if they are pointer-based. If the application
-    does not register such a callback using CRYPTO_THREADID_set_callback(), then
-    a default implementation is used - on Windows and BeOS this uses the
-    system's default thread identifying APIs, and on all other platforms it uses
-    the address of errno. The latter is satisfactory for thread-safety if and
-    only if the platform has a thread-local error number facility.
-
-    Dick: "sounds like CRYPTO_THREADID_set_callback() is not mandatory on our
-    2 OpenSSL platforms."
-
-    */
-
-    CRYPTO_set_locking_callback( &lock_callback );
-}
-
-
-static void kill_locks()
-{
-    CRYPTO_set_locking_callback( NULL );
-
-    delete[] s_crypto_locks;
-
-    s_crypto_locks = NULL;
-}
-
-#else
-
-inline void init_locks()    { /* dummy */ }
-inline void kill_locks()    { /* dummy */ }
-
-#endif
-
-/// At process termination, using atexit() keeps the CURL stuff out of the
-/// singletops and PGM_BASE.
-static void at_terminate()
-{
-    KICAD_CURL::Cleanup();
-}
-
-
-void KICAD_CURL::Init()
-{
-    // We test s_initialized twice in an effort to avoid
-    // unnecessarily locking s_lock.  This understands that the common case
-    // will not need to lock.
-    if( !s_initialized )
-    {
-        MUTLOCK lock( s_lock );
-
-        if( !s_initialized )
-        {
-            if( curl_global_init( CURL_GLOBAL_ALL ) != CURLE_OK )
-            {
-                THROW_IO_ERROR( "curl_global_init() failed." );
-            }
-
-            init_locks();
-
-            wxLogDebug( "Using %s", GetVersion() );
-
-            s_initialized = true;
-        }
-    }
-}
-
-
-void KICAD_CURL::Cleanup()
-{
-    /*
-
-    Calling MUTLOCK() from a static destructor will typically be bad, since the
-    s_lock may already have been statically destroyed itself leading to a boost
-    exception. (Remember C++ does not provide certain sequencing of static
-    destructor invocation.)
-
-    To prevent this we test s_initialized twice, which ensures that the MUTLOCK
-    is only instantiated on the first call, which should be from
-    PGM_BASE::destroy() which is first called earlier than static destruction.
-    Then when called again from the actual PGM_BASE::~PGM_BASE() function,
-    MUTLOCK will not be instantiated because s_initialized will be false.
-
-    */
-
-    if( s_initialized )
-    {
-        MUTLOCK lock( s_lock );
-
-        if( s_initialized )
-        {
-            curl_global_cleanup();
-
-            kill_locks();
-
-            atexit( &at_terminate );
-
-            s_initialized = false;
-        }
-    }
-}
-
-
 std::string KICAD_CURL::GetSimpleVersion()
 {
-    if( !s_initialized )
-        Init();
-
     curl_version_info_data* info = curl_version_info( CURLVERSION_NOW );
 
     std::string res;
diff --git a/common/kicad_curl/kicad_curl_easy.cpp b/common/kicad_curl/kicad_curl_easy.cpp
index 1e84afe..b227b41 100644
--- a/common/kicad_curl/kicad_curl_easy.cpp
+++ b/common/kicad_curl/kicad_curl_easy.cpp
@@ -23,6 +23,7 @@
  */
 
 #include <kicad_curl/kicad_curl_easy.h>
+#include <curl/curl.h>
 
 #include <cstddef>
 #include <exception>
@@ -30,6 +31,8 @@
 #include <sstream>
 #include <richio.h>
 
+#include <boost/interprocess/sync/scoped_lock.hpp>
+
 
 static size_t write_callback( void* contents, size_t size, size_t nmemb, void* userp )
 {
@@ -46,12 +49,6 @@ static size_t write_callback( void* contents, size_t size, size_t nmemb, void* u
 KICAD_CURL_EASY::KICAD_CURL_EASY() :
     m_headers( NULL )
 {
-    // Call KICAD_CURL::Init() from in here everytime, but only the first time
-    // will incur any overhead.  This strategy ensures that libcurl is never loaded
-    // unless it is needed.
-
-    KICAD_CURL::Init();
-
     m_CURL = curl_easy_init();
 
     if( !m_CURL )
@@ -59,6 +56,10 @@ KICAD_CURL_EASY::KICAD_CURL_EASY() :
         THROW_IO_ERROR( "Unable to initialize CURL session" );
     }
 
+    curl_tlssessioninfo info;
+    curl_easy_getinfo( m_CURL, CURLINFO_TLS_SESSION, (void *) &info );
+    m_WorkaroundOpenSSL = ( info.backend == CURLSSLBACKEND_OPENSSL );
+
     curl_easy_setopt( m_CURL, CURLOPT_WRITEFUNCTION, write_callback );
     curl_easy_setopt( m_CURL, CURLOPT_WRITEDATA, (void*) &m_buffer );
 }
@@ -75,6 +76,13 @@ KICAD_CURL_EASY::~KICAD_CURL_EASY()
 
 void KICAD_CURL_EASY::Perform()
 {
+    using namespace boost::interprocess;
+
+    scoped_lock<interprocess_mutex> lock( s_WorkaroundOpenSSLMutex, defer_lock );
+
+    if( m_WorkaroundOpenSSL )
+        lock.lock();
+
     if( m_headers )
     {
         curl_easy_setopt( m_CURL, CURLOPT_HTTPHEADER, m_headers );
@@ -92,3 +100,6 @@ void KICAD_CURL_EASY::Perform()
         THROW_IO_ERROR( msg );
     }
 }
+
+
+boost::interprocess::interprocess_mutex KICAD_CURL_EASY::s_WorkaroundOpenSSLMutex;
diff --git a/include/kicad_curl/kicad_curl.h b/include/kicad_curl/kicad_curl.h
index 51655a9..7804860 100644
--- a/include/kicad_curl/kicad_curl.h
+++ b/include/kicad_curl/kicad_curl.h
@@ -67,23 +67,6 @@ class KICAD_CURL
 
 public:
     /**
-     * Function Init
-     * calls curl_global_init for the application. It must be used only once
-     * and before any curl functions that perform requests.
-     *
-     * @return bool - True if successful, false if CURL returned an error
-     * @throw IO_ERROR on failure, hopefully with helpful text in it.
-     */
-    static void Init();
-
-    /**
-     * Function Cleanup
-     * calls curl_global_cleanup for the application. It must be used only after
-     * curl_global_init was called.
-     */
-    static void Cleanup();
-
-    /**
      * Function GetVersion
      * wrapper for curl_version(). Reports back a short string of loaded libraries.
      *
diff --git a/include/kicad_curl/kicad_curl_easy.h b/include/kicad_curl/kicad_curl_easy.h
index 80d427c..52c0206 100644
--- a/include/kicad_curl/kicad_curl_easy.h
+++ b/include/kicad_curl/kicad_curl_easy.h
@@ -46,6 +46,8 @@
 #include <curl/curl.h>
 #include <kicad_curl/kicad_curl.h>
 
+#include <boost/interprocess/sync/interprocess_mutex.hpp>
+
 
 /**
  * Class KICAD_CURL_EASY
@@ -179,6 +181,9 @@ private:
     CURL*           m_CURL;
     curl_slist*     m_headers;
     std::string     m_buffer;
+
+    bool            m_WorkaroundOpenSSL;
+    static boost::interprocess::interprocess_mutex s_WorkaroundOpenSSLMutex;
 };
 
 #endif // KICAD_CURL_EASY_H_