From: Eric Leblond Date: Fri, 29 May 2020 09:24:31 +0000 (+0200) Subject: magic: get rid of global lock X-Git-Tag: suricata-6.0.0-beta1~402 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ae5650d443c77054623585d39d5f1e98d8a6bf24;p=thirdparty%2Fsuricata.git 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 529ae19cec..51ab4ba9e6 100644 --- 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 d7d93c3c58..975cd19bdc 100644 --- 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 9eccf48b60..957a1697dd 100644 --- 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 42b52d7b12..7a458737cc 100644 --- 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 8b92a84205..567fcd4cfe 100644 --- 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 69a4f1056f..20ba08bb88 100644 --- 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 a2ffc7bac5..86202dc5a6 100644 --- 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 /* _ */