Change type of VCL_BLOB to newly introduced struct vrt_blob

Re-using struct vmod_priv helped simplicity, but had the following
shortcomings:

- in ddfda3d7 we documented what was
  long implied: return values of vmod functions are assumed immutable
  and so are blobs.

  So the blob pointer should be const, yet the struct vmod_priv pointer
  can't be.

- the struct vmod_priv free pointer implies that there would be
  automatic memory management provided by varnish core, yet this does not
  exist for a good reason: We would otherwise need to track all blobs
  ever returned by vmod functions/methods.

So we turn the data pointer into a const and remove the free function
callback.

We also add a type field, which is to be viewed similar to the miniobj
magic, except that it should not be asserted upon. The type field is
intended for additional (yet unreliable) checks of vmods using BLOBs
to carry around vmod-specific private data.

bin/varnishd/cache/cache_vrt.c

@@ -839,21 +839,24 @@ VRT_ipcmp(VCL_IP sua1, VCL_IP sua2)
 	return (VSA_Compare_IP(sua1, sua2));
 }
 
+/*
+ * the pointer passed as src must have at least VCL_TASK lifetime
+ */
 VCL_BLOB
-VRT_blob(VRT_CTX, const char *err, const void *src, size_t len)
+VRT_blob(VRT_CTX, const char *err, const void *src, size_t len, unsigned type)
 {
-	struct vmod_priv *p;
-	void *d;
+	struct vrt_blob *p;
 
 	p = (void *)WS_Alloc(ctx->ws, sizeof *p);
-	d = WS_Copy(ctx->ws, src, len);
-	if (p == NULL || d == NULL) {
+	if (p == NULL) {
 		VRT_fail(ctx, "Workspace overflow (%s)", err);
 		return (NULL);
 	}
-	memset(p, 0, sizeof *p);
+
+	p->type = type;
 	p->len = len;
-	p->priv = d;
+	p->blob = src;
+
 	return (p);
 }
 

bin/varnishd/cache/cache_vrt_var.c

@@ -891,12 +891,16 @@ VRT_BODY_L(resp)
 
 /*--------------------------------------------------------------------*/
 
+			/* digest */
+#define BLOB_HASH_TYPE 0x00d16357
+
 VCL_BLOB
 VRT_r_req_hash(VRT_CTX)
 {
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
-	return (VRT_blob(ctx, "req.hash", ctx->req->digest, DIGEST_LEN));
+	return (VRT_blob(ctx, "req.hash", ctx->req->digest, DIGEST_LEN,
+	    BLOB_HASH_TYPE));
 }
 
 VCL_BLOB
@@ -904,7 +908,8 @@ VRT_r_bereq_hash(VRT_CTX)
 {
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	CHECK_OBJ_NOTNULL(ctx->bo, BUSYOBJ_MAGIC);
-	return (VRT_blob(ctx, "bereq.hash", ctx->bo->digest, DIGEST_LEN));
+	return (VRT_blob(ctx, "bereq.hash", ctx->bo->digest, DIGEST_LEN,
+	    BLOB_HASH_TYPE));
 }
 
 /*--------------------------------------------------------------------*/

include/vrt.h

@@ -56,6 +56,8 @@
  *	HTTP_Copy() removed
  *	HTTP_Dup() added
  *	HTTP_Clone() added
+ *	changed type of VCL_BLOB to newly introduced struct vrt_blob *
+ *	changed VRT_blob()
  * 8.0 (2018-09-15)
  *	VRT_Strands() added
  *	VRT_StrandsWS() added
@@ -118,7 +120,7 @@
  *	vrt_acl type added
  */
 
-#define VRT_MAJOR_VERSION	8U
+#define VRT_MAJOR_VERSION	9U
 
 #define VRT_MINOR_VERSION	0U
 
@@ -150,6 +152,32 @@ struct strands {
 	const char	**p;
 };
 
+/*
+ * VCL_BLOB:
+ *
+ * opaque, immutable data (pointer + length), minimum lifetime is the VCL task.
+ *
+ * type (optional) is owned by the creator of the blob. blob consumers may use
+ * it for checks, but should not assert on it.
+ *
+ * The data behind the blob pointer is assumed to be immutable for the blob's
+ * lifetime.
+ *
+ * Memory management is either implicit or up to the vmod:
+ *
+ * - memory for shortlived blobs should come from the respective workspace
+ *
+ * - management of memory for longer lived blobs is up to the vmod
+ *   (in which case the blob will probably be embedded in an object or
+ *    referenced by other state with vcl lifetime)
+ */
+
+struct vrt_blob {
+	unsigned	type;
+	size_t		len;
+	const void	*blob;
+};
+
 /***********************************************************************
  * This is the central definition of the mapping from VCL types to
  * C-types.  The python scripts read these from here.
@@ -158,7 +186,7 @@ struct strands {
 
 typedef const struct vrt_acl *			VCL_ACL;
 typedef const struct director *			VCL_BACKEND;
-typedef const struct vmod_priv *		VCL_BLOB;
+typedef const struct vrt_blob *			VCL_BLOB;
 typedef const char *				VCL_BODY;
 typedef unsigned				VCL_BOOL;
 typedef int64_t					VCL_BYTES;
@@ -417,7 +445,7 @@ VCL_VOID VRT_hashdata(VRT_CTX, const char *str, ...);
 int VRT_strcmp(const char *s1, const char *s2);
 void VRT_memmove(void *dst, const void *src, unsigned len);
 VCL_BOOL VRT_ipcmp(VCL_IP, VCL_IP);
-VCL_BLOB VRT_blob(VRT_CTX, const char *, const void *, size_t);
+VCL_BLOB VRT_blob(VRT_CTX, const char *, const void *, size_t, unsigned);
 
 VCL_VOID VRT_Rollback(VRT_CTX, VCL_HTTP);
 

lib/libvmod_blob/vmod_blob.c

@@ -34,10 +34,13 @@
 #include "vcc_if.h"
 #include "vmod_blob.h"
 
+#define VMOD_BLOB_TYPE 0xfade4faa
+
 struct vmod_blob_blob {
 	unsigned magic;
 #define VMOD_BLOB_MAGIC 0xfade4fa9
-	struct vmod_priv blob;
+	struct vrt_blob blob;
+	void *freeptr;
 	char *encoding[__MAX_ENCODING][2];
 	pthread_mutex_t lock;
 };
@@ -105,14 +108,12 @@ static const struct vmod_blob_fptr {
 
 static char empty[1] = { '\0' };
 
-static const struct vmod_priv null_blob[1] =
-{
-	{
-		.priv = empty,
-		.len = 0,
-		.free = NULL
-	}
-};
+static const struct vrt_blob null_blob[1] = {{
+#define VMOD_BLOB_NULL_TYPE 0xfade4fa0
+	.type = VMOD_BLOB_NULL_TYPE,
+	.len = 0,
+	.blob = empty,
+}};
 
 static enum encoding
 parse_encoding(VCL_ENUM e)
@@ -187,6 +188,7 @@ vmod_blob__init(VRT_CTX, struct vmod_blob_blob **blobp, const char *vcl_name,
 {
 	struct vmod_blob_blob *b;
 	enum encoding dec = parse_encoding(decs);
+	void *buf;
 	ssize_t len;
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
@@ -201,36 +203,37 @@ vmod_blob__init(VRT_CTX, struct vmod_blob_blob **blobp, const char *vcl_name,
 	*blobp = b;
 	AZ(pthread_mutex_init(&b->lock, NULL));
 
+	b->blob.type = VMOD_BLOB_TYPE;
+
 	len = decode_l(dec, strings);
 	if (len == 0)
 		return;
 
 	assert(len > 0);
 
-	b->blob.priv = malloc(len);
-	if (b->blob.priv == NULL) {
+	buf = malloc(len);
+	if (buf == NULL) {
 		VERRNOMEM(ctx, "cannot create blob %s", vcl_name);
 		return;
 	}
 
 	errno = 0;
-	len = func[dec].decode(dec, b->blob.priv, len, -1, strings);
+	len = func[dec].decode(dec, buf, len, -1, strings);
 
 	if (len == -1) {
 		assert(errno == EINVAL);
-		free(b->blob.priv);
-		b->blob.priv = NULL;
+		free(buf);
 		VERR(ctx, "cannot create blob %s, illegal encoding beginning "
 		    "with \"%s\"", vcl_name, strings->p[0]);
 		return;
 	}
 	if (len == 0) {
-		b->blob.len = 0;
-		free(b->blob.priv);
-		b->blob.priv = NULL;
+		free(buf);
+		memcpy(&b->blob, null_blob, sizeof b->blob);
 		return;
 	}
 	b->blob.len = len;
+	b->blob.blob = b->freeptr = buf;
 }
 
 VCL_BLOB v_matchproto_(td_blob_blob_get)
@@ -238,7 +241,7 @@ vmod_blob_get(VRT_CTX, struct vmod_blob_blob *b)
 {
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	CHECK_OBJ_NOTNULL(b, VMOD_BLOB_MAGIC);
-	return &b->blob;
+	return (&b->blob);
 }
 
 VCL_STRING v_matchproto_(td_blob_blob_encode)
@@ -276,7 +279,7 @@ vmod_blob_encode(VRT_CTX, struct vmod_blob_blob *b, VCL_ENUM encs,
 					len =
 						func[enc].encode(
 							enc, kase, s, len,
-							b->blob.priv,
+							b->blob.blob,
 							b->blob.len);
 					assert(len >= 0);
 					if (len == 0) {
@@ -304,9 +307,9 @@ vmod_blob__fini(struct vmod_blob_blob **blobp)
 	b = *blobp;
 	*blobp = NULL;
 	CHECK_OBJ(b, VMOD_BLOB_MAGIC);
-	if (b->blob.priv != NULL) {
-		free(b->blob.priv);
-		b->blob.priv = NULL;
+	if (b->freeptr != NULL) {
+		free(b->freeptr);
+		b->blob.blob = NULL;
 	}
 	for (int i = 0; i < __MAX_ENCODING; i++)
 		for (int j = 0; j < 2; j++) {
@@ -326,9 +329,7 @@ VCL_BLOB v_matchproto_(td_blob_decode)
 vmod_decode(VRT_CTX, VCL_ENUM decs, VCL_INT length, VCL_STRANDS strings)
 {
 	enum encoding dec = parse_encoding(decs);
-	struct vmod_priv *b;
 	char *buf;
-	uintptr_t snap;
 	ssize_t len;
 	unsigned space;
 
@@ -337,12 +338,6 @@ vmod_decode(VRT_CTX, VCL_ENUM decs, VCL_INT length, VCL_STRANDS strings)
 	AN(strings);
 	CHECK_OBJ_NOTNULL(ctx->ws, WS_MAGIC);
 
-	snap = WS_Snapshot(ctx->ws);
-	if ((b = WS_Alloc(ctx->ws, sizeof(struct vmod_priv))) == NULL) {
-		ERRNOMEM(ctx, "cannot decode");
-		return NULL;
-	}
-
 	buf = WS_Front(ctx->ws);
 	space = WS_Reserve(ctx->ws, 0);
 
@@ -354,19 +349,17 @@ vmod_decode(VRT_CTX, VCL_ENUM decs, VCL_INT length, VCL_STRANDS strings)
 	if (len == -1) {
 		err_decode(ctx, strings->p[0]);
 		WS_Release(ctx->ws, 0);
-		WS_Reset(ctx->ws, snap);
 		return NULL;
 	}
 	if (len == 0) {
 		WS_Release(ctx->ws, 0);
-		WS_Reset(ctx->ws, snap);
 		return null_blob;
 	}
 	WS_Release(ctx->ws, len);
-	b->priv = buf;
-	b->len = len;
-	b->free = NULL;
-	return (b);
+
+	assert(len > 0);
+
+	return (VRT_blob(ctx, "blob.decode", buf, len, VMOD_BLOB_TYPE));
 }
 
 static VCL_STRING
@@ -387,7 +380,7 @@ encode(VRT_CTX, enum encoding enc, enum case_e kase, VCL_BLOB b)
 	buf = WS_Front(ctx->ws);
 	space = WS_Reserve(ctx->ws, 0);
 
-	len = func[enc].encode(enc, kase, buf, space, b->priv, b->len);
+	len = func[enc].encode(enc, kase, buf, space, b->blob, b->len);
 
 	if (len == -1) {
 		ERRNOMEM(ctx, "cannot encode");
@@ -424,7 +417,7 @@ vmod_transcode(VRT_CTX, VCL_ENUM decs, VCL_ENUM encs, VCL_ENUM case_s,
 	enum encoding dec = parse_encoding(decs);
 	enum encoding enc = parse_encoding(encs);
 	enum case_e kase = parse_case(case_s);
-	struct vmod_priv b;
+	struct vrt_blob b;
 	VCL_STRING r;
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
@@ -444,15 +437,15 @@ vmod_transcode(VRT_CTX, VCL_ENUM decs, VCL_ENUM encs, VCL_ENUM case_s,
 	size_t l = decode_l(dec, strings);
 	if (l == 0)
 		return "";
+
 	/* XXX: handle stack overflow? */
 	char buf[l];
-	b.free = NULL;
-	b.priv = buf;
 
 	if (length <= 0)
 		length = -1;
 	errno = 0;
 	b.len = func[dec].decode(dec, buf, l, length, strings);
+	b.blob = buf;
 
 	if (b.len == -1) {
 		err_decode(ctx, strings->p[0]);
@@ -487,7 +480,7 @@ vmod_same(VRT_CTX, VCL_BLOB b1, VCL_BLOB b2)
 		return 1;
 	if (b1 == NULL || b2 == NULL)
 		return 0;
-	return (b1->len == b2->len && b1->priv == b2->priv);
+	return (b1->len == b2->len && b1->blob == b2->blob);
 }
 
 VCL_BOOL v_matchproto_(td_blob_equal)
@@ -501,11 +494,11 @@ vmod_equal(VRT_CTX, VCL_BLOB b1, VCL_BLOB b2)
 		return 0;
 	if (b1->len != b2->len)
 		return 0;
-	if (b1->priv == b2->priv)
+	if (b1->blob == b2->blob)
 		return 1;
-	if (b1->priv == NULL || b2->priv == NULL)
+	if (b1->blob == NULL || b2->blob == NULL)
 		return 0;
-	return (memcmp(b1->priv, b2->priv, b1->len) == 0);
+	return (memcmp(b1->blob, b2->blob, b1->len) == 0);
 }
 
 VCL_INT v_matchproto_(td_blob_length)
@@ -521,21 +514,20 @@ vmod_length(VRT_CTX, VCL_BLOB b)
 VCL_BLOB v_matchproto_(td_blob_sub)
 vmod_sub(VRT_CTX, VCL_BLOB b, VCL_BYTES n, VCL_BYTES off)
 {
-	uintptr_t snap;
-	struct vmod_priv *sub;
-
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	assert(n >= 0);
 	assert(off >= 0);
 
-	if (b == NULL || b->len == 0 || b->priv == NULL) {
+	if (b == NULL || b->len == 0 || b->blob == NULL) {
 		ERR(ctx, "blob is empty in blob.sub()");
 		return NULL;
 	}
-	assert(b->len >= 0);
+
+	assert(b->len > 0);
+
 	if (off + n > b->len) {
 		VERR(ctx, "size %jd from offset %jd requires more bytes than "
-		     "blob length %d in blob.sub()",
+		     "blob length %zd in blob.sub()",
 		     (intmax_t)n, (intmax_t)off, b->len);
 		return NULL;
 	}
@@ -543,17 +535,7 @@ vmod_sub(VRT_CTX, VCL_BLOB b, VCL_BYTES n, VCL_BYTES off)
 	if (n == 0)
 		return null_blob;
 
-	snap = WS_Snapshot(ctx->ws);
-	if ((sub = WS_Alloc(ctx->ws, sizeof(*sub))) == NULL) {
-		ERRNOMEM(ctx, "Allocating BLOB result in blob.sub()");
-		return NULL;
-	}
-	if ((sub->priv = WS_Alloc(ctx->ws, n)) == NULL) {
-		VERRNOMEM(ctx, "Allocating %jd bytes in blob.sub()", (intmax_t)n);
-		WS_Reset(ctx->ws, snap);
-		return NULL;
-	}
-	memcpy(sub->priv, (char *)b->priv + off, n);
-	sub->len = n;
-	return sub;
+
+	return (VRT_blob(ctx, "blob.sub()",
+	    (const char *)b->blob + off, n, b->type));
 }

lib/libvmod_directors/vmod_shard.c

@@ -113,6 +113,8 @@ enum vmod_directors_shard_param_scope {
 
 struct vmod_directors_shard_param;
 
+#define VMOD_SHARD_SHARD_PARAM_BLOB		0xdf5ca116
+
 struct vmod_directors_shard_param {
 	unsigned				magic;
 #define VMOD_SHARD_SHARD_PARAM_MAGIC		0xdf5ca117
@@ -157,7 +159,7 @@ shard_param_task(VRT_CTX, const void *id,
     const struct vmod_directors_shard_param *pa);
 
 static const struct vmod_directors_shard_param *
-shard_param_blob(const VCL_BLOB blob);
+shard_param_blob(VCL_BLOB blob);
 
 static const struct vmod_directors_shard_param *
 vmod_shard_param_read(VRT_CTX, const void *id,
@@ -469,19 +471,19 @@ static uint32_t
 shard_blob_key(VCL_BLOB key_blob)
 {
 	uint8_t k[4] = { 0 };
-	uint8_t *b;
+	const uint8_t *b;
 	int i, ki;
 
-	assert(key_blob);
+	AN(key_blob);
+	AN(key_blob->blob);
 	assert(key_blob->len > 0);
-	assert(key_blob->priv != NULL);
 
 	if (key_blob->len >= 4)
 		ki = 0;
 	else
 		ki = 4 - key_blob->len;
 
-	b = key_blob->priv;
+	b = key_blob->blob;
 	for (i = 0; ki < 4; i++, ki++)
 		k[ki] = b[i];
 	assert(i <= key_blob->len);
@@ -570,7 +572,7 @@ shard_param_args(VRT_CTX,
 				return (NULL);
 			}
 			if (key_blob == NULL || key_blob->len <= 0 ||
-			    key_blob->priv == NULL) {
+			    key_blob->blob == NULL) {
 				sharddir_err(ctx, SLT_Error, "%s %s: "
 					     "by=BLOB but no or empty key_blob "
 					     "- using key 0",
@@ -1014,12 +1016,17 @@ vmod_shard_param_get_healthy(VRT_CTX,
 }
 
 static const struct vmod_directors_shard_param *
-shard_param_blob(const VCL_BLOB blob)
+shard_param_blob(VCL_BLOB blob)
 {
-	if (blob && blob->priv &&
-	    blob->len == sizeof(struct vmod_directors_shard_param) &&
-	    *(unsigned *)blob->priv == VMOD_SHARD_SHARD_PARAM_MAGIC)
-		return (blob->priv);
+	const struct vmod_directors_shard_param *p;
+
+	if (blob && blob->type == VMOD_SHARD_SHARD_PARAM_BLOB &&
+	    blob->blob != NULL &&
+	    blob->len == sizeof(struct vmod_directors_shard_param)) {
+		CAST_OBJ_NOTNULL(p, blob->blob, VMOD_SHARD_SHARD_PARAM_MAGIC);
+		return (p);
+	}
+
 	return (NULL);
 }
 
@@ -1027,20 +1034,9 @@ VCL_BLOB v_matchproto_(td_directors_shard_param_use)
 vmod_shard_param_use(VRT_CTX,
     struct vmod_directors_shard_param *p)
 {
-	struct vmod_priv *blob;
-
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	CHECK_OBJ_NOTNULL(p, VMOD_SHARD_SHARD_PARAM_MAGIC);
 
-	blob = (void *)WS_Alloc(ctx->ws, sizeof *blob);
-	if (blob == NULL) {
-		VRT_fail(ctx, "Workspace overflow (param.use())");
-		return (NULL);
-	}
-
-	memset(blob, 0, sizeof *blob);
-	blob->len = sizeof *p;
-	blob->priv = p;
-
-	return (blob);
+	return (VRT_blob(ctx, "xshard_param.use()", p, sizeof *p,
+	    VMOD_SHARD_SHARD_PARAM_BLOB));
 }