From: Jeremy Harris Date: Sun, 14 Mar 2021 17:25:11 +0000 (+0000) Subject: DKIM: verify using separate pool-pair, reset per message X-Git-Tag: exim-4.95-RC0~105 X-Git-Url: https://git.exim.org/exim.git/commitdiff_plain/ee3c2fea18d0c940c2256c6bf041f546c703c375 DKIM: verify using separate pool-pair, reset per message --- diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index 6993499e9..c1f6f9b6d 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -213,6 +213,11 @@ JH/44 Bug 2701: Fix list-expansion of dns_ipv4_lookup. Previously, it did dnssec_require_domains, dnssec_request_domains, srv_fail_domains, mx_fail_domains. +JH/45 Use a (new) separate store pool-pair for DKIM verify working data. + Previously the permanent pool was used, so the sore could not be freed. + This meant a connection with many messages would use continually-growing + memory. + Exim version 4.94 ----------------- diff --git a/src/exim_monitor/em_main.c b/src/exim_monitor/em_main.c index 88bf1fcb7..0a89fbb38 100644 --- a/src/exim_monitor/em_main.c +++ b/src/exim_monitor/em_main.c @@ -570,7 +570,8 @@ return ret; * Initialize * *************************************************/ -int main(int argc, char **argv) +int +main(int argc, char **argv) { int i; struct stat statdata; @@ -591,6 +592,7 @@ message_subdir[1] = 0; constructing file names and things. This call will initialize the store_get() function. */ +store_init(); big_buffer = store_get(big_buffer_size, FALSE); /* Set up the version string and date and output them */ diff --git a/src/src/dkim.c b/src/src/dkim.c index 92adb3589..a48f1a17a 100644 --- a/src/src/dkim.c +++ b/src/src/dkim.c @@ -109,12 +109,15 @@ dkim_exim_verify_init(BOOL dot_stuffing) { dkim_exim_init(); -/* There is a store-reset between header & body reception -so cannot use the main pool. Any allocs done by Exim -memory-handling must use the perm pool. */ +/* There is a store-reset between header & body reception for the main pool +(actually, after every header line) so cannot use that as we need the data we +store per-header, during header processing, at the end of body reception +for evaluating the signature. Any allocs done for dkim verify +memory-handling must use a different pool. We use a separate one that we +can reset per message. */ dkim_verify_oldpool = store_pool; -store_pool = POOL_PERM; +store_pool = POOL_MESSAGE; /* Free previous context if there is one */ @@ -139,7 +142,7 @@ dkim_exim_verify_feed(uschar * data, int len) { int rc; -store_pool = POOL_PERM; +store_pool = POOL_MESSAGE; if ( dkim_collect_input && (rc = pdkim_feed(dkim_verify_ctx, data, len)) != PDKIM_OK) { @@ -305,7 +308,7 @@ int rc; gstring * g = NULL; const uschar * errstr = NULL; -store_pool = POOL_PERM; +store_pool = POOL_MESSAGE; /* Delete eventual previous signature chain */ diff --git a/src/src/exim.c b/src/src/exim.c index 8f33bde26..cb11a2a38 100644 --- a/src/src/exim.c +++ b/src/src/exim.c @@ -1669,6 +1669,8 @@ extern char **environ; (void)gettimeofday(×tamp_startup, NULL); #endif +store_init(); /* Initialise the memory allocation susbsystem */ + /* If the Exim user and/or group and/or the configuration file owner/group were defined by ref:name at build time, we must now find the actual uid/gid values. This is a feature to make the lives of binary distributors easier. */ diff --git a/src/src/exim_dbmbuild.c b/src/src/exim_dbmbuild.c index 7cf2e4775..da8d5707a 100644 --- a/src/src/exim_dbmbuild.c +++ b/src/src/exim_dbmbuild.c @@ -45,7 +45,7 @@ void * store_get_3(int size, BOOL tainted, const char *filename, int linenumber) { return NULL; } void ** -store_reset_3(void **ptr, int pool, const char *filename, int linenumber) +store_reset_3(void **ptr, const char *filename, int linenumber) { return NULL; } void store_release_above_3(void *ptr, const char *func, int linenumber) diff --git a/src/src/exim_dbutil.c b/src/src/exim_dbutil.c index 7ca7a3073..2ae7ef44d 100644 --- a/src/src/exim_dbutil.c +++ b/src/src/exim_dbutil.c @@ -551,6 +551,8 @@ EXIM_CURSOR *cursor; uschar **argv = USS cargv; uschar keybuffer[1024]; +store_init(); + /* Check the arguments, and open the database */ dbdata_type = check_args(argc, argv, US"dumpdb", US""); @@ -765,6 +767,7 @@ uschar buffer[256]; uschar name[256]; rmark reset_point; +store_init(); name[0] = 0; /* No name set */ /* Sort out the database type, verify what we are working on and then process @@ -1134,6 +1137,8 @@ uschar **argv = USS cargv; uschar buffer[256]; uschar *key; +store_init(); + /* Scan the options */ for (i = 1; i < argc; i++) diff --git a/src/src/expand.c b/src/src/expand.c index 8be7bf97b..8a571b2d9 100644 --- a/src/src/expand.c +++ b/src/src/expand.c @@ -8641,6 +8641,7 @@ debug_selector = D_v; debug_file = stderr; debug_fd = fileno(debug_file); big_buffer = malloc(big_buffer_size); +store_init(); for (int i = 1; i < argc; i++) { diff --git a/src/src/functions.h b/src/src/functions.h index 5ffb23d1e..6d16e69c2 100644 --- a/src/src/functions.h +++ b/src/src/functions.h @@ -340,6 +340,9 @@ extern int match_isinlist(const uschar *, const uschar **, int, tree_node ** unsigned int *, int, BOOL, const uschar **); extern int match_check_string(const uschar *, const uschar *, int, BOOL, BOOL, BOOL, const uschar **); + +extern void message_start(void); +extern void message_tidyup(void); extern void md5_end(md5 *, const uschar *, int, uschar *); extern void md5_mid(md5 *, const uschar *); extern void md5_start(md5 *); @@ -522,6 +525,8 @@ extern int stdin_ferror(void); extern int stdin_ungetc(int); extern void store_exit(void); +extern void store_init(void); + extern gstring *string_append(gstring *, int, ...) WARN_UNUSED_RESULT; extern gstring *string_append_listele(gstring *, uschar, const uschar *) WARN_UNUSED_RESULT; extern gstring *string_append_listele_n(gstring *, uschar, const uschar *, unsigned) WARN_UNUSED_RESULT; diff --git a/src/src/host.c b/src/src/host.c index 6b9f674b8..c25ff2a74 100644 --- a/src/src/host.c +++ b/src/src/host.c @@ -3260,6 +3260,7 @@ uschar buffer[256]; disable_ipv6 = FALSE; primary_hostname = US""; +store_init(); store_pool = POOL_MAIN; debug_selector = D_host_lookup|D_interface; debug_file = stdout; diff --git a/src/src/parse.c b/src/src/parse.c index 2b2ffd341..885a01c0d 100644 --- a/src/src/parse.c +++ b/src/src/parse.c @@ -2083,6 +2083,7 @@ int main(void) int start, end, domain; uschar buffer[1024]; +store_init(); big_buffer = store_malloc(big_buffer_size); /* strip_trailing_dot = TRUE; */ diff --git a/src/src/smtp_in.c b/src/src/smtp_in.c index 03085c9e4..6d6370ffd 100644 --- a/src/src/smtp_in.c +++ b/src/src/smtp_in.c @@ -2109,7 +2109,11 @@ while (acl_warn_logged) acl_warn_logged = acl_warn_logged->next; store_free(this); } + +message_tidyup(); store_reset(reset_point); + +message_start(); return store_mark(); } @@ -2186,7 +2190,7 @@ while (done <= 0) case MAIL_CMD: smtp_mailcmd_count++; /* Count for no-mail log */ - if (sender_address != NULL) + if (sender_address) /* The function moan_smtp_batch() does not return. */ moan_smtp_batch(smtp_cmd_buffer, "503 Sender already given"); @@ -4533,6 +4537,7 @@ while (done <= 0) case MAIL_CMD: HAD(SCH_MAIL); smtp_mailcmd_count++; /* Count for limit and ratelimit */ + message_start(); was_rej_mail = TRUE; /* Reset if accepted */ env_mail_type_t * mail_args; /* Sanity check & validate args */ diff --git a/src/src/store.c b/src/src/store.c index df7078fea..9d155821b 100644 --- a/src/src/store.c +++ b/src/src/store.c @@ -37,11 +37,15 @@ The following different types of store are recognized: This means it can be freed when search_tidyup() is called to close down all the lookup caching. +- There is another pool (POOL_MESSAGE) used for medium-lifetime objects; within + a single message transaction but needed for longer than the use of the main + pool permits. Currently this means only receive-time DKIM information. + . Orthogonal to the three pool types, there are two classes of memory: untainted and tainted. The latter is used for values derived from untrusted input, and the string-expansion mechanism refuses to operate on such values (obviously, it can expand an untainted value to return a tainted result). The classes - are implemented by duplicating the three pool types. Pool resets are requested + are implemented by duplicating the four pool types. Pool resets are requested against the nontainted sibling and apply to both siblings. Only memory blocks requested for tainted use are regarded as tainted; anything @@ -113,11 +117,10 @@ even if the length is zero (which is used for getting a point to reset to). */ int store_pool = POOL_MAIN; -#define NPOOLS 6 static storeblock *chainbase[NPOOLS]; static storeblock *current_block[NPOOLS]; static void *next_yield[NPOOLS]; -static int yield_length[NPOOLS] = { -1, -1, -1, -1, -1, -1 }; +static int yield_length[NPOOLS]; /* pool_malloc holds the amount of memory used by the store pools; this goes up and down as store is reset or released. nonpool_malloc is the total got by @@ -150,17 +153,22 @@ static const uschar * pooluse[NPOOLS] = { [POOL_MAIN] = US"main", [POOL_PERM] = US"perm", [POOL_SEARCH] = US"search", +[POOL_MESSAGE] = US"message", [POOL_TAINT_MAIN] = US"main", [POOL_TAINT_PERM] = US"perm", [POOL_TAINT_SEARCH] = US"search", +[POOL_TAINT_SEARCH] = US"search", +[POOL_TAINT_MESSAGE] = US"message", }; static const uschar * poolclass[NPOOLS] = { [POOL_MAIN] = US"untainted", [POOL_PERM] = US"untainted", [POOL_SEARCH] = US"untainted", +[POOL_MESSAGE] = US"untainted", [POOL_TAINT_MAIN] = US"tainted", [POOL_TAINT_PERM] = US"tainted", [POOL_TAINT_SEARCH] = US"tainted", +[POOL_TAINT_MESSAGE] = US"tainted", }; #endif @@ -168,6 +176,16 @@ static const uschar * poolclass[NPOOLS] = { static void * internal_store_malloc(int, const char *, int); static void internal_store_free(void *, const char *, int linenumber); +/******************************************************************************/ +/* Initialisation, for things fragile with parameter channges when using +static initialisers. */ + +void +store_init(void) +{ +for (int i = 0; i < NPOOLS; i++) yield_length[i] = -1; +} + /******************************************************************************/ /* Test if a pointer refers to tainted memory. @@ -526,19 +544,19 @@ DEBUG(D_memory) rmark -store_reset_3(rmark r, int pool, const char *func, int linenumber) +store_reset_3(rmark r, const char *func, int linenumber) { void ** ptr = r; -if (pool >= POOL_TAINT_BASE) +if (store_pool >= POOL_TAINT_BASE) log_write(0, LOG_MAIN|LOG_PANIC_DIE, - "store_reset called for pool %d: %s %d\n", pool, func, linenumber); + "store_reset called for pool %d: %s %d\n", store_pool, func, linenumber); if (!r) log_write(0, LOG_MAIN|LOG_PANIC_DIE, "store_reset called with bad mark: %s %d\n", func, linenumber); -internal_store_reset(*ptr, pool + POOL_TAINT_BASE, func, linenumber); -internal_store_reset(ptr, pool, func, linenumber); +internal_store_reset(*ptr, store_pool + POOL_TAINT_BASE, func, linenumber); +internal_store_reset(ptr, store_pool, func, linenumber); return NULL; } @@ -837,4 +855,29 @@ DEBUG(D_memory) #endif } + +/******************************************************************************/ +/* Per-message pool management */ + +static rmark message_reset_point = NULL; + +void +message_start(void) +{ +int oldpool = store_pool; +store_pool = POOL_MESSAGE; +if (!message_reset_point) message_reset_point = store_mark(); +store_pool = oldpool; +} + +void message_tidyup(void) +{ +int oldpool; +if (!message_reset_point) return; +oldpool = store_pool; +store_pool = POOL_MESSAGE; +message_reset_point = store_reset(message_reset_point); +store_pool = oldpool; +} + /* End of store.c */ diff --git a/src/src/store.h b/src/src/store.h index 51dffab78..92deabf9b 100644 --- a/src/src/store.h +++ b/src/src/store.h @@ -13,15 +13,24 @@ /* Define symbols for identifying the store pools. */ -enum { POOL_MAIN, POOL_PERM, POOL_SEARCH, +#define NPOOLS 8 +enum { POOL_MAIN, + POOL_PERM, + POOL_SEARCH, + POOL_MESSAGE, + POOL_TAINT_BASE, - POOL_TAINT_MAIN = POOL_TAINT_BASE, POOL_TAINT_PERM, POOL_TAINT_SEARCH }; + + POOL_TAINT_MAIN = POOL_TAINT_BASE, + POOL_TAINT_PERM, + POOL_TAINT_SEARCH, + POOL_TAINT_MESSAGE }; /* This variable (the one for the current pool) is set by store_get() to its yield, and by store_reset() to NULL. This allows string_cat() to optimize its store handling. */ -extern void *store_last_get[6]; +extern void *store_last_get[NPOOLS]; /* This variable contains the current store pool number. */ @@ -45,7 +54,7 @@ tracing information for debugging. */ #define store_release_above(addr) \ store_release_above_3(addr, __FUNCTION__, __LINE__) #define store_reset(mark) \ - store_reset_3(mark, store_pool, __FUNCTION__, __LINE__) + store_reset_3(mark, __FUNCTION__, __LINE__) /* The real functions */ @@ -58,7 +67,7 @@ extern void *store_malloc_3(int, const char *, int) ALLOC ALLOC_SIZE(1) WARN_ extern rmark store_mark_3(const char *, int); extern void *store_newblock_3(void *, BOOL, int, int, const char *, int); extern void store_release_above_3(void *, const char *, int); -extern rmark store_reset_3(rmark, int, const char *, int); +extern rmark store_reset_3(rmark, const char *, int); #endif /* STORE_H */ diff --git a/src/src/string.c b/src/src/string.c index 4726fbe4e..7af87f92c 100644 --- a/src/src/string.c +++ b/src/src/string.c @@ -1692,6 +1692,7 @@ int main(void) uschar buffer[256]; printf("Testing is_ip_address\n"); +store_init(); while (fgets(CS buffer, sizeof(buffer), stdin) != NULL) {