[PULL 0/5] 9p queue 2022-02-10

[PULL 0/5] 9p queue 2022-02-10

[Christian Schoenebeck]

The following changes since commit 0a301624c2f4ced3331ffd5bce85b4274fe132af:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20220208' into staging (2022-02-08 11:40:08 +0000)

are available in the Git repository at:

  https://github.com/cschoenebeck/qemu.git tags/pull-9p-20220210

for you to fetch changes up to de19c79dad6a2cad54ae04ce754d47c07bf9bc93:

  9pfs: Fix segfault in do_readdir_many caused by struct dirent overread (2022-02-10 11:56:01 +0100)

----------------------------------------------------------------
9pfs: fixes and cleanup

* Fifth patch fixes a 9pfs server crash that happened on some systems due
  to incorrect (system dependant) handling of struct dirent size.

* Tests: Second patch fixes a test error that happened on some systems due
  mkdir() being called twice for creating the test directory for the 9p
  'local' tests.

* Tests: Third patch fixes a memory leak.

* Tests: The remaining two patches are code cleanup.

----------------------------------------------------------------
Christian Schoenebeck (2):
      tests/9pfs: use g_autofree where possible
      tests/9pfs: fix mkdir() being called twice

Greg Kurz (2):
      tests/9pfs: Fix leak of local_test_path
      tests/9pfs: Use g_autofree and g_autoptr where possible

Vitaly Chikunov (1):
      9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

 hw/9pfs/codir.c                |  3 +-
 include/qemu/osdep.h           | 13 ++++++
 tests/qtest/libqos/virtio-9p.c | 38 +++++++-----------
 tests/qtest/virtio-9p-test.c   | 90 +++++++++++++-----------------------------
 util/osdep.c                   | 21 ++++++++++
 5 files changed, 76 insertions(+), 89 deletions(-)

[PULL 3/5] tests/9pfs: Fix leak of local_test_path

[Christian Schoenebeck]

From: Greg Kurz <groug@kaod.org>

local_test_path is allocated in virtio_9p_create_local_test_dir() to hold the path
of the temporary directory. It should be freed in virtio_9p_remove_local_test_dir()
when the temporary directory is removed. Clarify the lifecycle of local_test_path
while here.

Based-on: <f6602123c6f7d0d593466231b04fba087817abbd.1642879848.git.qemu_oss@crudebyte.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
Message-Id: <20220201151508.190035-2-groug@kaod.org>
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index ef96ef006a..5d18e5eae5 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -39,8 +39,13 @@ static char *concat_path(const char* a, const char* b)
 
 void virtio_9p_create_local_test_dir(void)
 {
+    g_assert(local_test_path == NULL);
     struct stat st;
     char *pwd = g_get_current_dir();
+    /*
+     * template gets cached into local_test_path and freed in
+     * virtio_9p_remove_local_test_dir().
+     */
     char *template = concat_path(pwd, "qtest-9p-local-XXXXXX");
 
     local_test_path = mkdtemp(template);
@@ -66,6 +71,8 @@ void virtio_9p_remove_local_test_dir(void)
         /* ignore error, dummy check to prevent compiler error */
     }
     g_free(cmd);
+    g_free(local_test_path);
+    local_test_path = NULL;
 }
 
 char *virtio_9p_test_path(const char *path)
-- 
2.20.1

[PULL 5/5] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Christian Schoenebeck]

From: Vitaly Chikunov <vt@altlinux.org>

`struct dirent' returned from readdir(3) could be shorter (or longer)
than `sizeof(struct dirent)', thus memcpy of sizeof length will overread
into unallocated page causing SIGSEGV. Example stack trace:

 #0  0x00005555559ebeed v9fs_co_readdir_many (/usr/bin/qemu-system-x86_64 + 0x497eed)
 #1  0x00005555559ec2e9 v9fs_readdir (/usr/bin/qemu-system-x86_64 + 0x4982e9)
 #2  0x0000555555eb7983 coroutine_trampoline (/usr/bin/qemu-system-x86_64 + 0x963983)
 #3  0x00007ffff73e0be0 n/a (n/a + 0x0)

While fixing, provide a helper for any future `struct dirent' cloning.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/841
Cc: qemu-stable@nongnu.org
Co-authored-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Dmitry V. Levin <ldv@altlinux.org>
Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
Acked-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <20220206013419.849161-1-vt@altlinux.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/codir.c      |  3 +--
 include/qemu/osdep.h | 13 +++++++++++++
 util/osdep.c         | 21 +++++++++++++++++++++
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 032cce04c4..c0873bde16 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -143,8 +143,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
         } else {
             e = e->next = g_malloc0(sizeof(V9fsDirEnt));
         }
-        e->dent = g_malloc0(sizeof(struct dirent));
-        memcpy(e->dent, dent, sizeof(struct dirent));
+        e->dent = qemu_dirent_dup(dent);
 
         /* perform a full stat() for directory entry if requested by caller */
         if (dostat) {
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index d1660d67fa..ce12f64853 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -805,6 +805,19 @@ static inline int platform_does_not_support_system(const char *command)
 }
 #endif /* !HAVE_SYSTEM_FUNCTION */
 
+/**
+ * Duplicate directory entry @dent.
+ *
+ * It is highly recommended to use this function instead of open coding
+ * duplication of @c dirent objects, because the actual @c struct @c dirent
+ * size may be bigger or shorter than @c sizeof(struct dirent) and correct
+ * handling is platform specific (see gitlab issue #841).
+ *
+ * @dent - original directory entry to be duplicated
+ * @returns duplicated directory entry which should be freed with g_free()
+ */
+struct dirent *qemu_dirent_dup(struct dirent *dent);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/util/osdep.c b/util/osdep.c
index 42a0a4986a..67fbf22778 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -33,6 +33,7 @@
 extern int madvise(char *, size_t, int);
 #endif
 
+#include <dirent.h>
 #include "qemu-common.h"
 #include "qemu/cutils.h"
 #include "qemu/sockets.h"
@@ -615,3 +616,23 @@ writev(int fd, const struct iovec *iov, int iov_cnt)
     return readv_writev(fd, iov, iov_cnt, true);
 }
 #endif
+
+struct dirent *
+qemu_dirent_dup(struct dirent *dent)
+{
+    size_t sz = 0;
+#if defined _DIRENT_HAVE_D_RECLEN
+    /* Avoid use of strlen() if platform supports d_reclen. */
+    sz = dent->d_reclen;
+#endif
+    /*
+     * Test sz for zero even if d_reclen is available
+     * because some drivers may set d_reclen to zero.
+     */
+    if (sz == 0) {
+        /* Fallback to the most portable way. */
+        sz = offsetof(struct dirent, d_name) +
+                      strlen(dent->d_name) + 1;
+    }
+    return g_memdup(dent, sz);
+}
-- 
2.20.1

[PULL 2/5] tests/9pfs: fix mkdir() being called twice

[Christian Schoenebeck]

The 9p test cases use mkdtemp() to create a temporary directory for
running the 'local' 9p tests with real files/dirs. Unlike mktemp()
which only generates a unique file name, mkdtemp() also creates the
directory, therefore the subsequent mkdir() was wrong and caused
errors on some systems.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Fixes: 136b7af2 (tests/9pfs: fix test dir for parallel tests)
Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/832
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Greg Kurz <Greg Kurz <groug@kaod.org>
Message-Id: <f6602123c6f7d0d593466231b04fba087817abbd.1642879848.git.qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index b4e1143288..ef96ef006a 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -37,31 +37,19 @@ static char *concat_path(const char* a, const char* b)
     return g_build_filename(a, b, NULL);
 }
 
-static void init_local_test_path(void)
+void virtio_9p_create_local_test_dir(void)
 {
+    struct stat st;
     char *pwd = g_get_current_dir();
     char *template = concat_path(pwd, "qtest-9p-local-XXXXXX");
+
     local_test_path = mkdtemp(template);
     if (!local_test_path) {
         g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));
     }
-    g_assert(local_test_path);
     g_free(pwd);
-}
-
-void virtio_9p_create_local_test_dir(void)
-{
-    struct stat st;
-    int res;
-
-    init_local_test_path();
 
     g_assert(local_test_path != NULL);
-    res = mkdir(local_test_path, 0777);
-    if (res < 0) {
-        g_test_message("mkdir('%s') failed: %s", local_test_path,
-                       strerror(errno));
-    }
 
     /* ensure test directory exists now ... */
     g_assert(stat(local_test_path, &st) == 0);
-- 
2.20.1

[PULL 1/5] tests/9pfs: use g_autofree where possible

[Christian Schoenebeck]

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Message-Id: <E1mn1fA-0005qZ-TM@lizzy.crudebyte.com>
---
 tests/qtest/virtio-9p-test.c | 90 +++++++++++-------------------------
 1 file changed, 27 insertions(+), 63 deletions(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 41fed41de1..502e5ad0c7 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -84,7 +84,7 @@ static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
     size_t tag_len = qvirtio_config_readw(v9p->vdev, 0);
-    char *tag;
+    g_autofree char *tag = NULL;
     int i;
 
     g_assert_cmpint(tag_len, ==, strlen(MOUNT_TAG));
@@ -94,7 +94,6 @@ static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
         tag[i] = qvirtio_config_readb(v9p->vdev, i + 2);
     }
     g_assert_cmpmem(tag, tag_len, MOUNT_TAG, tag_len);
-    g_free(tag);
 }
 
 #define P9_MAX_SIZE 4096 /* Max size of a T-message or R-message */
@@ -580,7 +579,7 @@ static void do_version(QVirtio9P *v9p)
 {
     const char *version = "9P2000.L";
     uint16_t server_len;
-    char *server_version;
+    g_autofree char *server_version = NULL;
     P9Req *req;
 
     req = v9fs_tversion(v9p, P9_MAX_SIZE, version, P9_NOTAG);
@@ -588,8 +587,6 @@ static void do_version(QVirtio9P *v9p)
     v9fs_rversion(req, &server_len, &server_version);
 
     g_assert_cmpmem(server_version, server_len, version, strlen(version));
-
-    g_free(server_version);
 }
 
 /* utility function: walk to requested dir and return fid for that dir */
@@ -637,7 +634,7 @@ static void fs_walk(void *obj, void *data, QGuestAllocator *t_alloc)
     alloc = t_alloc;
     char *wnames[P9_MAXWELEM];
     uint16_t nwqid;
-    v9fs_qid *wqid;
+    g_autofree v9fs_qid *wqid = NULL;
     int i;
     P9Req *req;
 
@@ -655,8 +652,6 @@ static void fs_walk(void *obj, void *data, QGuestAllocator *t_alloc)
     for (i = 0; i < P9_MAXWELEM; i++) {
         g_free(wnames[i]);
     }
-
-    g_free(wqid);
 }
 
 static bool fs_dirents_contain_name(struct V9fsDirent *e, const char* name)
@@ -872,9 +867,9 @@ static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
     g_assert_cmpint(fs_dirents_contain_name(entries, "."), ==, true);
     g_assert_cmpint(fs_dirents_contain_name(entries, ".."), ==, true);
     for (int i = 0; i < QTEST_V9FS_SYNTH_READDIR_NFILES; ++i) {
-        char *name = g_strdup_printf(QTEST_V9FS_SYNTH_READDIR_FILE, i);
+        g_autofree char *name =
+            g_strdup_printf(QTEST_V9FS_SYNTH_READDIR_FILE, i);
         g_assert_cmpint(fs_dirents_contain_name(entries, name), ==, true);
-        g_free(name);
     }
 
     v9fs_free_dirents(entries);
@@ -984,7 +979,8 @@ static void fs_walk_dotdot(void *obj, void *data, QGuestAllocator *t_alloc)
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
     char *const wnames[] = { g_strdup("..") };
-    v9fs_qid root_qid, *wqid;
+    v9fs_qid root_qid;
+    g_autofree v9fs_qid *wqid = NULL;
     P9Req *req;
 
     do_version(v9p);
@@ -998,7 +994,6 @@ static void fs_walk_dotdot(void *obj, void *data, QGuestAllocator *t_alloc)
 
     g_assert_cmpmem(&root_qid, 13, wqid[0], 13);
 
-    g_free(wqid);
     g_free(wnames[0]);
 }
 
@@ -1027,7 +1022,7 @@ static void fs_write(void *obj, void *data, QGuestAllocator *t_alloc)
     alloc = t_alloc;
     static const uint32_t write_count = P9_MAX_SIZE / 2;
     char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_WRITE_FILE) };
-    char *buf = g_malloc0(write_count);
+    g_autofree char *buf = g_malloc0(write_count);
     uint32_t count;
     P9Req *req;
 
@@ -1045,7 +1040,6 @@ static void fs_write(void *obj, void *data, QGuestAllocator *t_alloc)
     v9fs_rwrite(req, &count);
     g_assert_cmpint(count, ==, write_count);
 
-    g_free(buf);
     g_free(wnames[0]);
 }
 
@@ -1125,7 +1119,7 @@ static void fs_flush_ignored(void *obj, void *data, QGuestAllocator *t_alloc)
 
 static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
 {
-    char *const name = g_strdup(cname);
+    g_autofree char *name = g_strdup(cname);
     uint32_t fid;
     P9Req *req;
 
@@ -1134,15 +1128,13 @@ static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname)
     req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rmkdir(req, NULL);
-
-    g_free(name);
 }
 
 /* create a regular file with Tlcreate and return file's fid */
 static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
                            const char *cname)
 {
-    char *const name = g_strdup(cname);
+    g_autofree char *name = g_strdup(cname);
     uint32_t fid;
     P9Req *req;
 
@@ -1152,7 +1144,6 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rlcreate(req, NULL, NULL);
 
-    g_free(name);
     return fid;
 }
 
@@ -1160,8 +1151,8 @@ static uint32_t do_lcreate(QVirtio9P *v9p, const char *path,
 static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
                        const char *to)
 {
-    char *const name = g_strdup(clink);
-    char *const dst = g_strdup(to);
+    g_autofree char *name = g_strdup(clink);
+    g_autofree char *dst = g_strdup(to);
     uint32_t fid;
     P9Req *req;
 
@@ -1170,9 +1161,6 @@ static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
     req = v9fs_tsymlink(v9p, fid, name, dst, 0, 0);
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_rsymlink(req, NULL);
-
-    g_free(dst);
-    g_free(name);
 }
 
 /* create a hard link named @a clink in directory @a path pointing to @a to */
@@ -1193,7 +1181,7 @@ static void do_hardlink(QVirtio9P *v9p, const char *path, const char *clink,
 static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
                         uint32_t flags)
 {
-    char *const name = g_strdup(rpath);
+    g_autofree char *name = g_strdup(rpath);
     uint32_t fid;
     P9Req *req;
 
@@ -1202,8 +1190,6 @@ static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
     req = v9fs_tunlinkat(v9p, fid, name, flags, 0);
     v9fs_req_wait_for_reply(req, NULL);
     v9fs_runlinkat(req);
-
-    g_free(name);
 }
 
 static void fs_readdir_split_128(void *obj, void *data,
@@ -1235,8 +1221,8 @@ static void fs_create_dir(void *obj, void *data, QGuestAllocator *t_alloc)
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
     struct stat st;
-    char *root_path = virtio_9p_test_path("");
-    char *new_dir = virtio_9p_test_path("01");
+    g_autofree char *root_path = virtio_9p_test_path("");
+    g_autofree char *new_dir = virtio_9p_test_path("01");
 
     g_assert(root_path != NULL);
 
@@ -1247,9 +1233,6 @@ static void fs_create_dir(void *obj, void *data, QGuestAllocator *t_alloc)
     g_assert(stat(new_dir, &st) == 0);
     /* ... and is actually a directory */
     g_assert((st.st_mode & S_IFMT) == S_IFDIR);
-
-    g_free(new_dir);
-    g_free(root_path);
 }
 
 static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
@@ -1257,8 +1240,8 @@ static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
     struct stat st;
-    char *root_path = virtio_9p_test_path("");
-    char *new_dir = virtio_9p_test_path("02");
+    g_autofree char *root_path = virtio_9p_test_path("");
+    g_autofree char *new_dir = virtio_9p_test_path("02");
 
     g_assert(root_path != NULL);
 
@@ -1273,9 +1256,6 @@ static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
     do_unlinkat(v9p, "/", "02", AT_REMOVEDIR);
     /* directory should be gone now */
     g_assert(stat(new_dir, &st) != 0);
-
-    g_free(new_dir);
-    g_free(root_path);
 }
 
 static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
@@ -1283,7 +1263,7 @@ static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
     struct stat st;
-    char *new_file = virtio_9p_test_path("03/1st_file");
+    g_autofree char *new_file = virtio_9p_test_path("03/1st_file");
 
     do_attach(v9p);
     do_mkdir(v9p, "/", "03");
@@ -1293,8 +1273,6 @@ static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
     g_assert(stat(new_file, &st) == 0);
     /* ... and is a regular file */
     g_assert((st.st_mode & S_IFMT) == S_IFREG);
-
-    g_free(new_file);
 }
 
 static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
@@ -1302,7 +1280,7 @@ static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
     struct stat st;
-    char *new_file = virtio_9p_test_path("04/doa_file");
+    g_autofree char *new_file = virtio_9p_test_path("04/doa_file");
 
     do_attach(v9p);
     do_mkdir(v9p, "/", "04");
@@ -1316,8 +1294,6 @@ static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
     do_unlinkat(v9p, "04", "doa_file", 0);
     /* file should be gone now */
     g_assert(stat(new_file, &st) != 0);
-
-    g_free(new_file);
 }
 
 static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
@@ -1325,8 +1301,8 @@ static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
     struct stat st;
-    char *real_file = virtio_9p_test_path("05/real_file");
-    char *symlink_file = virtio_9p_test_path("05/symlink_file");
+    g_autofree char *real_file = virtio_9p_test_path("05/real_file");
+    g_autofree char *symlink_file = virtio_9p_test_path("05/symlink_file");
 
     do_attach(v9p);
     do_mkdir(v9p, "/", "05");
@@ -1338,9 +1314,6 @@ static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
 
     /* check if created link exists now */
     g_assert(stat(symlink_file, &st) == 0);
-
-    g_free(symlink_file);
-    g_free(real_file);
 }
 
 static void fs_unlinkat_symlink(void *obj, void *data,
@@ -1349,8 +1322,8 @@ static void fs_unlinkat_symlink(void *obj, void *data,
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
     struct stat st;
-    char *real_file = virtio_9p_test_path("06/real_file");
-    char *symlink_file = virtio_9p_test_path("06/symlink_file");
+    g_autofree char *real_file = virtio_9p_test_path("06/real_file");
+    g_autofree char *symlink_file = virtio_9p_test_path("06/symlink_file");
 
     do_attach(v9p);
     do_mkdir(v9p, "/", "06");
@@ -1364,9 +1337,6 @@ static void fs_unlinkat_symlink(void *obj, void *data,
     do_unlinkat(v9p, "06", "symlink_file", 0);
     /* symlink should be gone now */
     g_assert(stat(symlink_file, &st) != 0);
-
-    g_free(symlink_file);
-    g_free(real_file);
 }
 
 static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
@@ -1374,8 +1344,8 @@ static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
     struct stat st_real, st_link;
-    char *real_file = virtio_9p_test_path("07/real_file");
-    char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
+    g_autofree char *real_file = virtio_9p_test_path("07/real_file");
+    g_autofree char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
 
     do_attach(v9p);
     do_mkdir(v9p, "/", "07");
@@ -1391,9 +1361,6 @@ static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
     g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
     g_assert(st_link.st_dev == st_real.st_dev);
     g_assert(st_link.st_ino == st_real.st_ino);
-
-    g_free(hardlink_file);
-    g_free(real_file);
 }
 
 static void fs_unlinkat_hardlink(void *obj, void *data,
@@ -1402,8 +1369,8 @@ static void fs_unlinkat_hardlink(void *obj, void *data,
     QVirtio9P *v9p = obj;
     alloc = t_alloc;
     struct stat st_real, st_link;
-    char *real_file = virtio_9p_test_path("08/real_file");
-    char *hardlink_file = virtio_9p_test_path("08/hardlink_file");
+    g_autofree char *real_file = virtio_9p_test_path("08/real_file");
+    g_autofree char *hardlink_file = virtio_9p_test_path("08/hardlink_file");
 
     do_attach(v9p);
     do_mkdir(v9p, "/", "08");
@@ -1419,9 +1386,6 @@ static void fs_unlinkat_hardlink(void *obj, void *data,
     g_assert(stat(hardlink_file, &st_link) != 0);
     /* and old file should still exist */
     g_assert(stat(real_file, &st_real) == 0);
-
-    g_free(hardlink_file);
-    g_free(real_file);
 }
 
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
-- 
2.20.1

[PULL 4/5] tests/9pfs: Use g_autofree and g_autoptr where possible

[Christian Schoenebeck]

From: Greg Kurz <groug@kaod.org>

It is recommended to use g_autofree or g_autoptr as it reduces
the odds of introducing memory leaks in future changes.

Signed-off-by: Greg Kurz <groug@kaod.org>
Message-Id: <20220201151508.190035-3-groug@kaod.org>
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/libqos/virtio-9p.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 5d18e5eae5..f51f0635cc 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -41,7 +41,7 @@ void virtio_9p_create_local_test_dir(void)
 {
     g_assert(local_test_path == NULL);
     struct stat st;
-    char *pwd = g_get_current_dir();
+    g_autofree char *pwd = g_get_current_dir();
     /*
      * template gets cached into local_test_path and freed in
      * virtio_9p_remove_local_test_dir().
@@ -52,7 +52,6 @@ void virtio_9p_create_local_test_dir(void)
     if (!local_test_path) {
         g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));
     }
-    g_free(pwd);
 
     g_assert(local_test_path != NULL);
 
@@ -65,12 +64,11 @@ void virtio_9p_create_local_test_dir(void)
 void virtio_9p_remove_local_test_dir(void)
 {
     g_assert(local_test_path != NULL);
-    char *cmd = g_strdup_printf("rm -fr '%s'\n", local_test_path);
+    g_autofree char *cmd = g_strdup_printf("rm -fr '%s'\n", local_test_path);
     int res = system(cmd);
     if (res < 0) {
         /* ignore error, dummy check to prevent compiler error */
     }
-    g_free(cmd);
     g_free(local_test_path);
     local_test_path = NULL;
 }
@@ -216,8 +214,8 @@ static void *virtio_9p_pci_create(void *pci_bus, QGuestAllocator *t_alloc,
 static void regex_replace(GString *haystack, const char *pattern,
                           const char *replace_fmt, ...)
 {
-    GRegex *regex;
-    char *replace, *s;
+    g_autoptr(GRegex) regex = NULL;
+    g_autofree char *replace = NULL, *s = NULL;
     va_list argp;
 
     va_start(argp, replace_fmt);
@@ -227,9 +225,6 @@ static void regex_replace(GString *haystack, const char *pattern,
     regex = g_regex_new(pattern, 0, 0, NULL);
     s = g_regex_replace(regex, haystack->str, -1, 0, replace, 0, NULL);
     g_string_assign(haystack, s);
-    g_free(s);
-    g_regex_unref(regex);
-    g_free(replace);
 }
 
 void virtio_9p_assign_local_driver(GString *cmd_line, const char *args)
-- 
2.20.1

Re: [PULL 0/5] 9p queue 2022-02-10

[Peter Maydell]

On Thu, 10 Feb 2022 at 11:33, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> The following changes since commit 0a301624c2f4ced3331ffd5bce85b4274fe132af:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20220208' into staging (2022-02-08 11:40:08 +0000)
>
> are available in the Git repository at:
>
>   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20220210
>
> for you to fetch changes up to de19c79dad6a2cad54ae04ce754d47c07bf9bc93:
>
>   9pfs: Fix segfault in do_readdir_many caused by struct dirent overread (2022-02-10 11:56:01 +0100)
>
> ----------------------------------------------------------------
> 9pfs: fixes and cleanup
>
> * Fifth patch fixes a 9pfs server crash that happened on some systems due
>   to incorrect (system dependant) handling of struct dirent size.
>
> * Tests: Second patch fixes a test error that happened on some systems due
>   mkdir() being called twice for creating the test directory for the 9p
>   'local' tests.
>
> * Tests: Third patch fixes a memory leak.
>
> * Tests: The remaining two patches are code cleanup.
>
> ----------------------------------------------------------------

Hi; this fails CI for the build-oss-fuzz job, which finds
a heap-buffer-overflow:
https://gitlab.com/qemu-project/qemu/-/jobs/2087610013

8/152 qemu:qtest+qtest-i386 / qtest-i386/qos-test ERROR 66.74s killed
by signal 6 SIGABRT
>>> QTEST_QEMU_BINARY=./qemu-system-i386 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon MALLOC_PERTURB_=120 G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_IMG=./qemu-img /builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test --tap -k
――――――――――――――――――――――――――――――――――――― ✀ ―――――――――――――――――――――――――――――――――――――
Listing only the last 100 lines from a long log.
For details see https://github.com/google/sanitizers/issues/189
==7270==WARNING: ASan doesn't fully support makecontext/swapcontext
functions and may produce false positives in some cases!
==7270==WARNING: ASan is ignoring requested __asan_handle_no_return:
stack type: default top: 0x7ffc79fb0000; bottom 0x7ff908ffd000; size:
0x000370fb3000 (14780411904)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
==7276==WARNING: ASan doesn't fully support makecontext/swapcontext
functions and may produce false positives in some cases!
==7276==WARNING: ASan is ignoring requested __asan_handle_no_return:
stack type: default top: 0x7fff7e4a8000; bottom 0x7fd6363fd000; size:
0x0029480ab000 (177302319104)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
==7282==WARNING: ASan doesn't fully support makecontext/swapcontext
functions and may produce false positives in some cases!
==7282==WARNING: ASan is ignoring requested __asan_handle_no_return:
stack type: default top: 0x7ffee6e7f000; bottom 0x7f32fb5fd000; size:
0x00cbeb882000 (875829927936)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
==7288==WARNING: ASan doesn't fully support makecontext/swapcontext
functions and may produce false positives in some cases!
==7288==WARNING: ASan is ignoring requested __asan_handle_no_return:
stack type: default top: 0x7ffc6118e000; bottom 0x7f6391cfd000; size:
0x0098cf491000 (656312700928)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
==7294==WARNING: ASan doesn't fully support makecontext/swapcontext
functions and may produce false positives in some cases!
==7294==WARNING: ASan is ignoring requested __asan_handle_no_return:
stack type: default top: 0x7ffef665d000; bottom 0x7f69dc8fd000; size:
0x009519d60000 (640383582208)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
==7300==WARNING: ASan doesn't fully support makecontext/swapcontext
functions and may produce false positives in some cases!
==7300==WARNING: ASan is ignoring requested __asan_handle_no_return:
stack type: default top: 0x7ffe33db0000; bottom 0x7f01421fd000; size:
0x00fcf1bb3000 (1086387335168)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
==7306==WARNING: ASan doesn't fully support makecontext/swapcontext
functions and may produce false positives in some cases!
==7306==WARNING: ASan is ignoring requested __asan_handle_no_return:
stack type: default top: 0x7ffebd618000; bottom 0x7ff1179fd000; size:
0x000da5c1b000 (58615508992)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
=================================================================
==7306==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x612000030768 at pc 0x562351066c74 bp 0x7ff1078c3a90 sp
0x7ff1078c3240
READ of size 48830 at 0x612000030768 thread T4
#0 0x562351066c73 in __interceptor_memcpy.part.0 asan_interceptors.cpp.o
#1 0x7ff1290d443f in g_memdup (/lib64/libglib-2.0.so.0+0x6e43f)
#2 0x56235134537a in do_readdir_many
/builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:146:19
#3 0x56235134537a in v9fs_co_readdir_many
/builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:225:5
#4 0x56235132d626 in v9fs_do_readdir
/builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:2430:13
#5 0x56235132d626 in v9fs_readdir
/builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:2543:13
#6 0x56235257101e in coroutine_trampoline
/builds/qemu-project/qemu/build-oss-fuzz/../util/coroutine-ucontext.c:173:9
#7 0x7ff126e0e84f (/lib64/libc.so.6+0x5784f)
0x612000030768 is located 0 bytes to the right of 296-byte region
[0x612000030640,0x612000030768)
allocated by thread T4 here:
#0 0x5623510a4e47 in malloc
(/builds/qemu-project/qemu/build-oss-fuzz/qemu-system-i386+0x1146e47)
#1 0x7ff1290c03d8 in g_malloc (/lib64/libglib-2.0.so.0+0x5a3d8)
#2 0x56235131e659 in synth_opendir
/builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p-synth.c:185:18
#3 0x5623513462f5 in v9fs_co_opendir
/builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:321:5
#4 0x5623513257d7 in v9fs_open
/builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:1959:15
#5 0x56235257101e in coroutine_trampoline
/builds/qemu-project/qemu/build-oss-fuzz/../util/coroutine-ucontext.c:173:9
#6 0x7ff126e0e84f (/lib64/libc.so.6+0x5784f)
Thread T4 created by T0 here:
#0 0x562351015926 in pthread_create
(/builds/qemu-project/qemu/build-oss-fuzz/qemu-system-i386+0x10b7926)
#1 0x5623525351ea in qemu_thread_create
/builds/qemu-project/qemu/build-oss-fuzz/../util/qemu-thread-posix.c:596:11
#2 0x5623525a4588 in do_spawn_thread
/builds/qemu-project/qemu/build-oss-fuzz/../util/thread-pool.c:134:5
#3 0x5623525a4588 in spawn_thread_bh_fn
/builds/qemu-project/qemu/build-oss-fuzz/../util/thread-pool.c:142:5
#4 0x562352569814 in aio_bh_call
/builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:141:5
#5 0x562352569814 in aio_bh_poll
/builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:169:13
#6 0x5623525248cc in aio_dispatch
/builds/qemu-project/qemu/build-oss-fuzz/../util/aio-posix.c:415:5
#7 0x56235256c34c in aio_ctx_dispatch
/builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:311:5
#8 0x7ff1290bb05e in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x5505e)
SUMMARY: AddressSanitizer: heap-buffer-overflow
asan_interceptors.cpp.o in __interceptor_memcpy.part.0
Shadow bytes around the buggy address:
0x0c247fffe090: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
0x0c247fffe0a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c247fffe0b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa
0x0c247fffe0c0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
0x0c247fffe0d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c247fffe0e0: 00 00 00 00 00 00 00 00 00 00 00 00 00[fa]fa fa
0x0c247fffe0f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c247fffe100: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c247fffe110: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c247fffe120: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c247fffe130: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==7306==ABORTING


thanks
-- PMM

Re: [PULL 0/5] 9p queue 2022-02-10

[Christian Schoenebeck]

On Sonntag, 13. Februar 2022 21:33:10 CET Peter Maydell wrote:
> On Thu, 10 Feb 2022 at 11:33, Christian Schoenebeck
> 
> <qemu_oss@crudebyte.com> wrote:
> > The following changes since commit 0a301624c2f4ced3331ffd5bce85b4274fe132af:
> >   Merge remote-tracking branch
> >   'remotes/pmaydell/tags/pull-target-arm-20220208' into staging
> >   (2022-02-08 11:40:08 +0000)> 
> > are available in the Git repository at:
> >   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20220210
> > 
> > for you to fetch changes up to de19c79dad6a2cad54ae04ce754d47c07bf9bc93:
> >   9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
> >   (2022-02-10 11:56:01 +0100)> 
> > ----------------------------------------------------------------
> > 9pfs: fixes and cleanup
> > 
> > * Fifth patch fixes a 9pfs server crash that happened on some systems due
> > 
> >   to incorrect (system dependant) handling of struct dirent size.
> > 
> > * Tests: Second patch fixes a test error that happened on some systems due
> > 
> >   mkdir() being called twice for creating the test directory for the 9p
> >   'local' tests.
> > 
> > * Tests: Third patch fixes a memory leak.
> > 
> > * Tests: The remaining two patches are code cleanup.
> > 
> > ----------------------------------------------------------------
> 
> Hi; this fails CI for the build-oss-fuzz job, which finds
> a heap-buffer-overflow:
> https://gitlab.com/qemu-project/qemu/-/jobs/2087610013

So this is about the 'dirent' patch:
https://github.com/cschoenebeck/qemu/commit/de19c79dad6a2cad54ae04ce754d47c07bf9bc93

In conjunction with the 9p fuzzing tests:
https://wiki.qemu.org/Documentation/9p#Fuzzing

I first thought it might be a false positive due to the unorthodox handling of
dirent duplication by that patch, but from the ASan output below I am not
really sure about that.

Is there a way to get the content of local variables?

Would it be possible that the following issue (g_memdup vs. g_memdup2) might
apply here?
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538

Best regards,
Christian Schoenebeck

> 
> 8/152 qemu:qtest+qtest-i386 / qtest-i386/qos-test ERROR 66.74s killed
> by signal 6 SIGABRT
> 
> >>> QTEST_QEMU_BINARY=./qemu-system-i386
> >>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
> >>> MALLOC_PERTURB_=120
> >>> G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.
> >>> sh QTEST_QEMU_IMG=./qemu-img
> >>> /builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test --tap -k
> ――――――――――――――――――――――――――――――――――――― ✀
> ――――――――――――――――――――――――――――――――――――― Listing only the last 100 lines from
> a long log.
> For details see https://github.com/google/sanitizers/issues/189
> ==7270==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> ==7270==WARNING: ASan is ignoring requested __asan_handle_no_return:
> stack type: default top: 0x7ffc79fb0000; bottom 0x7ff908ffd000; size:
> 0x000370fb3000 (14780411904)
> False positive error reports may follow
> For details see https://github.com/google/sanitizers/issues/189
> ==7276==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> ==7276==WARNING: ASan is ignoring requested __asan_handle_no_return:
> stack type: default top: 0x7fff7e4a8000; bottom 0x7fd6363fd000; size:
> 0x0029480ab000 (177302319104)
> False positive error reports may follow
> For details see https://github.com/google/sanitizers/issues/189
> ==7282==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> ==7282==WARNING: ASan is ignoring requested __asan_handle_no_return:
> stack type: default top: 0x7ffee6e7f000; bottom 0x7f32fb5fd000; size:
> 0x00cbeb882000 (875829927936)
> False positive error reports may follow
> For details see https://github.com/google/sanitizers/issues/189
> ==7288==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> ==7288==WARNING: ASan is ignoring requested __asan_handle_no_return:
> stack type: default top: 0x7ffc6118e000; bottom 0x7f6391cfd000; size:
> 0x0098cf491000 (656312700928)
> False positive error reports may follow
> For details see https://github.com/google/sanitizers/issues/189
> ==7294==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> ==7294==WARNING: ASan is ignoring requested __asan_handle_no_return:
> stack type: default top: 0x7ffef665d000; bottom 0x7f69dc8fd000; size:
> 0x009519d60000 (640383582208)
> False positive error reports may follow
> For details see https://github.com/google/sanitizers/issues/189
> ==7300==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> ==7300==WARNING: ASan is ignoring requested __asan_handle_no_return:
> stack type: default top: 0x7ffe33db0000; bottom 0x7f01421fd000; size:
> 0x00fcf1bb3000 (1086387335168)
> False positive error reports may follow
> For details see https://github.com/google/sanitizers/issues/189
> ==7306==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> ==7306==WARNING: ASan is ignoring requested __asan_handle_no_return:
> stack type: default top: 0x7ffebd618000; bottom 0x7ff1179fd000; size:
> 0x000da5c1b000 (58615508992)
> False positive error reports may follow
> For details see https://github.com/google/sanitizers/issues/189
> =================================================================
> ==7306==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x612000030768 at pc 0x562351066c74 bp 0x7ff1078c3a90 sp
> 0x7ff1078c3240
> READ of size 48830 at 0x612000030768 thread T4
> #0 0x562351066c73 in __interceptor_memcpy.part.0 asan_interceptors.cpp.o
> #1 0x7ff1290d443f in g_memdup (/lib64/libglib-2.0.so.0+0x6e43f)
> #2 0x56235134537a in do_readdir_many
> /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:146:19
> #3 0x56235134537a in v9fs_co_readdir_many
> /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:225:5
> #4 0x56235132d626 in v9fs_do_readdir
> /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:2430:13
> #5 0x56235132d626 in v9fs_readdir
> /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:2543:13
> #6 0x56235257101e in coroutine_trampoline
> /builds/qemu-project/qemu/build-oss-fuzz/../util/coroutine-ucontext.c:173:9
> #7 0x7ff126e0e84f (/lib64/libc.so.6+0x5784f)
> 0x612000030768 is located 0 bytes to the right of 296-byte region
> [0x612000030640,0x612000030768)
> allocated by thread T4 here:
> #0 0x5623510a4e47 in malloc
> (/builds/qemu-project/qemu/build-oss-fuzz/qemu-system-i386+0x1146e47)
> #1 0x7ff1290c03d8 in g_malloc (/lib64/libglib-2.0.so.0+0x5a3d8)
> #2 0x56235131e659 in synth_opendir
> /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p-synth.c:185:18
> #3 0x5623513462f5 in v9fs_co_opendir
> /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:321:5
> #4 0x5623513257d7 in v9fs_open
> /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:1959:15
> #5 0x56235257101e in coroutine_trampoline
> /builds/qemu-project/qemu/build-oss-fuzz/../util/coroutine-ucontext.c:173:9
> #6 0x7ff126e0e84f (/lib64/libc.so.6+0x5784f)
> Thread T4 created by T0 here:
> #0 0x562351015926 in pthread_create
> (/builds/qemu-project/qemu/build-oss-fuzz/qemu-system-i386+0x10b7926)
> #1 0x5623525351ea in qemu_thread_create
> /builds/qemu-project/qemu/build-oss-fuzz/../util/qemu-thread-posix.c:596:11
> #2 0x5623525a4588 in do_spawn_thread
> /builds/qemu-project/qemu/build-oss-fuzz/../util/thread-pool.c:134:5
> #3 0x5623525a4588 in spawn_thread_bh_fn
> /builds/qemu-project/qemu/build-oss-fuzz/../util/thread-pool.c:142:5
> #4 0x562352569814 in aio_bh_call
> /builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:141:5
> #5 0x562352569814 in aio_bh_poll
> /builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:169:13
> #6 0x5623525248cc in aio_dispatch
> /builds/qemu-project/qemu/build-oss-fuzz/../util/aio-posix.c:415:5
> #7 0x56235256c34c in aio_ctx_dispatch
> /builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:311:5
> #8 0x7ff1290bb05e in g_main_context_dispatch
> (/lib64/libglib-2.0.so.0+0x5505e) SUMMARY: AddressSanitizer:
> heap-buffer-overflow
> asan_interceptors.cpp.o in __interceptor_memcpy.part.0
> Shadow bytes around the buggy address:
> 0x0c247fffe090: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
> 0x0c247fffe0a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c247fffe0b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa
> 0x0c247fffe0c0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
> 0x0c247fffe0d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> =>0x0c247fffe0e0: 00 00 00 00 00 00 00 00 00 00 00 00 00[fa]fa fa
> 0x0c247fffe0f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c247fffe100: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c247fffe110: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c247fffe120: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c247fffe130: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> Shadow byte legend (one shadow byte represents 8 application bytes):
> Addressable: 00
> Partially addressable: 01 02 03 04 05 06 07
> Heap left redzone: fa
> Freed heap region: fd
> Stack left redzone: f1
> Stack mid redzone: f2
> Stack right redzone: f3
> Stack after return: f5
> Stack use after scope: f8
> Global redzone: f9
> Global init order: f6
> Poisoned by user: f7
> Container overflow: fc
> Array cookie: ac
> Intra object redzone: bb
> ASan internal: fe
> Left alloca redzone: ca
> Right alloca redzone: cb
> ==7306==ABORTING
> 
> 
> thanks
> -- PMM

Re: [PULL 0/5] 9p queue 2022-02-10

[Peter Maydell]

On Mon, 14 Feb 2022 at 09:47, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
> So this is about the 'dirent' patch:
> https://github.com/cschoenebeck/qemu/commit/de19c79dad6a2cad54ae04ce754d47c07bf9bc93
>
> In conjunction with the 9p fuzzing tests:
> https://wiki.qemu.org/Documentation/9p#Fuzzing
>
> I first thought it might be a false positive due to the unorthodox handling of
> dirent duplication by that patch, but from the ASan output below I am not
> really sure about that.
>
> Is there a way to get the content of local variables?

Yes. You can build locally with the clang sanitizers enabled and then
run under gdb and with the appropriate environment variables to tell the
sanitizer to abort() on failures.

> Would it be possible that the following issue (g_memdup vs. g_memdup2) might
> apply here?
> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538

It seems unlikely that the problem is that you're allocating more than
4 gigabytes and thus hitting a 64-to-32 truncation.

thanks
-- PMM

Re: [PULL 0/5] 9p queue 2022-02-10

[Christian Schoenebeck]

On Montag, 14. Februar 2022 10:55:17 CET Peter Maydell wrote:
> On Mon, 14 Feb 2022 at 09:47, Christian Schoenebeck
> 
> <qemu_oss@crudebyte.com> wrote:
> > So this is about the 'dirent' patch:
> > https://github.com/cschoenebeck/qemu/commit/de19c79dad6a2cad54ae04ce754d47
> > c07bf9bc93
> > 
> > In conjunction with the 9p fuzzing tests:
> > https://wiki.qemu.org/Documentation/9p#Fuzzing
> > 
> > I first thought it might be a false positive due to the unorthodox
> > handling of dirent duplication by that patch, but from the ASan output
> > below I am not really sure about that.
> > 
> > Is there a way to get the content of local variables?
> 
> Yes. You can build locally with the clang sanitizers enabled and then
> run under gdb and with the appropriate environment variables to tell the
> sanitizer to abort() on failures.

Well, it does no longer matter for this particular issue here, but it would be 
useful if the CI scripts would already dump the local variables to the logs. 

E.g. because the patch in question was about system dependant variations.

Another reason is the random data nature of fuzzing tests. Even though the 
latter could probably be reproduced with an appropriate seed.

Best regards,
Christian Schoenebeck

Re: [PULL 0/5] 9p queue 2022-02-10

[Greg Kurz]

On Mon, 14 Feb 2022 10:47:43 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Sonntag, 13. Februar 2022 21:33:10 CET Peter Maydell wrote:
> > On Thu, 10 Feb 2022 at 11:33, Christian Schoenebeck
> > 
> > <qemu_oss@crudebyte.com> wrote:
> > > The following changes since commit 0a301624c2f4ced3331ffd5bce85b4274fe132af:
> > >   Merge remote-tracking branch
> > >   'remotes/pmaydell/tags/pull-target-arm-20220208' into staging
> > >   (2022-02-08 11:40:08 +0000)> 
> > > are available in the Git repository at:
> > >   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20220210
> > > 
> > > for you to fetch changes up to de19c79dad6a2cad54ae04ce754d47c07bf9bc93:
> > >   9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
> > >   (2022-02-10 11:56:01 +0100)> 
> > > ----------------------------------------------------------------9d82f6a3e68c2
> > > 9pfs: fixes and cleanup
> > > 
> > > * Fifth patch fixes a 9pfs server crash that happened on some systems due
> > > 
> > >   to incorrect (system dependant) handling of struct dirent size.
> > > 
> > > * Tests: Second patch fixes a test error that happened on some systems due
> > > 
> > >   mkdir() being called twice for creating the test directory for the 9p
> > >   'local' tests.
> > > 
> > > * Tests: Third patch fixes a memory leak.
> > > 
> > > * Tests: The remaining two patches are code cleanup.
> > > 
> > > ----------------------------------------------------------------
> > 
> > Hi; this fails CI for the build-oss-fuzz job, which finds
> > a heap-buffer-overflow:
> > https://gitlab.com/qemu-project/qemu/-/jobs/2087610013
> 
> So this is about the 'dirent' patch:
> https://github.com/cschoenebeck/qemu/commit/de19c79dad6a2cad54ae04ce754d47c07bf9bc93
> 
> In conjunction with the 9p fuzzing tests:
> https://wiki.qemu.org/Documentation/9p#Fuzzing
> 
> I first thought it might be a false positive due to the unorthodox handling of
> dirent duplication by that patch, but from the ASan output below I am not
> really sure about that.
> 

No, this is an actual bug. See below.

> Is there a way to get the content of local variables?
> 
> Would it be possible that the following issue (g_memdup vs. g_memdup2) might
> apply here?
> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
> 
> Best regards,
> Christian Schoenebeck
> 
> > 
> > 8/152 qemu:qtest+qtest-i386 / qtest-i386/qos-test ERROR 66.74s killed
> > by signal 6 SIGABRT
> > 
> > >>> QTEST_QEMU_BINARY=./qemu-system-i386
> > >>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
> > >>> MALLOC_PERTURB_=120
> > >>> G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.
> > >>> sh QTEST_QEMU_IMG=./qemu-img
> > >>> /builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test --tap -k
> > ――――――――――――――――――――――――――――――――――――― ✀
> > ――――――――――――――――――――――――――――――――――――― Listing only the last 100 lines from
> > a long log.
> > For details see https://github.com/google/sanitizers/issues/189
> > ==7270==WARNING: ASan doesn't fully support makecontext/swapcontext
> > functions and may produce false positives in some cases!
> > ==7270==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > stack type: default top: 0x7ffc79fb0000; bottom 0x7ff908ffd000; size:
> > 0x000370fb3000 (14780411904)
> > False positive error reports may follow
> > For details see https://github.com/google/sanitizers/issues/189
> > ==7276==WARNING: ASan doesn't fully support makecontext/swapcontext
> > functions and may produce false positives in some cases!
> > ==7276==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > stack type: default top: 0x7fff7e4a8000; bottom 0x7fd6363fd000; size:
> > 0x0029480ab000 (177302319104)
> > False positive error reports may follow
> > For details see https://github.com/google/sanitizers/issues/189
> > ==7282==WARNING: ASan doesn't fully support makecontext/swapcontext
> > functions and may produce false positives in some cases!
> > ==7282==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > stack type: default top: 0x7ffee6e7f000; bottom 0x7f32fb5fd000; size:
> > 0x00cbeb882000 (875829927936)
> > False positive error reports may follow
> > For details see https://github.com/google/sanitizers/issues/189
> > ==7288==WARNING: ASan doesn't fully support makecontext/swapcontext
> > functions and may produce false positives in some cases!
> > ==7288==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > stack type: default top: 0x7ffc6118e000; bottom 0x7f6391cfd000; size:
> > 0x0098cf491000 (656312700928)
> > False positive error reports may follow
> > For details see https://github.com/google/sanitizers/issues/189
> > ==7294==WARNING: ASan doesn't fully support makecontext/swapcontext
> > functions and may produce false positives in some cases!
> > ==7294==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > stack type: default top: 0x7ffef665d000; bottom 0x7f69dc8fd000; size:
> > 0x009519d60000 (640383582208)
> > False positive error reports may follow
> > For details see https://github.com/google/sanitizers/issues/189
> > ==7300==WARNING: ASan doesn't fully support makecontext/swapcontext
> > functions and may produce false positives in some cases!
> > ==7300==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > stack type: default top: 0x7ffe33db0000; bottom 0x7f01421fd000; size:
> > 0x00fcf1bb3000 (1086387335168)
> > False positive error reports may follow
> > For details see https://github.com/google/sanitizers/issues/189
> > ==7306==WARNING: ASan doesn't fully support makecontext/swapcontext
> > functions and may produce false positives in some cases!
> > ==7306==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > stack type: default top: 0x7ffebd618000; bottom 0x7ff1179fd000; size:
> > 0x000da5c1b000 (58615508992)
> > False positive error reports may follow
> > For details see https://github.com/google/sanitizers/issues/189
> > =================================================================
> > ==7306==ERROR: AddressSanitizer: heap-buffer-overflow on address
> > 0x612000030768 at pc 0x562351066c74 bp 0x7ff1078c3a90 sp
> > 0x7ff1078c3240
> > READ of size 48830 at 0x612000030768 thread T4

The size looks pretty big to me. Test file paths in virtio-9p-test are
only a couple of bytes long...

> > #0 0x562351066c73 in __interceptor_memcpy.part.0 asan_interceptors.cpp.o
> > #1 0x7ff1290d443f in g_memdup (/lib64/libglib-2.0.so.0+0x6e43f)
> > #2 0x56235134537a in do_readdir_many
> > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:146:19
> > #3 0x56235134537a in v9fs_co_readdir_many
> > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:225:5
> > #4 0x56235132d626 in v9fs_do_readdir
> > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:2430:13
> > #5 0x56235132d626 in v9fs_readdir
> > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:2543:13
> > #6 0x56235257101e in coroutine_trampoline
> > /builds/qemu-project/qemu/build-oss-fuzz/../util/coroutine-ucontext.c:173:9
> > #7 0x7ff126e0e84f (/lib64/libc.so.6+0x5784f)
> > 0x612000030768 is located 0 bytes to the right of 296-byte region
> > [0x612000030640,0x612000030768)
> > allocated by thread T4 here:
> > #0 0x5623510a4e47 in malloc
> > (/builds/qemu-project/qemu/build-oss-fuzz/qemu-system-i386+0x1146e47)
> > #1 0x7ff1290c03d8 in g_malloc (/lib64/libglib-2.0.so.0+0x5a3d8)

i.e.

    synth_open = g_malloc(sizeof(*synth_open));

and we have:

typedef struct V9fsSynthOpenState {
    off_t offset;
    V9fsSynthNode *node;
    struct dirent dent;
} V9fsSynthOpenState;

It looks like that qemu_dirent_dup() ends up using an
uninitialized dent->d_reclen.

The synth backend should be fixed to honor d_reclen, or
at least to allocate with g_new0().

Cheers,

--
Greg

> > #2 0x56235131e659 in synth_opendir
> > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p-synth.c:185:18
> > #3 0x5623513462f5 in v9fs_co_opendir
> > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:321:5
> > #4 0x5623513257d7 in v9fs_open
> > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:1959:15
> > #5 0x56235257101e in coroutine_trampoline
> > /builds/qemu-project/qemu/build-oss-fuzz/../util/coroutine-ucontext.c:173:9
> > #6 0x7ff126e0e84f (/lib64/libc.so.6+0x5784f)
> > Thread T4 created by T0 here:
> > #0 0x562351015926 in pthread_create
> > (/builds/qemu-project/qemu/build-oss-fuzz/qemu-system-i386+0x10b7926)
> > #1 0x5623525351ea in qemu_thread_create
> > /builds/qemu-project/qemu/build-oss-fuzz/../util/qemu-thread-posix.c:596:11
> > #2 0x5623525a4588 in do_spawn_thread
> > /builds/qemu-project/qemu/build-oss-fuzz/../util/thread-pool.c:134:5
> > #3 0x5623525a4588 in spawn_thread_bh_fn
> > /builds/qemu-project/qemu/build-oss-fuzz/../util/thread-pool.c:142:5
> > #4 0x562352569814 in aio_bh_call
> > /builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:141:5
> > #5 0x562352569814 in aio_bh_poll
> > /builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:169:13
> > #6 0x5623525248cc in aio_dispatch
> > /builds/qemu-project/qemu/build-oss-fuzz/../util/aio-posix.c:415:5
> > #7 0x56235256c34c in aio_ctx_dispatch
> > /builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:311:5
> > #8 0x7ff1290bb05e in g_main_context_dispatch
> > (/lib64/libglib-2.0.so.0+0x5505e) SUMMARY: AddressSanitizer:
> > heap-buffer-overflow
> > asan_interceptors.cpp.o in __interceptor_memcpy.part.0
> > Shadow bytes around the buggy address:
> > 0x0c247fffe090: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
> > 0x0c247fffe0a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> > 0x0c247fffe0b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa
> > 0x0c247fffe0c0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
> > 0x0c247fffe0d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > =>0x0c247fffe0e0: 00 00 00 00 00 00 00 00 00 00 00 00 00[fa]fa fa
> > 0x0c247fffe0f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > 0x0c247fffe100: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > 0x0c247fffe110: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > 0x0c247fffe120: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > 0x0c247fffe130: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > Shadow byte legend (one shadow byte represents 8 application bytes):
> > Addressable: 00
> > Partially addressable: 01 02 03 04 05 06 07
> > Heap left redzone: fa
> > Freed heap region: fd
> > Stack left redzone: f1
> > Stack mid redzone: f2
> > Stack right redzone: f3
> > Stack after return: f5
> > Stack use after scope: f8
> > Global redzone: f9
> > Global init order: f6
> > Poisoned by user: f7
> > Container overflow: fc
> > Array cookie: ac
> > Intra object redzone: bb
> > ASan internal: fe
> > Left alloca redzone: ca
> > Right alloca redzone: cb
> > ==7306==ABORTING
> > 
> > 
> > thanks
> > -- PMM
> 
> 

Re: [PULL 0/5] 9p queue 2022-02-10

[Christian Schoenebeck]

On Montag, 14. Februar 2022 11:36:53 CET Greg Kurz wrote:
> On Mon, 14 Feb 2022 10:47:43 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Sonntag, 13. Februar 2022 21:33:10 CET Peter Maydell wrote:
> > > On Thu, 10 Feb 2022 at 11:33, Christian Schoenebeck
> > > 
> > > <qemu_oss@crudebyte.com> wrote:
> > > > The following changes since commit 
0a301624c2f4ced3331ffd5bce85b4274fe132af:
> > > >   Merge remote-tracking branch
> > > >   'remotes/pmaydell/tags/pull-target-arm-20220208' into staging
> > > >   (2022-02-08 11:40:08 +0000)>
> > > > 
> > > > are available in the Git repository at:
> > > >   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20220210
> > > > 
> > > > for you to fetch changes up to 
de19c79dad6a2cad54ae04ce754d47c07bf9bc93:
> > > >   9pfs: Fix segfault in do_readdir_many caused by struct dirent
> > > >   overread
> > > >   (2022-02-10 11:56:01 +0100)>
> > > > 
> > > > ----------------------------------------------------------------9d82f6
> > > > a3e68c2 9pfs: fixes and cleanup
> > > > 
> > > > * Fifth patch fixes a 9pfs server crash that happened on some systems
> > > > due
> > > > 
> > > >   to incorrect (system dependant) handling of struct dirent size.
> > > > 
> > > > * Tests: Second patch fixes a test error that happened on some systems
> > > > due
> > > > 
> > > >   mkdir() being called twice for creating the test directory for the
> > > >   9p
> > > >   'local' tests.
> > > > 
> > > > * Tests: Third patch fixes a memory leak.
> > > > 
> > > > * Tests: The remaining two patches are code cleanup.
> > > > 
> > > > ----------------------------------------------------------------
> > > 
> > > Hi; this fails CI for the build-oss-fuzz job, which finds
> > > a heap-buffer-overflow:
> > > https://gitlab.com/qemu-project/qemu/-/jobs/2087610013
> > 
> > So this is about the 'dirent' patch:
> > https://github.com/cschoenebeck/qemu/commit/de19c79dad6a2cad54ae04ce754d47
> > c07bf9bc93
> > 
> > In conjunction with the 9p fuzzing tests:
> > https://wiki.qemu.org/Documentation/9p#Fuzzing
> > 
> > I first thought it might be a false positive due to the unorthodox
> > handling of dirent duplication by that patch, but from the ASan output
> > below I am not really sure about that.
> 
> No, this is an actual bug. See below.

Yep, the patch would turn the 9p tests' synth driver buggy. :/ Vitaly, I fear 
the patch needs a v5.

> > Is there a way to get the content of local variables?
> > 
> > Would it be possible that the following issue (g_memdup vs. g_memdup2)
> > might apply here?
> > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-> > now/5538
> > 
> > Best regards,
> > Christian Schoenebeck
> > 
> > > 8/152 qemu:qtest+qtest-i386 / qtest-i386/qos-test ERROR 66.74s killed
> > > by signal 6 SIGABRT
> > > 
> > > >>> QTEST_QEMU_BINARY=./qemu-system-i386
> > > >>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemo
> > > >>> n
> > > >>> MALLOC_PERTURB_=120
> > > >>> G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daem
> > > >>> on.
> > > >>> sh QTEST_QEMU_IMG=./qemu-img
> > > >>> /builds/qemu-project/qemu/build-oss-fuzz/tests/qtest/qos-test --tap
> > > >>> -k
> > > 
> > > ――――――――――――――――――――――――――――――――――――― ✀
> > > ――――――――――――――――――――――――――――――――――――― Listing only the last 100 lines
> > > from
> > > a long log.
> > > For details see https://github.com/google/sanitizers/issues/189
> > > ==7270==WARNING: ASan doesn't fully support makecontext/swapcontext
> > > functions and may produce false positives in some cases!
> > > ==7270==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > > stack type: default top: 0x7ffc79fb0000; bottom 0x7ff908ffd000; size:
> > > 0x000370fb3000 (14780411904)
> > > False positive error reports may follow
> > > For details see https://github.com/google/sanitizers/issues/189
> > > ==7276==WARNING: ASan doesn't fully support makecontext/swapcontext
> > > functions and may produce false positives in some cases!
> > > ==7276==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > > stack type: default top: 0x7fff7e4a8000; bottom 0x7fd6363fd000; size:
> > > 0x0029480ab000 (177302319104)
> > > False positive error reports may follow
> > > For details see https://github.com/google/sanitizers/issues/189
> > > ==7282==WARNING: ASan doesn't fully support makecontext/swapcontext
> > > functions and may produce false positives in some cases!
> > > ==7282==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > > stack type: default top: 0x7ffee6e7f000; bottom 0x7f32fb5fd000; size:
> > > 0x00cbeb882000 (875829927936)
> > > False positive error reports may follow
> > > For details see https://github.com/google/sanitizers/issues/189
> > > ==7288==WARNING: ASan doesn't fully support makecontext/swapcontext
> > > functions and may produce false positives in some cases!
> > > ==7288==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > > stack type: default top: 0x7ffc6118e000; bottom 0x7f6391cfd000; size:
> > > 0x0098cf491000 (656312700928)
> > > False positive error reports may follow
> > > For details see https://github.com/google/sanitizers/issues/189
> > > ==7294==WARNING: ASan doesn't fully support makecontext/swapcontext
> > > functions and may produce false positives in some cases!
> > > ==7294==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > > stack type: default top: 0x7ffef665d000; bottom 0x7f69dc8fd000; size:
> > > 0x009519d60000 (640383582208)
> > > False positive error reports may follow
> > > For details see https://github.com/google/sanitizers/issues/189
> > > ==7300==WARNING: ASan doesn't fully support makecontext/swapcontext
> > > functions and may produce false positives in some cases!
> > > ==7300==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > > stack type: default top: 0x7ffe33db0000; bottom 0x7f01421fd000; size:
> > > 0x00fcf1bb3000 (1086387335168)
> > > False positive error reports may follow
> > > For details see https://github.com/google/sanitizers/issues/189
> > > ==7306==WARNING: ASan doesn't fully support makecontext/swapcontext
> > > functions and may produce false positives in some cases!
> > > ==7306==WARNING: ASan is ignoring requested __asan_handle_no_return:
> > > stack type: default top: 0x7ffebd618000; bottom 0x7ff1179fd000; size:
> > > 0x000da5c1b000 (58615508992)
> > > False positive error reports may follow
> > > For details see https://github.com/google/sanitizers/issues/189
> > > =================================================================
> > > ==7306==ERROR: AddressSanitizer: heap-buffer-overflow on address
> > > 0x612000030768 at pc 0x562351066c74 bp 0x7ff1078c3a90 sp
> > > 0x7ff1078c3240
> > > READ of size 48830 at 0x612000030768 thread T4
> 
> The size looks pretty big to me. Test file paths in virtio-9p-test are
> only a couple of bytes long...

Right.

> > > #0 0x562351066c73 in __interceptor_memcpy.part.0 asan_interceptors.cpp.o
> > > #1 0x7ff1290d443f in g_memdup (/lib64/libglib-2.0.so.0+0x6e43f)
> > > #2 0x56235134537a in do_readdir_many
> > > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:146:19
> > > #3 0x56235134537a in v9fs_co_readdir_many
> > > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:225:5
> > > #4 0x56235132d626 in v9fs_do_readdir
> > > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:2430:13
> > > #5 0x56235132d626 in v9fs_readdir
> > > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:2543:13
> > > #6 0x56235257101e in coroutine_trampoline
> > > /builds/qemu-project/qemu/build-oss-fuzz/../util/coroutine-ucontext.c:17
> > > 3:9
> > > #7 0x7ff126e0e84f (/lib64/libc.so.6+0x5784f)
> > > 0x612000030768 is located 0 bytes to the right of 296-byte region
> > > [0x612000030640,0x612000030768)
> > > allocated by thread T4 here:
> > > #0 0x5623510a4e47 in malloc
> > > (/builds/qemu-project/qemu/build-oss-fuzz/qemu-system-i386+0x1146e47)
> > > #1 0x7ff1290c03d8 in g_malloc (/lib64/libglib-2.0.so.0+0x5a3d8)
> 
> i.e.
> 
>     synth_open = g_malloc(sizeof(*synth_open));
> 
> and we have:
> 
> typedef struct V9fsSynthOpenState {
>     off_t offset;
>     V9fsSynthNode *node;
>     struct dirent dent;
> } V9fsSynthOpenState;
> 
> It looks like that qemu_dirent_dup() ends up using an
> uninitialized dent->d_reclen.
> 
> The synth backend should be fixed to honor d_reclen, or
> at least to allocate with g_new0().

Yes, I overlooked that this is not initialized with zero already.

With g_new0() d_reclen would be zero and qemu_dirent_dup() would then fallback 
to the portable branch (as I assumed it already would):

struct dirent *
qemu_dirent_dup(struct dirent *dent)
{
    size_t sz = 0;
#if defined _DIRENT_HAVE_D_RECLEN
    /* Avoid use of strlen() if platform supports d_reclen. */
    sz = dent->d_reclen;
#endif
    /*
     * Test sz for zero even if d_reclen is available
     * because some drivers may set d_reclen to zero.
     */
    if (sz == 0) {
        /* Fallback to the most portable way. */
        sz = offsetof(struct dirent, d_name) +
                      strlen(dent->d_name) + 1;
    }
    return g_memdup(dent, sz);
}

Additionally I would add NAME_MAX to the V9fsSynthOpenState allocation size, 
because it is known that some systems define dirent as flex-array (zero d_name 
size).

I know Greg would not favour this solution (using g_new0), but it's the most 
minimalistic and most portable solution. So I would favour it for now.

A cleaner solution on the long-term would be turning V9fsSynthOpenState's 
'dent' member into a pointer and adding a new function to osdep like:

struct dirent *
qemu_dirent_new(const char* name) {
    ...
}

But I would like to postpone that qemu_dirent_new() solution, e.g. because I 
guess some people would probably not like qemu_dirent_new() to have in osdep, 
as it is probably not a general purpose function, and I am not keen putting 
qemu_dirent_new() into a different location than qemu_dirent_dup(), because it 
would raise the danger that system dependent code might deviate in future.

Best regards,
Christian Schoenebeck

> 
> Cheers,
> 
> --
> Greg
> 
> > > #2 0x56235131e659 in synth_opendir
> > > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p-synth.c:185:18
> > > #3 0x5623513462f5 in v9fs_co_opendir
> > > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/codir.c:321:5
> > > #4 0x5623513257d7 in v9fs_open
> > > /builds/qemu-project/qemu/build-oss-fuzz/../hw/9pfs/9p.c:1959:15
> > > #5 0x56235257101e in coroutine_trampoline
> > > /builds/qemu-project/qemu/build-oss-fuzz/../util/coroutine-ucontext.c:17
> > > 3:9
> > > #6 0x7ff126e0e84f (/lib64/libc.so.6+0x5784f)
> > > Thread T4 created by T0 here:
> > > #0 0x562351015926 in pthread_create
> > > (/builds/qemu-project/qemu/build-oss-fuzz/qemu-system-i386+0x10b7926)
> > > #1 0x5623525351ea in qemu_thread_create
> > > /builds/qemu-project/qemu/build-oss-fuzz/../util/qemu-thread-posix.c:596
> > > :11
> > > #2 0x5623525a4588 in do_spawn_thread
> > > /builds/qemu-project/qemu/build-oss-fuzz/../util/thread-pool.c:134:5
> > > #3 0x5623525a4588 in spawn_thread_bh_fn
> > > /builds/qemu-project/qemu/build-oss-fuzz/../util/thread-pool.c:142:5
> > > #4 0x562352569814 in aio_bh_call
> > > /builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:141:5
> > > #5 0x562352569814 in aio_bh_poll
> > > /builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:169:13
> > > #6 0x5623525248cc in aio_dispatch
> > > /builds/qemu-project/qemu/build-oss-fuzz/../util/aio-posix.c:415:5
> > > #7 0x56235256c34c in aio_ctx_dispatch
> > > /builds/qemu-project/qemu/build-oss-fuzz/../util/async.c:311:5
> > > #8 0x7ff1290bb05e in g_main_context_dispatch
> > > (/lib64/libglib-2.0.so.0+0x5505e) SUMMARY: AddressSanitizer:
> > > heap-buffer-overflow
> > > asan_interceptors.cpp.o in __interceptor_memcpy.part.0
> > > Shadow bytes around the buggy address:
> > > 0x0c247fffe090: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
> > > 0x0c247fffe0a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> > > 0x0c247fffe0b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa
> > > 0x0c247fffe0c0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
> > > 0x0c247fffe0d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > =>0x0c247fffe0e0: 00 00 00 00 00 00 00 00 00 00 00 00 00[fa]fa fa
> > > 0x0c247fffe0f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > > 0x0c247fffe100: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > > 0x0c247fffe110: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > > 0x0c247fffe120: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > > 0x0c247fffe130: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > > Shadow byte legend (one shadow byte represents 8 application bytes):
> > > Addressable: 00
> > > Partially addressable: 01 02 03 04 05 06 07
> > > Heap left redzone: fa
> > > Freed heap region: fd
> > > Stack left redzone: f1
> > > Stack mid redzone: f2
> > > Stack right redzone: f3
> > > Stack after return: f5
> > > Stack use after scope: f8
> > > Global redzone: f9
> > > Global init order: f6
> > > Poisoned by user: f7
> > > Container overflow: fc
> > > Array cookie: ac
> > > Intra object redzone: bb
> > > ASan internal: fe
> > > Left alloca redzone: ca
> > > Right alloca redzone: cb
> > > ==7306==ABORTING
> > > 
> > > 
> > > thanks
> > > -- PMM

Re: [PULL 0/5] 9p queue 2022-02-10

[Vitaly Chikunov]

Christian,

On Mon, Feb 14, 2022 at 12:44:48PM +0100, Christian Schoenebeck wrote:
> On Montag, 14. Februar 2022 11:36:53 CET Greg Kurz wrote:
> > The synth backend should be fixed to honor d_reclen, or
> > at least to allocate with g_new0().
> 
> Yes, I overlooked that this is not initialized with zero already.
> 
> With g_new0() d_reclen would be zero and qemu_dirent_dup() would then fallback 
> to the portable branch (as I assumed it already would):

Perhaps, this additional change should be added (I only found two instances of
V9fsSynthOpenState allocation):

diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -182,7 +182,7 @@ static int synth_opendir(FsContext *ctx,
     V9fsSynthOpenState *synth_open;
     V9fsSynthNode *node = *(V9fsSynthNode **)fs_path->data;
 
-    synth_open = g_malloc(sizeof(*synth_open));
+    synth_open = g_malloc0(sizeof(*synth_open));
     synth_open->node = node;
     node->open_count++;
     fs->private = synth_open;
@@ -266,7 +266,7 @@ static int synth_open(FsContext *ctx, V9fsPath *fs_path,
     V9fsSynthOpenState *synth_open;
     V9fsSynthNode *node = *(V9fsSynthNode **)fs_path->data;
 
-    synth_open = g_malloc(sizeof(*synth_open));
+    synth_open = g_malloc0(sizeof(*synth_open));
     synth_open->node = node;
     node->open_count++;
     fs->private = synth_open;

> Additionally I would add NAME_MAX to the V9fsSynthOpenState allocation size, 
> because it is known that some systems define dirent as flex-array (zero d_name 
> size).

(To be precise) not just zero, but 1 byte. Also, to remind, for some
filesystems, such as CIFS, actual d_name size could be longer than NAME_MAX.
Because of that struct dirent cannot be allocated statically or with simple
sizeof.

> 
> I know Greg would not favour this solution (using g_new0), but it's the most 
> minimalistic and most portable solution. So I would favour it for now.

Why g_new0 and not just g_malloc0? This is smallest code change, which seems
appropriate for a bug fix.

Thanks,

> 
> A cleaner solution on the long-term would be turning V9fsSynthOpenState's 
> 'dent' member into a pointer and adding a new function to osdep like:
> 
> struct dirent *
> qemu_dirent_new(const char* name) {
>     ...
> }
> 
> But I would like to postpone that qemu_dirent_new() solution, e.g. because I 
> guess some people would probably not like qemu_dirent_new() to have in osdep, 
> as it is probably not a general purpose function, and I am not keen putting 
> qemu_dirent_new() into a different location than qemu_dirent_dup(), because it 
> would raise the danger that system dependent code might deviate in future.
> 
> Best regards,
> Christian Schoenebeck
> 

Re: [PULL 0/5] 9p queue 2022-02-10

[Christian Schoenebeck]

On Montag, 14. Februar 2022 15:43:51 CET Vitaly Chikunov wrote:
> Christian,
> 
> On Mon, Feb 14, 2022 at 12:44:48PM +0100, Christian Schoenebeck wrote:
> > On Montag, 14. Februar 2022 11:36:53 CET Greg Kurz wrote:
> > > The synth backend should be fixed to honor d_reclen, or
> > > at least to allocate with g_new0().
> > 
> > Yes, I overlooked that this is not initialized with zero already.
> > 
> > With g_new0() d_reclen would be zero and qemu_dirent_dup() would then
> > fallback
> > to the portable branch (as I assumed it already would):
> Perhaps, this additional change should be added (I only found two instances
> of V9fsSynthOpenState allocation):
> 
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -182,7 +182,7 @@ static int synth_opendir(FsContext *ctx,
>      V9fsSynthOpenState *synth_open;
>      V9fsSynthNode *node = *(V9fsSynthNode **)fs_path->data;
> 
> -    synth_open = g_malloc(sizeof(*synth_open));
> +    synth_open = g_malloc0(sizeof(*synth_open));
>      synth_open->node = node;
>      node->open_count++;
>      fs->private = synth_open;
> @@ -266,7 +266,7 @@ static int synth_open(FsContext *ctx, V9fsPath *fs_path,
> V9fsSynthOpenState *synth_open;
>      V9fsSynthNode *node = *(V9fsSynthNode **)fs_path->data;
> 
> -    synth_open = g_malloc(sizeof(*synth_open));
> +    synth_open = g_malloc0(sizeof(*synth_open));
>      synth_open->node = node;
>      node->open_count++;
>      fs->private = synth_open;

Either

   /*
    * Add NAME_MAX to ensure there is enough space for 'dent' member, because
    * some systems have d_name size of just 1, which would cause a buffer
    * overrun.
    */
	synth_open = g_malloc0(sizeof(*synth_open) + NAME_MAX);

Or more simple by adjusting struct V9fsSynthOpenState:

index 036d7e4a5b..eeb246f377 100644
--- a/hw/9pfs/9p-synth.h
+++ b/hw/9pfs/9p-synth.h
@@ -41,6 +41,11 @@ typedef struct V9fsSynthOpenState {
     off_t offset;
     V9fsSynthNode *node;
     struct dirent dent;
+    /*
+     * Ensure there is enough space for 'dent' above, some systems have a
+     * d_name size of just 1, which would cause a buffer overrun.
+     */
+    char dent_trailing_space[NAME_MAX];
 } V9fsSynthOpenState;
 
 int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode,

and of course still replacing g_malloc() by g_malloc0().

> > Additionally I would add NAME_MAX to the V9fsSynthOpenState allocation
> > size, because it is known that some systems define dirent as flex-array
> > (zero d_name size).
> 
> (To be precise) not just zero, but 1 byte. Also, to remind, for some
> filesystems, such as CIFS, actual d_name size could be longer than NAME_MAX.
> Because of that struct dirent cannot be allocated statically or with simple
> sizeof.

Yes, but the dir names in the synth driver are just short hard coded names
anyway, there is no access to a real filesystem going on in the synth driver. 

> > I know Greg would not favour this solution (using g_new0), but it's the
> > most minimalistic and most portable solution. So I would favour it for
> > now.
> Why g_new0 and not just g_malloc0? This is smallest code change, which seems
> appropriate for a bug fix.

Both are fine with me in this case.

> 
> Thanks,
> 
> > A cleaner solution on the long-term would be turning V9fsSynthOpenState's
> > 'dent' member into a pointer and adding a new function to osdep like:
> > 
> > struct dirent *
> > qemu_dirent_new(const char* name) {
> > 
> >     ...
> > 
> > }
> > 
> > But I would like to postpone that qemu_dirent_new() solution, e.g. because
> > I guess some people would probably not like qemu_dirent_new() to have in
> > osdep, as it is probably not a general purpose function, and I am not
> > keen putting qemu_dirent_new() into a different location than
> > qemu_dirent_dup(), because it would raise the danger that system
> > dependent code might deviate in future.
> > 
> > Best regards,
> > Christian Schoenebeck

Re: [PULL 0/5] 9p queue 2022-02-10

[Greg Kurz]

On Mon, 14 Feb 2022 17:43:51 +0300
Vitaly Chikunov <vt@altlinux.org> wrote:

> Christian,
> 
> On Mon, Feb 14, 2022 at 12:44:48PM +0100, Christian Schoenebeck wrote:
> > On Montag, 14. Februar 2022 11:36:53 CET Greg Kurz wrote:
> > > The synth backend should be fixed to honor d_reclen, or
> > > at least to allocate with g_new0().
> > 
> > Yes, I overlooked that this is not initialized with zero already.
> > 
> > With g_new0() d_reclen would be zero and qemu_dirent_dup() would then fallback 
> > to the portable branch (as I assumed it already would):
> 
> Perhaps, this additional change should be added (I only found two instances of
> V9fsSynthOpenState allocation):
> 
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -182,7 +182,7 @@ static int synth_opendir(FsContext *ctx,
>      V9fsSynthOpenState *synth_open;
>      V9fsSynthNode *node = *(V9fsSynthNode **)fs_path->data;
>  
> -    synth_open = g_malloc(sizeof(*synth_open));
> +    synth_open = g_malloc0(sizeof(*synth_open));
>      synth_open->node = node;
>      node->open_count++;
>      fs->private = synth_open;
> @@ -266,7 +266,7 @@ static int synth_open(FsContext *ctx, V9fsPath *fs_path,
>      V9fsSynthOpenState *synth_open;
>      V9fsSynthNode *node = *(V9fsSynthNode **)fs_path->data;
>  
> -    synth_open = g_malloc(sizeof(*synth_open));
> +    synth_open = g_malloc0(sizeof(*synth_open));
>      synth_open->node = node;
>      node->open_count++;
>      fs->private = synth_open;
> 
> > Additionally I would add NAME_MAX to the V9fsSynthOpenState allocation size, 
> > because it is known that some systems define dirent as flex-array (zero d_name 
> > size).
> 
> (To be precise) not just zero, but 1 byte. Also, to remind, for some
> filesystems, such as CIFS, actual d_name size could be longer than NAME_MAX.
> Because of that struct dirent cannot be allocated statically or with simple
> sizeof.
> 
> > 
> > I know Greg would not favour this solution (using g_new0), but it's the most 
> > minimalistic and most portable solution. So I would favour it for now.
> 
> Why g_new0 and not just g_malloc0? This is smallest code change, which seems
> appropriate for a bug fix.
> 


I prefer g_new0() for the exact reasons that are provided in QEMU's
official coding style docs/devel/style.rst:

---

Prefer ``g_new(T, n)`` instead of ``g_malloc(sizeof(T) * n)`` for the following
reasons:

* It catches multiplication overflowing size_t;
* It returns T ``*`` instead of void ``*``, letting compiler catch more type errors.

Declarations like

.. code-block:: c

    T *v = g_malloc(sizeof(*v))

are acceptable, though.

---

I'm fine with the acceptable version as well. The only important thing is
to fix the synth backend.

Cheers,

--
Greg

> Thanks,
> 
> > 
> > A cleaner solution on the long-term would be turning V9fsSynthOpenState's 
> > 'dent' member into a pointer and adding a new function to osdep like:
> > 
> > struct dirent *
> > qemu_dirent_new(const char* name) {
> >     ...
> > }
> > 
> > But I would like to postpone that qemu_dirent_new() solution, e.g. because I 
> > guess some people would probably not like qemu_dirent_new() to have in osdep, 
> > as it is probably not a general purpose function, and I am not keen putting 
> > qemu_dirent_new() into a different location than qemu_dirent_dup(), because it 
> > would raise the danger that system dependent code might deviate in future.
> > 
> > Best regards,
> > Christian Schoenebeck
> > 

Re: [PULL 0/5] 9p queue 2022-02-10

[Christian Schoenebeck]

On Dienstag, 15. Februar 2022 08:01:37 CET Greg Kurz wrote:
> On Mon, 14 Feb 2022 17:43:51 +0300
> 
> Vitaly Chikunov <vt@altlinux.org> wrote:
> > Why g_new0 and not just g_malloc0? This is smallest code change, which
> > seems appropriate for a bug fix.
> 
> I prefer g_new0() for the exact reasons that are provided in QEMU's
> official coding style docs/devel/style.rst:
[...]
> I'm fine with the acceptable version as well. The only important thing is
> to fix the synth backend.
> 
> Cheers,

Hi, is anybody working on a v5 of this patch? If not I will send one this 
evening to bring this issue forward, because it is blocking my queue.

Best regards,
Christian Schoenebeck

Re: [PULL 0/5] 9p queue 2022-02-10

[Greg Kurz]

On Wed, 16 Feb 2022 11:30:12 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Dienstag, 15. Februar 2022 08:01:37 CET Greg Kurz wrote:
> > On Mon, 14 Feb 2022 17:43:51 +0300
> > 
> > Vitaly Chikunov <vt@altlinux.org> wrote:
> > > Why g_new0 and not just g_malloc0? This is smallest code change, which
> > > seems appropriate for a bug fix.
> > 
> > I prefer g_new0() for the exact reasons that are provided in QEMU's
> > official coding style docs/devel/style.rst:
> [...]
> > I'm fine with the acceptable version as well. The only important thing is
> > to fix the synth backend.
> > 
> > Cheers,
> 
> Hi, is anybody working on a v5 of this patch? If not I will send one this 
> evening to bring this issue forward, because it is blocking my queue.
> 

I'm not, please post a patch and I'll review it ASAP.

Cheers,

--
Greg

> Best regards,
> Christian Schoenebeck
> 
> 

Re: [PULL 0/5] 9p queue 2022-02-10

[Philippe Mathieu-Daudé via]

On 16/2/22 11:30, Christian Schoenebeck wrote:
> On Dienstag, 15. Februar 2022 08:01:37 CET Greg Kurz wrote:
>> On Mon, 14 Feb 2022 17:43:51 +0300
>>
>> Vitaly Chikunov <vt@altlinux.org> wrote:
>>> Why g_new0 and not just g_malloc0? This is smallest code change, which
>>> seems appropriate for a bug fix.
>>
>> I prefer g_new0() for the exact reasons that are provided in QEMU's
>> official coding style docs/devel/style.rst:
> [...]
>> I'm fine with the acceptable version as well. The only important thing is
>> to fix the synth backend.
>>
>> Cheers,
> 
> Hi, is anybody working on a v5 of this patch? If not I will send one this
> evening to bring this issue forward, because it is blocking my queue.

If a patch blocks your tree queue, you can simply drop it, ask the
contributor to rework it, and keep merging your tree...

Re: [PULL 0/5] 9p queue 2022-02-10

[Vitaly Chikunov]

Christian,

On Wed, Feb 16, 2022 at 11:30:12AM +0100, Christian Schoenebeck wrote:
> On Dienstag, 15. Februar 2022 08:01:37 CET Greg Kurz wrote:
> > On Mon, 14 Feb 2022 17:43:51 +0300
> > 
> > Vitaly Chikunov <vt@altlinux.org> wrote:
> > > Why g_new0 and not just g_malloc0? This is smallest code change, which
> > > seems appropriate for a bug fix.
> > 
> > I prefer g_new0() for the exact reasons that are provided in QEMU's
> > official coding style docs/devel/style.rst:
> [...]
> > I'm fine with the acceptable version as well. The only important thing is
> > to fix the synth backend.
> > 
> > Cheers,
> 
> Hi, is anybody working on a v5 of this patch? If not I will send one this 
> evening to bring this issue forward, because it is blocking my queue.

I will send in a few hours if it's not too late.

Thanks,

> 
> Best regards,
> Christian Schoenebeck
> 

Re: [PULL 0/5] 9p queue 2022-02-10

[Christian Schoenebeck]

On Mittwoch, 16. Februar 2022 17:09:56 CET Vitaly Chikunov wrote:
> Christian,
> 
> On Wed, Feb 16, 2022 at 11:30:12AM +0100, Christian Schoenebeck wrote:
> > On Dienstag, 15. Februar 2022 08:01:37 CET Greg Kurz wrote:
> > > On Mon, 14 Feb 2022 17:43:51 +0300
> > > 
> > > Vitaly Chikunov <vt@altlinux.org> wrote:
> > > > Why g_new0 and not just g_malloc0? This is smallest code change, which
> > > > seems appropriate for a bug fix.
> > > 
> > > I prefer g_new0() for the exact reasons that are provided in QEMU's
> > 
> > > official coding style docs/devel/style.rst:
> > [...]
> > 
> > > I'm fine with the acceptable version as well. The only important thing
> > > is
> > > to fix the synth backend.
> > > 
> > > Cheers,
> > 
> > Hi, is anybody working on a v5 of this patch? If not I will send one this
> > evening to bring this issue forward, because it is blocking my queue.
> 
> I will send in a few hours if it's not too late.

Great! I would have been also just able to do that in several hours.

Best regards,
Christian Schoenebeck