tdb2: fix remapping inside tdb_traverse_read
authorRusty Russell <rusty@rustcorp.com.au>
Mon, 13 Sep 2010 04:29:15 +0000 (13:59 +0930)
committerRusty Russell <rusty@rustcorp.com.au>
Mon, 13 Sep 2010 04:29:15 +0000 (13:59 +0930)
Because we (temporarily!) marked the tdb read_only=true, any remap would
mmap PROT_READ, and the next store would SEGV.

Pulled in more test infrastructure from tdb.

ccan/tdb2/io.c
ccan/tdb2/private.h
ccan/tdb2/tdb.c
ccan/tdb2/test/external-agent.c [new file with mode: 0644]
ccan/tdb2/test/external-agent.h [new file with mode: 0644]
ccan/tdb2/test/logging.c
ccan/tdb2/test/logging.h
ccan/tdb2/test/run-001-encode.c
ccan/tdb2/test/run-001-fls.c
ccan/tdb2/test/run-remap-in-read_traverse.c [new file with mode: 0644]

index 9f0582e0df338998302422aeba94396ea72b9a15..aa79def1e38cc5b49b500d1b513eb9dc446fbbb7 100644 (file)
@@ -48,8 +48,7 @@ void tdb_mmap(struct tdb_context *tdb)
        if (tdb->flags & TDB_NOMMAP)
                return;
 
-       tdb->map_ptr = mmap(NULL, tdb->map_size, 
-                           PROT_READ|(tdb->read_only? 0:PROT_WRITE), 
+       tdb->map_ptr = mmap(NULL, tdb->map_size, tdb->mmap_flags,
                            MAP_SHARED, tdb->fd, 0);
 
        /*
index 091928095eba3daaf50323302fc0f59a277a0e7f..9b225521c74808d62bb9c559a08abd7bb18157df 100644 (file)
@@ -282,9 +282,12 @@ struct tdb_context {
        /* How much space has been mapped (<= current file size) */
        tdb_len_t map_size;
 
-       /* Opened read-only? */
+       /* Operating read-only? (Opened O_RDONLY, or in traverse_read) */
        bool read_only;
 
+       /* mmap read only? */
+       int mmap_flags;
+
        /* Error code for last tdb error. */
        enum TDB_ERROR ecode; 
 
index 1a229dc2b94c1538e5093a8df55d74b8dd8d74f8..7dc5aafbfc254c7d865813e34172e0b29893423d 100644 (file)
@@ -234,8 +234,11 @@ struct tdb_context *tdb_open(const char *name, int tdb_flags,
                tdb->read_only = true;
                /* read only databases don't do locking */
                tdb->flags |= TDB_NOLOCK;
-       } else
+               tdb->mmap_flags = PROT_READ;
+       } else {
                tdb->read_only = false;
+               tdb->mmap_flags = PROT_READ | PROT_WRITE;
+       }
 
        /* internal databases don't need any of the rest. */
        if (tdb->flags & TDB_INTERNAL) {
diff --git a/ccan/tdb2/test/external-agent.c b/ccan/tdb2/test/external-agent.c
new file mode 100644 (file)
index 0000000..bcf5d21
--- /dev/null
@@ -0,0 +1,194 @@
+#include "external-agent.h"
+#include "logging.h"
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <err.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <limits.h>
+#include <string.h>
+#include <errno.h>
+#include <ccan/tdb2/private.h>
+#include <ccan/tap/tap.h>
+#include <stdio.h>
+#include <stdarg.h>
+
+static struct tdb_context *tdb;
+
+#if 1 /* FIXME */
+static unsigned int locking_would_block = 0;
+static bool nonblocking_locks = false;
+#endif
+
+static enum agent_return do_operation(enum operation op, const char *name)
+{
+       TDB_DATA k;
+       enum agent_return ret;
+       TDB_DATA data;
+
+       if (op != OPEN && !tdb) {
+               diag("external: No tdb open!");
+               return OTHER_FAILURE;
+       }
+
+       k.dptr = (void *)name;
+       k.dsize = strlen(name);
+
+       locking_would_block = 0;
+       switch (op) {
+       case OPEN:
+               if (tdb) {
+                       diag("Already have tdb %s open", tdb->name);
+                       return OTHER_FAILURE;
+               }
+               tdb = tdb_open(name, TDB_DEFAULT, O_RDWR, 0, &tap_log_attr);
+               if (!tdb) {
+                       if (!locking_would_block)
+                               diag("Opening tdb gave %s", strerror(errno));
+                       ret = OTHER_FAILURE;
+               } else
+                       ret = SUCCESS;
+               break;
+       case FETCH:
+               data = tdb_fetch(tdb, k);
+               if (data.dptr == NULL) {
+                       if (tdb_error(tdb) == TDB_ERR_NOEXIST)
+                               ret = FAILED;
+                       else
+                               ret = OTHER_FAILURE;
+               } else if (data.dsize != k.dsize
+                          || memcmp(data.dptr, k.dptr, k.dsize) != 0) {
+                       ret = OTHER_FAILURE;
+               } else {
+                       ret = SUCCESS;
+               }
+               free(data.dptr);
+               break;
+       case STORE:
+               ret = tdb_store(tdb, k, k, 0) == 0 ? SUCCESS : OTHER_FAILURE;
+               break;
+#if 0 /* FIXME */
+       case TRANSACTION_START:
+               ret = tdb_transaction_start(tdb) == 0 ? SUCCESS : OTHER_FAILURE;
+               break;
+       case TRANSACTION_COMMIT:
+               ret = tdb_transaction_commit(tdb)==0 ? SUCCESS : OTHER_FAILURE;
+               break;
+       case NEEDS_RECOVERY:
+               ret = tdb_needs_recovery(tdb) ? SUCCESS : FAILED;
+               break;
+#endif
+       case CHECK:
+               ret = tdb_check(tdb, NULL, NULL) == 0 ? SUCCESS : OTHER_FAILURE;
+               break;
+       case CLOSE:
+               ret = tdb_close(tdb) == 0 ? SUCCESS : OTHER_FAILURE;
+               tdb = NULL;
+               break;
+       default:
+               ret = OTHER_FAILURE;
+       }
+
+       if (locking_would_block)
+               ret = WOULD_HAVE_BLOCKED;
+
+       return ret;
+}
+
+struct agent {
+       int cmdfd, responsefd;
+};
+
+/* Do this before doing any tdb stuff.  Return handle, or NULL. */
+struct agent *prepare_external_agent(void)
+{
+       int pid, ret;
+       int command[2], response[2];
+       char name[1+PATH_MAX];
+
+       if (pipe(command) != 0 || pipe(response) != 0)
+               return NULL;
+
+       pid = fork();
+       if (pid < 0)
+               return NULL;
+
+       if (pid != 0) {
+               struct agent *agent = malloc(sizeof(*agent));
+
+               close(command[0]);
+               close(response[1]);
+               agent->cmdfd = command[1];
+               agent->responsefd = response[0];
+               return agent;
+       }
+
+       close(command[1]);
+       close(response[0]);
+
+       /* We want to fail, not block. */
+       nonblocking_locks = true;
+       log_prefix = "external: ";
+       while ((ret = read(command[0], name, sizeof(name))) > 0) {
+               enum agent_return result;
+
+               result = do_operation(name[0], name+1);
+               if (write(response[1], &result, sizeof(result))
+                   != sizeof(result))
+                       err(1, "Writing response");
+       }
+       exit(0);
+}
+
+/* Ask the external agent to try to do an operation. */
+enum agent_return external_agent_operation(struct agent *agent,
+                                          enum operation op,
+                                          const char *name)
+{
+       enum agent_return res;
+       unsigned int len;
+       char *string;
+
+       if (!name)
+               name = "";
+       len = 1 + strlen(name) + 1;
+       string = malloc(len);
+
+       string[0] = op;
+       strcpy(string+1, name);
+
+       if (write(agent->cmdfd, string, len) != len
+           || read(agent->responsefd, &res, sizeof(res)) != sizeof(res))
+               res = AGENT_DIED;
+
+       free(string);
+       return res;
+}
+
+const char *agent_return_name(enum agent_return ret)
+{
+       return ret == SUCCESS ? "SUCCESS"
+               : ret == WOULD_HAVE_BLOCKED ? "WOULD_HAVE_BLOCKED"
+               : ret == AGENT_DIED ? "AGENT_DIED"
+               : ret == FAILED ? "FAILED"
+               : ret == OTHER_FAILURE ? "OTHER_FAILURE"
+               : "**INVALID**";
+}
+
+const char *operation_name(enum operation op)
+{
+       switch (op) {
+       case OPEN: return "OPEN";
+       case FETCH: return "FETCH";
+       case STORE: return "STORE";
+       case CHECK: return "CHECK";
+#if 0
+       case TRANSACTION_START: return "TRANSACTION_START";
+       case TRANSACTION_COMMIT: return "TRANSACTION_COMMIT";
+       case NEEDS_RECOVERY: return "NEEDS_RECOVERY";
+#endif
+       case CLOSE: return "CLOSE";
+       }
+       return "**INVALID**";
+}
diff --git a/ccan/tdb2/test/external-agent.h b/ccan/tdb2/test/external-agent.h
new file mode 100644 (file)
index 0000000..6d5c530
--- /dev/null
@@ -0,0 +1,42 @@
+#ifndef TDB2_TEST_EXTERNAL_AGENT_H
+#define TDB2_TEST_EXTERNAL_AGENT_H
+
+/* For locking tests, we need a different process to try things at
+ * various times. */
+enum operation {
+       OPEN,
+       FETCH,
+       STORE,
+#if 0
+       TRANSACTION_START,
+       TRANSACTION_COMMIT,
+       NEEDS_RECOVERY,
+#endif
+       CHECK,
+       CLOSE,
+};
+
+/* Do this before doing any tdb stuff.  Return handle, or -1. */
+struct agent *prepare_external_agent(void);
+
+enum agent_return {
+       SUCCESS,
+       WOULD_HAVE_BLOCKED,
+       AGENT_DIED,
+       FAILED, /* For fetch, or NEEDS_RECOVERY */
+       OTHER_FAILURE,
+};
+
+/* Ask the external agent to try to do an operation. 
+ * name == tdb name for OPEN/OPEN_WITH_CLEAR_IF_FIRST,
+ * record name for FETCH/STORE (store stores name as data too)
+ */
+enum agent_return external_agent_operation(struct agent *handle,
+                                          enum operation op,
+                                          const char *name);
+
+/* Mapping enum -> string. */
+const char *agent_return_name(enum agent_return ret);
+const char *operation_name(enum operation op);
+
+#endif /* TDB2_TEST_EXTERNAL_AGENT_H */
index 7fd3b1f2345e209b347c7a6e5fc380d38d61ec81..85bdc064c80a7fe179f848e789dc62d1806f2d15 100644 (file)
@@ -6,6 +6,8 @@
 #include "logging.h"
 
 unsigned tap_log_messages;
+const char *log_prefix = "";
+bool suppress_logging;
 
 union tdb_attribute tap_log_attr = {
        .log = { .base = { .attr = TDB_ATTRIBUTE_LOG },
@@ -19,10 +21,16 @@ void tap_log_fn(struct tdb_context *tdb,
        va_list ap;
        char *p;
 
+       if (suppress_logging)
+               return;
+
        va_start(ap, fmt);
        if (vasprintf(&p, fmt, ap) == -1)
                abort();
-       diag("tdb log level %u: %s", level, p);
+       /* Strip trailing \n: diag adds it. */
+       if (p[strlen(p)-1] == '\n')
+               p[strlen(p)-1] = '\0';
+       diag("tdb log level %u: %s%s", level, log_prefix, p);
        free(p);
        if (level != TDB_DEBUG_TRACE)
                tap_log_messages++;
index 510cfb88562024cf20cdd3f3c7431056e586619c..85b417d355282106ca5dca306521f180d1879481 100644 (file)
@@ -4,8 +4,9 @@
 #include <stdbool.h>
 #include <string.h>
 
+extern bool suppress_logging;
+extern const char *log_prefix;
 extern unsigned tap_log_messages;
-
 extern union tdb_attribute tap_log_attr;
 
 void tap_log_fn(struct tdb_context *tdb,
index a62cb1d70998fb291e3fb8d7c9570722ea4513b8..f04be3398ad9bb2d856dc8c6dc13435ca30341bd 100644 (file)
@@ -3,6 +3,7 @@
 #include <ccan/tdb2/lock.c>
 #include <ccan/tdb2/hash.c>
 #include <ccan/tdb2/io.c>
+#include <ccan/tdb2/check.c>
 #include <ccan/tap/tap.h>
 #include "logging.h"
 
index 6b6df48d397c102220fa56f4c192ba35ec6d56c5..d5f24925f7918d10dd9e4b2ba552ad9ffea03f46 100644 (file)
@@ -3,6 +3,7 @@
 #include <ccan/tdb2/lock.c>
 #include <ccan/tdb2/io.c>
 #include <ccan/tdb2/hash.c>
+#include <ccan/tdb2/check.c>
 #include <ccan/tap/tap.h>
 
 static unsigned int dumb_fls(uint64_t num)
diff --git a/ccan/tdb2/test/run-remap-in-read_traverse.c b/ccan/tdb2/test/run-remap-in-read_traverse.c
new file mode 100644 (file)
index 0000000..77fdc9e
--- /dev/null
@@ -0,0 +1,61 @@
+/* We had a bug where we marked the tdb read-only for a tdb_traverse_read.
+ * If we then expanded the tdb, we would remap read-only, and later SEGV. */
+#include <ccan/tdb2/tdb.c>
+#include <ccan/tdb2/free.c>
+#include <ccan/tdb2/lock.c>
+#include <ccan/tdb2/io.c>
+#include <ccan/tdb2/hash.c>
+#include <ccan/tdb2/check.c>
+#include <ccan/tdb2/traverse.c>
+#include <ccan/tap/tap.h>
+#include "external-agent.h"
+#include "logging.h"
+
+static bool file_larger(int fd, tdb_len_t size)
+{
+       struct stat st;
+
+       fstat(fd, &st);
+       return st.st_size != size;
+}
+
+static unsigned add_records_to_grow(struct agent *agent, int fd, tdb_len_t size)
+{
+       unsigned int i;
+
+       for (i = 0; !file_larger(fd, size); i++) {
+               char data[20];
+               sprintf(data, "%i", i);
+               if (external_agent_operation(agent, STORE, data) != SUCCESS)
+                       return 0;
+       }
+       diag("Added %u records to grow file", i);
+       return i;
+}
+
+int main(int argc, char *argv[])
+{
+       unsigned int i;
+       struct agent *agent;
+       struct tdb_context *tdb;
+       struct tdb_data d = { (unsigned char *)"hello", 5 };
+       const char filename[] = "run-remap-in-read_traverse.tdb";
+
+       plan_tests(4);
+
+       agent = prepare_external_agent();
+
+       tdb = tdb_open(filename, TDB_DEFAULT,
+                      O_RDWR|O_CREAT|O_TRUNC, 0600, &tap_log_attr);
+
+       ok1(external_agent_operation(agent, OPEN, filename) == SUCCESS);
+       i = add_records_to_grow(agent, tdb->fd, tdb->map_size);
+
+       /* Do a read traverse. */
+       ok1(tdb_traverse_read(tdb, NULL, NULL) == i);
+
+       /* Now store something! */
+       ok1(tdb_store(tdb, d, d, TDB_INSERT) == 0);
+       ok1(tap_log_messages == 0);
+       return exit_status();
+}