readsocket expansion: response caching

8 files changed, 391 insertions, 208 deletions

diff --git a/src/scripts/MakeLinks b/src/scripts/MakeLinks
index 14fdb0000..277a1c026 100755
--- a/src/scripts/MakeLinks
+++ b/src/scripts/MakeLinks
@@ -31,8 +31,8 @@ mkdir lookups
 cd lookups
 # Makefile is generated
 for f in README cdb.c dbmdb.c dnsdb.c dsearch.c ibase.c json.c ldap.h ldap.c \
-  lmdb.c lsearch.c mysql.c redis.c nis.c nisplus.c oracle.c passwd.c \
-  pgsql.c spf.c sqlite.c testdb.c whoson.c \
+  lmdb.c lsearch.c mysql.c nis.c nisplus.c oracle.c passwd.c \
+  pgsql.c readsock.c redis.c spf.c sqlite.c testdb.c whoson.c \
   lf_functions.h lf_check_file.c lf_quote.c lf_sqlperform.c
 do
   ln -s ../../src/lookups/$f $f
diff --git a/src/scripts/lookups-Makefile b/src/scripts/lookups-Makefile
index 498184ff6..63f3eb86e 100755
--- a/src/scripts/lookups-Makefile
+++ b/src/scripts/lookups-Makefile
@@ -182,6 +182,9 @@ then
   OBJ="${OBJ} lmdb.o"
 fi
 
+# readsock is always wanted as it implements the ${readsock } expansion
+OBJ="${OBJ} readsock.o"
+
 echo "MODS = $MODS"
 echo "OBJ = $OBJ"
 
diff --git a/src/src/drtables.c b/src/src/drtables.c
index 363c07bf4..7ecca5056 100644
--- a/src/src/drtables.c
+++ b/src/src/drtables.c
@@ -617,6 +617,8 @@ extern lookup_module_info testdb_lookup_module_info;
 extern lookup_module_info whoson_lookup_module_info;
 #endif
 
+extern lookup_module_info readsock_lookup_module_info;
+
 
 void
 init_lookup_list(void)
@@ -715,6 +717,8 @@ addlookupmodule(NULL, &testdb_lookup_module_info);
 addlookupmodule(NULL, &whoson_lookup_module_info);
 #endif
 
+addlookupmodule(NULL, &readsock_lookup_module_info);
+
 #ifdef LOOKUP_MODULE_DIR
 if (!(dd = exim_opendir(LOOKUP_MODULE_DIR)))
   {
diff --git a/src/src/expand.c b/src/src/expand.c
index cdc914f5e..5ae74ef52 100644
--- a/src/src/expand.c
+++ b/src/src/expand.c
@@ -3927,7 +3927,7 @@ Arguments:
 Returns:       new pointer for expandable string, terminated if non-null
 */
 
-static gstring *
+gstring *
 cat_file(FILE *f, gstring *yield, uschar *eol)
 {
 uschar buffer[1024];
@@ -3947,7 +3947,7 @@ return yield;
 
 
 #ifndef DISABLE_TLS
-static gstring *
+gstring *
 cat_file_tls(void * tls_ctx, gstring * yield, uschar * eol)
 {
 int rc;
@@ -5286,7 +5286,6 @@ while (*s != 0)
       host_item host;
       BOOL do_shutdown = TRUE;
       BOOL do_tls = FALSE;	/* Only set under ! DISABLE_TLS */
-      blob reqstr;
 
       if (expand_forbid & RDO_READSOCK)
         {
@@ -5304,218 +5303,77 @@ while (*s != 0)
         case 3: goto EXPAND_FAILED;
         }
 
-      /* Grab the request string, if any */
-
-      reqstr.data = sub_arg[1];
-      reqstr.len = Ustrlen(sub_arg[1]);
-
-      /* Sort out timeout, if given.  The second arg is a list with the first element
-      being a time value.  Any more are options of form "name=value".  Currently the
-      only options recognised are "shutdown" and "tls". */
-
-      if (sub_arg[2])
-        {
-	const uschar * list = sub_arg[2];
-	uschar * item;
-	int sep = 0;
-
-	if (  !(item = string_nextinlist(&list, &sep, NULL, 0))
-	   || !*item
-	   || (timeout = readconf_readtime(item, 0, FALSE)) < 0)
-          {
-          expand_string_message = string_sprintf("bad time value %s", item);
-          goto EXPAND_FAILED;
-          }
-
-	while ((item = string_nextinlist(&list, &sep, NULL, 0)))
-	  if (Ustrncmp(item, US"shutdown=", 9) == 0)
-	    { if (Ustrcmp(item + 9, US"no") == 0) do_shutdown = FALSE; }
-#ifndef DISABLE_TLS
-	  else if (Ustrncmp(item, US"tls=", 4) == 0)
-	    { if (Ustrcmp(item + 9, US"no") != 0) do_tls = TRUE; }
-#endif
-        }
-      else
-	sub_arg[3] = NULL;                     /* No eol if no timeout */
-
       /* If skipping, we don't actually do anything. Otherwise, arrange to
       connect to either an IP or a Unix socket. */
 
       if (!skipping)
         {
-        /* Handle an IP (internet) domain */
-
-        if (Ustrncmp(sub_arg[0], "inet:", 5) == 0)
-          {
-          int port;
-          uschar * port_name;
+	int stype = search_findtype(US"readsock", 8);
+	gstring * g = NULL;
+	void * handle;
+	int expand_setup = -1;
+	uschar * s;
 
-          server_name = sub_arg[0] + 5;
-          port_name = Ustrrchr(server_name, ':');
+	/* If the reqstr is empty, flag that and set a dummy */
 
-          /* Sort out the port */
+	if (!sub_arg[1][0])
+	  {
+	  g = string_append_listele(g, ',', US"send=no");
+	  sub_arg[1] = US"DUMMY";
+	  }
 
-          if (!port_name)
-            {
-            expand_string_message =
-              string_sprintf("missing port for readsocket %s", sub_arg[0]);
-            goto EXPAND_FAILED;
-            }
-          *port_name++ = 0;           /* Terminate server name */
+	/* Re-marshall the options */
 
-          if (isdigit(*port_name))
-            {
-            uschar *end;
-            port = Ustrtol(port_name, &end, 0);
-            if (end != port_name + Ustrlen(port_name))
-              {
-              expand_string_message =
-                string_sprintf("invalid port number %s", port_name);
-              goto EXPAND_FAILED;
-              }
-            }
-          else
-            {
-            struct servent *service_info = getservbyname(CS port_name, "tcp");
-            if (!service_info)
-              {
-              expand_string_message = string_sprintf("unknown port \"%s\"",
-                port_name);
-              goto EXPAND_FAILED;
-              }
-            port = ntohs(service_info->s_port);
-            }
+	if (sub_arg[2])
+	  {
+	  const uschar * list = sub_arg[2];
+	  uschar * item;
+	  int sep = 0;
 
-	  /*XXX we trust that the request is idempotent for TFO.  Hmm. */
-	  cctx.sock = ip_connectedsocket(SOCK_STREAM, server_name, port, port,
-		  timeout, &host, &expand_string_message,
-		  do_tls ? NULL : &reqstr);
-	  callout_address = NULL;
-	  if (cctx.sock < 0)
-	    goto SOCK_FAIL;
-	  if (!do_tls)
-	    reqstr.len = 0;
-          }
+	  /* First option has no tag and is timeout */
+	  if ((item = string_nextinlist(&list, &sep, NULL, 0)))
+	    g = string_append_listele(g, ',',
+		  string_sprintf("timeout=%s", item));
 
-        /* Handle a Unix domain socket */
+	  /* The rest of the options from the expansion */
+	  while ((item = string_nextinlist(&list, &sep, NULL, 0)))
+	    g = string_append_listele(g, ',', item);
 
-        else
-          {
-	  struct sockaddr_un sockun;         /* don't call this "sun" ! */
-          int rc;
+	  /* possibly plus an EOL string */
+	  if (sub_arg[3] && *sub_arg[3])
+	    g = string_append_listele(g, ',',
+		  string_sprintf("eol=%s", sub_arg[3]));
 
-          if ((cctx.sock = socket(PF_UNIX, SOCK_STREAM, 0)) == -1)
-            {
-            expand_string_message = string_sprintf("failed to create socket: %s",
-              strerror(errno));
-            goto SOCK_FAIL;
-            }
-
-          sockun.sun_family = AF_UNIX;
-          sprintf(sockun.sun_path, "%.*s", (int)(sizeof(sockun.sun_path)-1),
-            sub_arg[0]);
-	  server_name = US sockun.sun_path;
-
-          sigalrm_seen = FALSE;
-          ALARM(timeout);
-          rc = connect(cctx.sock, (struct sockaddr *)(&sockun), sizeof(sockun));
-          ALARM_CLR(0);
-          if (sigalrm_seen)
-            {
-            expand_string_message = US "socket connect timed out";
-            goto SOCK_FAIL;
-            }
-          if (rc < 0)
-            {
-            expand_string_message = string_sprintf("failed to connect to socket "
-              "%s: %s", sub_arg[0], strerror(errno));
-            goto SOCK_FAIL;
-            }
-	  host.name = server_name;
-	  host.address = US"";
-          }
+	  }
 
-        DEBUG(D_expand) debug_printf_indent("connected to socket %s\n", sub_arg[0]);
+	/* Gat a (possibly cached) handle for the connection */
 
-#ifndef DISABLE_TLS
-	if (do_tls)
+	if (!(handle = search_open(sub_arg[0], stype, 0, NULL, NULL)))
 	  {
-	  smtp_connect_args conn_args = {.host = &host };
-	  tls_support tls_dummy = {.sni=NULL};
-	  uschar * errstr;
-
-	  if (!tls_client_start(&cctx, &conn_args, NULL, &tls_dummy, &errstr))
-	    {
-	    expand_string_message = string_sprintf("TLS connect failed: %s", errstr);
-	    goto SOCK_FAIL;
-	    }
+	  if (*expand_string_message) goto EXPAND_FAILED;
+	  expand_string_message = search_error_message;
+	  search_error_message = NULL;
+	  goto SOCK_FAIL;
 	  }
-#endif
-
-	/* Allow sequencing of test actions */
-	testharness_pause_ms(100);
-
-        /* Write the request string, if not empty or already done */
-
-        if (reqstr.len)
-          {
-          DEBUG(D_expand) debug_printf_indent("writing \"%s\" to socket\n",
-            reqstr.data);
-          if ( (
-#ifndef DISABLE_TLS
-	      do_tls ? tls_write(cctx.tls_ctx, reqstr.data, reqstr.len, FALSE) :
-#endif
-			write(cctx.sock, reqstr.data, reqstr.len)) != reqstr.len)
-            {
-            expand_string_message = string_sprintf("request write to socket "
-              "failed: %s", strerror(errno));
-            goto SOCK_FAIL;
-            }
-          }
-
-        /* Shut down the sending side of the socket. This helps some servers to
-        recognise that it is their turn to do some work. Just in case some
-        system doesn't have this function, make it conditional. */
 
-#ifdef SHUT_WR
-	if (!do_tls && do_shutdown) shutdown(cctx.sock, SHUT_WR);
-#endif
-
-	testharness_pause_ms(100);
-
-        /* Now we need to read from the socket, under a timeout. The function
-        that reads a file can be used. */
-
-	if (!do_tls)
-	  fp = fdopen(cctx.sock, "rb");
-        sigalrm_seen = FALSE;
-        ALARM(timeout);
-        yield =
-#ifndef DISABLE_TLS
-	  do_tls ? cat_file_tls(cctx.tls_ctx, yield, sub_arg[3]) :
-#endif
-		    cat_file(fp, yield, sub_arg[3]);
-        ALARM_CLR(0);
+	/* Get (possibly cached) results for the lookup */
+	/* sspec: sub_arg[0]  req: sub_arg[1]  opts: g */
 
-#ifndef DISABLE_TLS
-	if (do_tls)
+	if ((s = search_find(handle, sub_arg[0], sub_arg[1], -1, NULL, 0, 0,
+				    &expand_setup, string_from_gstring(g))))
+	  yield = string_cat(yield, s);
+	else if (f.search_find_defer)
 	  {
-	  tls_close(cctx.tls_ctx, TRUE);
-	  close(cctx.sock);
+	  expand_string_message = search_error_message;
+	  search_error_message = NULL;
+	  goto SOCK_FAIL;
 	  }
 	else
-#endif
-	  (void)fclose(fp);
-
-        /* After a timeout, we restore the pointer in the result, that is,
-        make sure we add nothing from the socket. */
-
-        if (sigalrm_seen)
-          {
-          if (yield) yield->ptr = save_ptr;
-          expand_string_message = US "socket read timed out";
-          goto SOCK_FAIL;
-          }
+	  {	/* should not happen, at present */
+	  expand_string_message = search_error_message;
+	  search_error_message = NULL;
+	  goto SOCK_FAIL;
+	  }
         }
 
       /* The whole thing has worked (or we were skipping). If there is a
diff --git a/src/src/functions.h b/src/src/functions.h
index 8206c480e..8ea5e4ef6 100644
--- a/src/src/functions.h
+++ b/src/src/functions.h
@@ -149,6 +149,8 @@ extern void    bits_clear(unsigned int *, size_t, int *);
 extern void    bits_set(unsigned int *, size_t, int *);
 
 extern void    cancel_cutthrough_connection(BOOL, const uschar *);
+extern gstring *cat_file(FILE *, gstring *, uschar *);
+extern gstring *cat_file_tls(void *, gstring *, uschar *);
 extern int     check_host(void *, const uschar *, const uschar **, uschar **);
 extern uschar **child_exec_exim(int, BOOL, int *, BOOL, int, ...);
 extern pid_t   child_open_exim_function(int *, const uschar *);
diff --git a/src/src/lookups/Makefile b/src/src/lookups/Makefile
index 01910d542..9231d666c 100644
--- a/src/src/lookups/Makefile
+++ b/src/src/lookups/Makefile
@@ -43,6 +43,7 @@ nisplus.o:       $(PHDRS) nisplus.c
 oracle.o:        $(PHDRS) oracle.c
 passwd.o:        $(PHDRS) passwd.c
 pgsql.o:         $(PHDRS) pgsql.c
+readsock.o:      $(PHDRS) readsock.c
 redis.o:         $(PHDRS) redis.c
 spf.o:           $(PHDRS) spf.c
 sqlite.o:        $(PHDRS) sqlite.c
diff --git a/src/src/lookups/readsock.c b/src/src/lookups/readsock.c
new file mode 100644
index 000000000..c2088b7a5
--- /dev/null
+++ b/src/src/lookups/readsock.c
@@ -0,0 +1,319 @@
+/*************************************************
+*     Exim - an Internet mail transport agent    *
+*************************************************/
+
+/* Copyright (c) Jeremy Harris 2020 */
+/* See the file NOTICE for conditions of use and distribution. */
+
+#include "../exim.h"
+#include "lf_functions.h"
+
+
+static int
+internal_readsock_open(client_conn_ctx * cctx, const uschar * sspec,
+  int timeout, BOOL do_tls, uschar ** errmsg)
+{
+int sep = ',';
+uschar * ele;
+const uschar * server_name;
+host_item host;
+
+if (Ustrncmp(sspec, "inet:", 5) == 0)
+  {
+  int port;
+  uschar * port_name;
+
+  DEBUG(D_lookup)
+    debug_printf_indent("  new inet socket needed for readsocket\n");
+
+  server_name = sspec + 5;
+  port_name = Ustrrchr(server_name, ':');
+
+  /* Sort out the port */
+
+  if (!port_name)
+    {
+    /* expand_string_message results in an EXPAND_FAIL, from our
+    only caller.  Lack of it gets a SOCK_FAIL; we feed back via errmsg
+    for that, which gets copied to search_error_message. */
+
+    expand_string_message =
+      string_sprintf("missing port for readsocket %s", sspec);
+    return FAIL;
+    }
+  *port_name++ = 0;           /* Terminate server name */
+
+  if (isdigit(*port_name))
+    {
+    uschar *end;
+    port = Ustrtol(port_name, &end, 0);
+    if (end != port_name + Ustrlen(port_name))
+      {
+      expand_string_message =
+	string_sprintf("invalid port number %s", port_name);
+      return FAIL;
+      }
+    }
+  else
+    {
+    struct servent *service_info = getservbyname(CS port_name, "tcp");
+    if (!service_info)
+      {
+      expand_string_message = string_sprintf("unknown port \"%s\"",
+	port_name);
+      return FAIL;
+      }
+    port = ntohs(service_info->s_port);
+    }
+
+  /* Not having the request-string here in the open routine means
+  that we cannot do TFO; a pity */
+
+  cctx->sock = ip_connectedsocket(SOCK_STREAM, server_name, port, port,
+	  timeout, &host, errmsg, NULL);
+  callout_address = NULL;
+  if (cctx->sock < 0)
+    return FAIL;
+  }
+
+else
+  {
+  struct sockaddr_un sockun;         /* don't call this "sun" ! */
+  int rc;
+
+  DEBUG(D_lookup)
+    debug_printf_indent("  new unix socket needed for readsocket\n");
+
+  if ((cctx->sock = socket(PF_UNIX, SOCK_STREAM, 0)) == -1)
+    {
+    *errmsg = string_sprintf("failed to create socket: %s", strerror(errno));
+    return FAIL;
+    }
+
+  sockun.sun_family = AF_UNIX;
+  sprintf(sockun.sun_path, "%.*s", (int)(sizeof(sockun.sun_path)-1),
+    sspec);
+  server_name = US sockun.sun_path;
+
+  sigalrm_seen = FALSE;
+  ALARM(timeout);
+  rc = connect(cctx->sock, (struct sockaddr *)(&sockun), sizeof(sockun));
+  ALARM_CLR(0);
+  if (sigalrm_seen)
+    {
+    *errmsg = US "socket connect timed out";
+    goto bad;
+    }
+  if (rc < 0)
+    {
+    *errmsg = string_sprintf("failed to connect to socket "
+      "%s: %s", sspec, strerror(errno));
+    goto bad;
+    }
+  host.name = server_name;
+  host.address = US"";
+  }
+
+#ifndef DISABLE_TLS
+if (do_tls)
+  {
+  smtp_connect_args conn_args = {.host = &host };
+  tls_support tls_dummy = {.sni=NULL};
+  uschar * errstr;
+
+  if (!tls_client_start(cctx, &conn_args, NULL, &tls_dummy, &errstr))
+    {
+    *errmsg = string_sprintf("TLS connect failed: %s", errstr);
+    goto bad;
+    }
+  }
+#endif
+
+DEBUG(D_expand|D_lookup) debug_printf_indent("  connected to socket %s\n", sspec);
+return OK;
+
+bad:
+  close(cctx->sock);
+  return FAIL;
+}
+
+/* All use of allocations will be done against the POOL_SEARCH memory,
+which is freed once by search_tidyup(). */
+
+/*************************************************
+*              Open entry point                  *
+*************************************************/
+
+/* See local README for interface description */
+/* We just create a placeholder record with a closed socket, so
+that connection cacheing at the framework layer works. */
+
+static void *
+readsock_open(const uschar * filename, uschar ** errmsg)
+{
+client_conn_ctx * cctx = store_get(sizeof(*cctx), FALSE);
+cctx->sock = -1;
+cctx->tls_ctx = NULL;
+DEBUG(D_lookup) debug_printf_indent("readsock: allocated context\n");
+return cctx;
+}
+
+
+
+
+
+/*************************************************
+*         Find entry point for lsearch           *
+*************************************************/
+
+/* See local README for interface description */
+
+static int
+readsock_find(void * handle, const uschar * filename, const uschar * keystring,
+  int length, uschar ** result, uschar ** errmsg, uint * do_cache,
+  const uschar * opts)
+{
+client_conn_ctx * cctx = handle;
+int sep = ',';
+struct {
+	BOOL do_shutdown:1;
+	BOOL do_tls:1;
+	BOOL cache:1;
+} lf = {.do_shutdown = TRUE};
+uschar * eol = NULL;
+int timeout = 5;
+FILE * fp;
+gstring * yield;
+int ret = DEFER;
+
+DEBUG(D_lookup) debug_printf_indent("readsock: file=\"%s\" key=\"%s\" len=%d opts=\"%s\"\n", filename, keystring, length, opts);
+
+/* Parse options */
+
+if (opts) for (uschar * s; s = string_nextinlist(&opts, &sep, NULL, 0); )
+  if (Ustrncmp(s, "timeout=", 8) == 0)
+    timeout = readconf_readtime(s + 8, 0, FALSE);
+  else if (Ustrncmp(s, "shutdown=", 9) == 0)
+    lf.do_shutdown = Ustrcmp(s + 9, "no") != 0;
+#ifndef DISABLE_TLS
+  else if (Ustrncmp(s, "tls=", 4) == 0 && Ustrcmp(s + 4, US"no") != 0)
+    lf.do_tls = TRUE;
+#endif
+  else if (Ustrncmp(s, "eol=", 4) == 0)
+    eol = s + 4;
+  else if (Ustrcmp(s, "cache=yes") == 0)
+    lf.cache = TRUE;
+  else if (Ustrcmp(s, "send=no") == 0)
+    length = 0;
+
+if (!filename) return FAIL;	/* Server spec is required */
+
+/* Open the socket, if not cached */
+
+if (cctx->sock == -1)
+  if (internal_readsock_open(cctx, filename, timeout, lf.do_tls, errmsg) != OK)
+    return ret;
+
+testharness_pause_ms(100);	/* Allow sequencing of test actions */
+
+/* Write the request string, if not empty or already done */
+
+if (length)
+  {
+  if ((
+#ifndef DISABLE_TLS
+      cctx->tls_ctx ? tls_write(cctx->tls_ctx, keystring, length, FALSE) :
+#endif
+		      write(cctx->sock, keystring, length)) != length)
+    {
+    *errmsg = string_sprintf("request write to socket "
+      "failed: %s", strerror(errno));
+    goto out;
+    }
+  }
+
+/* Shut down the sending side of the socket. This helps some servers to
+recognise that it is their turn to do some work. Just in case some
+system doesn't have this function, make it conditional. */
+
+#ifdef SHUT_WR
+if (!cctx->tls_ctx && lf.do_shutdown)
+  shutdown(cctx->sock, SHUT_WR);
+#endif
+
+testharness_pause_ms(100);
+
+/* Now we need to read from the socket, under a timeout. The function
+that reads a file can be used.  If we're using a stdio buffered read,
+and might need later write ops on the socket, the stdio must be in
+writable mode or the underlying socket goes non-writable. */
+
+if (!cctx->tls_ctx)
+  fp = fdopen(cctx->sock, lf.do_shutdown ? "rb" : "wb");
+
+sigalrm_seen = FALSE;
+ALARM(timeout);
+yield =
+#ifndef DISABLE_TLS
+  cctx->tls_ctx ? cat_file_tls(cctx->tls_ctx, NULL, eol) :
+#endif
+		  cat_file(fp, NULL, eol);
+ALARM_CLR(0);
+
+if (sigalrm_seen)
+  { *errmsg = US "socket read timed out"; goto out; }
+
+*result = yield ? string_from_gstring(yield) : US"";
+ret = OK;
+if (!lf.cache) *do_cache = 0;
+
+out:
+
+(void) close(cctx->sock);
+cctx->sock = -1;
+return ret;
+}
+
+
+
+/*************************************************
+*              Close entry point                 *
+*************************************************/
+
+/* See local README for interface description */
+
+static void
+readsock_close(void * handle)
+{
+client_conn_ctx * cctx = handle;
+if (cctx->sock < 0) return;
+#ifndef DISABLE_TLS
+if (cctx->tls_ctx) tls_close(cctx->tls_ctx, TRUE);
+#endif
+close(cctx->sock);
+cctx->sock = -1;
+}
+
+
+
+static lookup_info readsock_lookup_info = {
+  .name = US"readsock",			/* lookup name */
+  .type = lookup_querystyle,
+  .open = readsock_open,		/* open function */
+  .check = NULL,
+  .find = readsock_find,		/* find function */
+  .close = readsock_close,
+  .tidy = NULL,
+  .quote = NULL,			/* no quoting function */
+  .version_report = NULL
+};
+
+
+#ifdef DYNLOOKUP
+#define readsock_lookup_module_info _lookup_module_info
+#endif
+
+static lookup_info *_lookup_list[] = { &readsock_lookup_info };
+lookup_module_info readsock_lookup_module_info = { LOOKUP_MODULE_INFO_MAGIC, _lookup_list, 1 };
+
+/* End of lookups/readsock.c */
diff --git a/src/src/search.c b/src/src/search.c
index d929322ba..d3511df31 100644
--- a/src/src/search.c
+++ b/src/src/search.c
@@ -239,12 +239,10 @@ Returns:     nothing
 static void
 tidyup_subtree(tree_node *t)
 {
-search_cache *c = (search_cache *)(t->data.ptr);
-if (t->left != NULL) tidyup_subtree(t->left);
-if (t->right != NULL) tidyup_subtree(t->right);
-if (c != NULL &&
-    c->handle != NULL &&
-    lookup_list[c->search_type]->close != NULL)
+search_cache * c = (search_cache *)(t->data.ptr);
+if (t->left)  tidyup_subtree(t->left);
+if (t->right) tidyup_subtree(t->right);
+if (c && c->handle && lookup_list[c->search_type]->close)
   lookup_list[c->search_type]->close(c->handle);
 }
 
@@ -522,7 +520,8 @@ else
   DEBUG(D_lookup)
     {
     if (t)
-      debug_printf_indent("cached data found but either wrong opts or dated; ");
+      debug_printf_indent("cached data found but %s; ",
+	e->expiry && e->expiry <= time(NULL) ? "out-of-date" : "wrong opts");
     debug_printf_indent("%s lookup required for %s%s%s\n",
       filename ? US"file" : US"database",
       keystring,
@@ -545,13 +544,10 @@ else
 
   else if (do_cache)
     {
-    if (!t) /* No existing entry.  Create new one. */
+    if (!t)	/* No existing entry.  Create new one. */
       {
       int len = keylength + 1;
       e = store_get(sizeof(expiring_data) + sizeof(tree_node) + len, is_tainted(keystring));
-      e->expiry = do_cache == UINT_MAX ? 0 : time(NULL)+do_cache;
-      e->opts = opts;
-      e->data.ptr = data;
       t = (tree_node *)(e+1);
       memcpy(t->name, keystring, len);
       t->data.ptr = e;
@@ -560,7 +556,7 @@ else
       /* Else previous, out-of-date cache entry.  Update with the */
       /* new result and forget the old one */
     e->expiry = do_cache == UINT_MAX ? 0 : time(NULL)+do_cache;
-    e->opts = opts;
+    e->opts = opts ? string_copy(opts) : NULL;
     e->data.ptr = data;
     }
 
@@ -570,7 +566,7 @@ else
   else
     {
     DEBUG(D_lookup) debug_printf_indent("lookup forced cache cleanup\n");
-    c->item_cache = NULL;
+    c->item_cache = NULL; 	/* forget all lookups on this connection */
     }
   }