Clarify and fix attribute data lifetime

Modify kdump_attr_ref_get() to return data that is guaranteed to stay
valid. In particular:

- string attributes are duplicated
- blob and bitmap attributes increment their reference count

Of course, the caller should free these resources again when the data is no
longer needed. Add a kdump_attr_discard() function for that purpose.

Rationale

When used on a string, blob or bitmap attribute, kdump_get_attr(),
kdump_get_typed_attr() and derivatives will return a pointer to the
underlying data without any lifetime guarantees. Such pointers may become
dangling whenever the caller touches the containing dump file object. In
the extreme case, another thread may invalidate the pointer before it is
even returned to the caller.

However, most users are happy with these limitations, so keep the simple
API unchanged, only updating documentation.

There is also an attribute API based on references (kdump_attr_ref_t).
This API was primarily intended to allow iterating over attributes in a
thread-safe manner (although this is not yet fully implemented, cf. ptesarik#71),
but it can be also used to get attribute values, see kdump_attr_ref_get().
Since this API is designed as thread-safe, it makes little sense to have a
thread-unsafe function to get attribute value.

It sounds fair that users of this API are now required to adapt their code
to avoid memory leaks.

Fixes: ptesarik#82
Signed-off-by: Petr Tesarik <[email protected]>

Author: ptesarik

NEWS

@@ -2,6 +2,7 @@ next
 ----
   * Incompatible API changes:
     - kdump_get_typed_attr(): parameters and type mismatch behaviour
+    - kdump_attr_ref_get(): result must be discarded
   * Support flattened ELF dump files.
   * Support partially rearranged makedumpfile split files.
   * Parse QEMU CPU state ELF notes.

examples/dumpattr.c

@@ -134,6 +134,7 @@ show_attr(kdump_ctx_t *ctx, kdump_attr_ref_t *ref, int indent, const char *key)
 		printf("<unknown>\n");
 	}
 
+	kdump_attr_discard(ctx, &attr);
 	return 0;
 }
 

include/libkdumpfile/kdumpfile.h.in

@@ -732,10 +732,18 @@ kdump_clear_attr(kdump_ctx_t *ctx, const char *key)
  * @param valp Value (filled on successful return).
  * @returns    Error status.
  *
- * Note that the caller does not hold a reference to the attribute, so
- * it is not generally safe to use this function in a multi-threaded
- * program, or across another library call which modifies the attribute
- * (explicitly or implicitly).
+ * This function is useful only when the caller can fully control the
+ * lifetime of the returned attribute value. In that case, the value
+ * is valid only as long as no changes are made to the containing dump
+ * file object.
+ *
+ * Note that attribute data (or the attribute hierarchy itself) may
+ * change as a side effect of another operation. Merely reading dump
+ * data or attributes will never invalidate the attribute value, i.e.
+ * it is safe to dereference pointers. However, the returned value may
+ * become stale, for example cache statistics.
+ *
+ * A more robust API is provided by @ref kdump_attr_ref_get.
  */
 kdump_status kdump_get_attr(kdump_ctx_t *ctx, const char *key,
 			    kdump_attr_t *valp);
@@ -750,8 +758,8 @@ kdump_status kdump_get_attr(kdump_ctx_t *ctx, const char *key,
  * If the attribute is of a different type, this function returns
  * KDUMP_ERR_INVALID and does not change @p valp.
  *
- * Note that the caller does not hold a reference to the attribute. See
- * the description of @ref kdump_get_attr for limitations.
+ * The caller must ensure the lifetime of the returned data is valid. See
+ * the description of @ref kdump_get_attr for more details.
  */
 kdump_status kdump_get_typed_attr(kdump_ctx_t *ctx, const char *key,
 				  kdump_attr_type_t type,
@@ -792,8 +800,8 @@ kdump_get_address_attr(kdump_ctx_t *ctx, const char *key, kdump_addr_t *addr)
  * @param str[out]  Filled with the attribute value on successful return.
  * @returns         Error status.
  *
- * Note that the caller does not hold a reference to the string. See
- * the description of @ref kdump_get_attr for limitations.
+ * The caller must ensure the lifetime of the returned data is valid. See
+ * the description of @ref kdump_get_attr for more details.
  */
 static inline kdump_status
 kdump_get_string_attr(kdump_ctx_t *ctx, const char *key, const char **str)
@@ -846,14 +854,42 @@ int kdump_attr_ref_isset(kdump_attr_ref_t *ref);
 /**  Get attribute data by reference.
  * @param      ctx   Dump file object.
  * @param[in]  ref   Attribute reference.
- * @param[out] valp  Attribute value (filled on successful return).
+ * @param[out] valp  Attribute value (filled on return).
+ * @returns          Error status.
  *
- * This works just like @ref kdump_get_attr, except that the attribute
- * is denoted by a reference rather than by its key path.
+ * This works similarly to @ref kdump_get_attr, except:
+ * - the attribute is denoted by a reference rather than by path,
+ * - the returned data stays valid even if changes are made to the dump
+ *   object.
+ *
+ * The returned value may be a dynamically allocated copy of the attribute
+ * value and/or it may hold an extra reference to the underlying objects.
+ * You should free up these resources with @ref kdump_attr_discard when
+ * the data is no longer needed.
+ *
+ * If the function returns an error, it is not necessary to discard the
+ * result. However, the type of @p valp is set to @c KDUMP_NIL, so it is
+ * always safe to call @ref kdump_attr_discard on the result. The goal of
+ * this feature is to simplify code flow in callers.
  */
 kdump_status kdump_attr_ref_get(kdump_ctx_t *ctx, const kdump_attr_ref_t *ref,
 				kdump_attr_t *valp);
 
+/** Discard attribute data value.
+ * @param ctx   Dump file object.
+ * @param attr  Attribute data.
+ *
+ * Perform any actions necessary to release resources by the attribute data.
+ * This function may decrement object references, free dynamically allocated
+ * memory, or even do nothing, depending on the data type.
+ *
+ * Call this function when the data returned by @ref kdump_attr_ref_get is no
+ * longer needed. Do *NOT* call this function on attribute data returned by
+ * @ref kdump_get_attr.
+ */
+void
+kdump_attr_discard(kdump_ctx_t *ctx, kdump_attr_t *attr);
+
 /**  Set attribute data by reference.
  * @param      ctx   Dump file object.
  * @param[in]  ref   Attribute reference.

python/kdumpfile.c

@@ -199,26 +199,37 @@ static PyObject *kdumpfile_read (PyObject *_self, PyObject *args, PyObject *kw)
 static PyObject *
 attr_new(kdumpfile_object *kdumpfile, kdump_attr_ref_t *ref, kdump_attr_t *attr)
 {
+	PyObject *obj = NULL;
+
 	if (attr->type != KDUMP_DIRECTORY)
 		kdump_attr_unref(kdumpfile->ctx, ref);
 
 	switch (attr->type) {
 		case KDUMP_NUMBER:
-			return PyLong_FromUnsignedLong(attr->val.number);
+			obj = PyLong_FromUnsignedLong(attr->val.number);
+			break;
 		case KDUMP_ADDRESS:
-			return PyLong_FromUnsignedLong(attr->val.address);
+			obj = PyLong_FromUnsignedLong(attr->val.address);
+			break;
 		case KDUMP_STRING:
-			return PyString_FromString(attr->val.string);
+			obj = PyString_FromString(attr->val.string);
+			break;
 		case KDUMP_DIRECTORY:
-			return attr_dir_new(kdumpfile, ref);
+			obj= attr_dir_new(kdumpfile, ref);
+			break;
 		case KDUMP_BITMAP:
-			return bmp_new(attr->val.bitmap);
+			obj = bmp_new(attr->val.bitmap);
+			break;
 		case KDUMP_BLOB:
-			return blob_new(attr->val.blob);
+			obj = blob_new(attr->val.blob);
+			break;
 		default:
 			PyErr_SetString(PyExc_RuntimeError, "Unhandled attr type");
-			return NULL;
 	}
+
+	kdump_attr_discard(kdumpfile->ctx, attr);
+
+	return obj;
 }
 
 PyDoc_STRVAR(get_addrxlat_ctx__doc__,
@@ -1604,14 +1615,15 @@ attr_iteritem_next(PyObject *_self)
 
 	result = PyTuple_New(2);
 	if (result == NULL)
-		return NULL;
+		goto err_attr;
 	key = PyString_FromString(self->iter.key);
 	if (!key)
 		goto err_result;
 	value = attr_new(self->kdumpfile, &self->iter.pos, &attr);
 	if (!value)
 		goto err_key;
 
+	kdump_attr_discard(self->kdumpfile->ctx, &attr);
 	PyTuple_SET_ITEM(result, 0, key);
 	PyTuple_SET_ITEM(result, 1, value);
 	return attr_iter_advance(self, result);
@@ -1620,6 +1632,8 @@ attr_iteritem_next(PyObject *_self)
 	Py_DECREF(key);
  err_result:
 	Py_DECREF(result);
+ err_attr:
+	kdump_attr_discard(self->kdumpfile->ctx, &attr);
 	return NULL;
 }
 

src/kdumpfile/attr.c

@@ -1321,6 +1321,36 @@ kdump_attr_ref_isset(kdump_attr_ref_t *ref)
 	return attr_isset(ref_attr(ref));
 }
 
+static kdump_status
+hold_attr_data(kdump_ctx_t *ctx, kdump_attr_t *valp)
+{
+	switch (valp->type) {
+	case KDUMP_NIL:
+	case KDUMP_DIRECTORY:
+	case KDUMP_NUMBER:
+	case KDUMP_ADDRESS:
+		/* Value is embedded: Nothing to be done. */
+		break;
+
+	case KDUMP_STRING:
+		valp->val.string = strdup(valp->val.string);
+		if (!valp->val.string)
+			return set_error(ctx, KDUMP_ERR_SYSTEM,
+					 "Cannot allocate string");
+		break;
+
+	case KDUMP_BITMAP:
+		internal_bmp_incref(valp->val.bitmap);
+		break;
+
+	case KDUMP_BLOB:
+		internal_blob_incref(valp->val.blob);
+		break;
+	}
+
+	return KDUMP_OK;
+}
+
 kdump_status
 kdump_attr_ref_get(kdump_ctx_t *ctx, const kdump_attr_ref_t *ref,
 		   kdump_attr_t *valp)
@@ -1332,10 +1362,21 @@ kdump_attr_ref_get(kdump_ctx_t *ctx, const kdump_attr_ref_t *ref,
 	rwlock_rdlock(&ctx->shared->lock);
 	valp->type = d->template->type;
 	ret = get_attr_data(ctx, d, &valp->val);
+	if (ret == KDUMP_OK)
+		ret = hold_attr_data(ctx, valp);
+	if (ret != KDUMP_OK)
+		valp->type = KDUMP_NIL;
 	rwlock_unlock(&ctx->shared->lock);
 	return ret;
 }
 
+void
+kdump_attr_discard(kdump_ctx_t *ctx, kdump_attr_t *attr)
+{
+	clear_error(ctx);
+	discard_value(&attr->val, attr->type, ATTR_DYNSTR);
+}
+
 kdump_status
 kdump_attr_ref_set(kdump_ctx_t *ctx, kdump_attr_ref_t *ref,
 		   const kdump_attr_t *valp)

src/kdumpfile/kdumpfile-priv.h

@@ -482,6 +482,9 @@ struct attr_flags {
 #define ATTR_PERSIST_INDIRECT	\
 	((struct attr_flags){ .persist = true, .indirect = true })
 
+/** Dynamically allocated attribute flags. */
+#define ATTR_DYNSTR	\
+	((struct attr_flags){ .dynstr = true })
 
 /**  Attribute template flags.
  */

src/kdumpfile/libkdumpfile.map

@@ -42,6 +42,7 @@ LIBKDUMPFILE_0 {
     kdump_attr_ref_type;
     kdump_attr_ref_isset;
     kdump_attr_ref_get;
+    kdump_attr_discard;
     kdump_attr_ref_set;
     kdump_set_sub_attr;
     kdump_attr_iter_start;

tests/attriter.c

@@ -137,6 +137,7 @@ main(int argc, char **argv)
 				rc = TEST_FAIL;
 			} else
 				printf("%s = %s\n", it.key, attr.val.string);
+			kdump_attr_discard(ctx, &attr);
 		}
 
 		res = kdump_attr_iter_next(ctx, &it);

tests/fdset.c

@@ -97,6 +97,7 @@ check_unset_file(kdump_ctx_t *ctx, kdump_attr_ref_t *fileset, const char *key,
 				key, kdump_get_err(ctx));
 			ret = TEST_FAIL;
 		}
+		kdump_attr_discard(ctx, &attr);
 		kdump_attr_unref(ctx, &tmpref);
 	}
 
@@ -156,6 +157,7 @@ check_fileset_zero(kdump_ctx_t *ctx, kdump_attr_ref_t *fileset, int fd)
 			"file.set.0.fd", attr.val.number, fd);
 		ret = TEST_FAIL;
 	}
+	kdump_attr_discard(ctx, &attr);
 	kdump_attr_unref(ctx, &tmpref);
 
 	return ret;
@@ -192,6 +194,7 @@ check_filename(kdump_ctx_t *ctx, kdump_attr_ref_t *fileset,
 			KDUMP_ATTR_FILE_SET, subkey, attr.val.string, name);
 		ret = TEST_FAIL;
 	}
+	kdump_attr_discard(ctx, &attr);
 	kdump_attr_unref(ctx, &tmpref);
 
 	return ret;
@@ -253,6 +256,7 @@ main(int argc, char **argv)
 			attr.val.number);
 		ret = TEST_FAIL;
 	}
+	kdump_attr_discard(ctx, &attr);
 
 	/* Check that file.set.0.fd does not exist. */
 	rc = check_not_exist(ctx, &fileset, "0.fd");

tests/subattr.c

@@ -83,6 +83,7 @@ main(int argc, char **argv)
 			rc = TEST_FAIL;
 		} else
 			printf("%s is a directory\n", ATTRDIR);
+		kdump_attr_discard(ctx, &attr);
 		kdump_attr_unref(ctx, &subref);
 	} else {
 		fprintf(stderr, "kdump_sub_attr_ref failed for %s: %s\n",
@@ -107,6 +108,7 @@ main(int argc, char **argv)
 			rc = TEST_FAIL;
 		} else
 			printf("%s = %s\n", ATTRPATH, attr.val.string);
+		kdump_attr_discard(ctx, &attr);
 		kdump_attr_unref(ctx, &subref);
 	} else {
 		fprintf(stderr, "kdump_sub_attr_ref failed for %s: %s\n",