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

[PULL v2 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 this, 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>
Tested-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Acked-by: Greg Kurz <groug@kaod.org>
Tested-by: Vitaly Chikunov <vt@altlinux.org>
Message-Id: <20220216181821.3481527-1-vt@altlinux.org>
[C.S. - Fix typo in source comment. ]
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p-synth.c   | 18 +++++++++++++++---
 hw/9pfs/9p-synth.h   |  5 +++++
 hw/9pfs/codir.c      |  3 +--
 include/qemu/osdep.h | 13 +++++++++++++
 util/osdep.c         | 21 +++++++++++++++++++++
 5 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index b38088e066..7a7cd5c5ba 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -182,7 +182,12 @@ static int synth_opendir(FsContext *ctx,
     V9fsSynthOpenState *synth_open;
     V9fsSynthNode *node = *(V9fsSynthNode **)fs_path->data;
 
-    synth_open = g_malloc(sizeof(*synth_open));
+    /*
+     * V9fsSynthOpenState contains 'struct dirent' which have OS-specific
+     * properties, thus it's zero cleared on allocation here and below
+     * in synth_open.
+     */
+    synth_open = g_new0(V9fsSynthOpenState, 1);
     synth_open->node = node;
     node->open_count++;
     fs->private = synth_open;
@@ -220,7 +225,14 @@ static void synth_rewinddir(FsContext *ctx, V9fsFidOpenState *fs)
 static void synth_direntry(V9fsSynthNode *node,
                                 struct dirent *entry, off_t off)
 {
-    strcpy(entry->d_name, node->name);
+    size_t sz = strlen(node->name) + 1;
+    /*
+     * 'entry' is always inside of V9fsSynthOpenState which have NAME_MAX
+     * back padding. Ensure we do not overflow it.
+     */
+    g_assert(sizeof(struct dirent) + NAME_MAX >=
+             offsetof(struct dirent, d_name) + sz);
+    memcpy(entry->d_name, node->name, sz);
     entry->d_ino = node->attr->inode;
     entry->d_off = off + 1;
 }
@@ -266,7 +278,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_new0(V9fsSynthOpenState, 1);
     synth_open->node = node;
     node->open_count++;
     fs->private = synth_open;
diff --git a/hw/9pfs/9p-synth.h b/hw/9pfs/9p-synth.h
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,
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 v2 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 v2 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

[PULL v2 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 v2 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 v2 0/5] 9p queue (previous 2022-02-10)

[Christian Schoenebeck]

The following changes since commit c13b8e9973635f34f3ce4356af27a311c993729c:

  Merge remote-tracking branch 'remotes/alistair/tags/pull-riscv-to-apply-20220216' into staging (2022-02-16 09:57:11 +0000)

are available in the Git repository at:

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

for you to fetch changes up to e64e27d5cb103b7764f1a05b6eda7e7fedd517c5:

  9pfs: Fix segfault in do_readdir_many caused by struct dirent overread (2022-02-17 16:57:58 +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/9p-synth.c             | 18 +++++++--
 hw/9pfs/9p-synth.h             |  5 +++
 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 ++++++++++
 7 files changed, 96 insertions(+), 92 deletions(-)

Re: [PULL v2 0/5] 9p queue (previous 2022-02-10)

[Peter Maydell]

On Thu, 17 Feb 2022 at 16:38, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> The following changes since commit c13b8e9973635f34f3ce4356af27a311c993729c:
>
>   Merge remote-tracking branch 'remotes/alistair/tags/pull-riscv-to-apply-20220216' into staging (2022-02-16 09:57:11 +0000)
>
> are available in the Git repository at:
>
>   https://github.com/cschoenebeck/qemu.git tags/pull-9p-20220217
>
> for you to fetch changes up to e64e27d5cb103b7764f1a05b6eda7e7fedd517c5:
>
>   9pfs: Fix segfault in do_readdir_many caused by struct dirent overread (2022-02-17 16:57:58 +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.
>
> ----------------------------------------------------------------


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/7.0
for any user-visible changes.

-- PMM

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

[Peter Maydell]

On Thu, 17 Feb 2022 at 16:43, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
> 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);

Hi; I just noticed this has landed in git recently.
Please don't add new prototypes to osdep.h -- it is
a header included by every single C file in the tree, so
making it bigger slows down compilation. osdep.h is supposed
to contain only:
 * things which everybody needs
 * things without which code would work on most platforms but
   fail to compile or misbehave on a minority of host OSes
   (ie system incompatibility handling)

This prototype is neither of those -- please find or create a more
appropriate header file for it, that can be included only by the
source files that actually need it.

thanks
-- PMM

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

[Christian Schoenebeck]

On Dienstag, 22. Februar 2022 14:21:52 CET Peter Maydell wrote:
> On Thu, 17 Feb 2022 at 16:43, Christian Schoenebeck
> 
> <qemu_oss@crudebyte.com> wrote:
> > 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);
> 
> Hi; I just noticed this has landed in git recently.
> Please don't add new prototypes to osdep.h -- it is
> a header included by every single C file in the tree, so
> making it bigger slows down compilation. osdep.h is supposed
> to contain only:
>  * things which everybody needs
>  * things without which code would work on most platforms but
>    fail to compile or misbehave on a minority of host OSes
>    (ie system incompatibility handling)
> 
> This prototype is neither of those -- please find or create a more
> appropriate header file for it, that can be included only by the
> source files that actually need it.
> 
> thanks
> -- PMM

Good to know, because the pending Darwin series would have added stuff to 
osdep.h as well:
https://lore.kernel.org/qemu-devel/20220220165056.72289-10-wwcohen@gmail.com/

We'll find a different place.

Thanks!

Best regards,
Christian Schoenebeck

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

[Greg Kurz]

On Tue, 22 Feb 2022 14:54:17 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Dienstag, 22. Februar 2022 14:21:52 CET Peter Maydell wrote:
> > On Thu, 17 Feb 2022 at 16:43, Christian Schoenebeck
> > 
> > <qemu_oss@crudebyte.com> wrote:
> > > 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);
> > 
> > Hi; I just noticed this has landed in git recently.
> > Please don't add new prototypes to osdep.h -- it is
> > a header included by every single C file in the tree, so
> > making it bigger slows down compilation. osdep.h is supposed
> > to contain only:
> >  * things which everybody needs
> >  * things without which code would work on most platforms but
> >    fail to compile or misbehave on a minority of host OSes
> >    (ie system incompatibility handling)
> > 
> > This prototype is neither of those -- please find or create a more
> > appropriate header file for it, that can be included only by the
> > source files that actually need it.
> > 
> > thanks
> > -- PMM
> 
> Good to know, because the pending Darwin series would have added stuff to 
> osdep.h as well:
> https://lore.kernel.org/qemu-devel/20220220165056.72289-10-wwcohen@gmail.com/
> 
> We'll find a different place.
> 

I suggest you move all that under hw/9pfs for now since it is
the only user and we don't really know if there will ever be
a new one (especially now that development on C virtiofsd is
slowing down).

Cheers,

--
Greg


> Thanks!
> 
> Best regards,
> Christian Schoenebeck
> 
>