GitList

Browse code

Make cl_load thread safe (bb #2333).

Parallel cl_load() crash (bb #2333).
Reason is twofold:
- cache.c had 2 'static' global variables, thus trying to initialize same cache
from multiple threads
- bytecode2llvm.cpp: something in LLVM 2.7 is crashing when loading in
parallel

Fix is to drop the 'static' on the variable (cache is per engine already).
This also fixes a potential memory leak in clamd!

The other part of the fix is to turn on the mutex around bytecode compilation
always. We don't call cl_load in parallel, so this doesn't affect clamd, but
some may need to call cl_load in parallel.

Török Edvin authored on 2010/11/05 04:14:14
Showing 4 changed files

ChangeLog index e1e6161..058f069 100644
libclamav/c++/bytecode2llvm.cpp index ff4eaa8..0859ad7 100644
libclamav/cache.c index aeb745a..3e947fc 100644
unit_tests/check_bytecode.c index cba3d4d..97427a5 100644

ChangeLog

History View file @ a7cf187

@@ -1,3 +1,7 @@
                     +Thu Nov  4 21:12:53 EET 2010 (edwin)
                     +------------------------------------
                     + * libclamav/cache.c,c++/bytecode2llvm.cpp}: make cl_load thread safe (bb #2333).
+                    +
                      Thu Nov  4 19:47:17 EET 2010 (edwin)
                      ------------------------------------
                       * freshclam: load database in subprocess (bb #2147).

libclamav/c++/bytecode2llvm.cpp

History View file @ a7cf187

@@ -1611,11 +1611,14 @@ class LLVMApiScopedLock {
                      	// we need to wrap all LLVM API calls with a giant mutex lock, but
                      	// only then.
                      	LLVMApiScopedLock() {
                     -	    if (!llvm_is_multithreaded())
                     +	    // It is safer to just run all codegen under the mutex,
                     +	    // it is not like we are going to codegen from multiple threads
                     +	    // at a time anyway.
                     +//	    if (!llvm_is_multithreaded())
                      		llvm_api_lock.acquire();
+                     	}
                      	~LLVMApiScopedLock() {
                     -	    if (!llvm_is_multithreaded())
                     +//	    if (!llvm_is_multithreaded())
                      		llvm_api_lock.release();
+                     	}
                      };

libclamav/cache.c

History View file @ a7cf187

@@ -589,7 +589,7 @@ struct CACHE {
                      /* Allocates the trees for the engine cache */
                      int cli_cache_init(struct cl_engine *engine) {
                     -    static struct CACHE *cache;
                     +    struct CACHE *cache;
                          unsigned int i, j;
                          if(!engine) {
@@ -623,7 +623,7 @@ int cli_cache_init(struct cl_engine *engine) {
                      /* Frees the engine cache */
                      void cli_cache_destroy(struct cl_engine *engine) {
                     -    static struct CACHE *cache;
                     +    struct CACHE *cache;
                          unsigned int i;
                          if(!engine || !(cache = engine->cache))

unit_tests/check_bytecode.c

History View file @ a7cf187

@@ -519,6 +519,43 @@ START_TEST (test_load_bytecode_int)
+                     }
                      END_TEST
                     +#ifdef CL_THREAD_SAFE
                     +static pthread_barrier_t barrier;
                     +static void* thread(void *arg)
                     +{
                     +    struct cl_engine *engine;
                     +    engine = cl_engine_new();
                     +    fail_unless(!!engine, "failed to create engine\n");
                     +    /* run all cl_load at once, to maximize chance of a crash
                     +     * in case of a race condition */
                     +    pthread_barrier_wait(&barrier);
                     +    runload("input/bytecode.cvd", engine, 5);
                     +    cl_engine_free(engine);
                     +    return NULL;
                     +}
+                    +
                     +START_TEST (test_parallel_load)
                     +{
                     +#define N 5
                     +    pthread_t threads[N];
                     +    unsigned i;
+                    +
                     +    cl_init(CL_INIT_DEFAULT);
                     +    pthread_barrier_init(&barrier, NULL, N);
                     +    for (i=0;i<N;i++) {
                     +	pthread_create(&threads[i], NULL, thread, NULL);
                     +    }
                     +    for (i=0;i<N;i++) {
                     +	pthread_join(threads[i], NULL);
                     +    }
                     +    /* DB load used to crash due to 'static' variable in cache.c,
                     +     * and also due to something wrong in LLVM 2.7.
                     +     * Enabled the mutex around codegen in bytecode2llvm.cpp, and this test is
                     +     * here to make sure it doesn't crash */
                     +}
                     +END_TEST
                     +#endif
+                    +
                      Suite *test_bytecode_suite(void)
+                     {
                          Suite *s = suite_create("bytecode");
@@ -575,6 +612,9 @@ Suite *test_bytecode_suite(void)
                          tcase_add_test(tc_cli_arith, test_load_bytecode_jit);
                          tcase_add_test(tc_cli_arith, test_load_bytecode_int);
                     +#ifdef CL_THREAD_SAFE
                     +    tcase_add_test(tc_cli_arith, test_parallel_load);
                     +#endif
                          return s;
+                     }