Lots of 'dnahelper' tests, all pass but one

Test fails because DNA helper logic does not yet impose a timeout on receiving
the "DONE" ACK after a request.

commandline.c

@@ -502,11 +502,17 @@ int app_dna_lookup(int argc, const char *const *argv, struct command_line_option
 		else if ((rx.packetTypeAndFlags&MDP_TYPE_MASK)==MDP_TX) {
 		  /* Extract DID, Name, URI from response. */
 		  if (strlen((char *)rx.in.payload)<512) {
-		    unsigned char sid[SID_SIZE];
+		    char sidhex[SID_STRLEN + 1];
 		    char did[DID_MAXSIZE + 1];
 		    char name[64];
 		    char uri[512];
-		    if (!parseDnaReply((char *)rx.in.payload, rx.in.payload_length, sid, did, name, uri)) {
+		    if ( !parseDnaReply((char *)rx.in.payload, rx.in.payload_length, sidhex, did, name, uri, NULL)
+		      || !str_is_subscriber_id(sidhex)
+		      || !str_is_did(did)
+		      || !str_is_uri(uri)
+		    ) {
+		      WHYF("Received malformed DNA reply: %s", alloca_toprint(160, (const char *)rx.in.payload, rx.in.payload_length));
+		    } else {
 		      /* Have we seen this response before? */
 		      int i;
 		      for(i=0;i<uri_count;i++)
@@ -521,8 +527,6 @@ int app_dna_lookup(int argc, const char *const *argv, struct command_line_option
 			  strcpy(uris[uri_count++],uri);
 			}
 		      }
-		    } else {
-		      WHYF("Received malformed DNA reply: %s", alloca_toprint(160, (const char *)rx.in.payload, rx.in.payload_length));
 		    }
 		  }
 		}
@@ -1641,18 +1645,22 @@ int app_node_info(int argc, const char *const *argv, struct command_line_option
 	}
 	
 	{
-	  unsigned char sid[SID_SIZE];
+	  char sidhex[SID_STRLEN + 1];
 	  char did[DID_MAXSIZE + 1];
 	  char name[64];
 	  char uri[512];
-	  if (parseDnaReply((char *)m2.in.payload, m2.in.payload_length, sid, did, name, uri) != -1) {
+	  if ( !parseDnaReply((char *)m2.in.payload, m2.in.payload_length, sidhex, did, name, uri, NULL)
+	    || !str_is_subscriber_id(sidhex)
+	    || !str_is_did(did)
+	    || !str_is_uri(uri)
+	  ) {
+	    WHYF("Received malformed DNA reply: %s", alloca_toprint(160, (const char *)m2.in.payload, m2.in.payload_length));
+	  } else {
 	    /* Got a good DNA reply, copy it into place */
 	    bcopy(did,mdp.nodeinfo.did,32);
 	    bcopy(name,mdp.nodeinfo.name,64);
 	    mdp.nodeinfo.resolve_did=1;
 	    break;
-	  } else {
-	    WHYF("Received malformed DNA reply: %s", alloca_toprint(160, (const char *)m2.in.payload, m2.in.payload_length));
 	  }
 	}
       }

dataformats.c

@@ -361,3 +361,45 @@ int safeZeroField(unsigned char *packet,int start,int count)
   return 0;
 }
 
+int is_uri_char_scheme(char c)
+{
+  return isalpha(c) || isdigit(c) || c == '+' || c == '-' || c == '.';
+}
+
+int is_uri_char_unreserved(char c)
+{
+  return isalpha(c) || isdigit(c) || c == '-' || c == '.' || c == '_' || c == '~';
+}
+
+int is_uri_char_reserved(char c)
+{
+  switch (c) {
+    case ':': case '/': case '?': case '#': case '[': case ']': case '@':
+    case '!': case '$': case '&': case '\'': case '(': case ')':
+    case '*': case '+': case ',': case ';': case '=':
+      return 1;
+  }
+  return 0;
+}
+
+/* Return true if the string resembles a URI.
+   Based on RFC-3986 generic syntax, assuming nothing about the hierarchical part.
+   @author Andrew Bettison <andrew@servalproject.com>
+ */
+int str_is_uri(const char *uri)
+{
+  const char *p = uri;
+  // Scheme is ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
+  if (!isalpha(*p++))
+    return 0;
+  while (is_uri_char_scheme(*p))
+    ++p;
+  // Scheme is followed by colon ":".
+  if (*p++ != ':')
+    return 0;
+  // Hierarchical part must contain only valid characters.
+  const char *q = p;
+  while (is_uri_char_unreserved(*p) || is_uri_char_reserved(*p))
+    ++p;
+  return p != q && *p == '\0';
+}

dna_helper.c

@@ -44,33 +44,35 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
 int
-parseDnaReply(const char *bytes, size_t count, unsigned char *sid, char *did, char *name, char *uri)
+parseDnaReply(const char *buf, size_t len, char *token, char *did, char *name, char *uri, const char **bufp)
 {
-  /* Replies look like: TOKEN|URI|DID|NAME| where TOKEN is a SID in hex format */
+  /* Replies look like: TOKEN|URI|DID|NAME| where TOKEN is usually a hex SID */
-  const char *b = bytes;
+  const char *b = buf;
-  const char *e = bytes + count;
+  const char *e = buf + len;
   char *p, *q;
-  if (count < SID_STRLEN || fromhex(sid, bytes, SID_SIZE) == -1)
+  for (p = token, q = token + SID_STRLEN; b != e && *b != '|' && p != q; ++p, ++b)
-    return -1;
+    *p = *b;
-  b += SID_STRLEN;
+  *p = '\0';
   if (b == e || *b++ != '|')
-    return -1;
+    return 0;
   for (p = uri, q = uri + 511; b != e && *b != '|' && p != q; ++p, ++b)
     *p = *b;
   *p = '\0';
   if (b == e || *b++ != '|')
-    return -1;
+    return 0;
   for (p = did, q = did + DID_MAXSIZE; b != e && *b != '|' && p != q; ++p, ++b)
     *p = *b;
   *p = '\0';
   if (b == e || *b++ != '|')
-    return -1;
+    return 0;
   for (p = name, q = name + 63; b != e && *b != '|' && p != q; ++p, ++b)
     *p = *b;
   *p = '\0';
-  if (b == e || *b != '|')
+  if (b == e || *b++ != '|')
-    return -1;
+    return 0;
-  return 0;
+  if (bufp)
+    *bufp = b;
+  return 1;
 }
 
 static pid_t dna_helper_pid = -1;
@@ -89,7 +91,8 @@ static struct sched_ent schedrestart = STRUCT_SCHED_ENT_UNUSED;
 static char request_buffer[SID_STRLEN + DID_MAXSIZE + 4];
 static char *request_bufptr = NULL;
 static char *request_bufend = NULL;
-overlay_mdp_data_frame request_mdp_data;
+static overlay_mdp_data_frame request_mdp_data;
+static char request_did[DID_MAXSIZE + 1];
 
 static int awaiting_reply = 0;
 static int discarding_until_nl = 0;
@@ -107,7 +110,7 @@ dna_helper_close_pipes()
 {
   if (dna_helper_stdin != -1) {
     if (debug & DEBUG_DNAHELPER)
-      DEBUGF("Closing DNA helper stdin pipe fd=%d", dna_helper_stdin);
+      DEBUGF("DNAHELPER closing stdin pipe fd=%d", dna_helper_stdin);
     close(dna_helper_stdin);
     dna_helper_stdin = -1;
   }
@@ -117,7 +120,7 @@ dna_helper_close_pipes()
   }
   if (dna_helper_stdout != -1) {
     if (debug & DEBUG_DNAHELPER)
-      DEBUGF("Closing DNA helper stdout pipe fd=%d", dna_helper_stdout);
+      DEBUGF("DNAHELPER closing stdout pipe fd=%d", dna_helper_stdout);
     close(dna_helper_stdout);
     dna_helper_stdout = -1;
   }
@@ -127,7 +130,7 @@ dna_helper_close_pipes()
   }
   if (dna_helper_stderr != -1) {
     if (debug & DEBUG_DNAHELPER)
-      DEBUGF("Closing DNA helper stderr pipe fd=%d", dna_helper_stderr);
+      DEBUGF("DNAHELPER closing stderr pipe fd=%d", dna_helper_stderr);
     close(dna_helper_stderr);
     dna_helper_stderr = -1;
   }
@@ -237,7 +240,7 @@ dna_helper_kill()
 {
   if (dna_helper_pid > 0) {
     if (debug & DEBUG_DNAHELPER)
-      DEBUGF("Sending SIGTERM to DNA helper pid=%d", dna_helper_pid);
+      DEBUGF("DNAHELPER sending SIGTERM to pid=%d", dna_helper_pid);
     if (kill(dna_helper_pid, SIGTERM) == -1)
       WHYF_perror("kill(%d, SIGTERM)", dna_helper_pid);
     // The process is wait()ed for in dna_helper_monitor() so that we do not block here.
@@ -251,12 +254,12 @@ dna_helper_harvest(int blocking)
 {
   if (dna_helper_pid > 0) {
     if (blocking && (debug & DEBUG_DNAHELPER))
-      DEBUGF("Waiting for DNA helper pid=%d to die", dna_helper_pid);
+      DEBUGF("DNAHELPER waiting for pid=%d to die", dna_helper_pid);
     int status;
     pid_t pid = waitpid(dna_helper_pid, &status, blocking ? 0 : WNOHANG);
     if (pid == dna_helper_pid) {
       strbuf b = strbuf_alloca(80);
-      INFOF("DNA helper pid=%u %s", pid, strbuf_str(strbuf_append_exit_status(b, status)));
+      INFOF("DNAHELPER process pid=%u %s", pid, strbuf_str(strbuf_append_exit_status(b, status)));
       unschedule(&sched_harvester);
       dna_helper_pid = -1;
       return 1;
@@ -272,7 +275,7 @@ dna_helper_harvest(int blocking)
 int dna_helper_shutdown()
 {
   if (debug & DEBUG_DNAHELPER)
-    DEBUG("Shutting down DNA helper");
+    DEBUG("DNAHELPER shutting down");
   dna_helper_close_pipes();
   switch (dna_helper_kill()) {
   case -1:
@@ -293,7 +296,7 @@ static void monitor_requests(struct sched_ent *alarm)
   }
   if (sched_requests.poll.revents & (POLLHUP | POLLERR)) {
     if (debug & DEBUG_DNAHELPER)
-      DEBUGF("Closing DNA helper stdin fd=%d", dna_helper_stdin);
+      DEBUGF("DNAHELPER closing stdin fd=%d", dna_helper_stdin);
     close(dna_helper_stdin);
     dna_helper_stdin = -1;
     unwatch(&sched_requests);
@@ -312,7 +315,7 @@ static void monitor_requests(struct sched_ent *alarm)
 	    request that arrives after a suitable pause has elapsed.  Losing the current request is not
 	    a big problem, because DNA preemptively retries.
 	  */
-	  WARN("Got SIGPIPE from DNA helper -- stopping DNA helper");
+	  INFO("DNAHELPER got SIGPIPE on write -- stopping process");
 	  dna_helper_kill();
 	} else if (written > 0) {
 	  request_bufptr += written;
@@ -347,7 +350,7 @@ void handle_reply_line(const char *bufp, size_t len)
   if (!dna_helper_started) {
     if (len == 8 && strncmp(bufp, "STARTED\n", 8) == 0) {
       if (debug & DEBUG_DNAHELPER)
-	DEBUGF("Got DNA helper STARTED ACK");
+	DEBUGF("DNAHELPER got STARTED ACK");
       dna_helper_started = 1;
       // Start sending request if there is one pending.
       if (request_bufptr) {
@@ -355,30 +358,49 @@ void handle_reply_line(const char *bufp, size_t len)
 	watch(&sched_requests);
       }
     } else {
-      WHYF("Malformed DNA helper start ACK %s", alloca_toprint(-1, bufp, len));
+      WHYF("DNAHELPER malformed start ACK %s", alloca_toprint(-1, bufp, len));
       dna_helper_kill();
     }
   } else if (awaiting_reply) {
     if (len == 5 && strncmp(bufp, "DONE\n", 5) == 0) {
       if (debug & DEBUG_DNAHELPER)
-	DEBUG("DNA helper reply DONE");
+	DEBUG("DNAHELPER reply DONE");
       awaiting_reply = 0;
       // Done
     } else {
-      unsigned char sid[SID_SIZE];
+      char sidhex[SID_STRLEN + 1];
       char did[DID_MAXSIZE + 1];
       char name[64];
       char uri[512];
-      if (parseDnaReply(bufp, len, sid, did, name, uri) != -1) {
+      const char *replyend = NULL;
+      if (!parseDnaReply(bufp, len, sidhex, did, name, uri, &replyend))
+	WHYF("DNAHELPER reply %s invalid -- ignored", alloca_toprint(-1, bufp, len));
+      else if (uri[0] == '\0')
+	WHYF("DNAHELPER reply %s contains empty URI -- ignored", alloca_toprint(-1, bufp, len));
+      else if (!str_is_uri(uri))
+	WHYF("DNAHELPER reply %s contains invalid URI -- ignored", alloca_toprint(-1, bufp, len));
+      else if (sidhex[0] == '\0')
+	WHYF("DNAHELPER reply %s contains empty token -- ignored", alloca_toprint(-1, bufp, len));
+      else if (!str_is_subscriber_id(sidhex))
+	WHYF("DNAHELPER reply %s contains invalid token -- ignored", alloca_toprint(-1, bufp, len));
+      else if (did[0] == '\0')
+	WHYF("DNAHELPER reply %s contains empty DID -- ignored", alloca_toprint(-1, bufp, len));
+      else if (!str_is_did(did))
+	WHYF("DNAHELPER reply %s contains invalid DID -- ignored", alloca_toprint(-1, bufp, len));
+      else if (strcmp(did, request_did) != 0)
+	WHYF("DNAHELPER reply %s contains mismatched DID -- ignored", alloca_toprint(-1, bufp, len));
+      else if (*replyend != '\n')
+	WHYF("DNAHELPER reply %s contains spurious trailing chars -- ignored", alloca_toprint(-1, bufp, len));
+      else {
 	if (debug & DEBUG_DNAHELPER)
-	  DEBUGF("DNA helper reply %s", alloca_toprint(-1, bufp, len));
+	  DEBUGF("DNAHELPER reply %s", alloca_toprint(-1, bufp, len));
+	unsigned char sid[SID_SIZE];
+	fromhex(sid, sidhex, SID_SIZE);
 	overlay_mdp_dnalookup_reply(&request_mdp_data, sid, uri, did, name);
-      } else {
-	WARNF("DNA helper invalid reply %s", alloca_toprint(-1, bufp, len));
       }
     }
   } else {
-    WARNF("DNA helper spurious reply %s", alloca_toprint(-1, bufp, len));
+    WARNF("DNAHELPER spurious output %s -- ignored", alloca_toprint(-1, bufp, len));
   }
 }
 
@@ -414,7 +436,7 @@ static void monitor_replies(struct sched_ent *alarm)
 	memmove(reply_buffer, bufp, len);
 	reply_bufend = reply_buffer + len;
       } else if (reply_bufend >= reply_buffer + sizeof reply_buffer) {
-	WHY("DNA helper reply buffer overrun");
+	WHY("DNAHELPER reply buffer overrun");
 	if (debug & DEBUG_DNAHELPER)
 	  DEBUGF("Discarding %s", alloca_toprint(-1, reply_buffer, sizeof reply_buffer));
 	reply_bufend = reply_buffer;
@@ -424,11 +446,12 @@ static void monitor_replies(struct sched_ent *alarm)
   }
   if (sched_replies.poll.revents & (POLLHUP | POLLERR)) {
     if (debug & DEBUG_DNAHELPER)
-      DEBUGF("Closing DNA helper stdout fd=%d", dna_helper_stdout);
+      DEBUGF("DNAHELPER closing stdout fd=%d", dna_helper_stdout);
     close(dna_helper_stdout);
     dna_helper_stdout = -1;
     unwatch(&sched_replies);
     sched_replies.poll.fd = -1;
+    dna_helper_kill();
   }
 }
 
@@ -442,12 +465,12 @@ static void monitor_errors(struct sched_ent *alarm)
   if (sched_errors.poll.revents & POLLIN) {
     char buffer[1024];
     ssize_t nread = read_nonblock(sched_errors.poll.fd, buffer, sizeof buffer);
-    if (nread > 0 && (debug & DEBUG_DNAHELPER))
+    if (nread > 0)
-      DEBUGF("DNA helper stderr %s", alloca_toprint(-1, buffer, nread));
+      WHYF("DNAHELPER stderr %s", alloca_toprint(-1, buffer, nread));
   }
   if (sched_errors.poll.revents & (POLLHUP | POLLERR)) {
     if (debug & DEBUG_DNAHELPER)
-      DEBUGF("Closing DNA helper stderr fd=%d", dna_helper_stderr);
+      DEBUGF("DNAHELPER closing stderr fd=%d", dna_helper_stderr);
     close(dna_helper_stderr);
     dna_helper_stderr = -1;
     unwatch(&sched_errors);
@@ -465,7 +488,7 @@ static void harvester(struct sched_ent *alarm)
   } else {
     const int delay_ms = 500;
     if (debug & DEBUG_DNAHELPER)
-      DEBUGF("DNA helper has died, pausing %d ms before restart", delay_ms);
+      DEBUGF("DNAHELPER process died, pausing %d ms before restart", delay_ms);
     dna_helper_pid = 0; // Will be set to -1 after delay
     schedrestart.function = restart_delayer;
     schedrestart.alarm = overlay_gettime_ms() + delay_ms;
@@ -477,7 +500,7 @@ static void restart_delayer(struct sched_ent *alarm)
 {
   if (dna_helper_pid == 0) {
     if (debug & DEBUG_DNAHELPER)
-      DEBUG("Re-enable DNA helper restart");
+      DEBUG("DNAHELPER re-enable restart");
     dna_helper_pid = -1;
   }
 }
@@ -486,7 +509,7 @@ int
 dna_helper_enqueue(overlay_mdp_frame *mdp, const char *did, const unsigned char *requestorSid)
 {
   if (debug & DEBUG_DNAHELPER)
-    DEBUGF("DNA helper request did=%s sid=%s", did, alloca_tohex_sid(requestorSid));
+    DEBUGF("DNAHELPER request did=%s sid=%s", did, alloca_tohex_sid(requestorSid));
   if (dna_helper_pid == 0)
     return 0;
   // Only try to restart a DNA helper process if the previous one is well and truly gone.
@@ -497,13 +520,13 @@ dna_helper_enqueue(overlay_mdp_frame *mdp, const char *did, const unsigned char
       /* Check if we have a helper configured. If not, then set
 	 dna_helper_pid to magic value of 0 so that we don't waste time
 	 in future looking up the dna helper configuration value. */
-      INFO("No DNA helper configured");
+      INFO("DNAHELPER none configured");
       dna_helper_pid = 0;
       return 0;
     }
     if (dna_helper_start(dna_helper_executable, dna_helper_arg1) == -1) {
       /* Something broke, bail out */
-      WHY("Failed to start DNA helper");
+      WHY("DNAHELPER start failed");
       return -1;
     }
   }
@@ -516,7 +539,7 @@ dna_helper_enqueue(overlay_mdp_frame *mdp, const char *did, const unsigned char
   if (dna_helper_stdin == -1)
     return 0;
   if (request_bufptr && request_bufptr != request_buffer) {
-    WARNF("Partially sent DNA helper request %s -- dropping new request", request_buffer);
+    WARNF("DNAHELPER partially sent request %s -- dropping new request", request_buffer);
     return 0;
   }
   char buffer[sizeof request_buffer];
@@ -527,17 +550,19 @@ dna_helper_enqueue(overlay_mdp_frame *mdp, const char *did, const unsigned char
   strbuf_putc(b, '|');
   strbuf_putc(b, '\n');
   if (strbuf_overrun(b)) {
-    WHYF("DNA helper request buffer overrun: %s -- request not sent", strbuf_str(b));
+    WHYF("DNAHELPER request buffer overrun: %s -- request not sent", strbuf_str(b));
     request_bufptr = request_bufend = NULL;
   } else {
     if (strbuf_str(b) != request_buffer) {
       if (strcmp(strbuf_str(b), request_buffer) != 0)
-	WARNF("Overwriting unsent DNA helper request %s", request_buffer);
+	WARNF("DNAHELPER overwriting unsent request %s", request_buffer);
       strcpy(request_buffer, strbuf_str(b));
     }
     request_bufptr = request_buffer;
     request_bufend = request_buffer + strbuf_len(b);
     request_mdp_data = mdp->out;
+    strncpy(request_did, did, sizeof request_did);
+    request_did[sizeof request_did - 1] = '\0';
   }
   if (dna_helper_started) {
     sched_requests.poll.fd = dna_helper_stdin;

fdqueue.c

@@ -96,6 +96,7 @@ int watch(struct sched_ent *alarm){
     if (fdcount>=MAX_WATCHED_FDS)
       return WHY("Too many file handles to watch");
     fd_callbacks[fdcount]=alarm;
+    alarm->poll.revents = 0;
     alarm->_poll_index=fdcount;
     fdcount++;
   }

serval.h

@@ -691,6 +691,7 @@ int str_is_subscriber_id(const char *sid);
 int strn_is_subscriber_id(const char *sid, size_t *lenp);
 int str_is_did(const char *did);
 int strn_is_did(const char *did, size_t *lenp);
+int str_is_uri(const char *uri);
 
 int stowSid(unsigned char *packet, int ofs, const char *sid);
 int stowDid(unsigned char *packet,int *ofs,char *did);
@@ -1567,7 +1568,7 @@ int dna_helper_shutdown();
 int dna_helper_enqueue(overlay_mdp_frame *mdp, const char *did, const unsigned char *requestorSid);
 int dna_return_resolution(overlay_mdp_frame *mdp, unsigned char *fromSid,
 			  const char *did,const char *name,const char *uri);
-int parseDnaReply(const char *bytes, size_t count, unsigned char *sid, char *did, char *name, char *uri);
+int parseDnaReply(const char *buf, size_t len, char *token, char *did, char *name, char *uri, const char **bufp);
 extern int sigPipeFlag;
 extern int sigIoFlag;
 void sigPipeHandler(int signal);

tests/dnahelper

@@ -24,6 +24,8 @@ source "${0%/*}/../testdefs.sh"
 setup() {
    setup_servald
    assert_no_servald_processes
+   setup_dnahelper
+   start_servald_instances +A
 }
 
 teardown() {
@@ -83,19 +85,97 @@ do
       # Empty URI
       echo "$token||$did|Eccles|"
       ;;
-   # Test malformed URI
+   *'|000051|')
-   # Test mismatched token
+      # Malformed URI
-   # Test empty token
+      echo "$token|Bluebottle|$did|Eccles|"
-   # Test long token
+      ;;
-   # Test empty DID
+   *'|000052|')
-   # Test mismatched DID
+      # Malformed URI
-   # Test long DID
+      echo "$token|sip://Sea goon|$did|Eccles|"
-   # Test malformed line
+      ;;
-   # Test long reply
+   *'|000053|')
-   # Test DONE timeout
+      # Malformed URI
-   # Test die
+      echo "$token|sip:|$did|Eccles|"
-   # Test fork and die
+      ;;
+   *'|000061|')
+      # Mismatched token
+      echo "1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF|did://$token/$did|$did|Eccles|"
+      ;;
+   *'|000062|')
+      # Empty token
+      echo "|did://$token/$did|$did|Eccles|"
+      ;;
+   *'|000063|')
+      # Invalid token (not SID)
+      echo "1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890ABCDEX|did://$token/$did|$did|Eccles|"
+      ;;
+   *'|000064|')
+      # Long token
+      echo "1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF0|did://$token/$did|$did|Eccles|"
+      ;;
+   *'|000065|')
+      # Short token
+      echo "1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890ABCDE|did://$token/$did|$did|Eccles|"
+      ;;
+   *'|000071|')
+      # Mismatched DID
+      echo "$token|sip://$token/$did|99999|Eccles|"
+      ;;
+   *'|000072|')
+      # Empty DID
+      echo "$token|sip://$token/$did||Eccles|"
+      ;;
+   *'|000073|')
+      # Invalid DID
+      echo "$token|sip://$token/$did|9999X|Eccles|"
+      ;;
+   *'|000074|')
+      # Long DID
+      echo "$token|sip://$token/$did|123456789012345678901234567890123|Eccles|"
+      ;;
+   *'|000075|')
+      # Short DID
+      echo "$token|sip://$token/$did|9999|Eccles|"
+      ;;
+   *'|000081|')
+      # Malformed reply, missing final delimiter
+      echo "$token|sip://$token/$did|9999|Eccles"
+      ;;
+   *'|000082|')
+      # Malformed reply, long name
+      echo "$token|sip://$token/$did|9999|Abcd efgh ijkl mnop qrst uvwx yzab cdef ghij klmn opqr stuv wxyz abcd efgh ijkl|"
+      ;;
+   *'|000083|')
+      # Malformed reply, empty line
+      echo
+      ;;
+   *'|000084|')
+      # Malformed reply, missing \n (so missing "DONE\n" line)
+      echo -n "$token|sip://$token@10.1.0.1|$did|Joe A. Bloggs|"
+      ;;
+   *'|000085|')
+      # Malformed reply, line too long
+      for i in 1 2 3 4 5 6 7 8 9 0; do
+         echo -n 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
+         echo -n 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
+         echo -n 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
+         echo -n 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
+         echo -n 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
+         echo -n 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
+         echo -n 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
+         echo -n 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
+         echo -n 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
+         echo -n 1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
+      done
+      echo
+      ;;
+   *'|00009|')
+      # Die unexpectedly
+      echo "goodbye cruel world" >&2
+      exit 42
+      ;;
    *'|'*'|')
+      echo "token=$token did=$did line=$line" >&2
       ;;
    *)
       echo "garbage line" >&2
@@ -111,15 +191,10 @@ EOF
    assertStdoutIs -e "STARTED\nToKeN|A|00000|B|\nDONE\n"
 }
 
-setup_single_instance() {
-   setup
-   setup_dnahelper
-   start_servald_instances +A
-}
-
 doc_ExecError="Non-existent DNA helper executable"
 setup_ExecError() {
-   setup
+   setup_servald
+   assert_no_servald_processes
    dnahelper=/non/existent
    assert [ ! -e "$dnahelper" ]
    start_servald_instances +A
@@ -128,40 +203,159 @@ test_ExecError() {
    executeOk_servald dna lookup 12345
 }
 
-doc_OneReply="DNA helper returns one line"
+doc_ReplyOk1="DNA helper returns one valid reply"
-setup_OneReply() {
+test_ReplyOk1() {
-   setup_single_instance
-}
-test_OneReply() {
    executeOk_servald dna lookup 00001
    assertStdoutIs -e "sip://$SIDA@10.1.0.1:00001:Joe A. Bloggs\n"
 }
 
-doc_TwoReplies="DNA helper returns two lines"
+doc_ReplyOk2="DNA helper returns two valid replies"
-setup_TwoReplies() {
+test_ReplyOk2() {
-   setup_single_instance
-}
-test_TwoReplies() {
    executeOk_servald dna lookup 00002
    assertStdoutIs -e "sip://$SIDA@10.1.0.1:00002:Joe A. Bloggs\nsip://$SIDA@10.1.0.2:00002:Joe B. Bloggs\n"
 }
 
-doc_ThreeReplies="DNA helper returns three lines"
+doc_ReplyOk3="DNA helper returns three valid replies"
-setup_ThreeReplies() {
+test_ReplyOk3() {
-   setup_single_instance
-}
-test_ThreeReplies() {
    executeOk_servald dna lookup 00003
    assertStdoutIs -e "sip://$SIDA@10.1.0.1:00003:Joe A. Bloggs\nsip://$SIDA@10.1.0.2:00003:Joe B. Bloggs\nsip://$SIDA@10.1.0.3:00003:Joe C. Bloggs\n"
 }
 
-doc_EmptyURI="DNA helper returns empry URI"
+doc_UriEmpty="DNA helper returns empty URI"
-setup_EmptyURI() {
+test_UriEmpty() {
-   setup_single_instance
-}
-test_EmptyURI() {
    executeOk_servald dna lookup 00004
    assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*empty URI'
+}
+
+doc_UriInvalid1="DNA helper returns invalid URI, missing scheme"
+test_UriInvalid1() {
+   executeOk_servald dna lookup 000051
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*Bluebottle.*invalid URI'
+}
+
+doc_UriInvalid2="DNA helper returns invalid URI, invalid char"
+test_UriInvalid2() {
+   executeOk_servald dna lookup 000052
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*sip://Sea goon.*invalid URI'
+}
+
+doc_UriInvalid3="DNA helper returns invalid URI, empty hierarchical part"
+test_UriInvalid3() {
+   executeOk_servald dna lookup 000053
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*sip:.*invalid URI'
+}
+
+doc_TokenMismatch="DNA helper returns mismatched token"
+test_TokenMismatch() {
+   executeOk_servald dna lookup 000061
+   assertStdoutIs ""
+   assertGrep --matches=0 "$LOGA" 'ERROR:.*DNAHELPER'
+}
+
+doc_TokenEmpty="DNA helper returns empty token"
+test_TokenEmpty() {
+   executeOk_servald dna lookup 000062
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*empty token'
+}
+
+doc_TokenInvalid="DNA helper returns invalid token"
+test_TokenInvalid() {
+   executeOk_servald dna lookup 000063
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*invalid token'
+}
+
+doc_TokenInvalidLong="DNA helper returns invalid token, too long"
+test_TokenInvalidLong() {
+   executeOk_servald dna lookup 000064
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*reply.*invalid'
+}
+
+doc_TokenInvalidShort="DNA helper returns invalid token, too short"
+test_TokenInvalidShort() {
+   executeOk_servald dna lookup 000065
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*invalid token'
+}
+
+doc_DidMismatch="DNA helper returns mismatched DID"
+test_DidMismatch() {
+   executeOk_servald dna lookup 000071
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*mismatched DID'
+}
+
+doc_DidEmpty="DNA helper returns empty DID"
+test_DidEmpty() {
+   executeOk_servald dna lookup 000072
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*empty DID'
+}
+
+doc_DidInvalid="DNA helper returns invalid DID"
+test_DidInvalid() {
+   executeOk_servald dna lookup 000073
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*invalid DID'
+}
+
+doc_DidInvalidLong="DNA helper returns invalid DID, too long"
+test_DidInvalidLong() {
+   executeOk_servald dna lookup 000074
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*reply.*invalid'
+}
+
+doc_DidInvalidShort="DNA helper returns invalid DID, too short"
+test_DidInvalidShort() {
+   executeOk_servald dna lookup 000075
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*invalid DID'
+}
+
+doc_ReplyInvalidMissingDelim="DNA helper returns invalid reply, missing delimiter"
+test_ReplyInvalidMissingDelim() {
+   executeOk_servald dna lookup 000081
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*reply.*invalid'
+}
+
+doc_ReplyInvalidLongName="DNA helper returns invalid reply, name too long"
+test_ReplyInvalidLongName() {
+   executeOk_servald dna lookup 000082
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*reply.*invalid'
+}
+
+doc_ReplyInvalidEmpty="DNA helper returns invalid reply, empty line"
+test_ReplyInvalidEmpty() {
+   executeOk_servald dna lookup 000083
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*reply "\\n" invalid'
+}
+
+doc_ReplyInvalidMissingNewline="DNA helper returns invalid reply, missing newline"
+test_ReplyInvalidMissingNewline() {
+   executeOk_servald dna lookup 000084
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*reply.*spurious'
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*reply timeout'
+}
+
+doc_HelperDies="DNA helper process dies unexpectedly and is restarted"
+test_HelperDies() {
+   executeOk_servald dna lookup 00009
+   assertStdoutIs ""
+   assertGrep "$LOGA" 'INFO:.*DNAHELPER.*process.*exited normally with status 42'
+   assertGrep "$LOGA" 'ERROR:.*DNAHELPER.*"goodbye cruel world\\n"'
+   executeOk_servald dna lookup 00001
+   assertStdoutIs -e "sip://$SIDA@10.1.0.1:00001:Joe A. Bloggs\n"
 }
 
 runTests "$@"