From: Rusty Russell Date: Mon, 13 Sep 2010 04:29:15 +0000 (+0930) Subject: tdb2: fix remapping inside tdb_traverse_read X-Git-Url: http://git.ozlabs.org/?p=ccan;a=commitdiff_plain;h=0455668028cfe8f0417037975bc3d7dd742974db tdb2: fix remapping inside tdb_traverse_read 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. --- diff --git a/ccan/tdb2/io.c b/ccan/tdb2/io.c index 9f0582e0..aa79def1 100644 --- a/ccan/tdb2/io.c +++ b/ccan/tdb2/io.c @@ -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); /* diff --git a/ccan/tdb2/private.h b/ccan/tdb2/private.h index 09192809..9b225521 100644 --- a/ccan/tdb2/private.h +++ b/ccan/tdb2/private.h @@ -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; diff --git a/ccan/tdb2/tdb.c b/ccan/tdb2/tdb.c index 1a229dc2..7dc5aafb 100644 --- a/ccan/tdb2/tdb.c +++ b/ccan/tdb2/tdb.c @@ -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 index 00000000..bcf5d215 --- /dev/null +++ b/ccan/tdb2/test/external-agent.c @@ -0,0 +1,194 @@ +#include "external-agent.h" +#include "logging.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +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 index 00000000..6d5c5306 --- /dev/null +++ b/ccan/tdb2/test/external-agent.h @@ -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 */ diff --git a/ccan/tdb2/test/logging.c b/ccan/tdb2/test/logging.c index 7fd3b1f2..85bdc064 100644 --- a/ccan/tdb2/test/logging.c +++ b/ccan/tdb2/test/logging.c @@ -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++; diff --git a/ccan/tdb2/test/logging.h b/ccan/tdb2/test/logging.h index 510cfb88..85b417d3 100644 --- a/ccan/tdb2/test/logging.h +++ b/ccan/tdb2/test/logging.h @@ -4,8 +4,9 @@ #include #include +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, diff --git a/ccan/tdb2/test/run-001-encode.c b/ccan/tdb2/test/run-001-encode.c index a62cb1d7..f04be339 100644 --- a/ccan/tdb2/test/run-001-encode.c +++ b/ccan/tdb2/test/run-001-encode.c @@ -3,6 +3,7 @@ #include #include #include +#include #include #include "logging.h" diff --git a/ccan/tdb2/test/run-001-fls.c b/ccan/tdb2/test/run-001-fls.c index 6b6df48d..d5f24925 100644 --- a/ccan/tdb2/test/run-001-fls.c +++ b/ccan/tdb2/test/run-001-fls.c @@ -3,6 +3,7 @@ #include #include #include +#include #include 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 index 00000000..77fdc9ef --- /dev/null +++ b/ccan/tdb2/test/run-remap-in-read_traverse.c @@ -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 +#include +#include +#include +#include +#include +#include +#include +#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(); +}