magic: get rid of global lock

Global magic context was involving a lock that appear to be really
costly for some traffic.

diff --git a/src/detect-filemagic.c b/src/detect-filemagic.c
index 529ae19cec71c57e7872e99769fe94b83c48282e..51ab4ba9e69882f8e206aab55feeac3444e2f6d3 100644 (file)
--- a/src/detect-filemagic.c
+++ b/src/detect-filemagic.c
@@ -164,38 +164,7 @@ void DetectFilemagicRegister(void)
  *  \retval -1 error
  *  \retval 0 ok
  */
-int FilemagicGlobalLookup(File *file)
-{
-    if (file == NULL || FileDataSize(file) == 0) {
-        SCReturnInt(-1);
-    }
-
-    const uint8_t *data = NULL;
-    uint32_t data_len = 0;
-    uint64_t offset = 0;
-
-    StreamingBufferGetData(file->sb,
-                           &data, &data_len, &offset);
-    if (offset == 0) {
-        if (FileDataSize(file) >= FILEMAGIC_MIN_SIZE) {
-            file->magic = MagicGlobalLookup(data, data_len);
-        } else if (file->state >= FILE_STATE_CLOSED) {
-            file->magic = MagicGlobalLookup(data, data_len);
-        }
-    }
-
-    SCReturnInt(0);
-}
-
-/**
- *  \brief run the magic check
- *
- *  \param file the file
- *
- *  \retval -1 error
- *  \retval 0 ok
- */
-static int FilemagicThreadLookup(magic_t *ctx, File *file)
+int FilemagicThreadLookup(magic_t *ctx, File *file)
 {
     if (ctx == NULL || file == NULL || FileDataSize(file) == 0) {
         SCReturnInt(-1);
@@ -340,43 +309,15 @@ error:
 
 static void *DetectFilemagicThreadInit(void *data /*@unused@*/)
 {
-    const char *filename = NULL;
-    FILE *fd = NULL;
-
     DetectFilemagicThreadData *t = SCCalloc(1, sizeof(DetectFilemagicThreadData));
     if (unlikely(t == NULL)) {
         SCLogError(SC_ERR_MEM_ALLOC, "couldn't alloc ctx memory");
         return NULL;
     }
 
-    t->ctx = magic_open(0);
-    if (t->ctx == NULL) {
-        SCLogError(SC_ERR_MAGIC_OPEN, "magic_open failed: %s", magic_error(t->ctx));
+    t->ctx = MagicInitContext();
+    if (t->ctx == NULL)
         goto error;
-    }
-
-    (void)ConfGet("magic-file", &filename);
-    if (filename != NULL) {
-        if (strlen(filename) == 0) {
-            /* set filename to NULL on *nix systems so magic_load uses system default path (see man libmagic) */
-            SCLogInfo("using system default magic-file");
-            filename = NULL;
-        }
-        else {
-            SCLogInfo("using magic-file %s", filename);
-
-            if ( (fd = fopen(filename, "r")) == NULL) {
-                SCLogWarning(SC_ERR_FOPEN, "Error opening file: \"%s\": %s", filename, strerror(errno));
-                goto error;
-            }
-            fclose(fd);
-        }
-    }
-
-    if (magic_load(t->ctx, filename) != 0) {
-        SCLogError(SC_ERR_MAGIC_LOAD, "magic_load failed: %s", magic_error(t->ctx));
-        goto error;
-    }
 
     return (void *)t;
 

diff --git a/src/detect-filemagic.h b/src/detect-filemagic.h
index d7d93c3c58993daafb8d7f16f50221081d1ae88e..975cd19bdcea1e25f622308c9a6fb449c65b02a9 100644 (file)
--- a/src/detect-filemagic.h
+++ b/src/detect-filemagic.h
@@ -39,7 +39,7 @@ typedef struct DetectFilemagicData {
 } DetectFilemagicData;
 
 /* prototypes */
-int FilemagicGlobalLookup(File *file);
+int FilemagicThreadLookup(magic_t *ctx, File *file);
 #endif
 void DetectFilemagicRegister (void);
 

diff --git a/src/output-file.c b/src/output-file.c
index 9eccf48b60d6316208542922b8a98d3462f37b80..957a1697dd05d4fb5be9a024504b8c2e44056198 100644 (file)
--- a/src/output-file.c
+++ b/src/output-file.c
@@ -32,6 +32,7 @@
 #include "detect-filemagic.h"
 #include "util-profiling.h"
 #include "util-validate.h"
+#include "util-magic.h"
 
 typedef struct OutputLoggerThreadStore_ {
     void *thread_data;
@@ -42,6 +43,9 @@ typedef struct OutputLoggerThreadStore_ {
  *  data for the packet loggers. */
 typedef struct OutputLoggerThreadData_ {
     OutputLoggerThreadStore *store;
+#ifdef HAVE_MAGIC
+    magic_t magic_ctx;
+#endif
 } OutputLoggerThreadData;
 
 /* logger instance, a module + a output ctx,
@@ -118,7 +122,7 @@ static void OutputFileLogFfc(ThreadVars *tv,
                 bool file_logged = false;
 #ifdef HAVE_MAGIC
                 if (FileForceMagic() && ff->magic == NULL) {
-                    FilemagicGlobalLookup(ff);
+                    FilemagicThreadLookup(&op_thread_data->magic_ctx, ff);
                 }
 #endif
                 const OutputFileLogger *logger = list;
@@ -191,6 +195,14 @@ static TmEcode OutputFileLogThreadInit(ThreadVars *tv, const void *initdata, voi
 
     *data = (void *)td;
 
+#ifdef HAVE_MAGIC
+    td->magic_ctx = MagicInitContext();
+    if (td->magic_ctx == NULL) {
+        SCFree(td);
+        return TM_ECODE_FAILED;
+    }
+#endif
+
     SCLogDebug("OutputFileLogThreadInit happy (*data %p)", *data);
 
     OutputFileLogger *logger = list;
@@ -241,6 +253,10 @@ static TmEcode OutputFileLogThreadDeinit(ThreadVars *tv, void *thread_data)
         logger = logger->next;
     }
 
+#ifdef HAVE_MAGIC
+    MagicDeinitContext(op_thread_data->magic_ctx);
+#endif
+
     SCFree(op_thread_data);
     return TM_ECODE_OK;
 }

diff --git a/src/output-filedata.c b/src/output-filedata.c
index 42b52d7b126625dc6b3abf5d99ca1e51ec516190..7a458737cc5d08b36dcada729487a86c0d265475 100644 (file)
--- a/src/output-filedata.c
+++ b/src/output-filedata.c
@@ -33,6 +33,7 @@
 #include "conf.h"
 #include "util-profiling.h"
 #include "util-validate.h"
+#include "util-magic.h"
 
 typedef struct OutputLoggerThreadStore_ {
     void *thread_data;
@@ -43,6 +44,9 @@ typedef struct OutputLoggerThreadStore_ {
  *  data for the packet loggers. */
 typedef struct OutputLoggerThreadData_ {
     OutputLoggerThreadStore *store;
+#ifdef HAVE_MAGIC
+    magic_t magic_ctx;
+#endif
 } OutputLoggerThreadData;
 
 /* logger instance, a module + a output ctx,
@@ -125,17 +129,18 @@ static int CallLoggers(ThreadVars *tv, OutputLoggerThreadStore *store_list,
     return file_logged;
 }
 
-static void OutputFiledataLogFfc(ThreadVars *tv, OutputLoggerThreadStore *store,
+static void OutputFiledataLogFfc(ThreadVars *tv, OutputLoggerThreadData *td,
         Packet *p, FileContainer *ffc, const uint8_t call_flags,
         const bool file_close, const bool file_trunc, const uint8_t dir)
 {
     if (ffc != NULL) {
+        OutputLoggerThreadStore *store = td->store;
         File *ff;
         for (ff = ffc->head; ff != NULL; ff = ff->next) {
             uint8_t file_flags = call_flags;
 #ifdef HAVE_MAGIC
             if (FileForceMagic() && ff->magic == NULL) {
-                FilemagicGlobalLookup(ff);
+                FilemagicThreadLookup(&td->magic_ctx, ff);
             }
 #endif
             SCLogDebug("ff %p", ff);
@@ -213,7 +218,6 @@ static TmEcode OutputFiledataLog(ThreadVars *tv, Packet *p, void *thread_data)
     }
 
     OutputLoggerThreadData *op_thread_data = (OutputLoggerThreadData *)thread_data;
-    OutputLoggerThreadStore *store = op_thread_data->store;
 
     /* no flow, no files */
     Flow * const f = p->flow;
@@ -230,9 +234,9 @@ static TmEcode OutputFiledataLog(ThreadVars *tv, Packet *p, void *thread_data)
     FileContainer *ffc_ts = AppLayerParserGetFiles(f, STREAM_TOSERVER);
     FileContainer *ffc_tc = AppLayerParserGetFiles(f, STREAM_TOCLIENT);
     SCLogDebug("ffc_ts %p", ffc_ts);
-    OutputFiledataLogFfc(tv, store, p, ffc_ts, STREAM_TOSERVER, file_close_ts, file_trunc, STREAM_TOSERVER);
+    OutputFiledataLogFfc(tv, op_thread_data, p, ffc_ts, STREAM_TOSERVER, file_close_ts, file_trunc, STREAM_TOSERVER);
     SCLogDebug("ffc_tc %p", ffc_tc);
-    OutputFiledataLogFfc(tv, store, p, ffc_tc, STREAM_TOCLIENT, file_close_tc, file_trunc, STREAM_TOCLIENT);
+    OutputFiledataLogFfc(tv, op_thread_data, p, ffc_tc, STREAM_TOCLIENT, file_close_tc, file_trunc, STREAM_TOCLIENT);
 
     return TM_ECODE_OK;
 }
@@ -304,6 +308,14 @@ static TmEcode OutputFiledataLogThreadInit(ThreadVars *tv, const void *initdata,
 
     *data = (void *)td;
 
+#ifdef HAVE_MAGIC
+    td->magic_ctx = MagicInitContext();
+    if (td->magic_ctx == NULL) {
+        SCFree(td);
+        return TM_ECODE_FAILED;
+    }
+#endif
+
     SCLogDebug("OutputFiledataLogThreadInit happy (*data %p)", *data);
 
     OutputFiledataLogger *logger = list;
@@ -413,6 +425,10 @@ static TmEcode OutputFiledataLogThreadDeinit(ThreadVars *tv, void *thread_data)
     }
     SCMutexUnlock(&g_waldo_mutex);
 
+#ifdef HAVE_MAGIC
+    MagicDeinitContext(op_thread_data->magic_ctx);
+#endif
+
     SCFree(op_thread_data);
     return TM_ECODE_OK;
 }

diff --git a/src/suricata.c b/src/suricata.c
index 8b92a842053a24c4d81cdf5a1001fab5e4134d75..567fcd4cfee3e6e959d59e8fd46ef51df4756648 100644 (file)
--- a/src/suricata.c
+++ b/src/suricata.c
@@ -350,9 +350,6 @@ static void GlobalsDestroy(SCInstance *suri)
         SCReferenceConfDeinit();
         SCClassConfDeinit();
     }
-#ifdef HAVE_MAGIC
-    MagicDeinit();
-#endif
     TmqhCleanup();
     TmModuleRunDeInit();
     ParseSizeDeinit();
@@ -2566,10 +2563,6 @@ int PostConfLoadedSetup(SCInstance *suri)
     }
 
     HostInitConfig(HOST_VERBOSE);
-#ifdef HAVE_MAGIC
-    if (MagicInit() != 0)
-        SCReturnInt(TM_ECODE_FAILED);
-#endif
     SCAsn1LoadConfig();
 
     CoredumpLoadConfig();

diff --git a/src/util-magic.c b/src/util-magic.c
index 69a4f1056f8ec510f6581bc541e3aac1e4758cea..20ba08bb8807746488d95d4a2c8d3d646e8762aa 100644 (file)
--- a/src/util-magic.c
+++ b/src/util-magic.c
@@ -35,28 +35,20 @@
 #include "util-magic.h"
 
 #ifdef HAVE_MAGIC
-static magic_t g_magic_ctx = NULL;
-static SCMutex g_magic_lock;
 
 /**
- *  \brief Initialize the "magic" context.
+ * \brief Initialize a "magic" context.
  */
-int MagicInit(void)
+magic_t MagicInitContext(void)
 {
-    BUG_ON(g_magic_ctx != NULL);
-
-    SCEnter();
-
+    magic_t ctx;
     const char *filename = NULL;
     FILE *fd = NULL;
 
-    SCMutexInit(&g_magic_lock, NULL);
-    SCMutexLock(&g_magic_lock);
-
-    g_magic_ctx = magic_open(0);
-    if (g_magic_ctx == NULL) {
+    ctx = magic_open(0);
+    if (ctx == NULL) {
         SCLogError(SC_ERR_MAGIC_OPEN, "magic_open failed: %s",
-                magic_error(g_magic_ctx));
+                magic_error(ctx));
         goto error;
     }
 
@@ -82,52 +74,26 @@ int MagicInit(void)
         }
     }
 
-    if (magic_load(g_magic_ctx, filename) != 0) {
+    if (magic_load(ctx, filename) != 0) {
         SCLogError(SC_ERR_MAGIC_LOAD, "magic_load failed: %s",
-                magic_error(g_magic_ctx));
+                magic_error(ctx));
         goto error;
     }
-
-    SCMutexUnlock(&g_magic_lock);
-    SCReturnInt(0);
+    return ctx;
 
 error:
-    if (g_magic_ctx != NULL) {
-        magic_close(g_magic_ctx);
-        g_magic_ctx = NULL;
+    if (ctx != NULL) {
+        magic_close(ctx);
+        ctx = NULL;
     }
-
-    SCMutexUnlock(&g_magic_lock);
-    SCReturnInt(-1);
+    return NULL;
 }
 
-/**
- *  \brief Find the magic value for a buffer.
- *
- *  \param buf the buffer
- *  \param buflen length of the buffer
- *
- *  \retval result pointer to null terminated string
- */
-char *MagicGlobalLookup(const uint8_t *buf, uint32_t buflen)
-{
-    const char *result = NULL;
-    char *magic = NULL;
-
-    SCMutexLock(&g_magic_lock);
-
-    if (buf != NULL && buflen > 0) {
-        result = magic_buffer(g_magic_ctx, (void *)buf, (size_t)buflen);
-        if (result != NULL) {
-            magic = SCStrdup(result);
-            if (unlikely(magic == NULL)) {
-                SCLogError(SC_ERR_MEM_ALLOC, "Unable to dup magic");
-            }
-        }
-    }
 
-    SCMutexUnlock(&g_magic_lock);
-    SCReturnPtr(magic, "const char");
+void MagicDeinitContext(magic_t ctx)
+{
+    if (ctx != NULL)
+        magic_close(ctx);
 }
 
 /**
@@ -156,17 +122,6 @@ char *MagicThreadLookup(magic_t *ctx, const uint8_t *buf, uint32_t buflen)
     SCReturnPtr(magic, "const char");
 }
 
-void MagicDeinit(void)
-{
-    SCMutexLock(&g_magic_lock);
-    if (g_magic_ctx != NULL) {
-        magic_close(g_magic_ctx);
-        g_magic_ctx = NULL;
-    }
-    SCMutexUnlock(&g_magic_lock);
-    SCMutexDestroy(&g_magic_lock);
-}
-
 #ifdef UNITTESTS
 
 #if defined OS_FREEBSD || defined OS_DARWIN
@@ -198,34 +153,6 @@ static int MagicInitTest01(void)
     return result;
 }
 
-/** \test magic init through api */
-static int MagicInitTest02(void)
-{
-    if (g_magic_ctx != NULL) {
-        printf("g_magic_ctx != NULL at start of the test: ");
-        return 0;
-    }
-
-    if (MagicInit() < 0) {
-        printf("MagicInit() failure\n");
-        return 0;
-    }
-
-    if (g_magic_ctx == NULL) {
-        printf("g_magic_ctx == NULL: ");
-        return 0;
-    }
-
-    MagicDeinit();
-
-    if (g_magic_ctx != NULL) {
-        printf("g_magic_ctx != NULL at end of the test: ");
-        return 0;
-    }
-
-    return 1;
-}
-
 /** \test magic lib calls -- lookup */
 static int MagicDetectTest01(void)
 {
@@ -412,20 +339,21 @@ end:
     return retval;
 }
 
+
 /** \test magic api calls -- lookup */
 static int MagicDetectTest05(void)
 {
     const char *result = NULL;
+    magic_t ctx = NULL;
     uint8_t buffer[] = { 0x25, 'P', 'D', 'F', '-', '1', '.', '3', 0x0d, 0x0a};
     size_t buffer_len = sizeof(buffer);
     int retval = 0;
 
-    if (MagicInit() < 0) {
-        printf("MagicInit() failure\n");
-        return 0;
-    }
 
-    result = MagicGlobalLookup(buffer, buffer_len);
+    ctx = MagicInitContext();
+    FAIL_IF(ctx == NULL);
+
+    result = MagicThreadLookup(&ctx, buffer, buffer_len);
     if (result == NULL || strncmp(result, "PDF document", 12) != 0) {
         printf("result %p:%s, not \"PDF document\": ", result,result?result:"(null)");
         goto end;
@@ -433,9 +361,10 @@ static int MagicDetectTest05(void)
 
     retval = 1;
 end:
-    MagicDeinit();
+    MagicDeinitContext(ctx);
     return retval;
 }
+
 #if 0
 /** \test magic api calls -- lookup */
 static int MagicDetectTest06(void)
@@ -482,6 +411,7 @@ end:
 static int MagicDetectTest07(void)
 {
     const char *result = NULL;
+    magic_t ctx = NULL;
     uint8_t buffer[] = {
         0x50, 0x4b, 0x03, 0x04, 0x14, 0x00, 0x00, 0x00,
         0x00, 0x00, 0x0b, 0x55, 0x2a, 0x36, 0x5e, 0xc6,
@@ -508,15 +438,16 @@ static int MagicDetectTest07(void)
     };
     size_t buffer_len = sizeof(buffer);
 
-    FAIL_IF(MagicInit() < 0);
+    ctx = MagicInitContext();
+    FAIL_IF(ctx == NULL);
 
-    result = MagicGlobalLookup(buffer, buffer_len);
+    result = MagicThreadLookup(&ctx, buffer, buffer_len);
     FAIL_IF_NULL(result);
 
     char *str = strstr(result, "OpenDocument Text");
     FAIL_IF_NULL(str);
 
-    MagicDeinit();
+    MagicDeinitContext(ctx);
     PASS;
 }
 
@@ -524,6 +455,7 @@ static int MagicDetectTest07(void)
 static int MagicDetectTest08(void)
 {
     const char *result = NULL;
+    magic_t ctx = NULL;
     uint8_t buffer[] = {
         0x50, 0x4b, 0x03, 0x04, 0x14, 0x00, 0x00, 0x08,
         0x00, 0x00, 0x52, 0x7b, 0x86, 0x3c, 0x8b, 0x70,
@@ -557,12 +489,10 @@ static int MagicDetectTest08(void)
     size_t buffer_len = sizeof(buffer);
     int retval = 0;
 
-    if (MagicInit() < 0) {
-        printf("MagicInit() failure\n");
-        return 0;
-    }
+    ctx = MagicInitContext();
+    FAIL_IF(ctx == NULL);
 
-    result = MagicGlobalLookup(buffer, buffer_len);
+    result = MagicThreadLookup(&ctx, buffer, buffer_len);
     if (result == NULL || strncmp(result, "OpenOffice.org 1.x", 18) != 0) {
         printf("result %p:%s, not \"OpenOffice.org 1.x\": ", result,result?result:"(null)");
         goto end;
@@ -570,7 +500,7 @@ static int MagicDetectTest08(void)
 
     retval = 1;
 end:
-    MagicDeinit();
+    MagicDeinitContext(ctx);
     return retval;
 }
 #if 0
@@ -616,6 +546,7 @@ end:
 static int MagicDetectTest10ValgrindError(void)
 {
     const char *result = NULL;
+    magic_t ctx = NULL;
     uint8_t buffer[] = {
         0xFF,0xD8,0xFF,0xE0,0x00,0x10,0x4A,0x46,0x49,0x46,0x00,0x01,0x01,0x01,0x01,0x2C,
         0x01,0x2C,0x00,0x00,0xFF,0xFE,0x00,0x4C,0x53,0x69,0x67,0x6E,0x61,0x74,0x75,0x72,
@@ -632,12 +563,11 @@ static int MagicDetectTest10ValgrindError(void)
     size_t buffer_len = sizeof(buffer);
     int retval = 0;
 
-    if (MagicInit() < 0) {
-        printf("MagicInit() failure\n");
-        return 0;
-    }
 
-    result = MagicGlobalLookup(buffer, buffer_len);
+    ctx = MagicInitContext();
+    FAIL_IF(ctx == NULL);
+
+    result = MagicThreadLookup(&ctx, buffer, buffer_len);
     if (result == NULL || strncmp(result, "JPEG", 4) != 0) {
         printf("result %p:%s, not \"JPEG\": ", result,result?result:"(null)");
         goto end;
@@ -645,7 +575,7 @@ static int MagicDetectTest10ValgrindError(void)
 
     retval = 1;
 end:
-    MagicDeinit();
+    MagicDeinitContext(ctx);
     return retval;
 }
 
@@ -657,7 +587,6 @@ void MagicRegisterTests(void)
 #ifdef HAVE_MAGIC
 #ifdef UNITTESTS
     UtRegisterTest("MagicInitTest01", MagicInitTest01);
-    UtRegisterTest("MagicInitTest02", MagicInitTest02);
     UtRegisterTest("MagicDetectTest01", MagicDetectTest01);
     //UtRegisterTest("MagicDetectTest02", MagicDetectTest02, 1);
     UtRegisterTest("MagicDetectTest03", MagicDetectTest03);

diff --git a/src/util-magic.h b/src/util-magic.h
index a2ffc7bac55077224ab9c58e695852cb1c5c125d..86202dc5a6b7a11c7ac61efaccd769f9497a388a 100644 (file)
--- a/src/util-magic.h
+++ b/src/util-magic.h
@@ -25,11 +25,11 @@
 #define __UTIL_MAGIC_H__
 
 #ifdef HAVE_MAGIC
-int MagicInit(void);
-void MagicDeinit(void);
-char *MagicGlobalLookup(const uint8_t *, uint32_t);
+magic_t MagicInitContext(void);
+void MagicDeinitContext(magic_t ctx);
+
 char *MagicThreadLookup(magic_t *, const uint8_t *, uint32_t);
 #endif
 void MagicRegisterTests(void);
 
-#endif /* __UTIL_MAGIC_H__ */
+#endif /* _ */