[Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Jean-Christophe Dubois]

qemu_malloc, qemu_free and friends are not used consistently in the qemu 
source code.

This is a first attempt to use these oveloaded functions consistently all over 
the place instead of the default glibc versions.

Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>

diff -rNu qemu.org/acl.c qemu/acl.c
--- qemu.org/acl.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/acl.c	2009-05-20 23:07:46.000000000 +0200
@@ -104,8 +104,8 @@
     acl->defaultDeny = 1;
     TAILQ_FOREACH(entry, &acl->entries, next) {
         TAILQ_REMOVE(&acl->entries, entry, next);
-        free(entry->match);
-        free(entry);
+        qemu_free(entry->match);
+        qemu_free(entry);
     }
     acl->nentries = 0;
 }
diff -rNu qemu.org/audio/paaudio.c qemu/audio/paaudio.c
--- qemu.org/audio/paaudio.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/audio/paaudio.c	2009-05-22 17:07:29.000000000 +0200
@@ -340,7 +340,7 @@
     return 0;
 
  fail3:
-    free (pa->pcm_buf);
+    qemu_free (pa->pcm_buf);
     pa->pcm_buf = NULL;
  fail2:
     pa_simple_free (pa->s);
@@ -394,7 +394,7 @@
     return 0;
 
  fail3:
-    free (pa->pcm_buf);
+    qemu_free (pa->pcm_buf);
     pa->pcm_buf = NULL;
  fail2:
     pa_simple_free (pa->s);
diff -rNu qemu.org/block/cloop.c qemu/block/cloop.c
--- qemu.org/block/cloop.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/block/cloop.c	2009-05-22 17:13:01.000000000 +0200
@@ -148,9 +148,9 @@
     BDRVCloopState *s = bs->opaque;
     close(s->fd);
     if(s->n_blocks>0)
-	free(s->offsets);
-    free(s->compressed_block);
-    free(s->uncompressed_block);
+	qemu_free(s->offsets);
+    qemu_free(s->compressed_block);
+    qemu_free(s->uncompressed_block);
     inflateEnd(&s->zstream);
 }
 
diff -rNu qemu.org/block/dmg.c qemu/block/dmg.c
--- qemu.org/block/dmg.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/block/dmg.c	2009-05-22 17:11:43.000000000 +0200
@@ -273,14 +273,14 @@
     BDRVDMGState *s = bs->opaque;
     close(s->fd);
     if(s->n_chunks>0) {
-	free(s->types);
-	free(s->offsets);
-	free(s->lengths);
-	free(s->sectors);
-	free(s->sectorcounts);
+	qemu_free(s->types);
+	qemu_free(s->offsets);
+	qemu_free(s->lengths);
+	qemu_free(s->sectors);
+	qemu_free(s->sectorcounts);
     }
-    free(s->compressed_chunk);
-    free(s->uncompressed_chunk);
+    qemu_free(s->compressed_chunk);
+    qemu_free(s->uncompressed_chunk);
     inflateEnd(&s->zstream);
 }
 
diff -rNu qemu.org/block/vvfat.c qemu/block/vvfat.c
--- qemu.org/block/vvfat.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/block/vvfat.c	2009-05-21 00:12:12.000000000 +0200
@@ -87,7 +87,7 @@
 static inline void array_free(array_t* array)
 {
     if(array->pointer)
-        free(array->pointer);
+        qemu_free(array->pointer);
     array->size=array->next=0;
 }
 
@@ -169,7 +169,7 @@
 
     memcpy(to,buf,is*count);
 
-    free(buf);
+    qemu_free(buf);
 
     return 0;
 }
@@ -732,7 +732,7 @@
 	snprintf(buffer,length,"%s/%s",dirname,entry->d_name);
 
 	if(stat(buffer,&st)<0) {
-	    free(buffer);
+	    qemu_free(buffer);
             continue;
 	}
 
@@ -755,7 +755,7 @@
 	    direntry->begin=0; /* do that later */
         if (st.st_size > 0x7fffffff) {
 	    fprintf(stderr, "File %s is larger than 2GB\n", buffer);
-	    free(buffer);
+	    qemu_free(buffer);
 	    return -2;
         }
 	direntry->size=cpu_to_le32(S_ISDIR(st.st_mode)?0:st.st_size);
@@ -882,7 +882,7 @@
     mapping->dir_index = 0;
     mapping->info.dir.parent_mapping_index = -1;
     mapping->first_mapping_index = -1;
-    mapping->path = strdup(dirname);
+    mapping->path = qemu_strdup(dirname);
     i = strlen(mapping->path);
     if (i > 0 && mapping->path[i - 1] == '/')
 	mapping->path[i - 1] = '\0';
@@ -1369,7 +1369,7 @@
 	assert(commit->path || commit->action == ACTION_WRITEOUT);
 	if (commit->action != ACTION_WRITEOUT) {
 	    assert(commit->path);
-	    free(commit->path);
+	    qemu_free(commit->path);
 	} else
 	    assert(commit->path == NULL);
     }
@@ -1632,10 +1632,10 @@
 
 	    /* rename */
 	    if (strcmp(basename, basename2))
-		schedule_rename(s, cluster_num, strdup(path));
+		schedule_rename(s, cluster_num, qemu_strdup(path));
 	} else if (is_file(direntry))
 	    /* new file */
-	    schedule_new_file(s, strdup(path), cluster_num);
+	    schedule_new_file(s, qemu_strdup(path), cluster_num);
 	else {
 	    assert(0);
 	    return 0;
@@ -1752,10 +1752,10 @@
 	mapping->mode &= ~MODE_DELETED;
 
 	if (strcmp(basename, basename2))
-	    schedule_rename(s, cluster_num, strdup(path));
+	    schedule_rename(s, cluster_num, qemu_strdup(path));
     } else
 	/* new directory */
-	schedule_mkdir(s, cluster_num, strdup(path));
+	schedule_mkdir(s, cluster_num, qemu_strdup(path));
 
     lfn_init(&lfn);
     do {
@@ -1776,7 +1776,7 @@
 	if (subret) {
 	    fprintf(stderr, "Error fetching direntries\n");
 	fail:
-	    free(cluster);
+	    qemu_free(cluster);
 	    return 0;
 	}
 
@@ -1844,7 +1844,7 @@
 	cluster_num = modified_fat_get(s, cluster_num);
     } while(!fat_eof(s, cluster_num));
 
-    free(cluster);
+    qemu_free(cluster);
     return ret;
 }
 
@@ -1990,7 +1990,7 @@
 
     /* free mapping */
     if (mapping->first_mapping_index < 0)
-	free(mapping->path);
+	qemu_free(mapping->path);
 
     /* remove from s->mapping */
     array_remove(&(s->mapping), mapping_index);
@@ -2390,7 +2390,7 @@
 		}
 	    }
 
-	    free(old_path);
+	    qemu_free(old_path);
 	    array_remove(&(s->commits), i);
 	    continue;
 	} else if (commit->action == ACTION_MKDIR) {
@@ -2758,7 +2758,7 @@
 static void write_target_close(BlockDriverState *bs) {
     BDRVVVFATState* s = bs->opaque;
     bdrv_delete(s->qcow);
-    free(s->qcow_filename);
+    qemu_free(s->qcow_filename);
 }
 
 static BlockDriver vvfat_write_target = {
@@ -2771,7 +2771,7 @@
 static int enable_write_target(BDRVVVFATState *s)
 {
     int size = sector2cluster(s, s->sector_count);
-    s->used_clusters = calloc(size, 1);
+    s->used_clusters = qemu_mallocz(size);
 
     array_init(&(s->commits), sizeof(commit_t));
 
@@ -2788,7 +2788,7 @@
     unlink(s->qcow_filename);
 #endif
 
-    s->bs->backing_hd = calloc(sizeof(BlockDriverState), 1);
+    s->bs->backing_hd = qemu_mallocz(sizeof(BlockDriverState));
     s->bs->backing_hd->drv = &vvfat_write_target;
     s->bs->backing_hd->opaque = s;
 
@@ -2804,7 +2804,7 @@
     array_free(&(s->directory));
     array_free(&(s->mapping));
     if(s->cluster_buffer)
-        free(s->cluster_buffer);
+        qemu_free(s->cluster_buffer);
 }
 
 static BlockDriver bdrv_vvfat = {
diff -rNu qemu.org/bsd-user/main.c qemu/bsd-user/main.c
--- qemu.org/bsd-user/main.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/bsd-user/main.c	2009-05-21 00:16:38.000000000 +0200
@@ -822,7 +822,7 @@
     while (*(wrk++))
         environ_count++;
 
-    target_environ = malloc((environ_count + 1) * sizeof(char *));
+    target_environ = qemu_malloc((environ_count + 1) * sizeof(char *));
     if (!target_environ)
         abort();
     for (wrk = environ, dst = target_environ; *wrk; wrk++) {
@@ -838,10 +838,10 @@
     }
 
     for (wrk = target_environ; *wrk; wrk++) {
-        free(*wrk);
+        qemu_free(*wrk);
     }
 
-    free(target_environ);
+    qemu_free(target_environ);
 
     if (qemu_log_enabled()) {
         log_page_dump();
diff -rNu qemu.org/bsd-user/path.c qemu/bsd-user/path.c
--- qemu.org/bsd-user/path.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/bsd-user/path.c	2009-05-21 00:18:42.000000000 +0200
@@ -45,8 +45,8 @@
                                   struct pathelem *parent,
                                   const char *name)
 {
-    struct pathelem *new = malloc(sizeof(*new));
-    new->name = strdup(name);
+    struct pathelem *new = qemu_malloc(sizeof(*new));
+    new->name = qemu_strdup(name);
     asprintf(&new->pathname, "%s/%s", root, name);
     new->num_entries = 0;
     return new;
@@ -75,7 +75,7 @@
 {
     root->num_entries++;
 
-    root = realloc(root, sizeof(*root)
+    root = qemu_realloc(root, sizeof(*root)
                    + sizeof(root->entries[0])*root->num_entries);
 
     root->entries[root->num_entries-1] = new_entry(root->pathname, root, 
name);
@@ -137,14 +137,14 @@
         pstrcpy(pref_buf, sizeof(pref_buf), cwd);
         pstrcat(pref_buf, pref_buf_len, "/");
         pstrcat(pref_buf, pref_buf_len, prefix);
-        free(cwd);
+        qemu_free(cwd);
     } else
         pstrcpy(pref_buf, sizeof(pref_buf), prefix + 1);
 
     base = new_entry("", NULL, pref_buf);
     base = add_dir_maybe(base);
     if (base->num_entries == 0) {
-        free (base);
+        qemu_free (base);
         base = NULL;
     } else {
         set_parents(base, base);
diff -rNu qemu.org/hw/baum.c qemu/hw/baum.c
--- qemu.org/hw/baum.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/hw/baum.c	2009-05-22 17:09:53.000000000 +0200
@@ -558,7 +558,7 @@
     if (ret == -1 && (brlapi_errno != BRLAPI_ERROR_LIBCERR || errno != 
EINTR)) {
         brlapi_perror("baum: brlapi_readKey");
         brlapi__closeConnection(baum->brlapi);
-        free(baum->brlapi);
+        qemu_free(baum->brlapi);
         baum->brlapi = NULL;
     }
 }
@@ -621,9 +621,9 @@
     qemu_free_timer(baum->cellCount_timer);
     brlapi__closeConnection(handle);
 fail_handle:
-    free(handle);
-    free(chr);
-    free(baum);
+    qemu_free(handle);
+    qemu_free(chr);
+    qemu_free(baum);
     return NULL;
 }
 
diff -rNu qemu.org/hw/bt-l2cap.c qemu/hw/bt-l2cap.c
--- qemu.org/hw/bt-l2cap.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/hw/bt-l2cap.c	2009-05-22 17:15:52.000000000 +0200
@@ -1220,7 +1220,7 @@
     for (cid = L2CAP_CID_ALLOC; cid < L2CAP_CID_MAX; cid ++)
         if (l2cap->cid[cid]) {
             l2cap->cid[cid]->params.close(l2cap->cid[cid]->params.opaque);
-            free(l2cap->cid[cid]);
+            qemu_free(l2cap->cid[cid]);
         }
 
     if (l2cap->role)
diff -rNu qemu.org/hw/nseries.c qemu/hw/nseries.c
--- qemu.org/hw/nseries.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/hw/nseries.c	2009-05-21 23:15:02.000000000 +0200
@@ -711,7 +711,7 @@
     fb_blank = memset(qemu_malloc(800 * 480 * 2), 0xff, 800 * 480 * 2);
     /* Display Memory Data Port */
     chip->block(chip->opaque, 1, fb_blank, 800 * 480 * 2, 800);
-    free(fb_blank);
+    qemu_free(fb_blank);
 }
 
 static void n8x0_dss_setup(struct n800_s *s)
diff -rNu qemu.org/hw/ppc440_bamboo.c qemu/hw/ppc440_bamboo.c
--- qemu.org/hw/ppc440_bamboo.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/hw/ppc440_bamboo.c	2009-05-21 23:13:52.000000000 +0200
@@ -45,7 +45,7 @@
     snprintf(path, pathlen, "%s/%s", bios_dir, BINARY_DEVICE_TREE_FILE);
 
     fdt = load_device_tree(path, &fdt_size);
-    free(path);
+    qemu_free(path);
     if (fdt == NULL)
         goto out;
 
diff -rNu qemu.org/hw/scsi-generic.c qemu/hw/scsi-generic.c
--- qemu.org/hw/scsi-generic.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/hw/scsi-generic.c	2009-05-21 23:12:57.000000000 +0200
@@ -564,7 +564,7 @@
 
     if (len == 0) {
         if (r->buf != NULL)
-            free(r->buf);
+            qemu_free(r->buf);
         r->buflen = 0;
         r->buf = NULL;
         ret = execute_command(s->bdrv, r, SG_DXFER_NONE, 
scsi_command_complete);
@@ -577,7 +577,7 @@
 
     if (r->buflen != len) {
         if (r->buf != NULL)
-            free(r->buf);
+            qemu_free(r->buf);
         r->buf = qemu_malloc(len);
         r->buflen = len;
     }
diff -rNu qemu.org/linux-user/main.c qemu/linux-user/main.c
--- qemu.org/linux-user/main.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/linux-user/main.c	2009-05-21 00:24:10.000000000 +0200
@@ -2464,7 +2464,7 @@
      * Prepare copy of argv vector for target.
      */
     target_argc = argc - optind;
-    target_argv = calloc(target_argc + 1, sizeof (char *));
+    target_argv = qemu_mallocz((target_argc + 1) * sizeof (char *));
     if (target_argv == NULL) {
 	(void) fprintf(stderr, "Unable to allocate memory for target_argv\n");
 	exit(1);
@@ -2489,15 +2489,15 @@
     }
 
     for (i = 0; i < target_argc; i++) {
-        free(target_argv[i]);
+        qemu_free(target_argv[i]);
     }
-    free(target_argv);
+    qemu_free(target_argv);
 
     for (wrk = target_environ; *wrk; wrk++) {
-        free(*wrk);
+        qemu_free(*wrk);
     }
 
-    free(target_environ);
+    qemu_free(target_environ);
 
     if (qemu_log_enabled()) {
         log_page_dump();
diff -rNu qemu.org/linux-user/syscall.c qemu/linux-user/syscall.c
--- qemu.org/linux-user/syscall.c	2009-05-16 17:57:26.000000000 +0200
+++ qemu/linux-user/syscall.c	2009-05-21 00:39:21.000000000 +0200
@@ -2094,7 +2094,7 @@
 
     nsems = semid_ds.sem_nsems;
 
-    *host_array = malloc(nsems*sizeof(unsigned short));
+    *host_array = qemu_malloc(nsems*sizeof(unsigned short));
     array = lock_user(VERIFY_READ, target_addr,
                       nsems*sizeof(unsigned short), 1);
     if (!array)
@@ -2133,7 +2133,7 @@
     for(i=0; i<nsems; i++) {
         __put_user((*host_array)[i], &array[i]);
     }
-    free(*host_array);
+    qemu_free(*host_array);
     unlock_user(array, target_addr, 1);
 
     return 0;
@@ -2379,11 +2379,11 @@
 
     if (!lock_user_struct(VERIFY_READ, target_mb, msgp, 0))
         return -TARGET_EFAULT;
-    host_mb = malloc(msgsz+sizeof(long));
+    host_mb = qemu_malloc(msgsz+sizeof(long));
     host_mb->mtype = (abi_long) tswapl(target_mb->mtype);
     memcpy(host_mb->mtext, target_mb->mtext, msgsz);
     ret = get_errno(msgsnd(msqid, host_mb, msgsz, msgflg));
-    free(host_mb);
+    qemu_free(host_mb);
     unlock_user_struct(target_mb, msgp, 0);
 
     return ret;
@@ -2401,7 +2401,7 @@
     if (!lock_user_struct(VERIFY_WRITE, target_mb, msgp, 0))
         return -TARGET_EFAULT;
 
-    host_mb = malloc(msgsz+sizeof(long));
+    host_mb = qemu_malloc(msgsz+sizeof(long));
     ret = get_errno(msgrcv(msqid, host_mb, msgsz, tswapl(msgtyp), msgflg));
 
     if (ret > 0) {
@@ -2416,7 +2416,7 @@
     }
 
     target_mb->mtype = tswapl(host_mb->mtype);
-    free(host_mb);
+    qemu_free(host_mb);
 
 end:
     if (target_mb)
@@ -5428,7 +5428,7 @@
             struct linux_dirent *dirp;
             abi_long count = arg3;
 
-	    dirp = malloc(count);
+	    dirp = qemu_malloc(count);
 	    if (!dirp) {
                 ret = -TARGET_ENOMEM;
                 goto fail;
@@ -5466,7 +5466,7 @@
 		ret = count1;
                 unlock_user(target_dirp, arg2, ret);
             }
-	    free(dirp);
+	    qemu_free(dirp);
         }
 #else
         {
diff -rNu qemu.org/migration.c qemu/migration.c
--- qemu.org/migration.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/migration.c	2009-05-20 23:03:32.000000000 +0200
@@ -266,7 +266,7 @@
         s->state = MIG_STATE_CANCELLED;
         migrate_fd_cleanup(s);
     }
-    free(s);
+    qemu_free(s);
 }
 
 void migrate_fd_wait_for_unfreeze(void *opaque)
diff -rNu qemu.org/net.c qemu/net.c
--- qemu.org/net.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/net.c	2009-05-18 23:24:26.000000000 +0200
@@ -214,7 +214,7 @@
                         struct sockaddr_in *saddr,
                         const char *input_str)
 {
-    char *str = strdup(input_str);
+    char *str = qemu_strdup(input_str);
     char *host_str = str;
     char *src_str;
     const char *src_str2;
@@ -243,11 +243,11 @@
     if (parse_host_port(saddr, src_str2) < 0)
         goto fail;
 
-    free(str);
+    qemu_free(str);
     return(0);
 
 fail:
-    free(str);
+    qemu_free(str);
     return -1;
 }
 
@@ -326,7 +326,7 @@
 
     snprintf(buf, sizeof(buf), "%s.%d", model, id);
 
-    return strdup(buf);
+    return qemu_strdup(buf);
 }
 
 VLANClientState *qemu_new_vlan_client(VLANState *vlan,
@@ -339,9 +339,9 @@
 {
     VLANClientState *vc, **pvc;
     vc = qemu_mallocz(sizeof(VLANClientState));
-    vc->model = strdup(model);
+    vc->model = qemu_strdup(model);
     if (name)
-        vc->name = strdup(name);
+        vc->name = qemu_strdup(name);
     else
         vc->name = assign_name(vc, model);
     vc->fd_read = fd_read;
@@ -368,8 +368,8 @@
             if (vc->cleanup) {
                 vc->cleanup(vc);
             }
-            free(vc->name);
-            free(vc->model);
+            qemu_free(vc->name);
+            qemu_free(vc->model);
             qemu_free(vc);
             break;
         } else
@@ -1147,7 +1147,7 @@
     s = qemu_mallocz(sizeof(VDEState));
     s->vde = vde_open(init_sock, (char *)"QEMU", &args);
     if (!s->vde){
-        free(s);
+        qemu_free(s);
         return -1;
     }
     s->vc = qemu_new_vlan_client(vlan, model, name, vde_from_qemu,
@@ -1515,8 +1515,8 @@
         return -1;
     }
     s->vlan = vlan;
-    s->model = strdup(model);
-    s->name = name ? strdup(name) : NULL;
+    s->model = qemu_strdup(model);
+    s->name = name ? qemu_strdup(name) : NULL;
     s->fd = fd;
     qemu_set_fd_handler(fd, net_socket_accept, NULL, s);
     return 0;
@@ -1746,7 +1746,7 @@
     int i, exit_status = 0;
 
     if (!nd->model)
-        nd->model = strdup(default_model);
+        nd->model = qemu_strdup(default_model);
 
     if (strcmp(nd->model, "?") != 0) {
         for (i = 0 ; models[i]; i++)
@@ -1781,7 +1781,7 @@
     vlan = qemu_find_vlan(vlan_id);
 
     if (get_param_value(buf, sizeof(buf), "name", p)) {
-        name = strdup(buf);
+        name = qemu_strdup(buf);
     }
     if (!strcmp(device, "nic")) {
         static const char * const nic_params[] = {
@@ -1818,7 +1818,7 @@
             }
         }
         if (get_param_value(buf, sizeof(buf), "model", p)) {
-            nd->model = strdup(buf);
+            nd->model = qemu_strdup(buf);
         }
         nd->vlan = vlan;
         nd->name = name;
@@ -1854,7 +1854,7 @@
             slirp_restrict = (buf[0] == 'y') ? 1 : 0;
         }
         if (get_param_value(buf, sizeof(buf), "ip", p)) {
-            slirp_ip = strdup(buf);
+            slirp_ip = qemu_strdup(buf);
         }
         vlan->nb_host_devs++;
         ret = net_slirp_init(vlan, device, name);
@@ -1870,7 +1870,7 @@
             ret = -1;
             goto out;
         }
-        vmc = malloc(sizeof(struct VMChannel));
+        vmc = qemu_malloc(sizeof(struct VMChannel));
         snprintf(name, 20, "vmchannel%ld", port);
         vmc->hd = qemu_chr_open(name, devname, NULL);
         if (!vmc->hd) {
@@ -2047,7 +2047,7 @@
     }
 out:
     if (name)
-        free(name);
+        qemu_free(name);
     return ret;
 }
 
@@ -2056,7 +2056,7 @@
     nd->vlan->nb_guest_devs--;
     nb_nics--;
     nd->used = 0;
-    free((void *)nd->model);
+    qemu_free((void *)nd->model);
 }
 
 static int net_host_check_device(const char *device)
diff -rNu qemu.org/qemu-char.c qemu/qemu-char.c
--- qemu.org/qemu-char.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/qemu-char.c	2009-05-18 23:37:37.000000000 +0200
@@ -1553,8 +1553,8 @@
     chr->chr_close = win_chr_close;
 
     if (win_chr_init(chr, filename) < 0) {
-        free(s);
-        free(chr);
+        qemu_free(s);
+        qemu_free(chr);
         return NULL;
     }
     qemu_chr_reset(chr);
@@ -1652,8 +1652,8 @@
     chr->chr_close = win_chr_close;
 
     if (win_chr_pipe_init(chr, filename) < 0) {
-        free(s);
-        free(chr);
+        qemu_free(s);
+        qemu_free(chr);
         return NULL;
     }
     qemu_chr_reset(chr);
@@ -1808,9 +1808,9 @@
 
 return_err:
     if (chr)
-        free(chr);
+        qemu_free(chr);
     if (s)
-        free(s);
+        qemu_free(s);
     if (fd >= 0)
         closesocket(fd);
     return NULL;
diff -rNu qemu.org/qemu-img.c qemu/qemu-img.c
--- qemu.org/qemu-img.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/qemu-img.c	2009-05-18 23:44:15.000000000 +0200
@@ -696,7 +696,7 @@
     bdrv_delete(out_bs);
     for (bs_i = 0; bs_i < bs_n; bs_i++)
         bdrv_delete(bs[bs_i]);
-    free(bs);
+    qemu_free(bs);
     return 0;
 }
 
diff -rNu qemu.org/qemu-io.c qemu/qemu-io.c
--- qemu.org/qemu-io.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/qemu-io.c	2009-05-18 23:48:23.000000000 +0200
@@ -311,14 +311,14 @@
 	}
 
 	if (Pflag) {
-		void* cmp_buf = malloc(pattern_count);
+		void* cmp_buf = qemu_malloc(pattern_count);
 		memset(cmp_buf, pattern, pattern_count);
 		if (memcmp(buf + pattern_offset, cmp_buf, pattern_count)) {
 			printf("Pattern verification failed at offset %lld, "
 				"%d bytes\n",
 				(long long) offset + pattern_offset, pattern_count);
 		}
-		free(cmp_buf);
+		qemu_free(cmp_buf);
 	}
 
 	if (qflag)
@@ -465,14 +465,14 @@
 	}
 
 	if (Pflag) {
-		void* cmp_buf = malloc(count);
+		void* cmp_buf = qemu_malloc(count);
 		memset(cmp_buf, pattern, count);
 		if (memcmp(buf, cmp_buf, count)) {
 			printf("Pattern verification failed at offset %lld, "
 				"%d bytes\n",
 				(long long) offset, count);
 		}
-		free(cmp_buf);
+		qemu_free(cmp_buf);
 	}
 
 	if (qflag)
diff -rNu qemu.org/readline.c qemu/readline.c
--- qemu.org/readline.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/readline.c	2009-05-18 23:26:12.000000000 +0200
@@ -246,7 +246,7 @@
     }
     if (idx == READLINE_MAX_CMDS) {
 	/* Need to get one free slot */
-	free(rs->history[0]);
+	qemu_free(rs->history[0]);
 	memcpy(rs->history, &rs->history[1],
 	       (READLINE_MAX_CMDS - 1) * sizeof(char *));
 	rs->history[READLINE_MAX_CMDS - 1] = NULL;
diff -rNu qemu.org/slirp/socket.c qemu/slirp/socket.c
--- qemu.org/slirp/socket.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/slirp/socket.c	2009-05-21 00:42:39.000000000 +0200
@@ -53,7 +53,7 @@
 {
   struct socket *so;
 
-  so = (struct socket *)malloc(sizeof(struct socket));
+  so = (struct socket *)qemu_malloc(sizeof(struct socket));
   if(so) {
     memset(so, 0, sizeof(struct socket));
     so->so_state = SS_NOFDREF;
@@ -82,7 +82,7 @@
   if(so->so_next && so->so_prev)
     remque(so);  /* crashes if so is not in a queue */
 
-  free(so);
+  qemu_free(so);
 }
 
 size_t sopreprbuf(struct socket *so, struct iovec *iov, int *np)
@@ -601,13 +601,13 @@
 	DEBUG_ARG("flags = %x", flags);
 
 	if ((so = socreate()) == NULL) {
-	  /* free(so);      Not sofree() ??? free(NULL) == NOP */
+	  /* qemu_free(so);      Not sofree() ??? qemu_free(NULL) == NOP */
 	  return NULL;
 	}
 
 	/* Don't tcp_attach... we don't need so_snd nor so_rcv */
 	if ((so->so_tcpcb = tcp_newtcpcb(so)) == NULL) {
-		free(so);
+		qemu_free(so);
 		return NULL;
 	}
 	insque(so,&tcb);
diff -rNu qemu.org/target-arm/helper.c qemu/target-arm/helper.c
--- qemu.org/target-arm/helper.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/target-arm/helper.c	2009-05-21 00:43:58.000000000 +0200
@@ -330,7 +330,7 @@
 
 void cpu_arm_close(CPUARMState *env)
 {
-    free(env);
+    qemu_free(env);
 }
 
 uint32_t cpsr_read(CPUARMState *env)
@@ -466,7 +466,7 @@
 
 static void allocate_mmon_state(CPUState *env)
 {
-    env->mmon_entry = malloc(sizeof (mmon_state));
+    env->mmon_entry = qemu_malloc(sizeof (mmon_state));
     memset (env->mmon_entry, 0, sizeof (mmon_state));
     env->mmon_entry->cpu_env = env;
     mmon_head = env->mmon_entry;
diff -rNu qemu.org/target-i386/helper.c qemu/target-i386/helper.c
--- qemu.org/target-i386/helper.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/target-i386/helper.c	2009-05-21 00:25:56.000000000 +0200
@@ -397,11 +397,11 @@
     x86_cpu_def->ext_features &= ~minus_ext_features;
     x86_cpu_def->ext2_features &= ~minus_ext2_features;
     x86_cpu_def->ext3_features &= ~minus_ext3_features;
-    free(s);
+    qemu_free(s);
     return 0;
 
 error:
-    free(s);
+    qemu_free(s);
     return -1;
 }
 
diff -rNu qemu.org/target-i386/kvm.c qemu/target-i386/kvm.c
--- qemu.org/target-i386/kvm.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/target-i386/kvm.c	2009-05-21 23:05:21.000000000 +0200
@@ -224,7 +224,7 @@
             }
         }
 
-        free(kvm_msr_list);
+        qemu_free(kvm_msr_list);
     }
 
     if (has_msr_star == 1)
diff -rNu qemu.org/target-ppc/kvm_ppc.c qemu/target-ppc/kvm_ppc.c
--- qemu.org/target-ppc/kvm_ppc.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/target-ppc/kvm_ppc.c	2009-05-21 23:04:21.000000000 +0200
@@ -51,7 +51,7 @@
 close:
     fclose(f);
 free:
-    free(path);
+    qemu_free(path);
 out:
     return ret;
 }
diff -rNu qemu.org/target-sparc/helper.c qemu/target-sparc/helper.c
--- qemu.org/target-sparc/helper.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/target-sparc/helper.c	2009-05-21 00:27:11.000000000 +0200
@@ -706,8 +706,8 @@
 
 static void cpu_sparc_close(CPUSPARCState *env)
 {
-    free(env->def);
-    free(env);
+    qemu_free(env->def);
+    qemu_free(env);
 }
 
 CPUSPARCState *cpu_sparc_init(const char *cpu_model)
@@ -1333,11 +1333,11 @@
 #ifdef DEBUG_FEATURES
     print_features(stderr, fprintf, cpu_def->features, NULL);
 #endif
-    free(s);
+    qemu_free(s);
     return 0;
 
  error:
-    free(s);
+    qemu_free(s);
     return -1;
 }
 
diff -rNu qemu.org/tcg/tcg.c qemu/tcg/tcg.c
--- qemu.org/tcg/tcg.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/tcg/tcg.c	2009-05-21 00:32:02.000000000 +0200
@@ -341,7 +341,7 @@
 #endif
         pstrcpy(buf, sizeof(buf), name);
         pstrcat(buf, sizeof(buf), "_0");
-        ts->name = strdup(buf);
+        ts->name = qemu_strdup(buf);
         ts++;
 
         ts->base_type = type;
@@ -356,7 +356,7 @@
 #endif
         pstrcpy(buf, sizeof(buf), name);
         pstrcat(buf, sizeof(buf), "_1");
-        ts->name = strdup(buf);
+        ts->name = qemu_strdup(buf);
 
         s->nb_globals += 2;
     } else
@@ -531,7 +531,7 @@
         } else {
             n *= 2;
         }
-        s->helpers = realloc(s->helpers, n * sizeof(TCGHelperInfo));
+        s->helpers = qemu_realloc(s->helpers, n * sizeof(TCGHelperInfo));
         s->allocated_helpers = n;
     }
     s->helpers[s->nb_helpers].func = (tcg_target_ulong)func;
diff -rNu qemu.org/vl.c qemu/vl.c
--- qemu.org/vl.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/vl.c	2009-05-18 23:00:36.000000000 +0200
@@ -1071,7 +1071,7 @@
         exit(0);
     }
 
-    arg = strdup(opt);
+    arg = qemu_strdup(opt);
 
     /* Reorder the array */
     name = strtok(arg, ",");
@@ -1100,7 +1100,7 @@
         name = strtok(NULL, ",");
     }
 
-    free(arg);
+    qemu_free(arg);
 
     if (cur) {
         /* Disable remaining timers */
@@ -5711,7 +5711,7 @@
                         fprintf(stderr, "Too many option ROMs\n");
                         exit(1);
                     }
-                    option_rom[nb_option_roms] = strdup(buf);
+                    option_rom[nb_option_roms] = qemu_strdup(buf);
                     nb_option_roms++;
                     netroms++;
                 }
diff -rNu qemu.org/vnc-auth-sasl.c qemu/vnc-auth-sasl.c
--- qemu.org/vnc-auth-sasl.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/vnc-auth-sasl.c	2009-05-18 23:27:42.000000000 +0200
@@ -34,8 +34,8 @@
         vs->sasl.runSSF = vs->sasl.waitWriteSSF = vs->sasl.wantSSF = 0;
         vs->sasl.encodedLength = vs->sasl.encodedOffset = 0;
         vs->sasl.encoded = NULL;
-        free(vs->sasl.username);
-        free(vs->sasl.mechlist);
+        qemu_free(vs->sasl.username);
+        qemu_free(vs->sasl.mechlist);
         vs->sasl.username = vs->sasl.mechlist = NULL;
         sasl_dispose(&vs->sasl.conn);
         vs->sasl.conn = NULL;
@@ -429,7 +429,7 @@
 
 static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, 
size_t len)
 {
-    char *mechname = malloc(len + 1);
+    char *mechname = qemu_malloc(len + 1);
     if (!mechname) {
         VNC_DEBUG("Out of memory reading mechname\n");
         vnc_client_error(vs);
@@ -462,7 +462,7 @@
         }
     }
 
-    free(vs->sasl.mechlist);
+    qemu_free(vs->sasl.mechlist);
     vs->sasl.mechlist = mechname;
 
     VNC_DEBUG("Validated mechname '%s'\n", mechname);
@@ -510,7 +510,7 @@
         goto authabort;
 
     if (!(remoteAddr = vnc_socket_remote_addr("%s;%s", vs->csock))) {
-        free(localAddr);
+        qemu_free(localAddr);
         goto authabort;
     }
 
@@ -522,8 +522,8 @@
                           NULL, /* Callbacks, not needed */
                           SASL_SUCCESS_DATA,
                           &vs->sasl.conn);
-    free(localAddr);
-    free(remoteAddr);
+    qemu_free(localAddr);
+    qemu_free(remoteAddr);
     localAddr = remoteAddr = NULL;
 
     if (err != SASL_OK) {
@@ -612,7 +612,7 @@
     }
     VNC_DEBUG("Available mechanisms for client: '%s'\n", mechlist);
 
-    if (!(vs->sasl.mechlist = strdup(mechlist))) {
+    if (!(vs->sasl.mechlist = qemu_strdup(mechlist))) {
         VNC_DEBUG("Out of memory");
         sasl_dispose(&vs->sasl.conn);
         vs->sasl.conn = NULL;
diff -rNu qemu.org/vnc.c qemu/vnc.c
--- qemu.org/vnc.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/vnc.c	2009-05-18 23:29:54.000000000 +0200
@@ -159,7 +159,7 @@
 
     monitor_printf(mon, "Client:\n");
     monitor_printf(mon, "%s", clientAddr);
-    free(clientAddr);
+    qemu_free(clientAddr);
 
 #ifdef CONFIG_VNC_TLS
     if (client->tls.session &&
@@ -190,7 +190,7 @@
 
         monitor_printf(mon, "Server:\n");
         monitor_printf(mon, "%s", serverAddr);
-        free(serverAddr);
+        qemu_free(serverAddr);
         monitor_printf(mon, "        auth: %s\n", 
vnc_auth_name(vnc_display));
 
         if (vnc_display->clients) {
@@ -533,8 +533,8 @@
                                   last_bg, last_fg, &has_bg, &has_fg);
         }
     }
-    free(last_fg);
-    free(last_bg);
+    qemu_free(last_fg);
+    qemu_free(last_bg);
 
 }
 
@@ -2123,7 +2123,7 @@
     if (strcmp(display, "none") == 0)
         return 0;
 
-    if (!(vs->display = strdup(display)))
+    if (!(vs->display = qemu_strdup(display)))
         return -1;
 
     options = display;
@@ -2275,7 +2275,7 @@
     if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
         fprintf(stderr, "Failed to initialize SASL auth %s",
                 sasl_errstring(saslErr, NULL, NULL));
-        free(vs->display);
+        qemu_free(vs->display);
         vs->display = NULL;
         return -1;
     }
@@ -2288,7 +2288,7 @@
         else
             vs->lsock = inet_connect(display, SOCK_STREAM);
         if (-1 == vs->lsock) {
-            free(vs->display);
+            qemu_free(vs->display);
             vs->display = NULL;
             return -1;
         } else {
@@ -2309,10 +2309,10 @@
             vs->lsock = inet_listen(display, dpy, 256, SOCK_STREAM, 5900);
         }
         if (-1 == vs->lsock) {
-            free(dpy);
+            qemu_free(dpy);
             return -1;
         } else {
-            free(vs->display);
+            qemu_free(vs->display);
             vs->display = dpy;
         }
     }
diff -rNu qemu.org/vnc-tls.c qemu/vnc-tls.c
--- qemu.org/vnc-tls.c	2009-05-16 17:57:27.000000000 +0200
+++ qemu/vnc-tls.c	2009-05-18 23:33:05.000000000 +0200
@@ -382,7 +382,7 @@
         vs->tls.session = NULL;
     }
     vs->tls.wiremode = VNC_WIREMODE_CLEAR;
-    free(vs->tls.dname);
+    qemu_free(vs->tls.dname);
 }
 
 

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Kevin Wolf]

Jean-Christophe Dubois schrieb:
> qemu_malloc, qemu_free and friends are not used consistently in the qemu 
> source code.
> 
> This is a first attempt to use these oveloaded functions consistently all over 
> the place instead of the default glibc versions.
> 
> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>

[...]

> diff -rNu qemu.org/qemu-io.c qemu/qemu-io.c
> --- qemu.org/qemu-io.c	2009-05-16 17:57:27.000000000 +0200
> +++ qemu/qemu-io.c	2009-05-18 23:48:23.000000000 +0200
> @@ -311,14 +311,14 @@
>  	}
>  
>  	if (Pflag) {
> -		void* cmp_buf = malloc(pattern_count);
> +		void* cmp_buf = qemu_malloc(pattern_count);
>  		memset(cmp_buf, pattern, pattern_count);
>  		if (memcmp(buf + pattern_offset, cmp_buf, pattern_count)) {
>  			printf("Pattern verification failed at offset %lld, "
>  				"%d bytes\n",
>  				(long long) offset + pattern_offset, pattern_count);
>  		}
> -		free(cmp_buf);
> +		qemu_free(cmp_buf);
>  	}
>  
>  	if (qflag)
> @@ -465,14 +465,14 @@
>  	}
>  
>  	if (Pflag) {
> -		void* cmp_buf = malloc(count);
> +		void* cmp_buf = qemu_malloc(count);
>  		memset(cmp_buf, pattern, count);
>  		if (memcmp(buf, cmp_buf, count)) {
>  			printf("Pattern verification failed at offset %lld, "
>  				"%d bytes\n",
>  				(long long) offset, count);
>  		}
> -		free(cmp_buf);
> +		qemu_free(cmp_buf);
>  	}
>  
>  	if (qflag)

Since recently qemu_malloc behaves differently from malloc with size =
0. This isn't allowed any more with qemu_malloc. So you need to check
for pattern_count == 0 and either print an error message or malloc a
different size, e.g. 1. I'm sure we don't want qemu-io to abort() in
such a case.

Or we could start over with a lengthy discussion about fixing qemu_malloc...

Kevin

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Anthony Liguori]

Kevin Wolf wrote:
> Since recently qemu_malloc behaves differently from malloc with size =
> 0. This isn't allowed any more with qemu_malloc. So you need to check
> for pattern_count == 0 and either print an error message or malloc a
> different size, e.g. 1. I'm sure we don't want qemu-io to abort() in
> such a case.
>
> Or we could start over with a lengthy discussion about fixing qemu_malloc...
>   

Having qemu_malloc(0) abort is silly.  Returning NULL or returning
malloc(1) are both reasonable options.

Putting the abort() in there is going to introduce a ton of subtle bugs,
I vote for changing qemu_malloc() to have a sane behavior.

Regards,

Anthony Liguori

> Kevin
>
>
>   

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[jcd]

Hi Kevin,

Thanks for pointing this. I guess it just sounds strange to me that somebody would want to alloc 0 bytes. But why not ...

I guess that if pattern_count/count is set to 0 we can just avoid the all processing of malloc/memset/memcmp/free anyway.

Would you be ok with something like:

if (Pflag && pattern_count) {

instead of: 

if (Pflag) {

JC

----- Mail Original -----
De: "Kevin Wolf" <kwolf@redhat.com>
À: "Jean-Christophe Dubois" <jcd@tribudubois.net>
Cc: qemu-devel@nongnu.org, "malc" <av1474@comtv.ru>
Envoyé: Vendredi 29 Mai 2009 10h42:38 GMT +01:00 Amsterdam / Berlin / Berne / Rome / Stockholm / Vienne
Objet: Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

Jean-Christophe Dubois schrieb:
> qemu_malloc, qemu_free and friends are not used consistently in the qemu 
> source code.
> 
> This is a first attempt to use these oveloaded functions consistently all over 
> the place instead of the default glibc versions.
> 
> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>

[...]

> diff -rNu qemu.org/qemu-io.c qemu/qemu-io.c
> --- qemu.org/qemu-io.c	2009-05-16 17:57:27.000000000 +0200
> +++ qemu/qemu-io.c	2009-05-18 23:48:23.000000000 +0200
> @@ -311,14 +311,14 @@
>  	}
>  
>  	if (Pflag) {
> -		void* cmp_buf = malloc(pattern_count);
> +		void* cmp_buf = qemu_malloc(pattern_count);
>  		memset(cmp_buf, pattern, pattern_count);
>  		if (memcmp(buf + pattern_offset, cmp_buf, pattern_count)) {
>  			printf("Pattern verification failed at offset %lld, "
>  				"%d bytes\n",
>  				(long long) offset + pattern_offset, pattern_count);
>  		}
> -		free(cmp_buf);
> +		qemu_free(cmp_buf);
>  	}
>  
>  	if (qflag)
> @@ -465,14 +465,14 @@
>  	}
>  
>  	if (Pflag) {
> -		void* cmp_buf = malloc(count);
> +		void* cmp_buf = qemu_malloc(count);
>  		memset(cmp_buf, pattern, count);
>  		if (memcmp(buf, cmp_buf, count)) {
>  			printf("Pattern verification failed at offset %lld, "
>  				"%d bytes\n",
>  				(long long) offset, count);
>  		}
> -		free(cmp_buf);
> +		qemu_free(cmp_buf);
>  	}
>  
>  	if (qflag)

Since recently qemu_malloc behaves differently from malloc with size =
0. This isn't allowed any more with qemu_malloc. So you need to check
for pattern_count == 0 and either print an error message or malloc a
different size, e.g. 1. I'm sure we don't want qemu-io to abort() in
such a case.

Or we could start over with a lengthy discussion about fixing qemu_malloc...

Kevin

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Kevin Wolf]

jcd@tribudubois.net schrieb:
> Thanks for pointing this. I guess it just sounds strange to me that somebody would want to alloc 0 bytes. But why not ...

Yes, this is a corner case. Not useful, but currently allowed and
definitely shouldn't crash the process.

> I guess that if pattern_count/count is set to 0 we can just avoid the all processing of malloc/memset/memcmp/free anyway.
> 
> Would you be ok with something like:
> 
> if (Pflag && pattern_count) {
> 
> instead of: 
> 
> if (Pflag) {

To be honest, the whole point of my mail was to provoke answers like
Anthony's and get qemu_malloc fixed. ;-)

But sure, the change you propose in qemu-io is fine and will make things
work even with current qemu_malloc.

Kevin

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[malc]

On Fri, 29 May 2009, Anthony Liguori wrote:

> Kevin Wolf wrote:
> > Since recently qemu_malloc behaves differently from malloc with size =
> > 0. This isn't allowed any more with qemu_malloc. So you need to check
> > for pattern_count == 0 and either print an error message or malloc a
> > different size, e.g. 1. I'm sure we don't want qemu-io to abort() in
> > such a case.
> >
> > Or we could start over with a lengthy discussion about fixing qemu_malloc...
> >   
> 
> Having qemu_malloc(0) abort is silly.  Returning NULL or returning
> malloc(1) are both reasonable options.

Dereference of NULL is UB[1] and dereferencing result of malloc(1) will
just plain work.

returning malloc(1) is the approach libiberty takes, readline opts to
abort on any malloc returning NULL, thus behaving like current qemu_malloc
on AIX and other OSes that opt to return NULL in *alloc(0) situation.

> 
> Putting the abort() in there is going to introduce a ton of subtle bugs,
> I vote for changing qemu_malloc() to have a sane behavior.

And those will be caught, given one a chance to analyze things, unlike
head in the sand approach of hoping things would just work.

After doing some research, after the aforementioned lengthy discussion,
the only free OS that straight-forwardly described what it does was
OpenBSD:

http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html

P.S. So far the abort that went into qemu_malloc caught one usage of zero
     allocation (once again coming from qcow2).

[1] For instance on AIX (and HP/UX depending on some tunables) read from 
    NULL will just work also.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Kevin Wolf]

malc schrieb:
>> Putting the abort() in there is going to introduce a ton of subtle bugs,
>> I vote for changing qemu_malloc() to have a sane behavior.
> 
> And those will be caught, given one a chance to analyze things, unlike
> head in the sand approach of hoping things would just work.
> 
> After doing some research, after the aforementioned lengthy discussion,
> the only free OS that straight-forwardly described what it does was
> OpenBSD:
> 
> http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html
> 
> P.S. So far the abort that went into qemu_malloc caught one usage of zero
>      allocation (once again coming from qcow2).

Zero allocation isn't a bug per se. Checking for NULL or dereferencing
is. Any value that can be freed, be it NULL or anything else, was
perfectly fine here without the abort() patch. So it actually was one of
the subtle bugs Anthony mentioned which are introduced by the abort().

Kevin

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[malc]

On Fri, 29 May 2009, Kevin Wolf wrote:

> malc schrieb:
> >> Putting the abort() in there is going to introduce a ton of subtle bugs,
> >> I vote for changing qemu_malloc() to have a sane behavior.
> > 
> > And those will be caught, given one a chance to analyze things, unlike
> > head in the sand approach of hoping things would just work.
> > 
> > After doing some research, after the aforementioned lengthy discussion,
> > the only free OS that straight-forwardly described what it does was
> > OpenBSD:
> > 
> > http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html
> > 
> > P.S. So far the abort that went into qemu_malloc caught one usage of zero
> >      allocation (once again coming from qcow2).
> 
> Zero allocation isn't a bug per se. Checking for NULL or dereferencing
> is. Any value that can be freed, be it NULL or anything else, was
> perfectly fine here without the abort() patch. So it actually was one of
> the subtle bugs Anthony mentioned which are introduced by the abort().

And once again, the code would have aborted on AIX with our without abort
patch. The reason for abort is to identify and reason about all the call
sites that do that.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Kevin Wolf]

malc schrieb:
> On Fri, 29 May 2009, Kevin Wolf wrote:
> 
>> malc schrieb:
>>>> Putting the abort() in there is going to introduce a ton of subtle bugs,
>>>> I vote for changing qemu_malloc() to have a sane behavior.
>>> And those will be caught, given one a chance to analyze things, unlike
>>> head in the sand approach of hoping things would just work.
>>>
>>> After doing some research, after the aforementioned lengthy discussion,
>>> the only free OS that straight-forwardly described what it does was
>>> OpenBSD:
>>>
>>> http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html
>>>
>>> P.S. So far the abort that went into qemu_malloc caught one usage of zero
>>>      allocation (once again coming from qcow2).
>> Zero allocation isn't a bug per se. Checking for NULL or dereferencing
>> is. Any value that can be freed, be it NULL or anything else, was
>> perfectly fine here without the abort() patch. So it actually was one of
>> the subtle bugs Anthony mentioned which are introduced by the abort().
> 
> And once again, the code would have aborted on AIX with our without abort
> patch. The reason for abort is to identify and reason about all the call
> sites that do that.

Why would it have aborted? If AIX aborts on malloc(0), its malloc is
seriously broken. But as I have understood from previous discussion, AIX
just returns NULL. Would have been okay for this code.

Kevin

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[malc]

On Fri, 29 May 2009, Kevin Wolf wrote:

> malc schrieb:
> > On Fri, 29 May 2009, Kevin Wolf wrote:
> > 
> >> malc schrieb:
> >>>> Putting the abort() in there is going to introduce a ton of subtle bugs,
> >>>> I vote for changing qemu_malloc() to have a sane behavior.
> >>> And those will be caught, given one a chance to analyze things, unlike
> >>> head in the sand approach of hoping things would just work.
> >>>
> >>> After doing some research, after the aforementioned lengthy discussion,
> >>> the only free OS that straight-forwardly described what it does was
> >>> OpenBSD:
> >>>
> >>> http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html
> >>>
> >>> P.S. So far the abort that went into qemu_malloc caught one usage of zero
> >>>      allocation (once again coming from qcow2).
> >> Zero allocation isn't a bug per se. Checking for NULL or dereferencing
> >> is. Any value that can be freed, be it NULL or anything else, was
> >> perfectly fine here without the abort() patch. So it actually was one of
> >> the subtle bugs Anthony mentioned which are introduced by the abort().
> > 
> > And once again, the code would have aborted on AIX with our without abort
> > patch. The reason for abort is to identify and reason about all the call
> > sites that do that.
> 
> Why would it have aborted? If AIX aborts on malloc(0), its malloc is
> seriously broken. But as I have understood from previous discussion, AIX
> just returns NULL. Would have been okay for this code.

Because of oom_check in qemu_malloc.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Kevin Wolf]

malc schrieb:
> On Fri, 29 May 2009, Kevin Wolf wrote:
> 
>> malc schrieb:
>>> On Fri, 29 May 2009, Kevin Wolf wrote:
>>>
>>>> malc schrieb:
>>>>>> Putting the abort() in there is going to introduce a ton of subtle bugs,
>>>>>> I vote for changing qemu_malloc() to have a sane behavior.
>>>>> And those will be caught, given one a chance to analyze things, unlike
>>>>> head in the sand approach of hoping things would just work.
>>>>>
>>>>> After doing some research, after the aforementioned lengthy discussion,
>>>>> the only free OS that straight-forwardly described what it does was
>>>>> OpenBSD:
>>>>>
>>>>> http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html
>>>>>
>>>>> P.S. So far the abort that went into qemu_malloc caught one usage of zero
>>>>>      allocation (once again coming from qcow2).
>>>> Zero allocation isn't a bug per se. Checking for NULL or dereferencing
>>>> is. Any value that can be freed, be it NULL or anything else, was
>>>> perfectly fine here without the abort() patch. So it actually was one of
>>>> the subtle bugs Anthony mentioned which are introduced by the abort().
>>> And once again, the code would have aborted on AIX with our without abort
>>> patch. The reason for abort is to identify and reason about all the call
>>> sites that do that.
>> Why would it have aborted? If AIX aborts on malloc(0), its malloc is
>> seriously broken. But as I have understood from previous discussion, AIX
>> just returns NULL. Would have been okay for this code.
> 
> Because of oom_check in qemu_malloc.

Ok, you're right there, of course. This is a bug in qemu_malloc and the
reason why we even discussed changing the check in qemu_malloc.

But this is not qcow2's fault, so the fix should really be local to
qemu_malloc like it already was for qemu_realloc.

Kevin

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Anthony Liguori]

malc wrote:
> On Fri, 29 May 2009, Anthony Liguori wrote:
>
>   
> Dereference of NULL is UB[1] and dereferencing result of malloc(1) will
> just plain work.
>   

So let's ignoring returning NULL as a possibility..

>> Putting the abort() in there is going to introduce a ton of subtle bugs,
>> I vote for changing qemu_malloc() to have a sane behavior.
>>     
>
> And those will be caught, given one a chance to analyze things, unlike
> head in the sand approach of hoping things would just work.
>
> After doing some research, after the aforementioned lengthy discussion,
> the only free OS that straight-forwardly described what it does was
> OpenBSD:
>
> http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html
>
> P.S. So far the abort that went into qemu_malloc caught one usage of zero
>      allocation (once again coming from qcow2).'
>   

But the zero allocation isn't a bug if we return malloc(1).  This is a
common convention and while it may not be portable to every platform's
underlying malloc, we can make this convention portable with qemu_malloc().

At the end of the day, the result is a harder to misuse qemu_malloc()
and that's a very good thing.  I don't want a user to "discover" a
non-portable use of malloc() while trying to do something important.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Anthony Liguori]

Kevin Wolf wrote:
> malc schrieb:
>   
>> Because of oom_check in qemu_malloc.
>>     
>
> Ok, you're right there, of course. This is a bug in qemu_malloc and the
> reason why we even discussed changing the check in qemu_malloc.
>
> But this is not qcow2's fault, so the fix should really be local to
> qemu_malloc like it already was for qemu_realloc.
>   

And Gerd's patch fixes the oom_check case for AIX along with keeping
more common malloc() semantics.

malc, do you object to me pushing Gerd's patch?

Regards,

Anthony Liguori

> Kevin
>   

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Gerd Hoffmann]

On 05/29/09 11:51, malc wrote:
>> Having qemu_malloc(0) abort is silly.  Returning NULL or returning
>> malloc(1) are both reasonable options.
>
> Dereference of NULL is UB[1] and dereferencing result of malloc(1) will
> just plain work.

malloc(0) itself isn't a bug.  Dereferencing the pointer is.
Code like this:

   buf = qemu_malloc(len);
   memcpy(buf, src, len);

will work perfectly fine when called with len=0 because it will not 
dereference buf for the len=0 case.  abort() in qemu_malloc for size=0 
will fire for no good reason.

> P.S. So far the abort that went into qemu_malloc caught one usage of zero
>       allocation (once again coming from qcow2).

That was a false positive.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[malc]

On Fri, 29 May 2009, Kevin Wolf wrote:

> malc schrieb:
> > On Fri, 29 May 2009, Kevin Wolf wrote:
> > 
> >> malc schrieb:
> >>> On Fri, 29 May 2009, Kevin Wolf wrote:
> >>>

[..snip..]

> > 
> > Because of oom_check in qemu_malloc.
> 
> Ok, you're right there, of course. This is a bug in qemu_malloc and the
> reason why we even discussed changing the check in qemu_malloc.
> 
> But this is not qcow2's fault, so the fix should really be local to
> qemu_malloc like it already was for qemu_realloc.

I'll rehash it one last time:

a. Original code in qcow2 did:
   p = qemu_malloc(nb_snapshots*sizeof_snapshot);
   if (!p) return_failure;

I.e. incorrect, when stuff==0 and platform==SYSV_derivative that picked
return NULL on size 0 among the IDB choices that standard provides.

b. Code after oom_check went in
   p = qemu_malloc(nb_snapshots*sizeof_snapshot);
   /* check gone */
   for (i = 0; i < nb_snapshots; i++)

I.e. incorrect, when stuff==0 and platform ... since oom_check would
kill it.

So the code in qcow2 was _never_ correct, not a single time. The fact
that it wouldn't crash since it doesn't dereference returned pointer
was/is irrelevant.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Kevin Wolf]

malc schrieb:
> On Fri, 29 May 2009, Kevin Wolf wrote:
> 
>> malc schrieb:
>>> On Fri, 29 May 2009, Kevin Wolf wrote:
>>>
>>>> malc schrieb:
>>>>> On Fri, 29 May 2009, Kevin Wolf wrote:
>>>>>
> 
> [..snip..]
> 
>>> Because of oom_check in qemu_malloc.
>> Ok, you're right there, of course. This is a bug in qemu_malloc and the
>> reason why we even discussed changing the check in qemu_malloc.
>>
>> But this is not qcow2's fault, so the fix should really be local to
>> qemu_malloc like it already was for qemu_realloc.
> 
> I'll rehash it one last time:
> 
> a. Original code in qcow2 did:
>    p = qemu_malloc(nb_snapshots*sizeof_snapshot);
>    if (!p) return_failure;
> 
> I.e. incorrect, when stuff==0 and platform==SYSV_derivative that picked
> return NULL on size 0 among the IDB choices that standard provides.
> 
> b. Code after oom_check went in
>    p = qemu_malloc(nb_snapshots*sizeof_snapshot);
>    /* check gone */
>    for (i = 0; i < nb_snapshots; i++)
> 
> I.e. incorrect, when stuff==0 and platform ... since oom_check would
> kill it.
> 
> So the code in qcow2 was _never_ correct, not a single time. The fact
> that it wouldn't crash since it doesn't dereference returned pointer
> was/is irrelevant.

In b, it was. It was just qemu_malloc that wasn't correct. Which is why
Eduardo sent a patch to fix it (without aborting innocent code). It
doesn't make sense to adjust all callers to work around a qemu_malloc bug.

Kevin

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[malc]

On Fri, 29 May 2009, Anthony Liguori wrote:

> malc wrote:
> > On Fri, 29 May 2009, Anthony Liguori wrote:
> >
> >
> > Dereference of NULL is UB[1] and dereferencing result of malloc(1) will
> > just plain work.
> >
>
> So let's ignoring returning NULL as a possibility..
>
> >> Putting the abort() in there is going to introduce a ton of subtle bugs,
> >> I vote for changing qemu_malloc() to have a sane behavior.
> >>
> >
> > And those will be caught, given one a chance to analyze things, unlike
> > head in the sand approach of hoping things would just work.
> >
> > After doing some research, after the aforementioned lengthy discussion,
> > the only free OS that straight-forwardly described what it does was
> > OpenBSD:
> >
> > http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html
> >
> > P.S. So far the abort that went into qemu_malloc caught one usage of zero
> >      allocation (once again coming from qcow2).'
> >
>
> But the zero allocation isn't a bug if we return malloc(1).  This is a
> common convention and while it may not be portable to every platform's
> underlying malloc, we can make this convention portable with qemu_malloc().

Yes we can. I argue that this is not a good convention.

>
> At the end of the day, the result is a harder to misuse qemu_malloc()
> and that's a very good thing.  I don't want a user to "discover" a
> non-portable use of malloc() while trying to do something important.

Options:

a. return NULL
b. return malloc(1)
c. abort
d. do what OpenBSD does

Pros/cons:

a. Pros: Simple to implement
         Matches one of original malloc behaviours

   Cons: Observable
         Dereference == UB
         Breaks the assumption that qemu_malloc should never return NULL


b. Pros: Simple to implement
         Matches one of original malloc behaviours

   Cons: Useless allocation
         No guard against accidental dereferences
         Might result in a return of a NULL


c. Pros: Simple to implement
         Helps in finding call-sites

   Cons: Doesn't match match the standard prescribed behaviours
         Will abort the application even if the call-site is prepared
         to cope with the fact that it requested zero bytes


d. Pros: Matches one of original malloc behaviours
         Provides safety net against accidental dereferences

   Cons: Not trivial to implement (and also complicates the whole family
         qemu_realloc/qemu_free)
         Doesn't help in identifying call-sites


In a nutshell what i argue is that, if someone doesn't need any memory
it shouldn't be asking for it, and it's not that unlikely that the
author never considered the possibility of his code requesting zero
bytes of memory, so in my view helping to locate all offenders and
auditing them is good enough reason for option c, that's why it's
there in repository.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[malc]

On Fri, 29 May 2009, Gerd Hoffmann wrote:

> On 05/29/09 11:51, malc wrote:
> > > Having qemu_malloc(0) abort is silly.  Returning NULL or returning
> > > malloc(1) are both reasonable options.
> > 
> > Dereference of NULL is UB[1] and dereferencing result of malloc(1) will
> > just plain work.
> 
> malloc(0) itself isn't a bug.  Dereferencing the pointer is.
> Code like this:
> 
>   buf = qemu_malloc(len);
>   memcpy(buf, src, len);
> 
> will work perfectly fine when called with len=0 because it will not
> dereference buf for the len=0 case.  abort() in qemu_malloc for size=0 will
> fire for no good reason.
> 
> > P.S. So far the abort that went into qemu_malloc caught one usage of zero
> >       allocation (once again coming from qcow2).
> 
> That was a false positive.

No actually it wasn't, the code with current and previous[1] versions of
qemu_malloc wouldn't work on AIX.

[1] But would work with qemu_malloc returning NULL or result of malloc(1)

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Gerd Hoffmann]

> a. return NULL
> b. return malloc(1)
> c. abort
> d. do what OpenBSD does

(e) return malloc(0), without wrapping it into oom_check().

> In a nutshell what i argue is that, if someone doesn't need any memory
> it shouldn't be asking for it, and it's not that unlikely that the
> author never considered the possibility of his code requesting zero
> bytes of memory,

If the calling code correctly keeps track of the allocated amount of 
memory (which it should do anyway for correctness and security reasons) 
the zero-length case will not cause any hickups.  It will happily copy 
zero bytes, do zero loop interations, or whatever else.

Aborting on qemu_malloc(0) forces the call sites to add a special case 
for len=0, even though correctly written code doesn't need a special 
case for it.

For the purpose of finding broken code returning NULL is IMHO the best 
option.  Although dereferencing NULL is undefined, in practice it will 
segfault in most cases so the bugs shouldn't stay unnoticed for long.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Markus Armbruster]

malc <av1474@comtv.ru> writes:

> On Fri, 29 May 2009, Anthony Liguori wrote:
>
>> malc wrote:
>> > On Fri, 29 May 2009, Anthony Liguori wrote:
>> >
>> >
>> > Dereference of NULL is UB[1] and dereferencing result of malloc(1) will
>> > just plain work.
>> >
>>
>> So let's ignoring returning NULL as a possibility..
>>
>> >> Putting the abort() in there is going to introduce a ton of subtle bugs,
>> >> I vote for changing qemu_malloc() to have a sane behavior.
>> >>
>> >
>> > And those will be caught, given one a chance to analyze things, unlike
>> > head in the sand approach of hoping things would just work.
>> >
>> > After doing some research, after the aforementioned lengthy discussion,
>> > the only free OS that straight-forwardly described what it does was
>> > OpenBSD:
>> >
>> > http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html

Neat.

What happens when malloc(0) can't obtain space in "shared protected
pages"?  I figure it fails and returns NULL.

>> > P.S. So far the abort that went into qemu_malloc caught one usage of zero
>> >      allocation (once again coming from qcow2).'
>> >
>>
>> But the zero allocation isn't a bug if we return malloc(1).  This is a
>> common convention and while it may not be portable to every platform's
>> underlying malloc, we can make this convention portable with qemu_malloc().
>
> Yes we can. I argue that this is not a good convention.
>
>>
>> At the end of the day, the result is a harder to misuse qemu_malloc()
>> and that's a very good thing.  I don't want a user to "discover" a
>> non-portable use of malloc() while trying to do something important.
>
> Options:
>
> a. return NULL
> b. return malloc(1)
> c. abort
> d. do what OpenBSD does
>
> Pros/cons:
>
> a. Pros: Simple to implement
>          Matches one of original malloc behaviours
>
>    Cons: Observable
>          Dereference == UB
>          Breaks the assumption that qemu_malloc should never return NULL
>
>
> b. Pros: Simple to implement
>          Matches one of original malloc behaviours
>
>    Cons: Useless allocation
>          No guard against accidental dereferences
>          Might result in a return of a NULL
>
>
> c. Pros: Simple to implement
>          Helps in finding call-sites
>
>    Cons: Doesn't match match the standard prescribed behaviours
>          Will abort the application even if the call-site is prepared
>          to cope with the fact that it requested zero bytes
>
>
> d. Pros: Matches one of original malloc behaviours
>          Provides safety net against accidental dereferences
>
>    Cons: Not trivial to implement (and also complicates the whole family
>          qemu_realloc/qemu_free)
>          Doesn't help in identifying call-sites

           Might result in a return of a NULL

> In a nutshell what i argue is that, if someone doesn't need any memory
> it shouldn't be asking for it, and it's not that unlikely that the
> author never considered the possibility of his code requesting zero
> bytes of memory, so in my view helping to locate all offenders and
> auditing them is good enough reason for option c, that's why it's
> there in repository.

Do we want a drop-in replacement for malloc() or not?

If we do, then options a, b and d are fine[*].  c is not, because it
breaks working code.  What the programmer thought when he wrote the
working code is immaterial.


[*] None of them guarantee that malloc(0) != NULL always, although b and
d make it unlikely.

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Paul Brook]

>(e) return malloc(0), without wrapping it into oom_check().

This is the worst of both worlds.

> For the purpose of finding broken code returning NULL is IMHO the best
> option.  Although dereferencing NULL is undefined, in practice it will
> segfault in most cases so the bugs shouldn't stay unnoticed for long.

The best way to find broken code is to have qemu_malloc(0) abort, and avoid 
ever trying to allocate a zero size block. Returning NULL is liable to 
introduce extra bugs because code often attributes special meaning to NULL 
pointers. It also breaks pointer comparison.

To some extent the answer to this question depends how much you trust your 
programmers. If you assume everyone knows the C standard well and always 
writes perfect code then malloc(0) is a legitimate technique, though IMHO of 
fairly limited benefit.

If you want maximize chances of catching accidental mistakes as early as 
possible then you should have malloc(0) abort, because it probably means 
someone forgot tho consider the empty case.

Paul

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Gerd Hoffmann]

On 05/29/09 15:07, Paul Brook wrote:
> The best way to find broken code is to have qemu_malloc(0) abort, and avoid
> ever trying to allocate a zero size block.

Forces all call sizes where size=0 is a perfectly legal case add extra 
code to prevent qemu from aborting, i.e. replace

   ptr = qemu_malloc(len);

with

   if (len) {
     ptr = qemu_malloc(len);
   } else {
     ptr = NULL; /* make sure we don't pass garbage to qemu_free() */
   }

> If you want maximize chances of catching accidental mistakes as early as
> possible then you should have malloc(0) abort, because it probably means
> someone forgot tho consider the empty case.

I don't share the assumption that malloc(0) is a bug in most cases.  And 
on the other hand the slightly different behavior might actually 
introduce bugs because people assume qemu_malloc() works like malloc().

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Glauber Costa]

> To some extent the answer to this question depends how much you trust your 
> programmers. If you assume everyone knows the C standard well and always 
> writes perfect code then malloc(0) is a legitimate technique, though IMHO of 
> fairly limited benefit.
> 
> If you want maximize chances of catching accidental mistakes as early as 
> possible then you should have malloc(0) abort, because it probably means 
> someone forgot tho consider the empty case.
> 

If we have to start programming like this because we don't trust our programmers,
(ultimately, ourselves), then we're pretty much doomed.

If anyone is contributing code to qemu these days, chances are that his code will
get reviewed. We'll then spot those things and kick them out. That's the only 
reasonable way to get quality.

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Anthony Liguori]

Paul Brook wrote:
>> For the purpose of finding broken code returning NULL is IMHO the best
>> option.  Although dereferencing NULL is undefined, in practice it will
>> segfault in most cases so the bugs shouldn't stay unnoticed for long.
>>     
>
> The best way to find broken code is to have qemu_malloc(0) abort, and avoid 
> ever trying to allocate a zero size block. Returning NULL is liable to 
> introduce extra bugs because code often attributes special meaning to NULL 
> pointers. It also breaks pointer comparison.
>
> To some extent the answer to this question depends how much you trust your 
> programmers. If you assume everyone knows the C standard well and always 
> writes perfect code then malloc(0) is a legitimate technique, though IMHO of 
> fairly limited benefit.
>   

I disagree.  The wrong reason we introduced oom_check() was because we
don't trust people to check returns.  Why would we trust them to check
size arguments especially when a lot of programmers are not going to
check size arguments by convention?

If we really want to fix up code, we can do so in a more benign way. 
Instead of aborting, let's return malloc(1) and printf a warning.  Then
we can fix sites without worrying about killing guests.

If we only have to fixup a few sites, then yeah, we can switch to
abort().  If it turns out there's a ton of these things and the code is
uglier, then we can give up and just accept these semantics.

Does that sound reasonable?

abort()'ing on malloc(0) is going to result in some really nasty crashes
that were entirely preventable in probably 99% of circumstances.

Regards,

Anthony Liguori

> If you want maximize chances of catching accidental mistakes as early as 
> possible then you should have malloc(0) abort, because it probably means 
> someone forgot tho consider the empty case.
>
> Paul
>
>
>   

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[malc]

On Fri, 29 May 2009, Anthony Liguori wrote:

> Paul Brook wrote:
> >> For the purpose of finding broken code returning NULL is IMHO the best
> >> option.  Although dereferencing NULL is undefined, in practice it will
> >> segfault in most cases so the bugs shouldn't stay unnoticed for long.
> >>     
> >
> > The best way to find broken code is to have qemu_malloc(0) abort, and avoid 
> > ever trying to allocate a zero size block. Returning NULL is liable to 
> > introduce extra bugs because code often attributes special meaning to NULL 
> > pointers. It also breaks pointer comparison.
> >
> > To some extent the answer to this question depends how much you trust your 
> > programmers. If you assume everyone knows the C standard well and always 
> > writes perfect code then malloc(0) is a legitimate technique, though IMHO of 
> > fairly limited benefit.
> >   
> 
> I disagree.  The wrong reason we introduced oom_check() was because we
> don't trust people to check returns.  Why would we trust them to check
> size arguments especially when a lot of programmers are not going to
> check size arguments by convention?
> 
> If we really want to fix up code, we can do so in a more benign way. 
> Instead of aborting, let's return malloc(1) and printf a warning.  Then
> we can fix sites without worrying about killing guests.

Printf a warning saying what exactly?

> 
> If we only have to fixup a few sites, then yeah, we can switch to
> abort().  If it turns out there's a ton of these things and the code is
> uglier, then we can give up and just accept these semantics.

We _already_ use abort, and so far only one caller was caught and
taken care of.

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Julian Seward]

On Friday 29 May 2009, Paul Brook wrote:
> >(e) return malloc(0), without wrapping it into oom_check().
>
> This is the worst of both worlds.
>
> > For the purpose of finding broken code returning NULL is IMHO the best
> > option.  Although dereferencing NULL is undefined, in practice it will
> > segfault in most cases so the bugs shouldn't stay unnoticed for long.
>
> The best way to find broken code is to have qemu_malloc(0) abort, and avoid
> ever trying to allocate a zero size block.

+1 for that.  Code that relies on malloc(0) doing any specific thing
is basically bad news when it comes to portability, robustness
and understandability.  Better to have qemu_malloc(0) abort, put up with
a couple of days of the trunk aborting, until these uses are fixed.
I'd be surprised if there were many cases anyway.

J

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Gerd Hoffmann]

On 05/29/09 19:17, Julian Seward wrote:
> On Friday 29 May 2009, Paul Brook wrote:
>> The best way to find broken code is to have qemu_malloc(0) abort, and avoid
>> ever trying to allocate a zero size block.
>
> +1 for that.  Code that relies on malloc(0) doing any specific thing
> is basically bad news when it comes to portability, robustness
> and understandability.

The *only* thing you can rely on is that the value returned by malloc(0) 
can be passed to free() without trouble.

Code like this ...

   buf = malloc(len);
   for (i = 0; i < len; i++)
      do_something_with(buf[i]);
   free(buf);

... works perfectly fine for len=0, no matter how malloc(0) is actually 
implemented because buf is never ever dereferenced then.

With the current qemu_malloc() implementation it will abort instead and 
you'll have to add extra code to make len=0 a special case for IMO no 
good reason.

> Better to have qemu_malloc(0) abort, put up with
> a couple of days of the trunk aborting, until these uses are fixed.

Oh, such cases could very well be outside the common code paths, so it 
doesn't explode instantly for everybody.  They'll be time bombs instead.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[David Turner]

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

On Fri, May 29, 2009 at 7:17 PM, Julian Seward <jseward@acm.org> wrote:

>
> +1 for that.  Code that relies on malloc(0) doing any specific thing
> is basically bad news when it comes to portability, robustness
> and understandability.  Better to have qemu_malloc(0) abort, put up with
> a couple of days of the trunk aborting, until these uses are fixed.
> I'd be surprised if there were many cases anyway.
>

I think there are two conflicting goals here:

- On one hand, people are suggesting to abort on malloc(0) because it is the
sign of buggy code,
  and would help spot it as soon as possible, before any damage is done.

- On the other hand, some people object that this assumption is sometimes
wrong (e.g. with
  arrays), where having malloc(0) returning NULL or anything else is totally
appropriate.

Would it make sense to separate the two issues with something like the
following:

- qemu_malloc(0) aborts and print a panic message
- qemu_calloc(count, itemsize) would return NULL for 'itemsize == 0', but
would abort for 'count == 0'
- array-allocating/parsing code MUST use qemu_calloc() exclusively. And
calloc() can also perform trivial
integer multiplication overflow checks too.

I would even suggest providing helper macros to make the programmer's intent
even more clear
and less error-prone, as in:

#define  QEMU_NEW(ptr)                    (ptr) = qemu_alloc(sizeof(*(ptr)))
#define  QEMU_NEW_ARRAY(ptr,cnt)   (ptr) = qemu_calloc((cnt),sizeof(*(ptr)))
#define  QEMU_RENEW_ARRAY(ptr,cnt)  (ptr) =
qemu_realloc((ptr),(cnt),sizeof(*(ptr)))
#define  QEMU_FREE_ARRAY(ptr)        qemu_free(ptr)

(yes, qemu_realloc() would take 3 parameters).

Any direct use of malloc()/qemu_malloc() in source code would be suspicious
and could
easily spotted to check it.


>
> J
>
>
>

[-- Attachment #2: Type: text/html, Size: 2428 bytes --]

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[David Turner]

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

Aargh, I was confused, I meant:

- qemu_calloc(count, itemsize) would abort for 'itemsize == 0', but return
NULL for 'count == 0'

sorry about that..

On Fri, May 29, 2009 at 11:12 PM, David Turner <digit@google.com> wrote:

>
>
> On Fri, May 29, 2009 at 7:17 PM, Julian Seward <jseward@acm.org> wrote:
>
>>
>> +1 for that.  Code that relies on malloc(0) doing any specific thing
>> is basically bad news when it comes to portability, robustness
>> and understandability.  Better to have qemu_malloc(0) abort, put up with
>> a couple of days of the trunk aborting, until these uses are fixed.
>> I'd be surprised if there were many cases anyway.
>>
>
> I think there are two conflicting goals here:
>
> - On one hand, people are suggesting to abort on malloc(0) because it is
> the sign of buggy code,
>   and would help spot it as soon as possible, before any damage is done.
>
> - On the other hand, some people object that this assumption is sometimes
> wrong (e.g. with
>   arrays), where having malloc(0) returning NULL or anything else is
> totally appropriate.
>
> Would it make sense to separate the two issues with something like the
> following:
>
> - qemu_malloc(0) aborts and print a panic message
> - qemu_calloc(count, itemsize) would return NULL for 'itemsize == 0', but
> would abort for 'count == 0'
> - array-allocating/parsing code MUST use qemu_calloc() exclusively. And
> calloc() can also perform trivial
> integer multiplication overflow checks too.
>
> I would even suggest providing helper macros to make the programmer's
> intent even more clear
> and less error-prone, as in:
>
> #define  QEMU_NEW(ptr)                    (ptr) =
> qemu_alloc(sizeof(*(ptr)))
> #define  QEMU_NEW_ARRAY(ptr,cnt)   (ptr) =
> qemu_calloc((cnt),sizeof(*(ptr)))
> #define  QEMU_RENEW_ARRAY(ptr,cnt)  (ptr) =
> qemu_realloc((ptr),(cnt),sizeof(*(ptr)))
> #define  QEMU_FREE_ARRAY(ptr)        qemu_free(ptr)
>
> (yes, qemu_realloc() would take 3 parameters).
>
> Any direct use of malloc()/qemu_malloc() in source code would be suspicious
> and could
> easily spotted to check it.
>
>
>>
>> J
>>
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 2978 bytes --]

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Jamie Lokier]

jcd@tribudubois.net wrote:
> Hi Kevin,
> 
> Thanks for pointing this. I guess it just sounds strange to me that
> somebody would want to alloc 0 bytes. But why not ...

Something that nobody pointed out is that sometimes you can have
zero-length structures these days.  E.g. if you have a struct
containing a few spinlocks in the Linux kernel, it will be zero length
when built on UP targets.  I doubt if QEMU has any such structures,
but it's the sort of thing that shouldn't break
qemu_malloc(sizeof(some_type)).

-- Jamie

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Gerd Hoffmann]

On 05/29/09 23:12, David Turner wrote:
> I would even suggest providing helper macros to make the programmer's intent
> even more clear
> and less error-prone, as in:
>
> #define  QEMU_NEW(ptr)                    (ptr) = qemu_alloc(sizeof(*(ptr)))
> #define  QEMU_NEW_ARRAY(ptr,cnt)   (ptr) = qemu_calloc((cnt),sizeof(*(ptr)))
> #define  QEMU_RENEW_ARRAY(ptr,cnt)  (ptr) =
> qemu_realloc((ptr),(cnt),sizeof(*(ptr)))
> #define  QEMU_FREE_ARRAY(ptr)        qemu_free(ptr)

The idea to have allocators for arrays (and have them allow zero-length 
arrays) is fine.  I wouldn't create two macros for new and renew array, 
you can just use usual realloc semantics (ptr == NULL -> alloc).

Also I don't like the syntax that much as you'll have the IMHO 
non-intuitive code like this:

   QEMU_NEW_ARRAY(ptr, ...);

instead of

   ptr = QEMU_NEW_ARRAY(...);

then.  I don't see another easy way to get the automagic sizeof(*ptr) 
stuff done though.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 05/29/09 23:12, David Turner wrote:
>> I would even suggest providing helper macros to make the programmer's
>> intent
>> even more clear
>> and less error-prone, as in:
>>
>> #define  QEMU_NEW(ptr)                    (ptr) =
>> qemu_alloc(sizeof(*(ptr)))
>> #define  QEMU_NEW_ARRAY(ptr,cnt)   (ptr) =
>> qemu_calloc((cnt),sizeof(*(ptr)))
>> #define  QEMU_RENEW_ARRAY(ptr,cnt)  (ptr) =
>> qemu_realloc((ptr),(cnt),sizeof(*(ptr)))
>> #define  QEMU_FREE_ARRAY(ptr)        qemu_free(ptr)
>
> The idea to have allocators for arrays (and have them allow
> zero-length arrays) is fine.  I wouldn't create two macros for new and
> renew array, you can just use usual realloc semantics (ptr == NULL ->
> alloc).
>
> Also I don't like the syntax that much as you'll have the IMHO
> non-intuitive code like this:
>
>   QEMU_NEW_ARRAY(ptr, ...);
>
> instead of
>
>   ptr = QEMU_NEW_ARRAY(...);
>
> then.  I don't see another easy way to get the automagic sizeof(*ptr)
> stuff done though.

I've always liked glib's memory functions.  It does OOM error handling
and returns NULL when size == 0.

http://library.gnome.org/devel/glib/stable/glib-Memory-Allocation.html

Regards,

Anthony Liguori
> cheers,
>   Gerd
>
>
>

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Avi Kivity]

Gerd Hoffmann wrote:
>
> The idea to have allocators for arrays (and have them allow 
> zero-length arrays) is fine.  I wouldn't create two macros for new and 
> renew array, you can just use usual realloc semantics (ptr == NULL -> 
> alloc).
>
> Also I don't like the syntax that much as you'll have the IMHO 
> non-intuitive code like this:
>
>   QEMU_NEW_ARRAY(ptr, ...);
>
> instead of
>
>   ptr = QEMU_NEW_ARRAY(...);
>
> then.  I don't see another easy way to get the automagic sizeof(*ptr) 
> stuff done though.
>

I too would like to QEMU_NEW() and QEMU_NEW_ARRAY() like you propose (or 
even better, new and new[]).


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Daniel P. Berrange]

On Tue, Jun 02, 2009 at 02:47:57AM -0500, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
> > On 05/29/09 23:12, David Turner wrote:
> >> I would even suggest providing helper macros to make the programmer's
> >> intent
> >> even more clear
> >> and less error-prone, as in:
> >>
> >> #define  QEMU_NEW(ptr)                    (ptr) =
> >> qemu_alloc(sizeof(*(ptr)))
> >> #define  QEMU_NEW_ARRAY(ptr,cnt)   (ptr) =
> >> qemu_calloc((cnt),sizeof(*(ptr)))
> >> #define  QEMU_RENEW_ARRAY(ptr,cnt)  (ptr) =
> >> qemu_realloc((ptr),(cnt),sizeof(*(ptr)))
> >> #define  QEMU_FREE_ARRAY(ptr)        qemu_free(ptr)
> >
> > The idea to have allocators for arrays (and have them allow
> > zero-length arrays) is fine.  I wouldn't create two macros for new and
> > renew array, you can just use usual realloc semantics (ptr == NULL ->
> > alloc).
> >
> > Also I don't like the syntax that much as you'll have the IMHO
> > non-intuitive code like this:
> >
> >   QEMU_NEW_ARRAY(ptr, ...);
> >
> > instead of
> >
> >   ptr = QEMU_NEW_ARRAY(...);
> >
> > then.  I don't see another easy way to get the automagic sizeof(*ptr)
> > stuff done though.
> 
> I've always liked glib's memory functions.  It does OOM error handling
> and returns NULL when size == 0.

If you look at the problems associated with malloc there are many common
programmer mistakes, of which failure to check for NULL is just one.
IMHO, if you're going to wrap malloc/calloc/etc, then you should aim
higher and try to address all the common problems.  David's suggestion
helps address the problem incorrect sizing too, of which there was an
example on this list only last week with VncState/VncDisplasy mixup.
Other problems including forgetting to initialize memory, which can be
solved by using calloc for everything (though in QEMU's case this may
have too much overhead). Double free is another which can be protected
against by having the free function also NULL-ify the pointer being
freed. 

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[David Turner]

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

On Tue, Jun 2, 2009 at 9:26 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:

> On 05/29/09 23:12, David Turner wrote:
>
>> I would even suggest providing helper macros to make the programmer's
>> intent
>> even more clear
>> and less error-prone, as in:
>>
>> #define  QEMU_NEW(ptr)                    (ptr) =
>> qemu_alloc(sizeof(*(ptr)))
>> #define  QEMU_NEW_ARRAY(ptr,cnt)   (ptr) =
>> qemu_calloc((cnt),sizeof(*(ptr)))
>> #define  QEMU_RENEW_ARRAY(ptr,cnt)  (ptr) =
>> qemu_realloc((ptr),(cnt),sizeof(*(ptr)))
>> #define  QEMU_FREE_ARRAY(ptr)        qemu_free(ptr)
>>
>
> The idea to have allocators for arrays (and have them allow zero-length
> arrays) is fine.  I wouldn't create two macros for new and renew array, you
> can just use usual realloc semantics (ptr == NULL -> alloc).
>




>
> Also I don't like the syntax that much as you'll have the IMHO
> non-intuitive code like this:
>
>  QEMU_NEW_ARRAY(ptr, ...);
>
> instead of
>
>  ptr = QEMU_NEW_ARRAY(...);
>
> then.  I don't see another easy way to get the automagic sizeof(*ptr) stuff
> done though.
>

The first version will extract the item size automatically for you, making
it less likely that you screw things in the second version's parameter list.
But I'm not going to fight against it.

Also, my code is usually a lot more aggressive than what I proposed. I don't
want to burn too many cycles on this, just stating that,
generally speaking, it is possible to use macros/wrappers to both make the
code's intent more clear and reduce potential errors.

I feel this much more worthwhile than imposing an abort on qemu_malloc(0),
which seems quite arbitrary, as discussed heavily on this thread.
 Apart from that, the exact details on how to achieve the above goal don't
matter much.



>
> cheers,
>  Gerd
>
>

[-- Attachment #2: Type: text/html, Size: 2688 bytes --]

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[David Turner]

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

On Tue, Jun 2, 2009 at 10:58 AM, Daniel P. Berrange <berrange@redhat.com>wrote:

> On Tue, Jun 02, 2009 at 02:47:57AM -0500, Anthony Liguori wrote:
> > Gerd Hoffmann wrote:
> > > On 05/29/09 23:12, David Turner wrote:
> > >> I would even suggest providing helper macros to make the programmer's
> > >> intent
> > >> even more clear
> > >> and less error-prone, as in:
> > >>
> > >> #define  QEMU_NEW(ptr)                    (ptr) =
> > >> qemu_alloc(sizeof(*(ptr)))
> > >> #define  QEMU_NEW_ARRAY(ptr,cnt)   (ptr) =
> > >> qemu_calloc((cnt),sizeof(*(ptr)))
> > >> #define  QEMU_RENEW_ARRAY(ptr,cnt)  (ptr) =
> > >> qemu_realloc((ptr),(cnt),sizeof(*(ptr)))
> > >> #define  QEMU_FREE_ARRAY(ptr)        qemu_free(ptr)
> > >
> > > The idea to have allocators for arrays (and have them allow
> > > zero-length arrays) is fine.  I wouldn't create two macros for new and
> > > renew array, you can just use usual realloc semantics (ptr == NULL ->
> > > alloc).
> > >
> > > Also I don't like the syntax that much as you'll have the IMHO
> > > non-intuitive code like this:
> > >
> > >   QEMU_NEW_ARRAY(ptr, ...);
> > >
> > > instead of
> > >
> > >   ptr = QEMU_NEW_ARRAY(...);
> > >
> > > then.  I don't see another easy way to get the automagic sizeof(*ptr)
> > > stuff done though.
> >
> > I've always liked glib's memory functions.  It does OOM error handling
> > and returns NULL when size == 0.
>
> If you look at the problems associated with malloc there are many common
> programmer mistakes, of which failure to check for NULL is just one.
> IMHO, if you're going to wrap malloc/calloc/etc, then you should aim
> higher and try to address all the common problems.  David's suggestion
> helps address the problem incorrect sizing too, of which there was an
> example on this list only last week with VncState/VncDisplasy mixup.
> Other problems including forgetting to initialize memory, which can be
> solved by using calloc for everything (though in QEMU's case this may
> have too much overhead). Double free is another which can be protected
> against by having the free function also NULL-ify the pointer being
> freed.
>

Agreed, that's the thing I do; and it works really well in practice.


>
> Regards,
> Daniel
> --
> |: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/:|
> |: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org:|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/:|
> |: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505
> :|
>

[-- Attachment #2: Type: text/html, Size: 3777 bytes --]

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Paul Brook]

> >  QEMU_NEW_ARRAY(ptr, ...);
> >
> > instead of
> >
> >  ptr = QEMU_NEW_ARRAY(...);
> >
> > then.  I don't see another easy way to get the automagic sizeof(*ptr)
> > stuff done though.
>
> The first version will extract the item size automatically for you, making
> it less likely that you screw things in the second version's parameter
> list.

Not if you do it properly it won't. Implicit casts are only silently allowed 
between void* and arbitrary pointers. A cast between two arbitrary pointers 
will generate a compiler diagnostic.

Paul

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Avi Kivity]

David Turner wrote:
>
> The first version will extract the item size automatically for you, 
> making it less likely that you screw things in the second version's 
> parameter list.
> But I'm not going to fight against it.
>
> Also, my code is usually a lot more aggressive than what I proposed. I 
> don't want to burn too many cycles on this, just stating that,
> generally speaking, it is possible to use macros/wrappers to both make 
> the code's intent more clear and reduce potential errors.

There's no need to reinvent a programming language in macros.  If we 
feel that type safety is a good thing, we should switch to a language 
that supports it.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[David Turner]

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

On Tue, Jun 2, 2009 at 8:13 PM, Paul Brook <paul@codesourcery.com> wrote:

> > >  QEMU_NEW_ARRAY(ptr, ...);
> > >
> > > instead of
> > >
> > >  ptr = QEMU_NEW_ARRAY(...);
> > >
> > > then.  I don't see another easy way to get the automagic sizeof(*ptr)
> > > stuff done though.
> >
> > The first version will extract the item size automatically for you,
> making
> > it less likely that you screw things in the second version's parameter
> > list.
>
> Not if you do it properly it won't.


If you do it properly, then bugs do not exist, and this thread has no reason
to exist.
We're talking about it precisely because we're human and people tend to make
mistakes.


> Implicit casts are only silently allowed
> between void* and arbitrary pointers. A cast between two arbitrary pointers
> will generate a compiler diagnostic.
>

Ok, let's say that the first one should only be used when 'ptr' is a typed
pointer so that
sizeof(*(ptr)) has a sense. I think it catches 95% of uses here.

Very frankly I don't see your point. Either you want to reduce the
probability of programming
errors, and find ways to achieve that, or you don't. I just don't think that
introducing an abort()
on qemu_malloc(0) makes sense because it goes against the principle of least
surprise, especially
for a lot of programmers that already do things properly.



>
> Paul
>

[-- Attachment #2: Type: text/html, Size: 2192 bytes --]

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Paul Brook]

On Tuesday 02 June 2009, David Turner wrote:
> On Tue, Jun 2, 2009 at 8:13 PM, Paul Brook <paul@codesourcery.com> wrote:
> > > >  QEMU_NEW_ARRAY(ptr, ...);
> > > >
> > > > instead of
> > > >
> > > >  ptr = QEMU_NEW_ARRAY(...);
> > > >
> > > > then.  I don't see another easy way to get the automagic sizeof(*ptr)
> > > > stuff done though.
> > >
> > > The first version will extract the item size automatically for you,
> >
> > making
> >
> > > it less likely that you screw things in the second version's parameter
> > > list.
> >
> > Not if you do it properly it won't.
>
> Very frankly I don't see your point.

My point is that

#define QEMU_NEW(type) ((type *)qemu_malloc(sizeof(type)))
  foo *ptr = QEMU_NEW(foo);

is just as safe as

#define QEMU_NEW(ptr) (ptr) = qemu_malloc(sizeof(*(ptr)))
  foo *ptr;
  QEMU_NEW(ptr);

Because the compiler will catch the type mismatch.

Paul

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[David Turner]

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

On Tue, Jun 2, 2009 at 10:04 PM, Paul Brook <paul@codesourcery.com> wrote:

>
>
> My point is that
>
> #define QEMU_NEW(type) ((type *)qemu_malloc(sizeof(type)))
>  foo *ptr = QEMU_NEW(foo);
>
> is just as safe as
>
> #define QEMU_NEW(ptr) (ptr) = qemu_malloc(sizeof(*(ptr)))
>  foo *ptr;
>  QEMU_NEW(ptr);
>
> Because the compiler will catch the type mismatch.
>

I still don't see the point.
There is no type mismatch in the first version since the C standard mandates
that a (void*) *must* be silently casted into any other typed pointer
(unlike C++ which forbids this).

I think you're afraid of the following case instead:

foo*  ptr;
ptr = QEMU_MEW(bar);   =>   compiler will complain that 'ptr' is not a bar*

But this is not possible with the first version anyway.



>
> Paul
>

[-- Attachment #2: Type: text/html, Size: 1390 bytes --]

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Gerd Hoffmann]

On 06/02/09 22:42, David Turner wrote:
>> #define QEMU_NEW(type) ((type *)qemu_malloc(sizeof(type)))
>> #define QEMU_NEW(ptr) (ptr) = qemu_malloc(sizeof(*(ptr)))
>>   foo *ptr;
>>   QEMU_NEW(ptr);
>>
>> Because the compiler will catch the type mismatch.
>>
>
> I still don't see the point.
> There is no type mismatch in the first version since the C standard mandates
> that a (void*) *must* be silently casted into any other typed pointer
> (unlike C++ which forbids this).
>
> I think you're afraid of the following case instead:
>
> foo*  ptr;
> ptr = QEMU_MEW(bar);   =>    compiler will complain that 'ptr' is not a bar*
>
> But this is not possible with the first version anyway.
>
>
>
>> Paul
>>
>

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Gerd Hoffmann]

On 06/02/09 22:42, David Turner wrote:
>> #define QEMU_NEW(type) ((type *)qemu_malloc(sizeof(type)))
>>   foo *ptr = QEMU_NEW(foo);

>> #define QEMU_NEW(ptr) (ptr) = qemu_malloc(sizeof(*(ptr)))
>>   foo *ptr;
>>   QEMU_NEW(ptr);

> I still don't see the point.

Point is: The first version is nicer than the second, and you are still 
type safe due to the compiler warning on stuff like this:

> foo*  ptr;
> ptr = QEMU_MEW(bar);

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Paul Brook]

> I think you're afraid of the following case instead:
>
> foo*  ptr;
> ptr = QEMU_MEW(bar);   =>   compiler will complain that 'ptr' is not a bar*
>
>But this is not possible with the first version anyway.

Rubbish. Works perfectly.

cat  test.c <<EOF
#include <stdlib.h>
#define QEMU_NEW(type) (type*)malloc(sizeof(type))
typedef struct { int x; } foo;
typedef struct { double y; } bar;
int main(int argc, char **argv)
{
  foo *ptr = QEMU_NEW(bar);
  return 0;
}
EOF
gcc test.c
test.c: In function ‘main’:
test.c:7: initialization from incompatible pointer type

Paul

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[David Turner]

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

On Tue, Jun 2, 2009 at 10:58 PM, Paul Brook <paul@codesourcery.com> wrote:

> > I think you're afraid of the following case instead:
> >
> > foo*  ptr;
> > ptr = QEMU_MEW(bar);   =>   compiler will complain that 'ptr' is not a
> bar*
> >
> >But this is not possible with the first version anyway.
>
> Rubbish. Works perfectly.
>

I think there is a misunderstanding here, what I mean is that:

#define  QEMU_NEW(ptr)  (ptr) = malloc(sizeof(*ptr))

foo*  ptr;
QEMU_NEW(ptr);

Will never try to allocate sizeof(bar) bytes, anyway.
The rest is taste/personal preference, and there is no point fighting about
it.

As I said, I just want to say that abort() on qemu_malloc(0) is not the
shiny good thing
some people are expecting, and that there are better ways to fight potential
developer errors.


>
> cat  test.c <<EOF
> #include <stdlib.h>
> #define QEMU_NEW(type) (type*)malloc(sizeof(type))
> typedef struct { int x; } foo;
> typedef struct { double y; } bar;
> int main(int argc, char **argv)
> {
>  foo *ptr = QEMU_NEW(bar);
>  return 0;
> }
> EOF
> gcc test.c
> test.c: In function ‘main’:
> test.c:7: initialization from incompatible pointer type
>
> Paul
>

[-- Attachment #2: Type: text/html, Size: 1792 bytes --]

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Jamie Lokier]

jcd@tribudubois.net wrote:
> 
> ----- "Jamie Lokier" <jamie@shareable.org> a écrit :
> 
> > jcd@tribudubois.net wrote:
> > > Hi Kevin,
> > > 
> > > Thanks for pointing this. I guess it just sounds strange to me that
> > > somebody would want to alloc 0 bytes. But why not ...
> > 
> > Something that nobody pointed out is that sometimes you can have
> > zero-length structures these days.  E.g. if you have a struct
> > containing a few spinlocks in the Linux kernel, it will be zero
> > length
> > when built on UP targets.  I doubt if QEMU has any such structures,
> > but it's the sort of thing that shouldn't break
> > qemu_malloc(sizeof(some_type)).
> 
> This is true but the kernel API is also making a clear distinction between the NULL returned value for allocation error and the specific ZERO_SIZE_PTR value returned for kmalloc(0, XXX). Things don't get mixed ...

Ooh, smart.

That crossed my mind for qemu_malloc() too: return (void*)16, will
SEGFAULT just like NULL but is non-NULL.

-- Jamie

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[jcd]

----- "Jamie Lokier" <jamie@shareable.org> a écrit :

> jcd@tribudubois.net wrote:
> > Hi Kevin,
> > 
> > Thanks for pointing this. I guess it just sounds strange to me that
> > somebody would want to alloc 0 bytes. But why not ...
> 
> Something that nobody pointed out is that sometimes you can have
> zero-length structures these days.  E.g. if you have a struct
> containing a few spinlocks in the Linux kernel, it will be zero
> length
> when built on UP targets.  I doubt if QEMU has any such structures,
> but it's the sort of thing that shouldn't break
> qemu_malloc(sizeof(some_type)).

This is true but the kernel API is also making a clear distinction between the NULL returned value for allocation error and the specific ZERO_SIZE_PTR value returned for kmalloc(0, XXX). Things don't get mixed ...

JC

> -- Jamie

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[jcd]

----- "malc" <av1474@comtv.ru> a écrit :

> On Fri, 29 May 2009, Anthony Liguori wrote:
> 
> > 
> > I disagree.  The wrong reason we introduced oom_check() was because
> we
> > don't trust people to check returns.  Why would we trust them to
> check
> > size arguments especially when a lot of programmers are not going
> to
> > check size arguments by convention?
> > 
> > If we really want to fix up code, we can do so in a more benign way.
> 
> > Instead of aborting, let's return malloc(1) and printf a warning. 
> Then
> > we can fix sites without worrying about killing guests.
> 
> Printf a warning saying what exactly?

I guess it should say malloc(0) was used with a backtrace to be usefull ...

> > If we only have to fixup a few sites, then yeah, we can switch to
> > abort().  If it turns out there's a ton of these things and the code
> is
> > uglier, then we can give up and just accept these semantics.
> 
> We _already_ use abort, and so far only one caller was caught and
> taken care of.

I am going to throw in a few more potential cases with my proposed patch. But not that many ...

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[jcd]

----- "Glauber Costa" <glommer@redhat.com> a écrit :

> If we have to start programming like this because we don't trust our
> programmers,
> (ultimately, ourselves), then we're pretty much doomed.

This is not that bad: 

It is just like having a "coding rule" on the qemu project stating that malloc(0) is forbiden (even if legal) and BTW it is also enforced in the qemu_malloc() implementation that should always be used.

OTOH if we believe all the contributed code (past, present and furure) is always perfect then the all qemu_malloc/qemu_free ... scheme can just be discarded (it adds to the complexity and decreases perf) and we go back to plain malloc/free trusting the programmer to know about the subtle platform differences.

> If anyone is contributing code to qemu these days, chances are that
> his code will
> get reviewed. We'll then spot those things and kick them out. That's
> the only 
> reasonable way to get quality.

Reviews are good and still needed but it doesn't hurt to also have some tools to catch corner cases that can be easily missed.

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[jcd]

So even if malloc(0) is legal per se, is it really exepected there is that much code in qemu relying on it?

It seems not (and there is also some qemu code considering NULL as a failure value regardless of the value of size parameter) and fixing the few places where this could be expected (like qemu-io.c) should not be a lot of work. All these "special" cases should be detected fairly quickly.

JC

----- "Paul Brook" <paul@codesourcery.com> a écrit :

> >(e) return malloc(0), without wrapping it into oom_check().
> 
> This is the worst of both worlds.
> 
> > For the purpose of finding broken code returning NULL is IMHO the
> best
> > option.  Although dereferencing NULL is undefined, in practice it
> will
> > segfault in most cases so the bugs shouldn't stay unnoticed for
> long.
> 
> The best way to find broken code is to have qemu_malloc(0) abort, and
> avoid 
> ever trying to allocate a zero size block. Returning NULL is liable to
> 
> introduce extra bugs because code often attributes special meaning to
> NULL 
> pointers. It also breaks pointer comparison.
> 
> To some extent the answer to this question depends how much you trust
> your 
> programmers. If you assume everyone knows the C standard well and
> always 
> writes perfect code then malloc(0) is a legitimate technique, though
> IMHO of 
> fairly limited benefit.
> 
> If you want maximize chances of catching accidental mistakes as early
> as 
> possible then you should have malloc(0) abort, because it probably
> means 
> someone forgot tho consider the empty case.
> 
> Paul

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[jcd]

----- "Markus Armbruster" <armbru@redhat.com> a écrit :
> 
> No.  malloc() implements the same, well-known spec everywhere.  Some
> people don't understand or don't like the spec, but that doesn't make
> implementations of the spec incompatible.  Here's what you can expect
> from malloc():
> 
> * After p = malloc(SIZE), you can access p[i] for 0 <= i < SIZE.  It
>   follows that you must not dereference p at all if SIZE == 0.
> 
> * If malloc(SIZE) returns a null pointer, and SIZE != 0, then
> malloc()
>   failed.
> 
> * Any result of malloc() can safely be passed to free().
> 
> A wrapper like qemu_malloc() can implement a different spec, of
> course.
> But only if the spec is compatible, the wrapper can be used as a
> drop-in
> replacement.  Making it gratuitously incompatible is foolish, because
> it
> *breaks* code that works just fine with malloc().
> 
> We already agreed on one compatible difference: qemu_malloc() shall
> not
> fail, but just terminate the program.
> 
> We're debating another difference, namely what qemu_malloc(0) shall
> do.
> Two fine compatible options have been proposed here:
> 
> 1. qemu_malloc(0) shall return a null pointer.
> 
> 2. qemu_malloc(0) shall return a non-null pointer that can be passed
> to
>    qemu_free()
> 
> The one that got committed is incompatible.

Thanks for the clarification Markus.

JC

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Markus Armbruster]

jcd@tribudubois.net writes:

[...]
> It seems to be just a fact that all malloc implementation are not
> compatible in their behavior. None of these implementations are
> necessarily wrong, but we just need to decide what is the allowed
> behavior for the qemu "platform" and then make sure that the
> qemu_malloc() implemetation is consistent on all underlying platforms.

No.  malloc() implements the same, well-known spec everywhere.  Some
people don't understand or don't like the spec, but that doesn't make
implementations of the spec incompatible.  Here's what you can expect
from malloc():

* After p = malloc(SIZE), you can access p[i] for 0 <= i < SIZE.  It
  follows that you must not dereference p at all if SIZE == 0.

* If malloc(SIZE) returns a null pointer, and SIZE != 0, then malloc()
  failed.

* Any result of malloc() can safely be passed to free().

A wrapper like qemu_malloc() can implement a different spec, of course.
But only if the spec is compatible, the wrapper can be used as a drop-in
replacement.  Making it gratuitously incompatible is foolish, because it
*breaks* code that works just fine with malloc().

We already agreed on one compatible difference: qemu_malloc() shall not
fail, but just terminate the program.

We're debating another difference, namely what qemu_malloc(0) shall do.
Two fine compatible options have been proposed here:

1. qemu_malloc(0) shall return a null pointer.

2. qemu_malloc(0) shall return a non-null pointer that can be passed to
   qemu_free()

The one that got committed is incompatible.

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[jcd]

----- "Kevin Wolf" <kwolf@redhat.com> a écrit :

> > So it seems to me you definitively need a way to dicriminate between
> the value returned on succesfull malloc(0) and the value returned on a
> failed malloc(1000).
> 
> I should not have mentioned the malloc(0) in this mail, it only adds
> to
> the confusion.
> 
> What I wanted to say is that a failed qemu_malloc doesn't return
> anything because it aborts (this code is in oom_check). So the caller
> needs not and even cannot check for failure.

OK, Sory, no failure case then ... My bad ...

Then it is quite important to make sure qemu_malloc is used everywhere to catch/abort any allocation error with oom_check if we don't want/expect to let the user code deal with failure.


JC

> 
> Kevin

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Kevin Wolf]

jcd@tribudubois.net schrieb:
> ----- "Kevin Wolf" <kwolf@redhat.com> a écrit :
> 
>> jcd@tribudubois.net schrieb:
>>> Kevin,
>>>
>>> I certainly understand your goal ... 
>>>
>>> Anyway, I will update my patche and resubmit.
>>>
>>> BTW it has to be noted that most of the time the return value of
>> malloc/qemu_malloc is not checked which can also prove problematic
>> whatever the qemu_malloc/malloc behavior is.
>>
>> It doesn't need to be checked, qemu_malloc never returns if it fails.
>> If
>> malloc(0) returns NULL (on success!) checking it is even wrong in
>> places
>> where this can happen.
> 
> I don't necessarily speak about the malloc(0) case.
> 
> Up to now the qemu_io code (for example) was using malloc() without checking for the returned value. If allocation fails, I believe it would be quite wrong to pass the returned (NULL) pointer to memcpy/memset/memcmp whatever platform you are considering ...
> 
> So it seems to me you definitively need a way to dicriminate between the value returned on succesfull malloc(0) and the value returned on a failed malloc(1000).

I should not have mentioned the malloc(0) in this mail, it only adds to
the confusion.

What I wanted to say is that a failed qemu_malloc doesn't return
anything because it aborts (this code is in oom_check). So the caller
needs not and even cannot check for failure.

Kevin

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[jcd]

----- "Kevin Wolf" <kwolf@redhat.com> a écrit :

> jcd@tribudubois.net schrieb:
> > Kevin,
> > 
> > I certainly understand your goal ... 
> > 
> > Anyway, I will update my patche and resubmit.
> > 
> > BTW it has to be noted that most of the time the return value of
> malloc/qemu_malloc is not checked which can also prove problematic
> whatever the qemu_malloc/malloc behavior is.
> 
> It doesn't need to be checked, qemu_malloc never returns if it fails.
> If
> malloc(0) returns NULL (on success!) checking it is even wrong in
> places
> where this can happen.

I don't necessarily speak about the malloc(0) case.

Up to now the qemu_io code (for example) was using malloc() without checking for the returned value. If allocation fails, I believe it would be quite wrong to pass the returned (NULL) pointer to memcpy/memset/memcmp whatever platform you are considering ...

So it seems to me you definitively need a way to dicriminate between the value returned on succesfull malloc(0) and the value returned on a failed malloc(1000).

So now, back to the malloc(0) case.

Granted, it would be important to have a standard behavior of malloc() on all platforms to be able to check for failure. And malloc(0) returning NULL on success (on some plateform) is not very compatible with other malloc() failure schemes. 

qemu_malloc is a nice place to unify all the malloc() behavior. Now it just has to be decided for the "qemu platform" if we allow the malloc(0) usage. 
- If yes, we have to allow it for all platforms in qemu_alloc() [using malloc(1) under the cover for example] returning a valid/non NULL pointer for all underlying platforms (even the ones that were returning NULL on success).
- If not, we can just abort() to catch these case early as it is proposed.

It seems to be just a fact that all malloc implementation are not compatible in their behavior. None of these implementations are necessarily wrong, but we just need to decide what is the allowed behavior for the qemu "platform" and then make sure that the qemu_malloc() implemetation is consistent on all underlying platforms.

> 
> Kevin

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[Kevin Wolf]

jcd@tribudubois.net schrieb:
> Kevin,
> 
> I certainly understand your goal ... 
> 
> Anyway, I will update my patche and resubmit.
> 
> BTW it has to be noted that most of the time the return value of malloc/qemu_malloc is not checked which can also prove problematic whatever the qemu_malloc/malloc behavior is.

It doesn't need to be checked, qemu_malloc never returns if it fails. If
malloc(0) returns NULL (on success!) checking it is even wrong in places
where this can happen.

Kevin

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently

[jcd]

Kevin,

I certainly understand your goal ... 

Anyway, I will update my patche and resubmit.

BTW it has to be noted that most of the time the return value of malloc/qemu_malloc is not checked which can also prove problematic whatever the qemu_malloc/malloc behavior is.

JC

----- Mail Original -----
De: "Kevin Wolf" <kwolf@redhat.com>

> I guess that if pattern_count/count is set to 0 we can just avoid the all processing of malloc/memset/memcmp/free anyway.
> 
> Would you be ok with something like:
> 
> if (Pflag && pattern_count) {
> 
> instead of: 
> 
> if (Pflag) {

To be honest, the whole point of my mail was to provoke answers like
Anthony's and get qemu_malloc fixed. ;-)

But sure, the change you propose in qemu-io is fine and will make things
work even with current qemu_malloc.

Kevin