[PATCH 3/5] tests/qtest/qos-test: dump qos graph if verbose
If qtests were run in verbose mode (i.e. if --verbose CL argument was
provided) then dump the generated qos graph (all nodes and edges,
along with their current individual availability status) to stdout,
which allows to identify problems in the created qos graph e.g. when
writing new qos tests.
See API doc comment on function qos_dump_graph() for details.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/qgraph.c | 45 +
tests/qtest/libqos/qgraph.h | 20 +
tests/qtest/qos-test.c | 3 +++
3 files changed, 68 insertions(+)
diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index 61faf6b27d..b3b1a31f81 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -805,3 +805,48 @@ void qos_delete_cmd_line(const char *name)
node->command_line = NULL;
}
}
+
+void qos_dump_graph(void)
+{
+GList *keys;
+GList *l;
+QOSGraphEdgeList *list;
+QOSGraphEdge *e, *next;
+QOSGraphNode *dest_node, *node;
+
+qos_printf("ALL QGRAPH EDGES: {\n");
+keys = g_hash_table_get_keys(edge_table);
+for (l = keys; l != NULL; l = l->next) {
+const gchar *key = l->data;
+qos_printf("\t src='%s'\n", key);
+list = get_edgelist(key);
+QSLIST_FOREACH_SAFE(e, list, edge_list, next) {
+dest_node = g_hash_table_lookup(node_table, e->dest);
+qos_printf("\t\t|-> dest='%s' type=%d (node=%p)",
+ e->dest, e->type, dest_node);
+if (!dest_node) {
+qos_printf_literal(" <--- ERROR !");
+}
+qos_printf_literal("\n");
+}
+}
+g_list_free(keys);
+qos_printf("}\n");
+
+qos_printf("ALL QGRAPH NODES: {\n");
+keys = g_hash_table_get_keys(node_table);
+for (l = keys; l != NULL; l = l->next) {
+const gchar *key = l->data;
+node = g_hash_table_lookup(node_table, key);
+qos_printf("\t name='%s' ", key);
+if (node->qemu_name) {
+qos_printf_literal("qemu_name='%s' ", node->qemu_name);
+}
+qos_printf_literal("type=%d cmd_line='%s' [%s]\n",
+ node->type, node->command_line,
+ node->available ? "available" : "UNAVAILBLE"
+);
+}
+g_list_free(keys);
+qos_printf("}\n");
+}
diff --git a/tests/qtest/libqos/qgraph.h b/tests/qtest/libqos/qgraph.h
index f472949f68..07a32535f1 100644
--- a/tests/qtest/libqos/qgraph.h
+++ b/tests/qtest/libqos/qgraph.h
@@ -586,5 +586,25 @@ QOSGraphObject *qos_machine_new(QOSGraphNode *node,
QTestState *qts);
QOSGraphObject *qos_driver_new(QOSGraphNode *node, QOSGraphObject *parent,
QGuestAllocator *alloc, void *arg);
+/**
+ * Just for debugging purpose: prints all currently existing nodes and
+ * edges to stdout.
+ *
+ * All qtests add themselves to the overall qos graph by calling qgraph
+ * functions that add device nodes and edges between the individual graph
+ * nodes for tests. As the actual graph is assmbled at runtime by the qos
+ * subsystem, it is sometimes not obvious how the overall graph looks like.
+ * E.g. when writing new tests it may happen that those new tests are simply
+ * ignored by the qtest framework.
+ *
+ * This function allows to identify problems in the created qgraph. Keep in
+ * mind: only tests with a path down from the actual test case node (leaf) up
+ * to the graph's root node are actually executed by the qtest framework. And
+ * the qtest framework uses QMP to automatically check which QEMU drivers are
+ * actually currently available, and accordingly qos marks certain pathes as
+ * 'unavailable' in such cases (e.g. when QEMU was compiled without support for
+ * a certain feature).
+ */
+void qos_dump_graph(void);
#endif
diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 8fdf87b183..d98ef78613 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -322,6 +322,9 @@ int main(int argc, char **argv)
qos_set_machines_devices_available();
qos_graph_foreach_test_path(walk_path);
+if (g_test_verbose()) {
+qos_dump_graph();
+}
g_test_run();
qtest_end();
qos_graph_destroy();
--
2.20.1
[PATCH 1/5] libqos/qgraph: add qos_node_create_driver_named()
So far the qos subsystem of the qtest framework had the limitation
that only one instance of the same official QEMU (QMP) driver name
could be created for qtests. That's because a) the created qos
node names must always be unique, b) the node name must match the
official QEMU driver name being instantiated and c) all nodes are
in a global space shared by all tests.
This patch removes this limitation by introducing a new function
qos_node_create_driver_named() which allows test case authors to
specify a node name being different from the actual associated
QEMU driver name. It fills the new 'qemu_name' field of
QOSGraphNode for that purpose.
Adjust build_driver_cmd_line() and qos_graph_node_set_availability()
to correctly deal with either accessing node name vs. node's
qemu_name correctly.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/qgraph.c | 54 ++--
tests/qtest/libqos/qgraph.h | 16 +
tests/qtest/libqos/qgraph_internal.h | 1 +
3 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index fc49cfa879..61faf6b27d 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -153,6 +153,7 @@ static QOSGraphNode *create_node(const char *name,
QOSNodeType type)
static void destroy_node(void *val)
{
QOSGraphNode *node = val;
+g_free(node->qemu_name);
g_free(node->command_line);
g_free(node);
}
@@ -286,7 +287,8 @@ static void build_machine_cmd_line(QOSGraphNode *node,
const char *args)
*/
static void build_driver_cmd_line(QOSGraphNode *node)
{
-node->command_line = g_strconcat(" -device ", node->name, NULL);
+const char *name = node->qemu_name ?: node->name;
+node->command_line = g_strconcat(" -device ", name, NULL);
}
/* qos_print_cb(): callback prints all path found by the DFS algorithm. */
@@ -631,6 +633,15 @@ void qos_node_create_driver(const char *name,
QOSCreateDriverFunc function)
node->u.driver.constructor = function;
}
+void qos_node_create_driver_named(const char *name, const char *qemu_name,
+ QOSCreateDriverFunc function)
+{
+QOSGraphNode *node = create_node(name, QNODE_DRIVER);
+node->qemu_name = g_strdup(qemu_name);
+build_driver_cmd_line(node);
+node->u.driver.constructor = function;
+}
+
void qos_node_contains(const char *container, const char *contained,
QOSGraphEdgeOptions *opts, ...)
{
@@ -663,7 +674,7 @@ void qos_node_consumes(const char *consumer, const char
*interface,
add_edge(interface, consumer, QEDGE_CONSUMED_BY, opts);
}
-void qos_graph_node_set_availability(const char *node, bool av)
+static void qos_graph_node_set_availability_explicit(const char *node, bool av)
{
QOSGraphEdgeList *elist;
QOSGraphNode *n = search_node(node);
@@ -678,9 +689,46 @@ void qos_graph_node_set_availability(const char *node,
bool av)
}
QSLIST_FOREACH_SAFE(e, elist, edge_list, next) {
if (e->type == QEDGE_CONTAINS || e->type == QEDGE_PRODUCES) {
-qos_graph_node_set_availability(e->dest, av);
+qos_graph_node_set_availability_explicit(e->dest, av);
+}
+}
+}
+
+/*
+ * Behaves as qos_graph_node_set_availability_explicit(), except that the
+ * former always matches by node name only, whereas this function matches both
+ * by node name and node's optional 'qemu_name' field.
+ */
+void qos_graph_node_set_availability(const char *node, bool av)
+{
+GList *l;
+QOSGraphEdgeList *elist;
+QOSGraphEdge *e, *next;
+QOSGraphNode *n;
+GList *keys = g_hash_table_get_keys(node_table);
+
+for (l = keys; l != NULL; l = l->next) {
+const gchar *key = l->data;
+n = g_hash_table_lookup(node_table, key);
+/*
+ * node's 'qemu_name' is set if there is more than one device with
+ * the same QEMU (QMP) device name
+ */
+const char *node_name = n->qemu_name ?: n->name;
+if (g_strcmp0(node_name, node) == 0) {
+n->available = av;
+elist = get_edgelist(n->name);
+if (elist) {
+QSLIST_FOREACH_SAFE(e, elist, edge_list, next) {
+if (e->type == QEDGE_CONTAINS || e->type == QEDGE_PRODUCES)
+{
+qos_graph_node_set_availability_explicit(e->dest, av);
+}
+}
+}
}
}
+g_list_free(keys);
}
void qos_graph_foreach_test_path(QOSTestCallback fn)
diff --git a/tests/qtest/libqos/qgraph.h b/tests/qtest/libqos/qgraph.h
index 5f63d352ca..f472949f68 100644
--- a/tests/qtest/libqos/qgraph.h
+++ b/tests/qtest/libqos/qgraph.h
@@ -452,6 +452,22 @@ void qos_node_create_machine_args(const char *name,
*/
void qos_node_create_driver(const char *name, QOSCreateDriverFunc function);
+/**
+ * Behaves as qos_node_create_driver() with the extension of
[PATCH 2/5] libqos/qgraph_internal: add qos_printf() and qos_printf_literal()
These two are macros wrapping regular printf() call. They are intended
to be used instead of calling printf() directly in order to avoid
breaking TAP output format.
TAP output format is enabled by using --tap command line argument.
Starting with glib 2.62 it is enabled by default.
Unfortunately there is currently no public glib API available to check
whether TAP output format is enabled. For that reason qos_printf()
simply always prepends a '#' character for now.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/qgraph_internal.h | 11 +++
1 file changed, 11 insertions(+)
diff --git a/tests/qtest/libqos/qgraph_internal.h
b/tests/qtest/libqos/qgraph_internal.h
index 974985dce9..c0025f5ab9 100644
--- a/tests/qtest/libqos/qgraph_internal.h
+++ b/tests/qtest/libqos/qgraph_internal.h
@@ -255,4 +255,15 @@ void qos_delete_cmd_line(const char *name);
*/
void qos_graph_node_set_availability(const char *node, bool av);
+/*
+ * Prepends a '#' character in front for not breaking TAP output format.
+ */
+#define qos_printf(...) printf("# " __VA_ARGS__)
+
+/*
+ * Intended for printing something literally, i.e. for appending text as is
+ * to a line already been started by qos_printf() before.
+ */
+#define qos_printf_literal printf
+
#endif
--
2.20.1
[PATCH 4/5] tests/qtest/qos-test: dump environment variables if verbose
If qtests are run in verbose mode (i.e. if --verbose CL argument
was provided) then print all environment variables to stdout
before running the individual tests.
It is common nowadays, at least being able to output all config
vectors in a build chain, especially if it is required to
investigate build- and test-issues on foreign/remote machines,
which includes environment variables. In the context of writing
new test cases this is also useful for finding out whether there
are already some existing options for common questions like is
there a preferred location for writing test files to? Is there
a maximum size for test data? Is there a deadline for running
tests?
Use qos_printf() instead of g_test_message() to avoid the latter
cluttering the output.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/qos-test.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index d98ef78613..b279b6f816 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -313,9 +313,16 @@ static void walk_path(QOSGraphNode *orig_path, int len)
* machine/drivers/test objects
* - Cleans up everything
*/
-int main(int argc, char **argv)
+int main(int argc, char **argv, char** envp)
{
g_test_init(&argc, &argv, NULL);
+if (g_test_verbose()) {
+qos_printf("ENVIRONMENT VARIABLES: {\n");
+for (char **env = envp; *env != 0; env++) {
+qos_printf("\t%s\n", *env);
+}
+qos_printf("}\n");
+}
qos_graph_init();
module_call_init(MODULE_INIT_QOM);
module_call_init(MODULE_INIT_LIBQOS);
--
2.20.1
[PATCH 0/5] enhance debugging with qtest framework
This series is a follow-up of the following previous series: https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg02251.html The 9p patches of the previous series have already been merged. This series consists of 2 parts: 1. libqos patch 1 removes a limitation of the qtest/libqos subsystem: support for more than one device using the same (official) QEMU device name. Like discussed in the previous series, if nobody finds this patch useful then just ignore it. I needed it in the previou series before but eventually decided for a different approach and personally don't need it in near future. 2. Patches 2 to 5 enhance debugging issues with the qtest framework. I would appreciate if they got merged, because I still find them useful while working on new test cases. Changes of these patches from derived series: * Squashed previous patches 1 & 2 -> [patch 1]. * Dropped ANSI color escape sequences [patch 3]. * Squashed previous patches 4 & 5 -> [patch 3]. * Extended commit log to provide more details about purpose [patch 4]. Christian Schoenebeck (5): libqos/qgraph: add qos_node_create_driver_named() libqos/qgraph_internal: add qos_printf() and qos_printf_literal() tests/qtest/qos-test: dump qos graph if verbose tests/qtest/qos-test: dump environment variables if verbose tests/qtest/qos-test: dump QEMU command if verbose tests/qtest/libqos/qgraph.c | 99 +++- tests/qtest/libqos/qgraph.h | 36 ++ tests/qtest/libqos/qgraph_internal.h | 12 tests/qtest/qos-test.c | 15 - 4 files changed, 158 insertions(+), 4 deletions(-) -- 2.20.1
[PATCH 5/5] tests/qtest/qos-test: dump QEMU command if verbose
If qtests are run in verbose mode (i.e. if --verbose CL argument
was provided) then print the assembled qemu command line for each
test.
Use qos_printf() instead of g_test_message() to avoid the latter
cluttering the output.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/qos-test.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index b279b6f816..f97d0a08fd 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -89,6 +89,9 @@ static void qos_set_machines_devices_available(void)
static void restart_qemu_or_continue(char *path)
{
+if (g_test_verbose()) {
+qos_printf("Run QEMU with: '%s'\n", path);
+}
/* compares the current command line with the
* one previously executed: if they are the same,
* don't restart QEMU, if they differ, stop previous
--
2.20.1
Re: [PATCH] MAINTAINERS: add my github tree URL
On Montag, 1. Februar 2021 11:08:10 CET Greg Kurz wrote: > On Sat, 30 Jan 2021 15:39:14 +0100 > > Christian Schoenebeck wrote: > > I already used this github URL for PRs before and will continue to use it > > in foreseeable future. > > > > Signed-off-by: Christian Schoenebeck > > --- > > Reviewed-by: Greg Kurz > > Unless you're planning to send a PR soon, I guess this can go > through the trivial tree. Yes, I would appreciate if that's going through the trivial queue, as it will indeed take a while for my next PR. Thanks! Best regards, Christian Schoenebeck
Re: 9pfs developers docs
On Montag, 1. Februar 2021 13:26:49 CET Greg Kurz wrote: > On Mon, 01 Feb 2021 12:30:52 +0100 > > Christian Schoenebeck wrote: > > On Montag, 1. Februar 2021 10:24:26 CET Greg Kurz wrote: > > > On Sun, 31 Jan 2021 19:23:52 +0100 > > > > > > Christian Schoenebeck wrote: > > > > Hi, > > > > > > Hi Christian, > > > > > > > I started setting up some developer documentation for 9pfs: > > > > https://wiki.qemu.org/Documentation/9p > > > > > > > > Still quite a bunch that should be added (e.g. there should be a > > > > section > > > > about threads and coroutines), but at least it's a start ... > > > > > > I agree that a bunch of other things should be documented, but that's > > > definitely a great start. Thanks for doing this ! > > > > > > Just one remark on the topology diagram: > > > > > > https://wiki.qemu.org/File:9pfs_topology.png > > > > > > It gives the impression that the 9p transport and server can > > > handle multiple guests, which they certainly don't : each > > > 9p server lives in exactly one device which is exposed to > > > exactly one guest. > > > > Right, I haven't considered that the diagram might be interpreted that > > way. My primary intention was to show the 3 main components of 9pfs from > > design perspective and secondary showing that multiple guests can share > > storage. > > > > So what would be better: a) duplicating the server side in the diagram > > (then the image might become a bit large in height), b) dropping the > > multiple guests, c) making the issue with server instances clear in the > > text? > I'd rather go for b) Updated the diagram on the wiki page. To keep noise low, I won't send emails on further changes to that wiki page. If you want to be auto notified then just add yourself to the watch list there. > > If there are other things that you might think should be outlined by > > additional diagram(s) let me know, then I can add that in one rush. > > > > -- > > > > BTW I'm no longer able to run the 'local' 9p tests, --slow doesn't work > > for > > me. If you don't have an idea what I might be missing, then I have to look > > why the CLI parameter is not interpreted. > > Is it that '-m slow' doesn't work when running 'qos-test' or > that 'make check-qtest SPEEP=slow' doesn't run the slow tests ? Ah, that's '-m slow', not '--slow'. Yeah, that works for qos-test. I added the '-m slow' switch to the wiki page as well. For now I can live with that, as I am more commonly calling qos-test directly. But it would be nice if the slow tests would make it into the general chain of all QEMU tests accordingly again. > The latter was discussed on IRC last year but I don't know if > anyone has tried to investigate this yet. > > Nov 24 11:36:53th_huth, Hi. FYI it seems that the meson > conversion > kinda broke 'make check SPEED=slow'. Test programs aren't passed '-m slow' > Nov 24 11:51:42th_huth: do you know who uses/tests SPEED=slow? > Nov > 24 11:52:03th_huth: I thought this was a block-related feature Nov > 24 11:52:44f4bug, it is supposedly used by gitlab CI > Nov 24 11:52:59.gitlab-ci.yml:MAKE_CHECK_ARGS: check-qtest > SPEED=slow Nov 24 12:50:53 groug, I'm also running make check > SPEED=slow manually sometimes ... I guess that got lost in the conversion > to ninja ... bonzini, did you ever try? Nov 24 12:51:03 no it > shouldn't > Nov 24 12:51:21 let me check... > Nov 24 12:51:40 ah, the tests are chosen correctly but -m slow is > lost Nov 24 12:52:02 yes that's what I see > Nov 24 12:54:04bonzini, missing bits in scripts/mtest2make.py ? > Nov 24 12:54:28 groug: sort of, but assuming that all executables > support -m slow wouldn't work > > Cc'ing Thomas and Paolo for additional details. > > > Best regards, > > Christian Schoenebeck
Re: macOS (Big Sur, Apple Silicon) 'make check' fails in test-crypto-tlscredsx509
On Dienstag, 2. Februar 2021 06:19:42 CET Roman Bolshakov wrote:
> 'make check' of libtasn1 doesn't succeed on x86_64 either.
>
> After a session of debugging I believe there's an issue with Clang 12.
> Here's a test program (it reproduces unexpected ASN1_VALUE_NOT_VALID
> from _asn1_time_der() in libtasn1):
>
> #include
>
> static int func2(char *foo) {
> fprintf(stderr, "%s:%d foo: %p\n", __func__, __LINE__, foo);
> if (foo == NULL) {
> fprintf(stderr, "%s:%d foo: %p\n", __func__, __LINE__, foo);
> return 1;
> }
> return 0;
> }
>
> int func1(char *foo) {
> int counter = 0;
> if (fprintf(stderr, "IO\n") > 0)
> counter += 10;
> fprintf(stderr, "%s:%d foo: %p counter %d\n", __func__, __LINE__,
> foo, counter); if(!func2(foo + counter)) {
> fprintf(stderr, "good\n");
> return 0;
> } else {
> fprintf(stderr, "broken\n");
> return 1;
> }
> }
>
> int main() {
> char *foo = NULL;
> return func1(foo);
> }
>
>
> What return value would you expect from the program?
>
> If the program is compiled with -O0/O1 it returns zero exit code.
> Here's the output:
> IO
> func1:16 foo: 0x0 counter 10
> func2:4 foo: 0xa
> good
>
> If it is compiled with -O2 it returns 1:
> IO
> func1:16 foo: 0x0 counter 10
> func2:4 foo: 0xa
> func2:6 foo: 0x0
> broken
>
> That happens because clang uses register behind foo from func1 (it has zero
> pointer) inside inlined func2 (it should have non zero pointer).
>
> So, immediate workaround would be to downgrade optimization level of
> libtasn1 to -O1 in homebrew.
Hu, confirmed.
clang 12.0.0 on x86_64 Mac fails on that demo with -O2,-O3,-Os, but works with
-O0,-O1.
clang 11.0.3 in contrast works with any optimization level.
It only fails BTW if that test uses exactly a NULL pointer, any other memory
address (e.g. just (void*)1) works:
#include
#define FLOOR_VALUE ((void*)1)
static int func2(char *foo) {
fprintf(stderr, "%s:%d foo: %p\n", __func__, __LINE__, foo);
if (foo == FLOOR_VALUE) {
fprintf(stderr, "%s:%d foo: %p\n", __func__, __LINE__, foo);
return 1;
}
return 0;
}
int func1(char *foo) {
int counter = 0;
if (fprintf(stderr, "IO\n") > 0)
counter += 1;
fprintf(stderr, "%s:%d foo: %p counter %d\n", __func__, __LINE__, foo,
counter);
if(!func2(foo + counter)) {
fprintf(stderr, "good\n");
return 0;
} else {
fprintf(stderr, "broken\n");
return 1;
}
}
int main() {
char *foo = FLOOR_VALUE;
return func1(foo);
}
Maybe that's some sort of new security feature in clang 12, in the sense of
something like this:
VeryLargeStruct *p = NULL;
p->farMember = value;
to segfault always reliably and exactly with address zero, instead of pure
luck as of NULL + veryLargeSize.
> I've submitted the issue to Apple bugtracker:
> FB8986815
>
> Best regards,
> Roman
They could argue that operating on a NULL pointer is undefined behaviour.
Best regards,
Christian Schoenebeck
Re: macOS (Big Sur, Apple Silicon) 'make check' fails in test-crypto-tlscredsx509
On Dienstag, 2. Februar 2021 15:50:24 CET Eric Blake wrote: > > If the program is compiled with -O0/O1 it returns zero exit code. > > Here's the output: > > IO > > func1:16 foo: 0x0 counter 10 > > func2:4 foo: 0xa > > good > > > > If it is compiled with -O2 it returns 1: > > IO > > func1:16 foo: 0x0 counter 10 > > func2:4 foo: 0xa > > func2:6 foo: 0x0 > > And this proves the point that the compiler was able to exploit the > undefined behavior in your program. > > > broken > > > > That happens because clang uses register behind foo from func1 (it has > > zero > > pointer) inside inlined func2 (it should have non zero pointer). > > > > So, immediate workaround would be to downgrade optimization level of > > libtasn1 to -O1 in homebrew. > > > > I've submitted the issue to Apple bugtracker: > > FB8986815 > > Yes, it's annoying that as compilers get smarter, it exposes the > presence of unspecified code in weird ways. But I don't see this as a > bug in clang, but as a bug in libtasn1 for assuming undefined behavior > produces a sane result. You are right Eric, but nevertheless it's a very aggressive behaviour change being introduced way too silent, especially regarding backward compatibility like this case proofs. Personally I find the new semantic NULL + n == NULL makes sense, as it adds safety, but I do consider it a bug that clang did not even throw a warning. Even when I add -Wnull-pointer-arithmetic it does not complain to me at all. Best regards, Christian Schoenebeck
Re: [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing
On Montag, 18. Januar 2021 00:09:24 CET Alexander Bulekov wrote:
> virtio-9p devices are often used to expose a virtual-filesystem to the
> guest. There have been some bugs reported in this device, such as
> CVE-2018-19364, and CVE-2021-20181. We should fuzz this device
>
> This patch adds two virtio-9p configurations:
> * One with the widely used -fsdev local driver. This driver leaks some
>state in the form of files/directories created in the shared dir.
> * One with the synth driver. While it is not used in the real world, this
>driver won't leak leak state between fuzz inputs.
>
> Signed-off-by: Alexander Bulekov
> ---
> CC: Christian Schoenebeck
> CC: Greg Kurz
>
> I considered adding an atexit handler to remove the temp directory,
> however I am worried that there might be some error that results in a
> call to exit(), rather than abort(), which will cause problems for
> future fork()-ed fuzzers. I don't think there are such calls in the 9p
> code, however there might be something in the APIs used by 9p. As this
> code is primarily for ephemeral OSS-Fuzz conainers, this shouldn't be
> too much of an issue.
Yes, dealing with signal handlers for that is probably a bit intransparent and
would leave a questionable feeling about its reliability.
What about __attribute__((destructor)) to auto delete the fuzzer directory,
like virtio-9p-test.c does for the same task?
> tests/qtest/fuzz/generic_fuzz_configs.h | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
> b/tests/qtest/fuzz/generic_fuzz_configs.h index 1a133655ee..f99657cdbc
> 100644
> --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> @@ -19,6 +19,16 @@ typedef struct generic_fuzz_config {
> gchar* (*argfunc)(void); /* Result must be freeable by g_free() */
> } generic_fuzz_config;
>
> +static inline gchar *generic_fuzzer_virtio_9p_args(void){
> +char tmpdir[] = "/tmp/qemu-fuzz.XX";
> +g_assert_nonnull(mkdtemp(tmpdir));
> +
> +return g_strdup_printf("-machine q35 -nodefaults "
> +"-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> +"-fsdev local,id=hshare,path=%s,security_model=mapped-xattr,"
> +"writeout=immediate,fmode=0600,dmode=0700", tmpdir);
> +}
> +
> const generic_fuzz_config predefined_configs[] = {
> {
> .name = "virtio-net-pci-slirp",
> @@ -60,6 +70,16 @@ const generic_fuzz_config predefined_configs[] = {
> .name = "virtio-mouse",
> .args = "-machine q35 -nodefaults -device virtio-mouse",
> .objects = "virtio*",
> +},{
> +.name = "virtio-9p",
> +.argfunc = generic_fuzzer_virtio_9p_args,
> +.objects = "virtio*",
> +},{
> +.name = "virtio-9p-synth",
> +.args = "-machine q35 -nodefaults "
> +"-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> +"-fsdev synth,id=hshare",
> +.objects = "virtio*",
> },{
> .name = "e1000",
> .args = "-M q35 -nodefaults "
Re: [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing
On Dienstag, 19. Januar 2021 16:44:31 CET Darren Kenny wrote: > On Tuesday, 2021-01-19 at 10:12:29 -05, Alexander Bulekov wrote: > > On 210118 1540, Darren Kenny wrote: > >> On Monday, 2021-01-18 at 10:30:33 -05, Alexander Bulekov wrote: > >> > On 210118 1334, Christian Schoenebeck wrote: > >> >> On Montag, 18. Januar 2021 00:09:24 CET Alexander Bulekov wrote: > >> >> > virtio-9p devices are often used to expose a virtual-filesystem to > >> >> > the > >> >> > guest. There have been some bugs reported in this device, such as > >> >> > CVE-2018-19364, and CVE-2021-20181. We should fuzz this device > >> >> > > >> >> > This patch adds two virtio-9p configurations: > >> >> > * One with the widely used -fsdev local driver. This driver leaks > >> >> > some > >> >> > > >> >> >state in the form of files/directories created in the shared dir. > >> >> > > >> >> > * One with the synth driver. While it is not used in the real > >> >> > world, this > >> >> > > >> >> >driver won't leak leak state between fuzz inputs. > >> >> > > >> >> > Signed-off-by: Alexander Bulekov > >> >> > --- > >> >> > CC: Christian Schoenebeck > >> >> > CC: Greg Kurz > >> >> > > >> >> > I considered adding an atexit handler to remove the temp directory, > >> >> > however I am worried that there might be some error that results in > >> >> > a > >> >> > call to exit(), rather than abort(), which will cause problems for > >> >> > future fork()-ed fuzzers. I don't think there are such calls in the > >> >> > 9p > >> >> > code, however there might be something in the APIs used by 9p. As > >> >> > this > >> >> > code is primarily for ephemeral OSS-Fuzz conainers, this shouldn't > >> >> > be > >> >> > too much of an issue. > >> >> > >> >> Yes, dealing with signal handlers for that is probably a bit > >> >> intransparent and would leave a questionable feeling about its > >> >> reliability. > >> >> > >> >> What about __attribute__((destructor)) to auto delete the fuzzer > >> >> directory, > >> >> like virtio-9p-test.c does for the same task? > >> > > >> > My worry is that we will end up deleting it while it is still in use. > >> > The scenario I am worried about: > >> > [parent process ] set up temp directory for virtio-9p > >> > [parent process ] initialize QEMU > >> > [parent process ] fork() and wait() > >> > [child process 1] Run the fuzzing input. > >> > [child process 1] Once the input has been executed, call _Exit(). This > >> > should skip the atexit()/__attribute((destructor)) handlers. For > >> > reasons > >> > related to libfuzzer, we need to do this so that libfuzzer doesn't > >> > treat > >> > each child exit()-ing as a crash. > >> > [parent process ] wait() returns. > >> > [parent process ] generate a new input.. fork() and wait() > >> > [child process 2] Run the fuzzing input. > >> > [child process 2] Somewhere we hit an abort(). libfuzzer hooks the > >> > abort > >> > and dumps the crashing input and stack trace. Since abort() doesn't > >> > call > >> > exit handlers, it will skip over atexit()/__attribute((destructor)) > >> > handlers [parent process ] wait() returns. > >> > [parent process ] generate a new input.. fork() and wait() > >> > [child process 3] Run the fuzzing input. > >> > [child process 3] Somewhere we hit an exit(). This will dump the > >> > input/stacktrace and it will run the exit handlers (removing the shared > >> > 9p directory) > >> > [parent process ] wait() returns. generate a new input.. fork() and > >> > wait() > >> > [child process 4] ... > >> > >> OK, that answer's my question :) > >> > >> > Now all the subsequent forked children will refer to a shared directory > >> > that we already removed. Ideally, we would run the cleanup handler only > >> > after the parent process exit()s. I think there are some ways to do > >> > this, by placing the atexit() call in a place only reachable by the > >> > parent. However, on OSS-Fuzz this shouldn't be a problem as everything > >> > is cleaned up automatically anyway.. > >> > >> Yep, agreed. > >> > >> > I am more worried about the fact that files/directories/links that are > >> > created by 9p in the child processes, persist across inputs. I think > >> > Thomas suggested a way to work-around this for PATCH 1/3. We could have > >> > a function that runs in the parent after each wait() returns, that > >> > would > >> > remove all the files in the temp directory and scrub the extended > >> > attributes applied by 9p to the shared dir. > >> > >> Hmm, that sounds like something to consider, but it may also end up > >> slowing down the execution during the turn-around - guess it depends on > >> how much noise is being generated. > > > > I've ben running the fuzzer for a couple days, and I haven't noticed any > > issues with unreproducible inputs (yet). Is this something we can add > > later, if it becomes a problem? > > Sure, I'm good with that: > > Reviewed-by: Darren Kenny > > Thanks, > > Darren. Same with me: Reviewed-by: Christian Schoene
