9:18 a.m.
I accidentally passed an incorrect parameter to a next function in a
filter: 'next->can_write (handle)' (should be: 'next->can_write
(next)').
Static checking didn't catch this because the handle has type void*
and it took me a very long time to work out what the problem was.
Add some dynamic asserts that we are passing correct structs around to
help in future and to better catch any internal inconsistency.
With this commit, the same mistake above results in the assertion:
nbdkit: threadlocal.c:260: threadlocal_push_context: Assertion `ctx->magic ==
CONTEXT_MAGIC' failed.
with stack trace:
#6 0x0000000000420639 in threadlocal_push_context (ctx=0x7f8415bd7540)
at threadlocal.c:260
#7 0x0000000000407440 in backend_can_write (c=0x7f8415bd7540) at backend.c:458
#8 0x00007f841667a3eb in readonly_can_write (next=0xcfca160,
handle=0x7f8415bd7540) at readonly.c:92
---
server/internal.h | 9 +++++++++
server/backend.c | 26 ++++++++++++++++++++++++++
server/connections.c | 1 +
server/threadlocal.c | 20 ++++++++++++++++++--
4 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/server/internal.h b/server/internal.h
index 573e9d646..3bc250fa6 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -226,6 +226,9 @@ enum {
struct context {
struct nbdkit_next_ops next; /* Must be first member, for ABI reasons */
+ uint64_t magic; /* Magic number used to validate struct. */
+#define CONTEXT_MAGIC 0xc011c011c011
+
void *handle; /* Plugin or filter handle. */
struct backend *b; /* Backend that provided handle. */
struct context *c_next; /* Underlying context, only when b->next != NULL. */
@@ -257,6 +260,9 @@ typedef enum {
} conn_status;
struct connection {
+ uint64_t magic; /* Magic number used to validate struct. */
+#define CONN_MAGIC 0xc055c055c055
+
/* Listed in precedence order: do not grab earlier locks in this list
* while holding a later lock.
*/
@@ -355,6 +361,9 @@ struct backend {
*/
struct backend *next;
+ uint64_t magic; /* Magic number used to validate struct. */
+#define BACKEND_MAGIC 0xbacbacbacbac
+
/* A unique index used to fetch the handle from the connections
* object. The plugin (last in the chain) has index 0, and the
* filters have index 1, 2, ... depending how "far" they are from
diff --git a/server/backend.c b/server/backend.c
index b6c445af7..6232b69d1 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -67,6 +67,7 @@ void
backend_init (struct backend *b, struct backend *next, size_t index,
const char *filename, void *dl, const char *type)
{
+ b->magic = BACKEND_MAGIC;
b->next = next;
b->i = index;
b->type = type;
@@ -254,6 +255,7 @@ backend_open (struct backend *b, int readonly, const char
*exportname,
nbdkit_error ("malloc: %m");
return NULL;
}
+ c->magic = CONTEXT_MAGIC;
PUSH_CONTEXT_FOR_SCOPE (c);
controlpath_debug ("%s: open readonly=%d exportname=\"%s\"
tls=%d",
@@ -311,6 +313,7 @@ backend_prepare (struct context *c)
PUSH_CONTEXT_FOR_SCOPE (c);
struct backend *b = c->b;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle);
assert (c->state & HANDLE_OPEN);
@@ -338,6 +341,8 @@ backend_finalize (struct context *c)
PUSH_CONTEXT_FOR_SCOPE (c);
struct backend *b = c->b;
+ assert (b->magic == BACKEND_MAGIC);
+
/* Call these in reverse order to .prepare above, starting from the
* filter furthest away from the plugin, and matching .close order.
*/
@@ -368,6 +373,7 @@ backend_close (struct context *c)
struct context *c_next = c->c_next;
/* outer-to-inner order, opposite .open */
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle);
assert (c->state & HANDLE_OPEN);
controlpath_debug ("%s: close", b->name);
@@ -394,6 +400,7 @@ backend_export_description (struct context *c)
struct backend *b = c->b;
const char *s;
+ assert (b->magic == BACKEND_MAGIC);
controlpath_debug ("%s: export_description", b->name);
assert (c->handle && (c->state & HANDLE_CONNECTED));
@@ -415,6 +422,7 @@ backend_get_size (struct context *c)
PUSH_CONTEXT_FOR_SCOPE (c);
struct backend *b = c->b;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->exportsize == -1) {
controlpath_debug ("%s: get_size", b->name);
@@ -431,6 +439,7 @@ backend_block_size (struct context *c,
struct backend *b = c->b;
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->minimum_block_size != -1) {
*minimum = c->minimum_block_size;
@@ -456,6 +465,7 @@ backend_can_write (struct context *c)
PUSH_CONTEXT_FOR_SCOPE (c);
struct backend *b = c->b;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->can_write == -1) {
controlpath_debug ("%s: can_write", b->name);
@@ -470,6 +480,7 @@ backend_can_flush (struct context *c)
PUSH_CONTEXT_FOR_SCOPE (c);
struct backend *b = c->b;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->can_flush == -1) {
controlpath_debug ("%s: can_flush", b->name);
@@ -484,6 +495,7 @@ backend_is_rotational (struct context *c)
PUSH_CONTEXT_FOR_SCOPE (c);
struct backend *b = c->b;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->is_rotational == -1) {
controlpath_debug ("%s: is_rotational", b->name);
@@ -499,6 +511,7 @@ backend_can_trim (struct context *c)
struct backend *b = c->b;
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->can_trim == -1) {
controlpath_debug ("%s: can_trim", b->name);
@@ -519,6 +532,7 @@ backend_can_zero (struct context *c)
struct backend *b = c->b;
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->can_zero == -1) {
controlpath_debug ("%s: can_zero", b->name);
@@ -539,6 +553,7 @@ backend_can_fast_zero (struct context *c)
struct backend *b = c->b;
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->can_fast_zero == -1) {
controlpath_debug ("%s: can_fast_zero", b->name);
@@ -558,6 +573,7 @@ backend_can_extents (struct context *c)
PUSH_CONTEXT_FOR_SCOPE (c);
struct backend *b = c->b;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->can_extents == -1) {
controlpath_debug ("%s: can_extents", b->name);
@@ -573,6 +589,7 @@ backend_can_fua (struct context *c)
struct backend *b = c->b;
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->can_fua == -1) {
controlpath_debug ("%s: can_fua", b->name);
@@ -592,6 +609,7 @@ backend_can_multi_conn (struct context *c)
PUSH_CONTEXT_FOR_SCOPE (c);
struct backend *b = c->b;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->can_multi_conn == -1) {
controlpath_debug ("%s: can_multi_conn", b->name);
@@ -606,6 +624,7 @@ backend_can_cache (struct context *c)
PUSH_CONTEXT_FOR_SCOPE (c);
struct backend *b = c->b;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
if (c->can_cache == -1) {
controlpath_debug ("%s: can_cache", b->name);
@@ -623,6 +642,7 @@ backend_pread (struct context *c,
struct backend *b = c->b;
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
assert (backend_valid_range (c, offset, count));
assert (flags == 0);
@@ -645,6 +665,7 @@ backend_pwrite (struct context *c,
bool fua = !!(flags & NBDKIT_FLAG_FUA);
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
assert (c->can_write == 1);
assert (backend_valid_range (c, offset, count));
@@ -668,6 +689,7 @@ backend_flush (struct context *c,
struct backend *b = c->b;
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
assert (c->can_flush == 1);
assert (flags == 0);
@@ -689,6 +711,7 @@ backend_trim (struct context *c,
bool fua = !!(flags & NBDKIT_FLAG_FUA);
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
assert (c->can_write == 1);
assert (c->can_trim == 1);
@@ -716,6 +739,7 @@ backend_zero (struct context *c,
bool fast = !!(flags & NBDKIT_FLAG_FAST_ZERO);
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
assert (c->can_write == 1);
assert (c->can_zero > NBDKIT_ZERO_NONE);
@@ -784,6 +808,7 @@ backend_extents (struct context *c,
struct backend *b = c->b;
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
assert (c->can_extents >= 0);
assert (backend_valid_range (c, offset, count));
@@ -815,6 +840,7 @@ backend_cache (struct context *c,
struct backend *b = c->b;
int r;
+ assert (b->magic == BACKEND_MAGIC);
assert (c->handle && (c->state & HANDLE_CONNECTED));
assert (c->can_cache > NBDKIT_CACHE_NONE);
assert (backend_valid_range (c, offset, count));
diff --git a/server/connections.c b/server/connections.c
index a354dc026..13b67f3f9 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -258,6 +258,7 @@ new_connection (int sockin, int sockout, int nworkers)
perror ("malloc");
return NULL;
}
+ conn->magic = CONN_MAGIC;
conn->status_pipe[0] = conn->status_pipe[1] = -1;
pthread_mutex_init (&conn->request_lock, NULL);
diff --git a/server/threadlocal.c b/server/threadlocal.c
index 784a08361..a75b2dcee 100644
--- a/server/threadlocal.c
+++ b/server/threadlocal.c
@@ -225,8 +225,12 @@ struct connection *
threadlocal_get_conn (void)
{
struct threadlocal *threadlocal = pthread_getspecific (threadlocal_key);
+ struct connection *conn;
- return threadlocal ? threadlocal->conn : NULL;
+ conn = threadlocal ? threadlocal->conn : NULL;
+ if (conn)
+ assert (conn->magic == CONN_MAGIC);
+ return conn;
}
/* Get the current context associated with this thread, if available */
@@ -234,8 +238,12 @@ struct context *
threadlocal_get_context (void)
{
struct threadlocal *threadlocal = pthread_getspecific (threadlocal_key);
+ struct context *ctx;
- return threadlocal ? threadlocal->ctx : NULL;
+ ctx = threadlocal ? threadlocal->ctx : NULL;
+ if (ctx)
+ assert (ctx->magic == CONTEXT_MAGIC);
+ return ctx;
}
/* Set (or clear) the context using the current thread. This function
@@ -248,10 +256,18 @@ threadlocal_push_context (struct context *ctx)
struct threadlocal *threadlocal = pthread_getspecific (threadlocal_key);
struct context *ret = NULL;
+ if (ctx)
+ assert (ctx->magic == CONTEXT_MAGIC);
if (threadlocal) {
ret = threadlocal->ctx;
threadlocal->ctx = ctx;
}
+ /* Note for validation of structs: ret here may be a freed pointer.
+ * It is only returned here so that PUSH_CONTEXT_FOR_SCOPE can save
+ * it on the caller stack and restore it when the function returns.
+ * Since the function should never touch this we don't have to
+ * validate it.
+ */
return ret;
}
--
2.42.0