[Qemu-devel] [PATCH v4 20/28] qstring: Add qstring_consume_str()

[Eric Blake <eblake@redhat.com>]

Similar to g_string_free(), there are cases where we want to
destroy our reference to a QString while grabbing its contents,
where we want to avoid use-after-free but also avoid a needless
strdup(). But unlike g_string_free(), we are at least sensible
enough to add this feature via a different function name,
instead of trying to overload two completely separate concepts
into a single g_string_free().  Do this by introducing
qstring_consume_str(), then use it where it makes sense.

In the case of monitor.c, note that QString _always_ has a
non-NULL embedded string with at least one byte allocated for
a terminating NUL, so special-casing on length 0 was wasted code.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v4: new patch, suggested by Markus
---
 include/qapi/qmp/qstring.h |  1 +
 monitor.c                  |  6 +-----
 qobject/qstring.c          | 23 +++++++++++++++++++++++
 qom/object.c               |  3 +--
 tests/check-qstring.c      | 31 +++++++++++++++++++++++++++----
 5 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index a987f3b..2d55c87 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -27,6 +27,7 @@ QString *qstring_from_str(const char *str);
 QString *qstring_from_substr(const char *str, int start, int end);
 size_t qstring_get_length(const QString *qstring);
 const char *qstring_get_str(const QString *qstring);
+char *qstring_consume_str(QString *qstring);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
 void qstring_append_printf(QString *qstring, const char *fmt, ...)
diff --git a/monitor.c b/monitor.c
index 2f56488..934d539 100644
--- a/monitor.c
+++ b/monitor.c
@@ -661,11 +661,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     cur_mon = old_mon;

     qemu_mutex_lock(&hmp.out_lock);
-    if (qstring_get_length(hmp.outbuf) > 0) {
-        output = g_strdup(qstring_get_str(hmp.outbuf));
-    } else {
-        output = g_strdup("");
-    }
+    output = qstring_consume_str(hmp.outbuf);
     qemu_mutex_unlock(&hmp.out_lock);

 out:
diff --git a/qobject/qstring.c b/qobject/qstring.c
index fbfae27..7a438e9 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -145,6 +145,29 @@ const char *qstring_get_str(const QString *qstring)
 }

 /**
+ * qstring_consume_str(): Destructively convert a QString to string
+ *
+ * The reference count of @qstring is decremented by one, and the
+ * caller is responsible for calling g_free() on the result.  In the
+ * common case where @qstring was not shared, this is faster than
+ * using strdup() on the result of qstring_get_str(); otherwise, the
+ * result is a copy and remaining users of @qstring are unaffected.
+ */
+char *qstring_consume_str(QString *qstring)
+{
+    char *result;
+
+    if (qstring->base.refcnt == 1) {
+        result = qstring->string;
+        qstring->string = NULL;
+    } else {
+        result = g_strdup(qstring->string);
+    }
+    qobject_decref(&qstring->base);
+    return result;
+}
+
+/**
  * qstring_destroy_obj(): Free all memory allocated by a QString
  * object
  */
diff --git a/qom/object.c b/qom/object.c
index 26e7d58..7960729 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1106,10 +1106,9 @@ char *object_property_get_str(Object *obj, const char *name,
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name, "string");
         retval = NULL;
     } else {
-        retval = g_strdup(qstring_get_str(qstring));
+        retval = qstring_consume_str(qstring);
     }

-    QDECREF(qstring);
     return retval;
 }

diff --git a/tests/check-qstring.c b/tests/check-qstring.c
index 9877b42..e6f58e0 100644
--- a/tests/check-qstring.c
+++ b/tests/check-qstring.c
@@ -1,7 +1,7 @@
 /*
  * QString unit-tests.
  *
- * Copyright (C) 2009 Red Hat Inc.
+ * Copyright (C) 2009-2016 Red Hat Inc.
  *
  * Authors:
  *  Luiz Capitulino <lcapitulino@redhat.com>
@@ -32,9 +32,7 @@ static void qstring_from_str_test(void)
     g_assert(strcmp(str, qstring->string) == 0);
     g_assert(qobject_type(QOBJECT(qstring)) == QTYPE_QSTRING);

-    // destroy doesn't exit yet
-    g_free(qstring->string);
-    g_free(qstring);
+    QDECREF(qstring);
 }

 static void qstring_destroy_test(void)
@@ -56,6 +54,30 @@ static void qstring_get_str_test(void)
     QDECREF(qstring);
 }

+static void qstring_consume_str_test(void)
+{
+    QString *qstring;
+    char *ret_str;
+    const char *str = "QEMU/KVM";
+    char *ptr;
+
+    qstring = qstring_from_str(str);
+    QINCREF(qstring);
+    g_assert_cmpint(qstring->base.refcnt, ==, 2);
+    ptr = qstring->string;
+
+    ret_str = qstring_consume_str(qstring);
+    g_assert_cmpint(qstring->base.refcnt, ==, 1);
+    g_assert(ret_str != ptr);
+    g_assert_cmpstr(ret_str, ==, str);
+    g_free(ret_str);
+
+    ret_str = qstring_consume_str(qstring);
+    g_assert(ret_str == ptr);
+    g_assert_cmpstr(ret_str, ==, str);
+    g_free(ret_str);
+}
+
 static void qstring_append_chr_test(void)
 {
     int i;
@@ -103,6 +125,7 @@ int main(int argc, char **argv)
     g_test_add_func("/public/append_chr", qstring_append_chr_test);
     g_test_add_func("/public/from_substr", qstring_from_substr_test);
     g_test_add_func("/public/to_qstring", qobject_to_qstring_test);
+    g_test_add_func("/public/consume_str", qstring_consume_str_test);

     return g_test_run();
 }
-- 
2.5.5