Memory management: drop variables identified as going out-of-scope

Fixes crash in transport re-using bad $sender_ip_address from callout

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; i<REGEX_VARS; i++) regex_vars[i] = NULL; }
+
+  store_reset(reset_point);
   }
 
 exim_exit(EXIT_SUCCESS);   /* Never returns */
diff --git a/src/src/expand.c b/src/src/expand.c
index d35bf9901..d2fcd2358 100644
--- a/src/src/expand.c
+++ b/src/src/expand.c
@@ -4801,8 +4801,10 @@ while (*s != 0)
             port = ntohs(service_info->s_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