[Qemu-devel] Trivial memory allocation fixes & cleanups

[Qemu-devel] Trivial memory allocation fixes & cleanups

[Markus Armbruster]

v2: Windows compile fix in PATCH 08/11 (thanks eblake!), rest identical

Markus Armbruster (11):
  g_malloc(0) and g_malloc0(0) return NULL; simplify
  g_strdup(NULL) returns NULL; simplify
  hw/9pfs: Fix unchecked strdup() by converting to g_strdup()
  readline: Fix unchecked strdup() by converting to g_strdup()
  spice: Fix unchecked strdup() by converting to g_strdup()
  virtfs-proxy-helper: Fix unchecked strdup() by conv. to g_strdup()
  qemu-log: Fix unchecked strdup() by converting to g_strdup()
  qemu-log: Plug trivial memory leak in cpu_set_log_filename()
  libcacard: Fix unchecked strdup() by converting to g_strdup()
  qapi: Fix unchecked strdup() by converting to g_strdup()
  qemu-ga: Fix unchecked strdup() by converting to g_strdup()

 block/qcow2-refcount.c      |  6 +-----
 block/vdi.c                 |  4 +---
 fsdev/virtfs-proxy-helper.c |  4 ++--
 hw/9pfs/virtio-9p-device.c  |  8 ++------
 hw/9pfs/virtio-9p-local.c   |  8 ++++----
 hw/9pfs/virtio-9p.c         |  6 +-----
 hw/block-common.c           |  4 +---
 hw/vhost.c                  |  7 ++-----
 libcacard/vcard_emul_nss.c  |  4 ++--
 libcacard/vreader.c         |  2 +-
 libcacard/vscclient.c       |  4 ++--
 net/net.c                   |  4 +---
 qapi/qmp-registry.c         |  2 +-
 qemu-log.c                  | 15 ++++++++++-----
 qga/commands.c              |  2 +-
 readline.c                  |  4 ++--
 slirp/slirp.c               |  8 ++------
 ui/spice-core.c             |  4 ++--
 util/qemu-option.c          |  8 ++------
 19 files changed, 40 insertions(+), 64 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH v2 01/11] g_malloc(0) and g_malloc0(0) return NULL; simplify

[Markus Armbruster]

Once upon a time, it was decided that qemu_malloc(0) should abort.
Switching to glib retired that bright idea.  Some code that was added
to cope with it (e.g. in commits 702ef63, b76b6e9) is still around.
Bury it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2-refcount.c | 6 +-----
 block/vdi.c            | 4 +---
 hw/9pfs/virtio-9p.c    | 6 +-----
 hw/vhost.c             | 7 ++-----
 4 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 6a95aa6..bc1784c 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -737,11 +737,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
      * l1_table_offset when it is the current s->l1_table_offset! Be careful
      * when changing this! */
     if (l1_table_offset != s->l1_table_offset) {
-        if (l1_size2 != 0) {
-            l1_table = g_malloc0(align_offset(l1_size2, 512));
-        } else {
-            l1_table = NULL;
-        }
+        l1_table = g_malloc0(align_offset(l1_size2, 512));
         l1_allocated = 1;
         if (bdrv_pread(bs->file, l1_table_offset,
                        l1_table, l1_size2) != l1_size2)
diff --git a/block/vdi.c b/block/vdi.c
index 021abaa..8dfefdb 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -429,9 +429,7 @@ static int vdi_open(BlockDriverState *bs, int flags)
 
     bmap_size = header.blocks_in_image * sizeof(uint32_t);
     bmap_size = (bmap_size + SECTOR_SIZE - 1) / SECTOR_SIZE;
-    if (bmap_size > 0) {
-        s->bmap = g_malloc(bmap_size * SECTOR_SIZE);
-    }
+    s->bmap = g_malloc(bmap_size * SECTOR_SIZE);
     if (bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap, bmap_size) < 0) {
         goto fail_free_bmap;
     }
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 0aaf0d2..b795839 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -3101,11 +3101,7 @@ static void v9fs_xattrcreate(void *opaque)
     xattr_fidp->fs.xattr.flags = flags;
     v9fs_string_init(&xattr_fidp->fs.xattr.name);
     v9fs_string_copy(&xattr_fidp->fs.xattr.name, &name);
-    if (size) {
-        xattr_fidp->fs.xattr.value = g_malloc(size);
-    } else {
-        xattr_fidp->fs.xattr.value = NULL;
-    }
+    xattr_fidp->fs.xattr.value = g_malloc(size);
     err = offset;
     put_fid(pdu, file_fidp);
 out_nofid:
diff --git a/hw/vhost.c b/hw/vhost.c
index cee8aad..0dd2a9a 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -269,11 +269,8 @@ static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t size)
     vhost_log_chunk_t *log;
     uint64_t log_base;
     int r, i;
-    if (size) {
-        log = g_malloc0(size * sizeof *log);
-    } else {
-        log = NULL;
-    }
+
+    log = g_malloc0(size * sizeof *log);
     log_base = (uint64_t)(unsigned long)log;
     r = ioctl(dev->control, VHOST_SET_LOG_BASE, &log_base);
     assert(r >= 0);
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 02/11] g_strdup(NULL) returns NULL; simplify

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/9pfs/virtio-9p-device.c | 6 +-----
 hw/block-common.c          | 4 +---
 net/net.c                  | 4 +---
 slirp/slirp.c              | 8 ++------
 util/qemu-option.c         | 8 ++------
 5 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 6f427df..6eab7f7 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -85,11 +85,7 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
     }
 
     s->ctx.export_flags = fse->export_flags;
-    if (fse->path) {
-        s->ctx.fs_root = g_strdup(fse->path);
-    } else {
-        s->ctx.fs_root = NULL;
-    }
+    s->ctx.fs_root = g_strdup(fse->path);
     s->ctx.exops.get_st_gen = NULL;
     len = strlen(conf->tag);
     if (len > MAX_TAG_LEN - 1) {
diff --git a/hw/block-common.c b/hw/block-common.c
index 0f1b64e..d21ec3a 100644
--- a/hw/block-common.c
+++ b/hw/block-common.c
@@ -18,9 +18,7 @@ void blkconf_serial(BlockConf *conf, char **serial)
     if (!*serial) {
         /* try to fall back to value set with legacy -drive serial=... */
         dinfo = drive_get_by_blockdev(conf->bs);
-        if (dinfo->serial) {
-            *serial = g_strdup(dinfo->serial);
-        }
+        *serial = g_strdup(dinfo->serial);
     }
 }
 
diff --git a/net/net.c b/net/net.c
index 02b5458..0ff5262 100644
--- a/net/net.c
+++ b/net/net.c
@@ -566,9 +566,7 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
         assert(peer);
         nd->netdev = peer;
     }
-    if (name) {
-        nd->name = g_strdup(name);
-    }
+    nd->name = g_strdup(name);
     if (nic->has_model) {
         nd->model = g_strdup(nic->model);
     }
diff --git a/slirp/slirp.c b/slirp/slirp.c
index e93b578..0e6e232 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -225,12 +225,8 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
         pstrcpy(slirp->client_hostname, sizeof(slirp->client_hostname),
                 vhostname);
     }
-    if (tftp_path) {
-        slirp->tftp_prefix = g_strdup(tftp_path);
-    }
-    if (bootfile) {
-        slirp->bootp_filename = g_strdup(bootfile);
-    }
+    slirp->tftp_prefix = g_strdup(tftp_path);
+    slirp->bootp_filename = g_strdup(bootfile);
     slirp->vdhcp_startaddr = vdhcp_start;
     slirp->vnameserver_addr = vnameserver;
 
diff --git a/util/qemu-option.c b/util/qemu-option.c
index f532b76..c12e724 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -643,9 +643,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
         QTAILQ_INSERT_TAIL(&opts->head, opt, next);
     }
     opt->desc = desc;
-    if (value) {
-        opt->str = g_strdup(value);
-    }
+    opt->str = g_strdup(value);
     qemu_opt_parse(opt, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
@@ -792,9 +790,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
         }
     }
     opts = g_malloc0(sizeof(*opts));
-    if (id) {
-        opts->id = g_strdup(id);
-    }
+    opts->id = g_strdup(id);
     opts->list = list;
     loc_save(&opts->loc);
     QTAILQ_INIT(&opts->head);
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 03/11] hw/9pfs: Fix unchecked strdup() by converting to g_strdup()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/9pfs/virtio-9p-device.c | 2 +-
 hw/9pfs/virtio-9p-local.c  | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 6eab7f7..74155fb 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -94,7 +94,7 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
         exit(1);
     }
 
-    s->tag = strdup(conf->tag);
+    s->tag = g_strdup(conf->tag);
     s->ctx.uid = -1;
 
     s->ops = fse->ops;
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 1136021..f1b1c83 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -46,7 +46,7 @@ static const char *local_mapped_attr_path(FsContext *ctx,
                                           const char *path, char *buffer)
 {
     char *dir_name;
-    char *tmp_path = strdup(path);
+    char *tmp_path = g_strdup(path);
     char *base_name = basename(tmp_path);
 
     /* NULL terminate the directory */
@@ -55,7 +55,7 @@ static const char *local_mapped_attr_path(FsContext *ctx,
 
     snprintf(buffer, PATH_MAX, "%s/%s/%s/%s",
              ctx->fs_root, dir_name, VIRTFS_META_DIR, base_name);
-    free(tmp_path);
+    g_free(tmp_path);
     return buffer;
 }
 
@@ -130,7 +130,7 @@ static int local_create_mapped_attr_dir(FsContext *ctx, const char *path)
 {
     int err;
     char attr_dir[PATH_MAX];
-    char *tmp_path = strdup(path);
+    char *tmp_path = g_strdup(path);
 
     snprintf(attr_dir, PATH_MAX, "%s/%s/%s",
              ctx->fs_root, dirname(tmp_path), VIRTFS_META_DIR);
@@ -139,7 +139,7 @@ static int local_create_mapped_attr_dir(FsContext *ctx, const char *path)
     if (err < 0 && errno == EEXIST) {
         err = 0;
     }
-    free(tmp_path);
+    g_free(tmp_path);
     return err;
 }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 04/11] readline: Fix unchecked strdup() by converting to g_strdup()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 readline.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/readline.c b/readline.c
index a0c9638..d6e04d4 100644
--- a/readline.c
+++ b/readline.c
@@ -247,14 +247,14 @@ static void readline_hist_add(ReadLineState *rs, const char *cmdline)
     }
     if (idx == READLINE_MAX_CMDS) {
 	/* Need to get one free slot */
-	free(rs->history[0]);
+        g_free(rs->history[0]);
 	memmove(rs->history, &rs->history[1],
 	        (READLINE_MAX_CMDS - 1) * sizeof(char *));
 	rs->history[READLINE_MAX_CMDS - 1] = NULL;
 	idx = READLINE_MAX_CMDS - 1;
     }
     if (new_entry == NULL)
-	new_entry = strdup(cmdline);
+        new_entry = g_strdup(cmdline);
     rs->history[idx] = new_entry;
     rs->hist_entry = -1;
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 05/11] spice: Fix unchecked strdup() by converting to g_strdup()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 ui/spice-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 3f2c565..bcc4199 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -848,8 +848,8 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
 int qemu_spice_set_passwd(const char *passwd,
                           bool fail_if_conn, bool disconnect_if_conn)
 {
-    free(auth_passwd);
-    auth_passwd = strdup(passwd);
+    g_free(auth_passwd);
+    auth_passwd = g_strdup(passwd);
     return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn);
 }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 06/11] virtfs-proxy-helper: Fix unchecked strdup() by conv. to g_strdup()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 fsdev/virtfs-proxy-helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 6b9afd3..36f6616 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -1039,7 +1039,7 @@ int main(int argc, char **argv)
         }
         switch (c) {
         case 'p':
-            rpath = strdup(optarg);
+            rpath = g_strdup(optarg);
             break;
         case 'n':
             is_daemon = false;
@@ -1048,7 +1048,7 @@ int main(int argc, char **argv)
             sock = atoi(optarg);
             break;
         case 's':
-            sock_name = strdup(optarg);
+            sock_name = g_strdup(optarg);
             break;
         case 'u':
             own_u = atoi(optarg);
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 07/11] qemu-log: Fix unchecked strdup() by converting to g_strdup()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-log.c b/qemu-log.c
index b655b30..64a1b88 100644
--- a/qemu-log.c
+++ b/qemu-log.c
@@ -84,7 +84,7 @@ void qemu_set_log(int log_flags, bool use_own_buffers)
 
 void cpu_set_log_filename(const char *filename)
 {
-    logfilename = strdup(filename);
+    logfilename = g_strdup(filename);
     if (qemu_logfile) {
         fclose(qemu_logfile);
         qemu_logfile = NULL;
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 08/11] qemu-log: Plug trivial memory leak in cpu_set_log_filename()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-log.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/qemu-log.c b/qemu-log.c
index 64a1b88..30c8b01 100644
--- a/qemu-log.c
+++ b/qemu-log.c
@@ -21,10 +21,12 @@
 #include "qemu/log.h"
 
 #ifdef WIN32
-static const char *logfilename = "qemu.log";
+#define DEFAULT_LOGFILENAME "qemu.log"
 #else
-static const char *logfilename = "/tmp/qemu.log";
+#define DEFAULT_LOGFILENAME "/tmp/qemu.log"
 #endif
+
+static const char *logfilename;
 FILE *qemu_logfile;
 int qemu_loglevel;
 static int log_append = 0;
@@ -54,11 +56,13 @@ void qemu_log_mask(int mask, const char *fmt, ...)
 /* enable or disable low levels log */
 void qemu_set_log(int log_flags, bool use_own_buffers)
 {
+    const char *fname = logfilename ?: DEFAULT_LOGFILENAME;
+
     qemu_loglevel = log_flags;
     if (qemu_loglevel && !qemu_logfile) {
-        qemu_logfile = fopen(logfilename, log_append ? "a" : "w");
+        qemu_logfile = fopen(fname, log_append ? "a" : "w");
         if (!qemu_logfile) {
-            perror(logfilename);
+            perror(fname);
             _exit(1);
         }
         /* must avoid mmap() usage of glibc by setting a buffer "by hand" */
@@ -84,6 +88,7 @@ void qemu_set_log(int log_flags, bool use_own_buffers)
 
 void cpu_set_log_filename(const char *filename)
 {
+    g_free((void *)logfilename);
     logfilename = g_strdup(filename);
     if (qemu_logfile) {
         fclose(qemu_logfile);
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 09/11] libcacard: Fix unchecked strdup() by converting to g_strdup()

[Markus Armbruster]

Note that we already free with g_free().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 libcacard/vcard_emul_nss.c | 4 ++--
 libcacard/vreader.c        | 2 +-
 libcacard/vscclient.c      | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 5f565e0..df79476 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -454,7 +454,7 @@ vreader_emul_new(PK11SlotInfo *slot, VCardEmulType type, const char *params)
 
     new_reader_emul->slot = PK11_ReferenceSlot(slot);
     new_reader_emul->default_type = type;
-    new_reader_emul->type_params = strdup(params);
+    new_reader_emul->type_params = g_strdup(params);
     new_reader_emul->present = PR_FALSE;
     new_reader_emul->series = 0;
     new_reader_emul->saved_vcard = NULL;
@@ -997,7 +997,7 @@ vcard_emul_init(const VCardEmulOptions *options)
     /* We should control this with options. For now we mirror out any
      * removable hardware slot */
     default_card_type = options->hw_card_type;
-    default_type_params = strdup(options->hw_type_params);
+    default_type_params = g_strdup(options->hw_type_params);
 
     SECMOD_GetReadLock(module_lock);
     for (mlp = module_list; mlp; mlp = mlp->next) {
diff --git a/libcacard/vreader.c b/libcacard/vreader.c
index 313349b..f3efc27 100644
--- a/libcacard/vreader.c
+++ b/libcacard/vreader.c
@@ -49,7 +49,7 @@ vreader_new(const char *name, VReaderEmul *private,
     reader = (VReader *)g_malloc(sizeof(VReader));
     qemu_mutex_init(&reader->lock);
     reader->reference_count = 1;
-    reader->name = name ? strdup(name) : NULL;
+    reader->name = g_strdup(name);
     reader->card = NULL;
     reader->id = (vreader_id_t)-1;
     reader->reader_private = private;
diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
index 2fce52b..9b744f2 100644
--- a/libcacard/vscclient.c
+++ b/libcacard/vscclient.c
@@ -503,8 +503,8 @@ main(
         command_line_options = vcard_emul_options(emul_args);
     }
 
-    qemu_host = strdup(argv[argc - 2]);
-    qemu_port = strdup(argv[argc - 1]);
+    qemu_host = g_strdup(argv[argc - 2]);
+    qemu_port = g_strdup(argv[argc - 1]);
     sock = connect_to_qemu(qemu_host, qemu_port);
     if (sock == -1) {
         fprintf(stderr, "error opening socket, exiting.\n");
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 10/11] qapi: Fix unchecked strdup() by converting to g_strdup()

[Markus Armbruster]

Note that we already free with g_free().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qmp-registry.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index 70cdbca..28bbbe8 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -92,7 +92,7 @@ char **qmp_get_command_list(void)
     list_head = list = g_malloc0(count * sizeof(char *));
 
     QTAILQ_FOREACH(cmd, &qmp_commands, node) {
-        *list = strdup(cmd->name);
+        *list = g_strdup(cmd->name);
         list++;
     }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 11/11] qemu-ga: Fix unchecked strdup() by converting to g_strdup()

[Markus Armbruster]

I figure it's freed somewhere deep down in QAPI, with g_free().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qga/commands.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/commands.c b/qga/commands.c
index 7ffb35e..528b082 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -61,7 +61,7 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
 
     while (*cmd_list) {
         cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
-        cmd_info->name = strdup(*cmd_list);
+        cmd_info->name = g_strdup(*cmd_list);
         cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
 
         cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList));
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH v2 08/11] qemu-log: Plug trivial memory leak in cpu_set_log_filename()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

On 01/16/2013 10:32 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-log.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 03/11] hw/9pfs: Fix unchecked strdup() by converting to g_strdup()

[Stefan Hajnoczi]

On Wed, Jan 16, 2013 at 06:32:12PM +0100, Markus Armbruster wrote:
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 6eab7f7..74155fb 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -94,7 +94,7 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
>          exit(1);
>      }
>  
> -    s->tag = strdup(conf->tag);
> +    s->tag = g_strdup(conf->tag);
>      s->ctx.uid = -1;
>  
>      s->ops = fse->ops;

s->tag is leaked.  Want to send a follow-up patch to g_free() it?

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2 06/11] virtfs-proxy-helper: Fix unchecked strdup() by conv. to g_strdup()

[Stefan Hajnoczi]

On Wed, Jan 16, 2013 at 06:32:15PM +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  fsdev/virtfs-proxy-helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index 6b9afd3..36f6616 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -1039,7 +1039,7 @@ int main(int argc, char **argv)
>          }
>          switch (c) {
>          case 'p':
> -            rpath = strdup(optarg);
> +            rpath = g_strdup(optarg);
>              break;
>          case 'n':
>              is_daemon = false;
> @@ -1048,7 +1048,7 @@ int main(int argc, char **argv)
>              sock = atoi(optarg);
>              break;
>          case 's':
> -            sock_name = strdup(optarg);
> +            sock_name = g_strdup(optarg);

rpath and sock_name are leaked.  Not important though because they are
in main().

Stefan

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2 08/11] qemu-log: Plug trivial memory leak in cpu_set_log_filename()

[Stefan Hajnoczi]

On Wed, Jan 16, 2013 at 06:32:17PM +0100, Markus Armbruster wrote:
> diff --git a/qemu-log.c b/qemu-log.c
> index 64a1b88..30c8b01 100644
> --- a/qemu-log.c
> +++ b/qemu-log.c
> @@ -21,10 +21,12 @@
>  #include "qemu/log.h"
>  
>  #ifdef WIN32
> -static const char *logfilename = "qemu.log";
> +#define DEFAULT_LOGFILENAME "qemu.log"
>  #else
> -static const char *logfilename = "/tmp/qemu.log";
> +#define DEFAULT_LOGFILENAME "/tmp/qemu.log"
>  #endif
> +
> +static const char *logfilename;
[...]
> @@ -84,6 +88,7 @@ void qemu_set_log(int log_flags, bool use_own_buffers)
>  
>  void cpu_set_log_filename(const char *filename)
>  {
> +    g_free((void *)logfilename);

Might as well drop the const from the variable declaration to indicate
that we allocate/free this string.

Stefan

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2 06/11] virtfs-proxy-helper: Fix unchecked strdup() by conv. to g_strdup()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Wed, Jan 16, 2013 at 06:32:15PM +0100, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  fsdev/virtfs-proxy-helper.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
>> index 6b9afd3..36f6616 100644
>> --- a/fsdev/virtfs-proxy-helper.c
>> +++ b/fsdev/virtfs-proxy-helper.c
>> @@ -1039,7 +1039,7 @@ int main(int argc, char **argv)
>>          }
>>          switch (c) {
>>          case 'p':
>> -            rpath = strdup(optarg);
>> +            rpath = g_strdup(optarg);
>>              break;
>>          case 'n':
>>              is_daemon = false;
>> @@ -1048,7 +1048,7 @@ int main(int argc, char **argv)
>>              sock = atoi(optarg);
>>              break;
>>          case 's':
>> -            sock_name = strdup(optarg);
>> +            sock_name = g_strdup(optarg);
>
> rpath and sock_name are leaked.  Not important though because they are
> in main().

Yup.  Freeing stuff before main returns isn't worth your while or mine
:)

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2 08/11] qemu-log: Plug trivial memory leak in cpu_set_log_filename()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Wed, Jan 16, 2013 at 06:32:17PM +0100, Markus Armbruster wrote:
>> diff --git a/qemu-log.c b/qemu-log.c
>> index 64a1b88..30c8b01 100644
>> --- a/qemu-log.c
>> +++ b/qemu-log.c
>> @@ -21,10 +21,12 @@
>>  #include "qemu/log.h"
>>  
>>  #ifdef WIN32
>> -static const char *logfilename = "qemu.log";
>> +#define DEFAULT_LOGFILENAME "qemu.log"
>>  #else
>> -static const char *logfilename = "/tmp/qemu.log";
>> +#define DEFAULT_LOGFILENAME "/tmp/qemu.log"
>>  #endif
>> +
>> +static const char *logfilename;
> [...]
>> @@ -84,6 +88,7 @@ void qemu_set_log(int log_flags, bool use_own_buffers)
>>  
>>  void cpu_set_log_filename(const char *filename)
>>  {
>> +    g_free((void *)logfilename);
>
> Might as well drop the const from the variable declaration to indicate
> that we allocate/free this string.

Never thought of const that way.  Dropping const from logfilename is
fine with me.

I wish free()'s parameter was const void *.  Can't be changed now.  Sad
that glib didn't get it right either.

Re: [Qemu-devel] [PATCH v2 03/11] hw/9pfs: Fix unchecked strdup() by converting to g_strdup()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Wed, Jan 16, 2013 at 06:32:12PM +0100, Markus Armbruster wrote:
>> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
>> index 6eab7f7..74155fb 100644
>> --- a/hw/9pfs/virtio-9p-device.c
>> +++ b/hw/9pfs/virtio-9p-device.c
>> @@ -94,7 +94,7 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
>>          exit(1);
>>      }
>>  
>> -    s->tag = strdup(conf->tag);
>> +    s->tag = g_strdup(conf->tag);
>>      s->ctx.uid = -1;
>>  
>>      s->ops = fse->ops;
>
> s->tag is leaked.  Want to send a follow-up patch to g_free() it?

I'll give it a try.