From 90341c71c19c82ba8b1cbf4d1693940b8bb8f70b Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Wed, 8 Feb 2017 01:19:39 +0000 Subject: [PATCH 1/1] Memory management: drop variables identified as going out-of-scope Fixes crash in transport re-using bad $sender_ip_address from callout --- doc/doc-txt/ChangeLog | 8 ++++++++ src/src/daemon.c | 11 +++++++++++ src/src/exim.c | 41 +++++++++++++++++++++++++++++++++++------ src/src/expand.c | 15 +++++++++++---- src/src/queue.c | 33 +++++++++++++++++++++------------ src/src/smtp_in.c | 21 ++++++++++++++++----- 6 files changed, 102 insertions(+), 27 deletions(-) diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index 6cd472434..c23f9fe2d 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -81,6 +81,14 @@ PP/04 Bug 2018: Also handle Proxy Protocol v2 safely. PP/05 FreeBSD compat: handle that Ports no longer create /usr/bin/perl +JH/16 Drop variables when they go out of scope. Memory management drops a whole + region in one operation, for speed, and this leaves assigned pointers + dangling. Add checks run only under the testsuite which checks all + variables at a store-reset and panics on a dangling pointer; add code + explicitly nulling out all the variables discovered. Fixes one known + bug: a transport crash, where a dangling pointer for $sending_ip_address + originally assigned in a verify callout, is re-used. + Exim version 4.88 ----------------- diff --git a/src/src/daemon.c b/src/src/daemon.c index 6e904e1f9..d4fe7759c 100644 --- a/src/src/daemon.c +++ b/src/src/daemon.c @@ -564,6 +564,15 @@ if (pid == 0) /* Reclaim up the store used in accepting this message */ + return_path = sender_address = NULL; + authenticated_sender = NULL; + sending_ip_address = NULL; + deliver_host_address = deliver_host = + deliver_domain_orig = deliver_localpart_orig = NULL; + dnslist_domain = dnslist_matched = NULL; +#ifndef DISABLE_DKIM + dkim_cur_signer = NULL; +#endif store_reset(reset_point); /* If queue_only is set or if there are too many incoming connections in @@ -734,6 +743,8 @@ else (void)close(dup_accept_socket); the incoming host address and an expanded active_hostname. */ log_close_all(); +interface_address = +sender_host_address = NULL; store_reset(reset_point); sender_host_address = NULL; } diff --git a/src/src/exim.c b/src/src/exim.c index 5e11559ad..2237476d7 100644 --- a/src/src/exim.c +++ b/src/src/exim.c @@ -3195,7 +3195,10 @@ for (i = 1; i < argc; i++) } else { + int old_pool = store_pool; + store_pool = POOL_PERM; received_protocol = string_copyn(argrest, hn - argrest); + store_pool = old_pool; sender_host_name = hn + 1; } } @@ -5121,12 +5124,21 @@ if (host_checking) if (smtp_start_session()) { - reset_point = store_get(0); - for (;;) + for (reset_point = store_get(0); ; store_reset(reset_point)) { - store_reset(reset_point); if (smtp_setup_msg() <= 0) break; if (!receive_msg(FALSE)) break; + + return_path = sender_address = NULL; + dnslist_domain = dnslist_matched = NULL; +#ifndef DISABLE_DKIM + dkim_cur_signer = NULL; +#endif + acl_var_m = NULL; + deliver_localpart_orig = NULL; + deliver_domain_orig = NULL; + callout_address = sending_ip_address = NULL; + sender_rate = sender_rate_limit = sender_rate_period = NULL; } smtp_log_no_mail(); } @@ -5249,8 +5261,11 @@ if (smtp_input) } else { - if (received_protocol == NULL) + int old_pool = store_pool; + store_pool = POOL_PERM; + if (!received_protocol) received_protocol = string_sprintf("local%s", called_as); + store_pool = old_pool; set_process_info("accepting a local non-SMTP message from <%s>", sender_address); } @@ -5364,7 +5379,6 @@ collapsed). */ while (more) { - store_reset(reset_point); message_id[0] = 0; /* Handle the SMTP case; call smtp_setup_mst() to deal with the initial SMTP @@ -5406,7 +5420,7 @@ while (more) more = receive_msg(extract_recipients); if (message_id[0] == 0) { - if (more) continue; + if (more) goto moreloop; smtp_log_no_mail(); /* Log no mail if configured */ exim_exit(EXIT_FAILURE); } @@ -5769,6 +5783,21 @@ while (more) #ifndef SIG_IGN_WORKS while (waitpid(-1, NULL, WNOHANG) > 0); #endif + +moreloop: + return_path = sender_address = NULL; + authenticated_sender = NULL; + deliver_localpart_orig = NULL; + deliver_domain_orig = NULL; + deliver_host = deliver_host_address = NULL; + dnslist_domain = dnslist_matched = NULL; + malware_name = NULL; + callout_address = NULL; + sending_ip_address = NULL; + acl_var_m = NULL; + { int i; for(i=0; is_port); } - if ((fd = ip_connectedsocket(SOCK_STREAM, server_name, port, port, - timeout, NULL, &expand_string_message)) < 0) + fd = ip_connectedsocket(SOCK_STREAM, server_name, port, port, + timeout, NULL, &expand_string_message); + callout_address = NULL; + if (fd < 0) goto SOCK_FAIL; } @@ -7786,6 +7788,7 @@ typedef struct { uschar * region_start; uschar * region_end; const uschar *var_name; + const uschar *var_data; } err_ctx; static void @@ -7793,7 +7796,10 @@ assert_variable_notin(uschar * var_name, uschar * var_data, void * ctx) { err_ctx * e = ctx; if (var_data >= e->region_start && var_data < e->region_end) + { e->var_name = CUS var_name; + e->var_data = CUS var_data; + } } void @@ -7821,8 +7827,9 @@ for (v = var_table; v < var_table + var_table_size; v++) assert_variable_notin(US v->name, *(USS v->value), &e); if (e.var_name) - log_write(0, LOG_MAIN|LOG_PANIC_DIE, "live variable '%s' destroyed by reset_store" - " at %s:%d\n", e.var_name, e.filename, e.linenumber); + log_write(0, LOG_MAIN|LOG_PANIC_DIE, + "live variable '%s' destroyed by reset_store at %s:%d\n- value '%.64s'", + e.var_name, e.filename, e.linenumber, e.var_data); } diff --git a/src/src/queue.c b/src/src/queue.c index 714c73256..50e4aaef3 100644 --- a/src/src/queue.c +++ b/src/src/queue.c @@ -606,6 +606,9 @@ for (i = (queue_run_in_order? -1 : 0); /* Recover store used when reading the header */ + received_protocol = NULL; + sender_address = sender_ident = NULL; + authenticated_id = authenticated_sender = NULL; store_reset(reset_point2); if (!wanted) continue; /* With next message */ } @@ -857,19 +860,16 @@ if (option >= 8) option -= 8; /* Now scan the chain and print information, resetting store used each time. */ -reset_point = store_get(0); - -for (; f != NULL; f = f->next) +for (reset_point = store_get(0); f; f = f->next) { int rc, save_errno; int size = 0; BOOL env_read; - store_reset(reset_point); message_size = 0; message_subdir[0] = f->dir_uschar; rc = spool_read_header(f->text, FALSE, count <= 0); - if (rc == spool_read_notopen && errno == ENOENT && count <= 0) continue; + if (rc == spool_read_notopen && errno == ENOENT && count <= 0) goto next; save_errno = errno; env_read = (rc == spool_read_OK || rc == spool_read_hdrerror); @@ -901,8 +901,7 @@ for (; f != NULL; f = f->next) /* Collect delivered addresses from any J file */ fname[ptr] = 'J'; - jread = Ufopen(fname, "rb"); - if (jread != NULL) + if ((jread = Ufopen(fname, "rb"))) { while (Ufgets(big_buffer, big_buffer_size, jread) != NULL) { @@ -917,7 +916,7 @@ for (; f != NULL; f = f->next) fprintf(stdout, "%s ", string_format_size(size, big_buffer)); for (i = 0; i < 16; i++) fputc(f->text[i], stdout); - if (env_read && sender_address != NULL) + if (env_read && sender_address) { printf(" <%s>", sender_address); if (sender_set_untrusted) printf(" (%s)", originator_login); @@ -940,7 +939,7 @@ for (; f != NULL; f = f->next) if (rc != spool_read_hdrerror) { printf("\n\n"); - continue; + goto next; } } @@ -948,7 +947,7 @@ for (; f != NULL; f = f->next) printf("\n"); - if (recipients_list != NULL) + if (recipients_list) { for (i = 0; i < recipients_count; i++) { @@ -957,12 +956,22 @@ for (; f != NULL; f = f->next) if (!delivered || option != 1) printf(" %s %s\n", (delivered != NULL)? "D":" ", recipients_list[i].address); - if (delivered != NULL) delivered->data.val = TRUE; + if (delivered) delivered->data.val = TRUE; } - if (option == 2 && tree_nonrecipients != NULL) + if (option == 2 && tree_nonrecipients) queue_list_extras(tree_nonrecipients); printf("\n"); } + +next: + received_protocol = NULL; + sender_fullhost = sender_helo_name = + sender_rcvhost = sender_host_address = sender_address = sender_ident = NULL; + sender_host_authenticated = authenticated_sender = authenticated_id = NULL; + interface_address = NULL; + acl_var_m = NULL; + + store_reset(reset_point); } } diff --git a/src/src/smtp_in.c b/src/src/smtp_in.c index 2ea5b271d..bd83de045 100644 --- a/src/src/smtp_in.c +++ b/src/src/smtp_in.c @@ -1878,7 +1878,6 @@ Returns: nothing static void smtp_reset(void *reset_point) { -store_reset(reset_point); recipients_list = NULL; rcpt_count = rcpt_defer_count = rcpt_fail_count = raw_recipients_count = recipients_count = recipients_list_max = 0; @@ -1901,7 +1900,12 @@ submission_mode = FALSE; /* Can be set by ACL */ suppress_local_fixups = suppress_local_fixups_default; /* Can be set by ACL */ active_local_from_check = local_from_check; /* Can be set by ACL */ active_local_sender_retain = local_sender_retain; /* Can be set by ACL */ -sender_address = NULL; +sending_ip_address = NULL; +return_path = sender_address = NULL; +sender_data = NULL; /* Can be set by ACL */ +deliver_localpart_orig = NULL; +deliver_domain_orig = NULL; +callout_address = NULL; submission_name = NULL; /* Can be set by ACL */ raw_sender = NULL; /* After SMTP rewrite, before qualifying */ sender_address_unrewritten = NULL; /* Set only after verify rewrite */ @@ -1914,6 +1918,7 @@ authenticated_sender = NULL; bmi_run = 0; bmi_verdicts = NULL; #endif +dnslist_domain = dnslist_matched = NULL; #ifndef DISABLE_DKIM dkim_signers = NULL; dkim_disable_verify = FALSE; @@ -1921,6 +1926,7 @@ dkim_collect_input = FALSE; #endif dsn_ret = 0; dsn_envid = NULL; +deliver_host = deliver_host_address = NULL; /* Can be set by ACL */ #ifndef DISABLE_PRDR prdr_requested = FALSE; #endif @@ -1947,13 +1953,13 @@ acl_var_m = NULL; not the first message in an SMTP session and the previous message caused them to be referenced in an ACL. */ -if (message_body != NULL) +if (message_body) { store_free(message_body); message_body = NULL; } -if (message_body_end != NULL) +if (message_body_end) { store_free(message_body_end); message_body_end = NULL; @@ -1963,12 +1969,13 @@ if (message_body_end != NULL) repetition in the same message, but it seems right to repeat them for different messages. */ -while (acl_warn_logged != NULL) +while (acl_warn_logged) { string_item *this = acl_warn_logged; acl_warn_logged = acl_warn_logged->next; store_free(this); } +store_reset(reset_point); } @@ -4469,9 +4476,13 @@ while (done <= 0) case ENV_MAIL_OPT_UTF8: if (smtputf8_advertised) { + int old_pool = store_pool; + DEBUG(D_receive) debug_printf("smtputf8 requested\n"); message_smtputf8 = allow_utf8_domains = TRUE; + store_pool = POOL_PERM; received_protocol = string_sprintf("utf8%s", received_protocol); + store_pool = old_pool; } break; #endif -- 2.30.2