GitHub plugin: fix threading issues when libcurl is build against openssl.

* Dick Hollenbeck also contributed commit r6440.  I inadvertently forgot to set
  the Bazaar author tag before I committed it.  My apologies.
* Switch to static linking of libcurl and on linux and windows and also
  statically link in only required portions of openssl.
* Add the required thread locks which openssl needs.
* Remove the get curl version call from BASEFRAME since it pulls in curl and
  openssl into every derived wxFrame class link image.
* Remove curl function from PGM_BASE, switch to atexit() instead.  Anything in
  PGM_BASE made the singletops bigger.
* Tested on Linux, Windows, and OSX.

common/CMakeLists.txt

@@ -12,6 +12,12 @@ include_directories(
     ${INC_AFTER}
     )
 
+
+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(
@@ -285,7 +291,8 @@ add_dependencies( common lib-dependencies )
 add_dependencies( common version_header )
 target_link_libraries( common
     ${Boost_LIBRARIES}
-#    ${CURL_LIBRARIES}      we dynamically link to this ON DEMAND, not at load time
+    ${CURL_LIBRARIES}
+    ${OPENSSL_LIBRARIES}        # empty on Apple
     )
 
 

common/basicframe.cpp

@@ -28,7 +28,6 @@
  * @brief EDA_BASE_FRAME class implementation.
  */
 
-#include <kicad_curl/kicad_curl.h> /* Include before any wx file */
 #include <wx/aboutdlg.h>
 #include <wx/fontdlg.h>
 #include <wx/clipbrd.h>
@@ -573,8 +572,6 @@ void EDA_BASE_FRAME::CopyVersionInfoToClipboard( wxCommandEvent&  event )
                 << ( BOOST_VERSION / 100 % 1000 ) << wxT( "." )
                 << ( BOOST_VERSION % 100 ) << wxT( "\n" );
 
-    msg_version <<  KICAD_CURL::GetSimpleVersion() << wxT( "\n" );
-
     msg_version << wxT( "         USE_WX_GRAPHICS_CONTEXT=" );
 #ifdef USE_WX_GRAPHICS_CONTEXT
     msg_version << wxT( "ON\n" );

common/kicad_curl/kicad_curl.cpp

@@ -2,6 +2,7 @@
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
  * Copyright (C) 2015 Mark Roszko <mark.roszko@gmail.com>
+ * Copyright (C) 2016 SoftPLC Corporation, Dick Hollenbeck <dick@softplc.com>
  * Copyright (C) 2015 KiCad Developers, see CHANGELOG.TXT for contributors.
  *
  * This program is free software; you can redistribute it and/or
@@ -26,56 +27,106 @@
 #include <wx/dynlib.h>
 
 #include <macros.h>
+#include <fctsys.h>
 #include <kicad_curl/kicad_curl.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;
+static MUTEX s_lock;        // for s_initialized
 
+// Assume that on these platforms libcurl uses OpenSSL
+#if defined(__linux__) || defined(_WIN32)
 
-void        (CURL_EXTERN * KICAD_CURL::easy_cleanup)    ( CURL* curl );
+#include <openssl/crypto.h>
-CURL*       (CURL_EXTERN * KICAD_CURL::easy_init)       ( void );
-CURLcode    (CURL_EXTERN * KICAD_CURL::easy_perform)    ( CURL* curl );
-CURLcode    (CURL_EXTERN * KICAD_CURL::easy_setopt)     ( CURL* curl, CURLoption option, ... );
-const char* (CURL_EXTERN * KICAD_CURL::easy_strerror)   ( CURLcode );
-CURLcode    (CURL_EXTERN * KICAD_CURL::global_init)     ( long flags );
-void        (CURL_EXTERN * KICAD_CURL::global_cleanup)  ( void );
-curl_slist* (CURL_EXTERN * KICAD_CURL::slist_append)    ( curl_slist*, const char* );
-void        (CURL_EXTERN * KICAD_CURL::slist_free_all)  ( curl_slist* );
-char*       (CURL_EXTERN * KICAD_CURL::version)         ( void );
-curl_version_info_data* (CURL_EXTERN * KICAD_CURL::version_info) (CURLversion);
 
+static MUTEX* s_crypto_locks;
 
-struct DYN_LOOKUP
+static void lock_callback( int mode, int type, const char* file, int line )
 {
-    const char* name;
+    (void)file;
-    void**      address;
+    (void)line;
-};
 
-// May need to modify "name" for each platform according to how libcurl is built on
+    wxASSERT( s_crypto_locks && unsigned( type ) < unsigned( CRYPTO_num_locks() ) );
-// that platform and the spelling or partial mangling of C function names.  On linux
+
-// there is no mangling.
+    //DBG( printf( "%s: mode=0x%x type=%d file=%s line=%d\n", __func__, mode, type, file, line );)
-#define DYN_PAIR( basename )    { "curl_" #basename, (void**) &KICAD_CURL::basename }
+
+    if( mode & CRYPTO_LOCK )
+    {
+        s_crypto_locks[ type ].lock();
+    }
+    else
+    {
+        s_crypto_locks[ type ].unlock();
+    }
+}
 
 
-const DYN_LOOKUP KICAD_CURL::dyn_funcs[] = {
+static void init_locks()
-    DYN_PAIR( easy_cleanup ),
+{
-    DYN_PAIR( easy_init ),
+    s_crypto_locks = new MUTEX[ CRYPTO_num_locks() ];
-    DYN_PAIR( easy_perform ),
+
-    DYN_PAIR( easy_setopt ),
+    // From http://linux.die.net/man/3/crypto_set_id_callback:
-    DYN_PAIR( easy_strerror ),
+
-    DYN_PAIR( global_init ),
+    /*
-    DYN_PAIR( global_cleanup ),
+
-    DYN_PAIR( slist_append ),
+    OpenSSL can safely be used in multi-threaded applications provided that at
-    DYN_PAIR( slist_free_all ),
+    least two callback functions are set, locking_function and threadid_func.
-    DYN_PAIR( version ),
+
-    DYN_PAIR( version_info ),
+    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()
@@ -89,56 +140,14 @@ void KICAD_CURL::Init()
 
         if( !s_initialized )
         {
-            // dynamically load the library.
+            if( curl_global_init( CURL_GLOBAL_ALL ) != CURLE_OK )
-            wxDynamicLibrary dso;
-            wxString canonicalName = dso.CanonicalizeName( wxT( "curl" ) );
-
-            // This is an ugly hack for MinGW builds.  We should probably use something
-            // like objdump to get the actual library file name from the link file.
-#if defined( __MINGW32__ )
-            canonicalName = dso.CanonicalizeName( wxT( "curl-4" ) );
-            canonicalName = wxT( "lib" ) + canonicalName;
-#endif
-
-            if( !dso.Load( canonicalName, wxDL_NOW | wxDL_GLOBAL ) )
-            {
-                // Failure: error reporting UI was done via wxLogSysError().
-                std::string msg = StrPrintf( "%s not wxDynamicLibrary::Load()ed",
-                                             static_cast< const char *>( canonicalName.mb_str() ) );
-                THROW_IO_ERROR( msg );
-            }
-
-            // get addresses.
-
-            for( unsigned i=0; i < DIM(dyn_funcs); ++i )
-            {
-                *dyn_funcs[i].address = dso.GetSymbol( dyn_funcs[i].name );
-
-                if( *dyn_funcs[i].address == NULL )
-                {
-                    // Failure: error reporting UI was done via wxLogSysError().
-                    // No further reporting required here.
-
-                    std::string msg = StrPrintf( "%s has no function %s",
-                                                 static_cast<const char*>( canonicalName.mb_str() ),
-                                                 dyn_funcs[i].name );
-
-                    THROW_IO_ERROR( msg );
-                }
-            }
-
-            if( KICAD_CURL::global_init( CURL_GLOBAL_ALL ) != CURLE_OK )
             {
                 THROW_IO_ERROR( "curl_global_init() failed." );
             }
 
-            wxLogDebug( "Using %s", GetVersion() );
+            init_locks();
 
-            // Tell dso's wxDynamicLibrary destructor not to Unload() the program image,
+            wxLogDebug( "Using %s", GetVersion() );
-            // since everything is fine before this.  In those cases where THROW_IO_ERROR
-            // is called, dso is destroyed and the DSO/DLL is unloaded before returning in
-            // those error cases.
-            (void) dso.Detach();
 
             s_initialized = true;
         }
@@ -169,14 +178,11 @@ void KICAD_CURL::Cleanup()
 
         if( s_initialized )
         {
+            curl_global_cleanup();
 
-            KICAD_CURL::global_cleanup();
+            kill_locks();
 
-            // dyn_funcs are not good for anything now, assuming process is ending soon here.
+            atexit( &at_terminate );
-            for( unsigned i=0; i < DIM(dyn_funcs);  ++i )
-            {
-                *dyn_funcs[i].address = 0;
-            }
 
             s_initialized = false;
         }
@@ -189,7 +195,7 @@ std::string KICAD_CURL::GetSimpleVersion()
     if( !s_initialized )
         Init();
 
-    curl_version_info_data *info = KICAD_CURL::version_info( CURLVERSION_NOW );
+    curl_version_info_data* info = curl_version_info( CURLVERSION_NOW );
 
     std::string res;
 

common/kicad_curl/kicad_curl_easy.cpp

@@ -52,29 +52,24 @@ KICAD_CURL_EASY::KICAD_CURL_EASY() :
 
     KICAD_CURL::Init();
 
-    // Do not catch exception from KICAD_CURL::Init() at this level.
+    m_CURL = curl_easy_init();
-    // Instantiation of this instance will fail if Init() throws, thus ensuring
-    // that this instance cannot be subsequently used.
-    // Caller needs a try catch around KICAD_CURL_EASY instantiation.
-
-    m_CURL = KICAD_CURL::easy_init();
 
     if( !m_CURL )
     {
         THROW_IO_ERROR( "Unable to initialize CURL session" );
     }
 
-    KICAD_CURL::easy_setopt( m_CURL, CURLOPT_WRITEFUNCTION, write_callback );
+    curl_easy_setopt( m_CURL, CURLOPT_WRITEFUNCTION, write_callback );
-    KICAD_CURL::easy_setopt( m_CURL, CURLOPT_WRITEDATA, (void*) &m_buffer );
+    curl_easy_setopt( m_CURL, CURLOPT_WRITEDATA, (void*) &m_buffer );
 }
 
 
 KICAD_CURL_EASY::~KICAD_CURL_EASY()
 {
     if( m_headers )
-        KICAD_CURL::slist_free_all( m_headers );
+        curl_slist_free_all( m_headers );
 
-    KICAD_CURL::easy_cleanup( m_CURL );
+    curl_easy_cleanup( m_CURL );
 }
 
 
@@ -82,13 +77,13 @@ void KICAD_CURL_EASY::Perform()
 {
     if( m_headers )
     {
-        KICAD_CURL::easy_setopt( m_CURL, CURLOPT_HTTPHEADER, m_headers );
+        curl_easy_setopt( m_CURL, CURLOPT_HTTPHEADER, m_headers );
     }
 
     // bonus: retain worst case memory allocation, should re-use occur
     m_buffer.clear();
 
-    CURLcode res = KICAD_CURL::easy_perform( m_CURL );
+    CURLcode res = curl_easy_perform( m_CURL );
 
     if( res != CURLE_OK )
     {

common/pgm_base.cpp

@@ -30,7 +30,6 @@
  *        (locale handling)
  */
 
-#include <kicad_curl/kicad_curl.h> /* Include before any wx file */
 #include <fctsys.h>
 #include <wx/html/htmlwin.h>
 #include <wx/fs_zip.h>
@@ -289,8 +288,6 @@ void PGM_BASE::destroy()
 {
     // unlike a normal destructor, this is designed to be called more than once safely:
 
-    KICAD_CURL::Cleanup();
-
     delete m_common_settings;
     m_common_settings = 0;
 

include/kicad_curl/kicad_curl.h

@@ -92,7 +92,7 @@ public:
      */
     static const char* GetVersion()
     {
-        return KICAD_CURL::version();
+        return curl_version();
     }
 
 
@@ -103,26 +103,6 @@ public:
      * @return std::string - Generated version string
      */
     static std::string GetSimpleVersion();
-private:
-
-    // Alphabetically:
-    // dynamically looked up libcurl function pointers whose prototypes were
-    // taken from the system's libcurl headers.
-
-    static void         (CURL_EXTERN * easy_cleanup)    ( CURL* curl );
-    static CURL*        (CURL_EXTERN * easy_init)       ( void );
-    static CURLcode     (CURL_EXTERN * easy_perform)    ( CURL* curl );
-    static CURLcode     (CURL_EXTERN * easy_setopt)     ( CURL* curl, CURLoption option, ... );
-    static const char*  (CURL_EXTERN * easy_strerror)   ( CURLcode );
-    static CURLcode     (CURL_EXTERN * global_init)     ( long flags );
-    static void         (CURL_EXTERN * global_cleanup)  ( void );
-    static curl_slist*  (CURL_EXTERN * slist_append)    ( curl_slist*, const char* );
-    static void         (CURL_EXTERN * slist_free_all)  ( curl_slist* );
-    static char*        (CURL_EXTERN * version)         ( void );
-    static curl_version_info_data* (CURL_EXTERN * version_info) (CURLversion);
-
-    /// A tuple of ASCII function names and pointers to pointers to functions
-    static const DYN_LOOKUP dyn_funcs[];
 };
 
 #endif // KICAD_CURL_H_

include/kicad_curl/kicad_curl_easy.h

@@ -89,7 +89,7 @@ public:
     void SetHeader( const std::string& aName, const std::string& aValue )
     {
         std::string header = aName + ':' + aValue;
-        m_headers = KICAD_CURL::slist_append( m_headers, header.c_str() );
+        m_headers = curl_slist_append( m_headers, header.c_str() );
     }
 
     /**
@@ -150,7 +150,7 @@ public:
      */
     const std::string GetErrorText( CURLcode aCode )
     {
-        return KICAD_CURL::easy_strerror( aCode );
+        return curl_easy_strerror( aCode );
     }
 
     /**
@@ -163,7 +163,7 @@ public:
      */
     template <typename T> CURLcode SetOption( CURLoption aOption, T aArg )
     {
-        return KICAD_CURL::easy_setopt( m_CURL, aOption, aArg );
+        return curl_easy_setopt( m_CURL, aOption, aArg );
     }
 
     /**