Fix compilation with _FORTIFY_SOURCE

---
 osdep.c       |   27 +++++++++++++++++++++++++++
 qemu-common.h |    1 +
 2 files changed, 28 insertions(+), 0 deletions(-)

osdep.c +27 -0

index 1310684..09fbc99 100644
@@ -243,6 +243,33 @@ int qemu_open(const char *name, int flags, ...)
     return ret;
 }
 
+/*
+ * A variant of write(2) which handles partial write.
+ *
+ * Return the number of bytes transferred.
+ * Set errno if fewer than `count' bytes are written.
+ */
+ssize_t qemu_write_full(int fd, const void *buf, size_t count)
+{
+    ssize_t ret = 0;
+    ssize_t total = 0;
+
+    while (count) {
+        ret = write(fd, buf, count);
+        if (ret < 0) {
+            if (errno == EINTR)
+                continue;
+            break;
+        }
+
+        count -= ret;
+        buf += ret;
+        total += ret;
+    }
+
+    return total;
+}
+
 #ifndef _WIN32
 /*
  * Creates a pipe with FD_CLOEXEC set on both file descriptors

qemu-common.h +1 -0

index 8630f8c..a8144cb 100644
@@ -160,6 +160,7 @@ void qemu_mutex_lock_iothread(void);
 void qemu_mutex_unlock_iothread(void);
 
 int qemu_open(const char *name, int flags, ...);
+ssize_t qemu_write_full(int fd, const void *buf, size_t count);
 void qemu_set_cloexec(int fd);
 
 #ifndef _WIN32

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #4 lore raw

PATCH 2/17 force to test result for qemu_write_full()

---
 qemu-common.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

qemu-common.h +2 -1

index a8144cb..f009796 100644
@@ -160,7 +160,8 @@ void qemu_mutex_lock_iothread(void);
 void qemu_mutex_unlock_iothread(void);
 
 int qemu_open(const char *name, int flags, ...);
-ssize_t qemu_write_full(int fd, const void *buf, size_t count);
+ssize_t qemu_write_full(int fd, const void *buf, size_t count)
+    QEMU_WARN_UNUSED_RESULT;
 void qemu_set_cloexec(int fd);
 
 #ifndef _WIN32

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #5 lore raw

PATCH 3/17 posix-aio-compat.c: fix warning with _FORTIFY_SOURCE

From: Kirill A. Shutemov <[email protected]>

CC posix-aio-compat.o
cc1: warnings being treated as errors
posix-aio-compat.c: In function 'aio_signal_handler':
posix-aio-compat.c:505: error: ignoring return value of 'write', declared with attribute warn_unused_result
make: *** [posix-aio-compat.o] Error 1

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 posix-aio-compat.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

posix-aio-compat.c +4 -1

index dc14f53..b43c531 100644
@@ -501,8 +501,11 @@ static void aio_signal_handler(int signum)
 {
     if (posix_aio_state) {
         char byte = 0;
+        ssize_t ret;
 
-        write(posix_aio_state->wfd, &byte, sizeof(byte));
+        ret = write(posix_aio_state->wfd, &byte, sizeof(byte));
+        if (ret < 0 && errno != EAGAIN)
+            die("write()");
     }
 
     qemu_service_io();

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #6 lore raw

PATCH 4/17 block/cow.c: fix warnings with _FORTIFY_SOURCE

From: Kirill A. Shutemov <[email protected]>

CC block/cow.o
cc1: warnings being treated as errors
block/cow.c: In function 'cow_create':
block/cow.c:251: error: ignoring return value of 'write', declared with attribute warn_unused_result block/cow.c:253: error: ignoring return value of 'ftruncate', declared with attribute warn_unused_result
make: *** [block/cow.o] Error 1

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 block/cow.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

block/cow.c +16 -3

index a70854e..3733385 100644
@@ -209,6 +209,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
     struct stat st;
     int64_t image_sectors = 0;
     const char *image_filename = NULL;
+    int ret;
 
     /* Read out options */
     while (options && options->name) {

@@ -248,11 +249,23 @@ static int cow_create(const char *filename, QEMUOptionParameter *options)
     }
     cow_header.sectorsize = cpu_to_be32(512);
     cow_header.size = cpu_to_be64(image_sectors * 512);
-    write(cow_fd, &cow_header, sizeof(cow_header));
+    ret = qemu_write_full(cow_fd, &cow_header, sizeof(cow_header));
+    if (ret != sizeof(cow_header)) {
+        ret = -1;
+        goto exit;
+    }
+
     /* resize to include at least all the bitmap */
-    ftruncate(cow_fd, sizeof(cow_header) + ((image_sectors + 7) >> 3));
+    ret = ftruncate(cow_fd, sizeof(cow_header) + ((image_sectors + 7) >> 3));
+    if (ret) {
+        ret = -errno;
+        goto exit;
+    }
+
+    ret = 0;
+exit:
     close(cow_fd);
-    return 0;
+    return ret;
 }
 
 static void cow_flush(BlockDriverState *bs)

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #7 lore raw

PATCH 5/17 block/qcow.c: fix warnings with _FORTIFY_SOURCE

From: Kirill A. Shutemov <[email protected]>

CC block/qcow.o
cc1: warnings being treated as errors
block/qcow.c: In function 'qcow_create':
block/qcow.c:804: error: ignoring return value of 'write', declared with attribute warn_unused_result block/qcow.c:806: error: ignoring return value of 'write', declared with attribute warn_unused_result block/qcow.c:811: error: ignoring return value of 'write', declared with attribute warn_unused_result
make: *** [block/qcow.o] Error 1

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 block/qcow.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

block/qcow.c +22 -4

index 1e3e59b..003db1e 100644
@@ -750,6 +750,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
     int64_t total_size = 0;
     const char *backing_file = NULL;
     int flags = 0;
+    int ret;
 
     /* Read out options */
     while (options && options->name) {

@@ -801,17 +802,34 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
     }
 
     /* write all the data */
-    write(fd, &header, sizeof(header));
+    ret = qemu_write_full(fd, &header, sizeof(header));
+    if (ret != sizeof(header)) {
+        ret = -1;
+        goto exit;
+    }
+
     if (backing_file) {
-        write(fd, backing_file, backing_filename_len);
+        ret = qemu_write_full(fd, backing_file, backing_filename_len);
+        if (ret != backing_filename_len) {
+            ret = -1;
+            goto exit;
+        }
+
     }
     lseek(fd, header_size, SEEK_SET);
     tmp = 0;
     for(i = 0;i < l1_size; i++) {
-        write(fd, &tmp, sizeof(tmp));
+        ret = qemu_write_full(fd, &tmp, sizeof(tmp));
+        if (ret != sizeof(tmp)) {
+            ret = -1;
+            goto exit;
+        }
     }
+
+    ret = 0;
+exit:
     close(fd);
-    return 0;
+    return ret;
 }
 
 static int qcow_make_empty(BlockDriverState *bs)

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #8 lore raw

PATCH 6/17 block/vmdk.o: fix warnings with _FORTIFY_SOURCE

From: Kirill A. Shutemov <[email protected]>

CC block/vmdk.o
cc1: warnings being treated as errors
block/vmdk.c: In function 'vmdk_snapshot_create':
block/vmdk.c:236: error: ignoring return value of 'ftruncate', declared with attribute warn_unused_result block/vmdk.c: In function 'vmdk_create':
block/vmdk.c:775: error: ignoring return value of 'write', declared with attribute warn_unused_result block/vmdk.c:776: error: ignoring return value of 'write', declared with attribute warn_unused_result block/vmdk.c:778: error: ignoring return value of 'ftruncate', declared with attribute warn_unused_result block/vmdk.c:784: error: ignoring return value of 'write', declared with attribute warn_unused_result block/vmdk.c:790: error: ignoring return value of 'write', declared with attribute warn_unused_result block/vmdk.c:807: error: ignoring return value of 'write', declared with attribute warn_unused_result
make: *** [block/vmdk.o] Error 1

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 block/vmdk.c |   50 ++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 40 insertions(+), 10 deletions(-)

block/vmdk.c +40 ~~-10~~

index 4e48622..18c691a 100644
@@ -233,7 +233,8 @@ static int vmdk_snapshot_create(const char *filename, const char *backing_file)
     memset(&header, 0, sizeof(header));
     memcpy(&header,&hdr[4], sizeof(header)); // skip the VMDK4_MAGIC
 
-    ftruncate(snp_fd, header.grain_offset << 9);
+    if (ftruncate(snp_fd, header.grain_offset << 9))
+        goto fail;
     /* the descriptor offset = 0x200 */
     if (lseek(p_fd, 0x200, SEEK_SET) == -1)
         goto fail;

@@ -716,6 +717,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
     int64_t total_size = 0;
     const char *backing_file = NULL;
     int flags = 0;
+    int ret;
 
     // Read out options
     while (options && options->name) {

@@ -772,22 +774,44 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
     header.check_bytes[3] = 0xa;
 
     /* write all the data */
-    write(fd, &magic, sizeof(magic));
-    write(fd, &header, sizeof(header));
+    ret = qemu_write_full(fd, &magic, sizeof(magic));
+    if (ret != sizeof(magic)) {
+        ret = -1;
+        goto exit;
+    }
+    ret = qemu_write_full(fd, &header, sizeof(header));
+    if (ret != sizeof(header)) {
+        ret = -1;
+        goto exit;
+    }
 
-    ftruncate(fd, header.grain_offset << 9);
+    ret = ftruncate(fd, header.grain_offset << 9);
+    if (ret < 0) {
+        ret = -1;
+        goto exit;
+    }
 
     /* write grain directory */
     lseek(fd, le64_to_cpu(header.rgd_offset) << 9, SEEK_SET);
     for (i = 0, tmp = header.rgd_offset + gd_size;
-         i < gt_count; i++, tmp += gt_size)
-        write(fd, &tmp, sizeof(tmp));
+         i < gt_count; i++, tmp += gt_size) {
+        ret = qemu_write_full(fd, &tmp, sizeof(tmp));
+        if (ret != sizeof(tmp)) {
+            ret = -1;
+            goto exit;
+        }
+    }
 
     /* write backup grain directory */
     lseek(fd, le64_to_cpu(header.gd_offset) << 9, SEEK_SET);
     for (i = 0, tmp = header.gd_offset + gd_size;
-         i < gt_count; i++, tmp += gt_size)
-        write(fd, &tmp, sizeof(tmp));
+         i < gt_count; i++, tmp += gt_size) {
+        ret = qemu_write_full(fd, &tmp, sizeof(tmp));
+        if (ret != sizeof(tmp)) {
+            ret = -1;
+            goto exit;
+        }
+    }
 
     /* compose the descriptor */
     real_filename = filename;

@@ -804,10 +828,16 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
 
     /* write the descriptor */
     lseek(fd, le64_to_cpu(header.desc_offset) << 9, SEEK_SET);
-    write(fd, desc, strlen(desc));
+    ret = qemu_write_full(fd, desc, strlen(desc));
+    if (ret != strlen(desc)) {
+        ret = -1;
+        goto exit;
+    }
 
+    ret = 0;
+exit:
     close(fd);
-    return 0;
+    return ret;
 }
 
 static void vmdk_close(BlockDriverState *bs)

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #9 lore raw

PATCH 7/17 block/vvfat.c: fix warnings with _FORTIFY_SOURCE

From: Kirill A. Shutemov <[email protected]>

CC block/vvfat.o
cc1: warnings being treated as errors
block/vvfat.c: In function 'commit_one_file':
block/vvfat.c:2259: error: ignoring return value of 'ftruncate', declared with attribute warn_unused_result
make: *** [block/vvfat.o] Error 1
CC block/vvfat.o
In file included from /usr/include/stdio.h:912,

                 from ./qemu-common.h:19,
                 from block/vvfat.c:27:

In function 'snprintf',
inlined from 'init_directories' at block/vvfat.c:871,
inlined from 'vvfat_open' at block/vvfat.c:1068:
/usr/include/bits/stdio2.h:65: error: call to __builtin___snprintf_chk will always overflow destination buffer
make: *** [block/vvfat.o] Error 1

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 block/vvfat.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

block/vvfat.c +7 -2

index 063f731..df957e5 100644
@@ -868,7 +868,8 @@ static int init_directories(BDRVVVFATState* s,
     {
 	direntry_t* entry=array_get_next(&(s->directory));
 	entry->attributes=0x28; /* archive | volume label */
-	snprintf((char*)entry->name,11,"QEMU VVFAT");
+	memcpy(entry->name,"QEMU VVF",8);
+	memcpy(entry->extension,"AT ",3);
     }
 
     /* Now build FAT, and write back information into directory */

@@ -2256,7 +2257,11 @@ static int commit_one_file(BDRVVVFATState* s,
 	c = c1;
     }
 
-    ftruncate(fd, size);
+    if (ftruncate(fd, size)) {
+        perror("ftruncate()");
+        close(fd);
+        return -4;
+    }
     close(fd);
 
     return commit_mappings(s, first_cluster, dir_index);

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #25 lore raw

PATCH 8/17 block/qcow2.c: fix warnings with _FORTIFY_SOURCE

From: Kirill A. Shutemov <[email protected]>

CC block/qcow2.o
cc1: warnings being treated as errors
block/qcow2.c: In function 'qcow_create2':
block/qcow2.c:829: error: ignoring return value of 'write', declared with attribute warn_unused_result block/qcow2.c:838: error: ignoring return value of 'write', declared with attribute warn_unused_result block/qcow2.c:839: error: ignoring return value of 'write', declared with attribute warn_unused_result block/qcow2.c:841: error: ignoring return value of 'write', declared with attribute warn_unused_result block/qcow2.c:844: error: ignoring return value of 'write', declared with attribute warn_unused_result block/qcow2.c:849: error: ignoring return value of 'write', declared with attribute warn_unused_result block/qcow2.c:852: error: ignoring return value of 'write', declared with attribute warn_unused_result block/qcow2.c:855: error: ignoring return value of 'write', declared with attribute warn_unused_result
make: *** [block/qcow2.o] Error 1

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 block/qcow2.c |   55 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 45 insertions(+), 10 deletions(-)

block/qcow2.c +45 ~~-10~~

index 6622eba..1bf94c5 100644
@@ -842,7 +842,7 @@ static int qcow_create2(const char *filename, int64_t total_size,
     uint64_t tmp, offset;
     QCowCreateState s1, *s = &s1;
     QCowExtension ext_bf = {0, 0};
-
+    int ret;
 
     memset(s, 0, sizeof(*s));

@@ -925,7 +925,11 @@ static int qcow_create2(const char *filename, int64_t total_size,
         ref_clusters * s->cluster_size);
 
     /* write all the data */
-    write(fd, &header, sizeof(header));
+    ret = qemu_write_full(fd, &header, sizeof(header));
+    if (ret != sizeof(header)) {
+        ret = -1;
+        goto exit;
+    }
     if (backing_file) {
         if (backing_format_len) {
             char zero[16];

@@ -934,25 +938,56 @@ static int qcow_create2(const char *filename, int64_t total_size,
             memset(zero, 0, sizeof(zero));
             cpu_to_be32s(&ext_bf.magic);
             cpu_to_be32s(&ext_bf.len);
-            write(fd, &ext_bf, sizeof(ext_bf));
-            write(fd, backing_format, backing_format_len);
+            ret = qemu_write_full(fd, &ext_bf, sizeof(ext_bf));
+            if (ret != sizeof(ext_bf)) {
+                ret = -1;
+                goto exit;
+            }
+            ret = qemu_write_full(fd, backing_format, backing_format_len);
+            if (ret != backing_format_len) {
+                ret = -1;
+                goto exit;
+            }
             if (padding > 0) {
-                write(fd, zero, padding);
+                ret = qemu_write_full(fd, zero, padding);
+                if (ret != padding) {
+                    ret = -1;
+                    goto exit;
+                }
             }
         }
-        write(fd, backing_file, backing_filename_len);
+        ret = qemu_write_full(fd, backing_file, backing_filename_len);
+        if (ret != backing_filename_len) {
+            ret = -1;
+            goto exit;
+        }
     }
     lseek(fd, s->l1_table_offset, SEEK_SET);
     tmp = 0;
     for(i = 0;i < l1_size; i++) {
-        write(fd, &tmp, sizeof(tmp));
+        ret = qemu_write_full(fd, &tmp, sizeof(tmp));
+        if (ret != sizeof(tmp)) {
+            ret = -1;
+            goto exit;
+        }
     }
     lseek(fd, s->refcount_table_offset, SEEK_SET);
-    write(fd, s->refcount_table, s->cluster_size);
+    ret = qemu_write_full(fd, s->refcount_table, s->cluster_size);
+    if (ret != s->cluster_size) {
+        ret = -1;
+        goto exit;
+    }
 
     lseek(fd, s->refcount_block_offset, SEEK_SET);
-    write(fd, s->refcount_block, ref_clusters * s->cluster_size);
+    ret = qemu_write_full(fd, s->refcount_block,
+		    ref_clusters * s->cluster_size);
+    if (ret != s->cluster_size) {
+        ret = -1;
+        goto exit;
+    }
 
+    ret = 0;
+exit:
     qemu_free(s->refcount_table);
     qemu_free(s->refcount_block);
     close(fd);

@@ -966,7 +1001,7 @@ static int qcow_create2(const char *filename, int64_t total_size,
         bdrv_close(bs);
     }
 
-    return 0;
+    return ret;
 }
 
 static int qcow_create(const char *filename, QEMUOptionParameter *options)

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #26 lore raw

PATCH 9/17 net/slirp.c: fix warning with _FORTIFY_SOURCE

From: Kirill A. Shutemov <[email protected]>

CC net/slirp.o
cc1: warnings being treated as errors
net/slirp.c: In function 'slirp_smb_cleanup':
net/slirp.c:470: error: ignoring return value of 'system', declared with attribute warn_unused_result
make: *** [net/slirp.o] Error 1

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 net/slirp.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

net/slirp.c +8 -1

index 3f91c4b..b75ad16 100644
@@ -464,10 +464,17 @@ int net_slirp_redir(const char *redir_str)
 static void slirp_smb_cleanup(SlirpState *s)
 {
     char cmd[128];
+    int ret;
 
     if (s->smb_dir[0] != '\0') {
         snprintf(cmd, sizeof(cmd), "rm -rf %s", s->smb_dir);
-        system(cmd);
+        ret = system(cmd);
+        if (!WIFEXITED(ret)) {
+            qemu_error("'%s' failed.\n", cmd);
+        } else if (WEXITSTATUS(ret)) {
+            qemu_error("'%s' failed. Error code: %d\n",
+                    cmd, WEXITSTATUS(ret));
+        }
         s->smb_dir[0] = '\0';
     }
 }

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #27 lore raw

PATCH 10/17 usb-linux.c: fix warning with _FORTIFY_SOURCE

From: Kirill A. Shutemov <[email protected]>

CC usb-linux.o
cc1: warnings being treated as errors
usb-linux.c: In function 'usb_host_read_file':
usb-linux.c:1204: error: ignoring return value of 'fgets', declared with attribute warn_unused_result
make: *** [usb-linux.o] Error 1

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 usb-linux.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

usb-linux.c +1 -2

index 88728e9..be1d979 100644
@@ -1201,9 +1201,8 @@ static int usb_host_read_file(char *line, size_t line_size, const char *device_f
              device_file);
     f = fopen(filename, "r");
     if (f) {
-        fgets(line, line_size, f);
+        ret = fgets(line, line_size, f) != NULL;
         fclose(f);
-        ret = 1;
 #if 0
     } else {
         if (mon)

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #28 lore raw

PATCH 11/17 vl.c: fix warning with _FORTIFY_SOURCE

From: Kirill A. Shutemov <[email protected]>

CC i386-softmmu/vl.o
cc1: warnings being treated as errors
/usr/src/RPM/BUILD/qemu-0.11.92/vl.c: In function 'qemu_event_increment':
/usr/src/RPM/BUILD/qemu-0.11.92/vl.c:3404: error: ignoring return value of 'write', declared with attribute warn_unused_result /usr/src/RPM/BUILD/qemu-0.11.92/vl.c: In function 'main':
/usr/src/RPM/BUILD/qemu-0.11.92/vl.c:5774: error: ignoring return value of 'write', declared with attribute warn_unused_result /usr/src/RPM/BUILD/qemu-0.11.92/vl.c:6064: error: ignoring return value of 'chdir', declared with attribute warn_unused_result /usr/src/RPM/BUILD/qemu-0.11.92/vl.c:6083: error: ignoring return value of 'chdir', declared with attribute warn_unused_result make[1]: *** [vl.o] Error 1

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 vl.c |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)

vl.c +18 -4

index 06cb40d..7c29179 100644
@@ -3176,11 +3176,17 @@ static int io_thread_fd = -1;
 static void qemu_event_increment(void)
 {
     static const char byte = 0;
+    ssize_t ret;
 
     if (io_thread_fd == -1)
         return;
 
-    write(io_thread_fd, &byte, sizeof(byte));
+    ret = write(io_thread_fd, &byte, sizeof(byte));
+    if (ret < 0 && (errno != EINTR && errno != EAGAIN)) {
+        fprintf(stderr, "qemu_event_increment: write() filed: %s\n",
+                strerror(errno));
+        exit (1);
+    }
 }
 
 static void qemu_event_read(void *opaque)

@@ -5585,7 +5591,9 @@ int main(int argc, char **argv, char **envp)
 #ifndef _WIN32
         if (daemonize) {
             uint8_t status = 1;
-            write(fds[1], &status, 1);
+            if (write(fds[1], &status, 1) != 1) {
+                perror("daemonize. Writing to pipe\n");
+            }
         } else
 #endif
             fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));

@@ -5884,7 +5892,10 @@ int main(int argc, char **argv, char **envp)
 	if (len != 1)
 	    exit(1);
 
-	chdir("/");
+        if (chdir("/")) {
+            perror("not able to chdir to /");
+            exit(1);
+        }
 	TFR(fd = qemu_open("/dev/null", O_RDWR));
 	if (fd == -1)
 	    exit(1);

@@ -5903,7 +5914,10 @@ int main(int argc, char **argv, char **envp)
             fprintf(stderr, "chroot failed\n");
             exit(1);
         }
-        chdir("/");
+        if (chdir("/")) {
+            perror("not able to chdir to /");
+            exit(1);
+        }
     }
 
     if (run_as) {

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #29 lore raw

PATCH 12/17 monitor.c: fix warnings with _FORTIFY_SOURCE

From: Kirill A. Shutemov <[email protected]>

CC i386-softmmu/monitor.o
cc1: warnings being treated as errors
/usr/src/RPM/BUILD/qemu-0.11.92/monitor.c: In function 'do_memory_save':
/usr/src/RPM/BUILD/qemu-0.11.92/monitor.c:1318: error: ignoring return value of 'fwrite', declared with attribute warn_unused_result /usr/src/RPM/BUILD/qemu-0.11.92/monitor.c: In function 'do_physical_memory_save':
/usr/src/RPM/BUILD/qemu-0.11.92/monitor.c:1345: error: ignoring return value of 'fwrite', declared with attribute warn_unused_result make[1]: *** [monitor.o] Error 1

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 monitor.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

monitor.c +10 -2

index b824e7c..2a20ad7 100644
@@ -1320,10 +1320,14 @@ static void do_memory_save(Monitor *mon, const QDict *qdict, QObject **ret_data)
         if (l > size)
             l = size;
         cpu_memory_rw_debug(env, addr, buf, l, 0);
-        fwrite(buf, 1, l, f);
+        if (fwrite(buf, 1, l, f) != l) {
+            monitor_printf(mon, "fwrite() error in do_memory_save\n");
+            goto exit;
+        }
         addr += l;
         size -= l;
     }
+exit:
     fclose(f);
 }

@@ -1347,11 +1351,15 @@ static void do_physical_memory_save(Monitor *mon, const QDict *qdict,
         if (l > size)
             l = size;
         cpu_physical_memory_rw(addr, buf, l, 0);
-        fwrite(buf, 1, l, f);
+        if (fwrite(buf, 1, l, f) != l) {
+            monitor_printf(mon, "fwrite() error in do_physical_memory_save\n");
+            goto exit;
+        }
         fflush(f);
         addr += l;
         size -= l;
     }
+exit:
     fclose(f);
 }

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #30 lore raw

PATCH 13/17 linux-user/mmap.c: fix warnings with _FORTIFY_SOURCE

From: Kirill A. Shutemov <[email protected]>

CC i386-linux-user/mmap.o
cc1: warnings being treated as errors
/usr/src/RPM/BUILD/qemu-0.11.92/linux-user/mmap.c: In function 'mmap_frag':
/usr/src/RPM/BUILD/qemu-0.11.92/linux-user/mmap.c:253: error: ignoring return value of 'pread', declared with attribute warn_unused_result /usr/src/RPM/BUILD/qemu-0.11.92/linux-user/mmap.c: In function 'target_mmap':
/usr/src/RPM/BUILD/qemu-0.11.92/linux-user/mmap.c:477: error: ignoring return value of 'pread', declared with attribute warn_unused_result make[1]: *** [mmap.o] Error 1

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 linux-user/mmap.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

linux-user/mmap.c +4 -2

index 144fb7c..c1c7e48 100644
@@ -250,7 +250,8 @@ static int mmap_frag(abi_ulong real_start,
             mprotect(host_start, qemu_host_page_size, prot1 | PROT_WRITE);
 
         /* read the corresponding file data */
-        pread(fd, g2h(start), end - start, offset);
+        if (pread(fd, g2h(start), end - start, offset) == -1)
+            return -1;
 
         /* put final protection */
         if (prot_new != (prot1 | PROT_WRITE))

@@ -474,7 +475,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
                                   -1, 0);
             if (retaddr == -1)
                 goto fail;
-            pread(fd, g2h(start), len, offset);
+            if (pread(fd, g2h(start), len, offset) == -1)
+                goto fail;
             if (!(prot & PROT_WRITE)) {
                 ret = target_mprotect(start, len, prot);
                 if (ret != 0) {

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:56 UTC #31 lore raw

PATCH 14/17 check pipe() return value

---
 hw/xen_domainbuild.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

hw/xen_domainbuild.c +9 -4

index 20d731d..2f59856 100644
@@ -156,15 +156,18 @@ quit:
     return;
 }
 
-static void xen_domain_watcher(void)
+static int xen_domain_watcher(void)
 {
     int qemu_running = 1;
     int fd[2], i, n, rc;
     char byte;
 
-    pipe(fd);
+    if (pipe(fd) != 0) {
+        qemu_log("%s: Huh? pipe error: %s\n", __FUNCTION__, strerror(errno));
+        return -1;
+    }
     if (fork() != 0)
-        return; /* not child */
+        return 0; /* not child */
 
     /* close all file handles, except stdio/out/err,
      * our watch pipe and the xen interface handle */

@@ -238,7 +241,9 @@ int xen_domain_build_pv(const char *kernel, const char *ramdisk,
     }
     qemu_log("xen: created domain %d\n", xen_domid);
     atexit(xen_domain_cleanup);
-    xen_domain_watcher();
+    if (xen_domain_watcher() == -1) {
+        goto err;
+    }
 
     xenstore_domain_init1(kernel, ramdisk, cmdline);

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:57 UTC #32 lore raw

PATCH 15/17 Enable _FORTIFY_SOURCE=2

From: Kirill A. Shutemov <[email protected]>

_FORTIFY_SOURCE is a Glibc feature which adds memory and string function protection.

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 configure |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

configure +1 -1

index 5631bbb..5556b9d 100755
@@ -97,7 +97,7 @@ CFLAGS="-g $CFLAGS"
 QEMU_CFLAGS="-Wall -Wundef -Wendif-labels -Wwrite-strings -Wmissing-prototypes $QEMU_CFLAGS"
 QEMU_CFLAGS="-Wstrict-prototypes -Wredundant-decls $QEMU_CFLAGS"
 QEMU_CFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE $QEMU_CFLAGS"
-QEMU_CFLAGS="-U_FORTIFY_SOURCE $QEMU_CFLAGS"
+QEMU_CFLAGS="-D_FORTIFY_SOURCE=2 $QEMU_CFLAGS"
 QEMU_CFLAGS="-I. -I\$(SRC_PATH) $QEMU_CFLAGS"
 LDFLAGS="-g $LDFLAGS"

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:57 UTC #33 lore raw

PATCH 16/17 Check availavility of -fstack-protector-all

---
 configure |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

configure +1 -1

index 5556b9d..d8af978 100755
@@ -101,7 +101,7 @@ QEMU_CFLAGS="-D_FORTIFY_SOURCE=2 $QEMU_CFLAGS"
 QEMU_CFLAGS="-I. -I\$(SRC_PATH) $QEMU_CFLAGS"
 LDFLAGS="-g $LDFLAGS"
 
-gcc_flags="-Wold-style-declaration -Wold-style-definition"
+gcc_flags="-Wold-style-declaration -Wold-style-definition -fstack-protector-all"
 cat > $TMPC << EOF
 int main(void) { }
 EOF

Signature · 1 line

1.6.5.2

Juan Quintela

author

2010-01-19 23:57 UTC #34 lore raw

PATCH 17/17 mmap_frag() users only check for -1 error

---
 linux-user/mmap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

linux-user/mmap.c +1 -1

index c1c7e48..25fc0b2 100644
@@ -243,7 +243,7 @@ static int mmap_frag(abi_ulong real_start,
            possible while it is a shared mapping */
         if ((flags & MAP_TYPE) == MAP_SHARED &&
             (prot & PROT_WRITE))
-            return -EINVAL;
+            return -1;
 
         /* adjust protection to be able to read */
         if (!(prot1 & PROT_WRITE))

Signature · 1 line

1.6.5.2

malc

2010-01-20 00:46 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 01/17] Introduce qemu_write_full()

Quoted 30 lines · Juan Quintela

From: Kirill A. Shutemov <[email protected]>

A variant of write(2) which handles partial write.

Signed-off-by Kirill A. Shutemov <[email protected]>

---
 osdep.c       |   27 +++++++++++++++++++++++++++
 qemu-common.h |    1 +
 2 files changed, 28 insertions(+), 0 deletions(-)

osdep.c +11 -0

index 1310684..09fbc99 100644
@@ -243,6 +243,33 @@ int qemu_open(const char *name, int flags, ...)
     return ret;
 }
 
+/*
+ * A variant of write(2) which handles partial write.
+ *
+ * Return the number of bytes transferred.
+ * Set errno if fewer than `count' bytes are written.
+ */
+ssize_t qemu_write_full(int fd, const void *buf, size_t count)
+{
+    ssize_t ret = 0;
+    ssize_t total = 0;
+

osdep.c int qemu_open(const char *name, int flags, ...) · line 266

Quoted 10 lines

osdep.c +10 -0

+    while (count) {
+        ret = write(fd, buf, count);
+        if (ret < 0) {
+            if (errno == EINTR)
+                continue;
+            break;
+        }
+
+        count -= ret;
+        buf += ret;

Constraint violation (6.5.6)

Quoted 22 lines

osdep.c +6 -0

+        total += ret;
+    }
+
+    return total;
+}
+
 #ifndef _WIN32
 /*
  * Creates a pipe with FD_CLOEXEC set on both file descriptors

qemu-common.h +1 -0

index 8630f8c..a8144cb 100644
@@ -160,6 +160,7 @@ void qemu_mutex_lock_iothread(void);
 void qemu_mutex_unlock_iothread(void);
 
 int qemu_open(const char *name, int flags, ...);
+ssize_t qemu_write_full(int fd, const void *buf, size_t count);
 void qemu_set_cloexec(int fd);
 
 #ifndef _WIN32

Signature · 1 line

mailto:[email protected]

Kirill A. Shutemov

2010-01-20 06:19 UTC #10 lore raw

[Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

In reply to Markus Armbruster

Quoted 30 lines · Juan Quintela

From: Kirill A. Shutemov <[email protected]>
CC block/vvfat.o
cc1: warnings being treated as errors
block/vvfat.c: In function 'commit_one_file':
block/vvfat.c:2259: error: ignoring return value of 'ftruncate', declared with attribute warn_unused_result
make: *** [block/vvfat.o] Error 1
CC block/vvfat.o
In file included from /usr/include/stdio.h:912,
from ./qemu-common.h:19,
from block/vvfat.c:27:
In function 'snprintf',
inlined from 'init_directories' at block/vvfat.c:871,
inlined from 'vvfat_open' at block/vvfat.c:1068:
/usr/include/bits/stdio2.h:65: error: call to __builtin___snprintf_chk will always overflow destination buffer
make: *** [block/vvfat.o] Error 1
Signed-off-by Kirill A. Shutemov <[email protected]>
Signed-off-by Juan Quintela <[email protected]>
---
block/vvfat.c | 9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
block/vvfat.c +0 -0
index 063f731..df957e5 100644
@@ -868,7 +868,8 @@ static int init_directories(BDRVVVFATState* s,
     {
        direntry_t* entry=array_get_next(&(s->directory));

Quoted 5 lines

entry->attributes=0x28; /* archive | volume label */
- snprintf((char*)entry->name,11,"QEMU VVFAT");
+ memcpy(entry->name,"QEMU VVF",8);
+ memcpy(entry->extension,"AT ",3);
}

Better to use

memcpy(entry->name, "QEMU VVFAT", 11);

memcpy() doesn't check bounds.

Quoted 18 lines

/* Now build FAT, and write back information into directory */

block/vvfat.c +5 -2

@@ -2256,7 +2257,11 @@ static int commit_one_file(BDRVVVFATState* s,
        c = c1;
     }
 
-    ftruncate(fd, size);
+    if (ftruncate(fd, size)) {
+        perror("ftruncate()");
+        close(fd);
+        return -4;
+    }
     close(fd);
 
     return commit_mappings(s, first_cluster, dir_index);
--

1.6.5.2

Daniel P. Berrange

2010-01-20 10:33 UTC #11 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Quoted 39 lines · Kirill A. Shutemov

Quoted 30 lines · Juan Quintela
From: Kirill A. Shutemov <[email protected]>
CC block/vvfat.o
cc1: warnings being treated as errors
block/vvfat.c: In function 'commit_one_file':
block/vvfat.c:2259: error: ignoring return value of 'ftruncate', declared with attribute warn_unused_result
make: *** [block/vvfat.o] Error 1
CC block/vvfat.o
In file included from /usr/include/stdio.h:912,
from ./qemu-common.h:19,
from block/vvfat.c:27:
In function 'snprintf',
inlined from 'init_directories' at block/vvfat.c:871,
inlined from 'vvfat_open' at block/vvfat.c:1068:
/usr/include/bits/stdio2.h:65: error: call to __builtin___snprintf_chk will always overflow destination buffer
make: *** [block/vvfat.o] Error 1
Signed-off-by Kirill A. Shutemov <[email protected]>
Signed-off-by Juan Quintela <[email protected]>
---
block/vvfat.c | 9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
block/vvfat.c +0 -0
index 063f731..df957e5 100644
@@ -868,7 +868,8 @@ static int init_directories(BDRVVVFATState* s,
     {
        direntry_t* entry=array_get_next(&(s->directory));
Quoted 5 lines
entry->attributes=0x28; /* archive | volume label */
- snprintf((char*)entry->name,11,"QEMU VVFAT");
+ memcpy(entry->name,"QEMU VVF",8);
+ memcpy(entry->extension,"AT ",3);
}
Better to use

Quoted 3 lines

memcpy(entry->name, "QEMU VVFAT", 11);
memcpy() doesn't check bounds.

It doesn't *currently* check bounds. If we want to explicitly fill 2 fields at once, then we should redeclare this to have a union with one part comprising the entire buffer, thus avoiding the need for delibrate buffer overruns.

Regards,
Daniel

Signature · 4 lines

|: 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 :|

Kirill A. Shutemov

2010-01-20 11:09 UTC #12 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Quoted 40 lines · Daniel P. Berrange

Quoted 39 lines · Kirill A. Shutemov
Quoted 30 lines · Juan Quintela
From: Kirill A. Shutemov <[email protected]>
CC block/vvfat.o
cc1: warnings being treated as errors
block/vvfat.c: In function 'commit_one_file':
block/vvfat.c:2259: error: ignoring return value of 'ftruncate', declared with attribute warn_unused_result
make: *** [block/vvfat.o] Error 1
CC block/vvfat.o
In file included from /usr/include/stdio.h:912,
from ./qemu-common.h:19,
from block/vvfat.c:27:
In function 'snprintf',
inlined from 'init_directories' at block/vvfat.c:871,
inlined from 'vvfat_open' at block/vvfat.c:1068:
/usr/include/bits/stdio2.h:65: error: call to __builtin___snprintf_chk will always overflow destination buffer
make: *** [block/vvfat.o] Error 1
Signed-off-by Kirill A. Shutemov <[email protected]>
Signed-off-by Juan Quintela <[email protected]>
---
block/vvfat.c | 9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
block/vvfat.c +0 -0
index 063f731..df957e5 100644
@@ -868,7 +868,8 @@ static int init_directories(BDRVVVFATState* s,
     {
        direntry_t* entry=array_get_next(&(s->directory));
Quoted 5 lines
entry->attributes=0x28; /* archive | volume label */
- snprintf((char*)entry->name,11,"QEMU VVFAT");
+ memcpy(entry->name,"QEMU VVF",8);
+ memcpy(entry->extension,"AT ",3);
}
Better to use

Quoted 5 lines

Quoted 3 lines
memcpy(entry->name, "QEMU VVFAT", 11);
memcpy() doesn't check bounds.
It doesn't *currently* check bounds.

No. memcpy() will never check bounds. It's totaly different from strcpy,
http://gcc.gnu.org/ml/gcc-patches/2009-06/msg00419.html

Quoted 13 lines

If we want to explicitly
fill 2 fields at once, then we should redeclare this to have a union with one part comprising the entire buffer, thus avoiding the need for delibrate buffer overruns.
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 :|

Kevin Wolf

2010-01-20 11:46 UTC #13 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Am 20.01.2010 12:09, schrieb Kirill A. Shutemov:

Quoted 42 lines

Quoted 40 lines · Daniel P. Berrange
Quoted 39 lines · Kirill A. Shutemov
Quoted 28 lines · Juan Quintela
From: Kirill A. Shutemov <[email protected]>
CC block/vvfat.o
cc1: warnings being treated as errors
block/vvfat.c: In function 'commit_one_file':
block/vvfat.c:2259: error: ignoring return value of 'ftruncate', declared with attribute warn_unused_result
make: *** [block/vvfat.o] Error 1
CC block/vvfat.o
In file included from /usr/include/stdio.h:912,
                from ./qemu-common.h:19,
                from block/vvfat.c:27:
In function 'snprintf',
inlined from 'init_directories' at block/vvfat.c:871,
inlined from 'vvfat_open' at block/vvfat.c:1068:
/usr/include/bits/stdio2.h:65: error: call to __builtin___snprintf_chk will always overflow destination buffer
make: *** [block/vvfat.o] Error 1
Signed-off-by Kirill A. Shutemov <[email protected]>
Signed-off-by Juan Quintela <[email protected]>
---
 block/vvfat.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
block/vvfat.c +0 -0
index 063f731..df957e5 100644
block/vvfat.c static int init_directories(BDRVVVFATState* s, · line 873
Quoted 7 lines
block/vvfat.c +2 -1
@@ -868,7 +868,8 @@ static int init_directories(BDRVVVFATState* s,
    {
       direntry_t* entry=array_get_next(&(s->directory));
       entry->attributes=0x28; /* archive | volume label */
-       snprintf((char*)entry->name,11,"QEMU VVFAT");
+       memcpy(entry->name,"QEMU VVF",8);
+       memcpy(entry->extension,"AT ",3);
    }
Better to use

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
memcpy(entry->name, "QEMU VVFAT", 11);
memcpy() doesn't check bounds.
It doesn't *currently* check bounds.
No. memcpy() will never check bounds. It's totaly different from strcpy,
http://gcc.gnu.org/ml/gcc-patches/2009-06/msg00419.html

Regardless if deliberately overflowing the buffer works or doesn't making it explicit is better. Someone might reorder the struct or add new fields in between (okay, unlikely in this case, but still) and you'll overflow into fields you never wanted to touch.

Kevin

Markus Armbruster

2010-01-20 12:15 UTC #14 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Quoted 5 lines · Kevin Wolf

Am 20.01.2010 12:09, schrieb Kirill A. Shutemov:
Quoted 4 lines
Quoted 2 lines · Daniel P. Berrange
Quoted 1 line · Kirill A. Shutemov
On Wed, Jan 20, 2010 at 1:56 AM, Juan Quintela <[email protected]> wrote:

[...]

Quoted 15 lines

block/vvfat.c static int init_directories(BDRVVVFATState* s, · line 873

Quoted 12 lines

block/vvfat.c +2 -1

index 063f731..df957e5 100644
@@ -868,7 +868,8 @@ static int init_directories(BDRVVVFATState* s,
    {
       direntry_t* entry=array_get_next(&(s->directory));
       entry->attributes=0x28; /* archive | volume label */
-       snprintf((char*)entry->name,11,"QEMU VVFAT");
+       memcpy(entry->name,"QEMU VVF",8);
+       memcpy(entry->extension,"AT ",3);
    }

Better to use

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
memcpy(entry->name, "QEMU VVFAT", 11);
memcpy() doesn't check bounds.

No, this is evil, and may well be flagged by static analysis tools.

Quoted 9 lines

Quoted 4 lines
Quoted 1 line
It doesn't *currently* check bounds.
No. memcpy() will never check bounds. It's totaly different from strcpy,
http://gcc.gnu.org/ml/gcc-patches/2009-06/msg00419.html
Regardless if deliberately overflowing the buffer works or doesn't making it explicit is better. Someone might reorder the struct or add new fields in between (okay, unlikely in this case, but still) and you'll overflow into fields you never wanted to touch.

Moreover, compilers are free to put padding between members name and extension.

Kirill A. Shutemov

2010-01-20 12:36 UTC #15 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Quoted 23 lines · Markus Armbruster

Quoted 5 lines · Kevin Wolf
Am 20.01.2010 12:09, schrieb Kirill A. Shutemov:
Quoted 4 lines
Quoted 2 lines · Daniel P. Berrange
Quoted 1 line · Kirill A. Shutemov
On Wed, Jan 20, 2010 at 1:56 AM, Juan Quintela <[email protected]> wrote:
[...]
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 7 lines
block/vvfat.c +0 -0
index 063f731..df957e5 100644
@@ -868,7 +868,8 @@ static int init_directories(BDRVVVFATState* s,
     {
        direntry_t* entry=array_get_next(&(s->directory));
Quoted 5 lines
entry->attributes=0x28; /* archive | volume label */
- snprintf((char*)entry->name,11,"QEMU VVFAT");
+ memcpy(entry->name,"QEMU VVF",8);
+ memcpy(entry->extension,"AT ",3);
}
Better to use

Quoted 5 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
memcpy(entry->name, "QEMU VVFAT", 11);
memcpy() doesn't check bounds.
No, this is evil, and may well be flagged by static analysis tools.

If so, the tool is stupid.

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
Quoted 1 line
It doesn't *currently* check bounds.
No. memcpy() will never check bounds. It's totaly different from strcpy,
http://gcc.gnu.org/ml/gcc-patches/2009-06/msg00419.html

Quoted 7 lines

Quoted 4 lines
Regardless if deliberately overflowing the buffer works or doesn't making it explicit is better. Someone might reorder the struct or add new fields in between (okay, unlikely in this case, but still) and you'll overflow into fields you never wanted to touch.
Moreover, compilers are free to put padding between members name and extension.

No, compiler can't add anything between. 'char' is always byte-aligned.

Markus Armbruster

2010-01-20 13:03 UTC #16 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Quoted 42 lines · Kirill A. Shutemov

Quoted 23 lines · Markus Armbruster
Quoted 5 lines · Kevin Wolf
Am 20.01.2010 12:09, schrieb Kirill A. Shutemov:
Quoted 4 lines
Quoted 2 lines · Daniel P. Berrange
Quoted 1 line · Kirill A. Shutemov
On Wed, Jan 20, 2010 at 1:56 AM, Juan Quintela <[email protected]> wrote:
[...]
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 7 lines
block/vvfat.c +0 -0
index 063f731..df957e5 100644
@@ -868,7 +868,8 @@ static int init_directories(BDRVVVFATState* s,
     {
        direntry_t* entry=array_get_next(&(s->directory));
Quoted 5 lines
entry->attributes=0x28; /* archive | volume label */
- snprintf((char*)entry->name,11,"QEMU VVFAT");
+ memcpy(entry->name,"QEMU VVF",8);
+ memcpy(entry->extension,"AT ",3);
}
Better to use
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
memcpy(entry->name, "QEMU VVFAT", 11);
memcpy() doesn't check bounds.
No, this is evil, and may well be flagged by static analysis tools.
If so, the tool is stupid.
Quoted 10 lines
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
It doesn't *currently* check bounds.
No. memcpy() will never check bounds. It's totaly different from strcpy,
http://gcc.gnu.org/ml/gcc-patches/2009-06/msg00419.html
Regardless if deliberately overflowing the buffer works or doesn't making it explicit is better. Someone might reorder the struct or add new fields in between (okay, unlikely in this case, but still) and you'll overflow into fields you never wanted to touch.

Quoted 4 lines

Quoted 2 lines
Moreover, compilers are free to put padding between members name and extension.
No, compiler can't add anything between. 'char' is always byte-aligned.

You got some reading to do then.

Gleb Natapov

2010-01-20 13:08 UTC #17 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Quoted 44 lines · Markus Armbruster

Quoted 42 lines · Kirill A. Shutemov
Quoted 23 lines · Markus Armbruster
Quoted 5 lines · Kevin Wolf
Am 20.01.2010 12:09, schrieb Kirill A. Shutemov:
Quoted 4 lines
Quoted 2 lines · Daniel P. Berrange
Quoted 1 line · Kirill A. Shutemov
On Wed, Jan 20, 2010 at 1:56 AM, Juan Quintela <[email protected]> wrote:
[...]
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 7 lines
block/vvfat.c +0 -0
index 063f731..df957e5 100644
@@ -868,7 +868,8 @@ static int init_directories(BDRVVVFATState* s,
     {
        direntry_t* entry=array_get_next(&(s->directory));
Quoted 5 lines
entry->attributes=0x28; /* archive | volume label */
- snprintf((char*)entry->name,11,"QEMU VVFAT");
+ memcpy(entry->name,"QEMU VVF",8);
+ memcpy(entry->extension,"AT ",3);
}
Better to use
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
memcpy(entry->name, "QEMU VVFAT", 11);
memcpy() doesn't check bounds.
No, this is evil, and may well be flagged by static analysis tools.
If so, the tool is stupid.
Quoted 10 lines
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
It doesn't *currently* check bounds.
No. memcpy() will never check bounds. It's totaly different from strcpy,
http://gcc.gnu.org/ml/gcc-patches/2009-06/msg00419.html
Regardless if deliberately overflowing the buffer works or doesn't making it explicit is better. Someone might reorder the struct or add new fields in between (okay, unlikely in this case, but still) and you'll overflow into fields you never wanted to touch.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Moreover, compilers are free to put padding between members name and extension.
No, compiler can't add anything between. 'char' is always byte-aligned.
You got some reading to do then.

To be fair this structure is packed, but this is not the reason to write stupid code of course.

--
Gleb.

Kirill A. Shutemov

2010-01-20 13:51 UTC #21 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Quoted 4 lines · Markus Armbruster

Quoted 1 line · Kirill A. Shutemov
No, compiler can't add anything between. 'char' is always byte-aligned.
You got some reading to do then.

Do you want to say that it's not true? Could you provide an example when 'char' isn't byte-aligned?

Kirill A. Shutemov

2010-01-20 14:02 UTC #18 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

In reply to Gleb Natapov

2010/1/20 Gleb Natapov <[email protected]>:

Quoted 8 lines · Markus Armbruster

Quoted 5 lines
Quoted 1 line · Kirill A. Shutemov
No, compiler can't add anything between. 'char' is always byte-aligned.
You got some reading to do then.
To be fair this structure is packed, but this is not the reason to write stupid code of course.

In what way does it stupid? Compiler can't insert pads between two arrays of 'char'.

Gleb Natapov

2010-01-20 14:12 UTC #19 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Quoted 5 lines · Kirill A. Shutemov

2010/1/20 Gleb Natapov <[email protected]>:
Quoted 4 lines · Markus Armbruster
Quoted 3 lines
Quoted 1 line · Kirill A. Shutemov
No, compiler can't add anything between. 'char' is always byte-aligned.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
You got some reading to do then.
To be fair this structure is packed, but this is not the reason to write stupid code of course.
In what way does it stupid? Compiler can't insert pads between two arrays of 'char'.

Because this is not wise write tricky code especially if it buys you nothing. This is not C obfuscation contest and it will bite you later. What to search where extension[] is initialized? "grep extension" will not find it! Need to change structure layout? Have to change unrelated code too, but, of course, can't even know that. So can you please tell us why that code should stay like it is now?

--
Gleb.

Markus Armbruster

2010-01-20 14:43 UTC #22 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

[Some quoted material restored]

Quoted 12 lines · Kirill A. Shutemov

Quoted 11 lines · Markus Armbruster
Quoted 10 lines · Kirill A. Shutemov
Quoted 8 lines · Markus Armbruster
Quoted 4 lines · Kevin Wolf
Regardless if deliberately overflowing the buffer works or doesn't making it explicit is better. Someone might reorder the struct or add new fields in between (okay, unlikely in this case, but still) and you'll overflow into fields you never wanted to touch.
Moreover, compilers are free to put padding between members name and extension.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
No, compiler can't add anything between. 'char' is always byte-aligned.
You got some reading to do then.
Do you want to say that it's not true? Could you provide an example when 'char' isn't byte-aligned?

I was wrong, because I overlooked __attribute__((packed)).

ISO/IEC 9899:1999 6.7.2.1 guarantees a number of things for structs, chiefly members stay in order, no padding before the first member. But it does not restrict padding between members or at the end in any way, so compilers may pad there how they see fit. In particular, there is no rule that a compiler may only add padding to satisfy the next member's alignment requirement.

__attribute__((packed)) tightens the spec to eliminate padding.

Regardless, the deliberate buffer overflow to hopefully save a few bytes and cycles is a dirty, brittle trick for no good reason. If this really must be hand-optimized (numbers, please), do it cleanly, the way Dan suggested.

malc

2010-01-20 15:00 UTC #20 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Quoted 5 lines · Kirill A. Shutemov

2010/1/20 Gleb Natapov <[email protected]>:
Quoted 4 lines · Markus Armbruster
Quoted 3 lines
Quoted 1 line · Kirill A. Shutemov
No, compiler can't add anything between. 'char' is always byte-aligned.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
You got some reading to do then.
To be fair this structure is packed, but this is not the reason to write stupid code of course.
In what way does it stupid? Compiler can't insert pads between two arrays of 'char'.

Can you cite chapter and verse of the standard where it's spelled out?

Signature · 1 line

mailto:[email protected]

Anthony Liguori

2010-01-20 15:59 UTC #24 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Quoted 40 lines · Kirill A. Shutemov

Quoted 28 lines · Juan Quintela
From: Kirill A. Shutemov<[email protected]>
CC block/vvfat.o
cc1: warnings being treated as errors
block/vvfat.c: In function 'commit_one_file':
block/vvfat.c:2259: error: ignoring return value of 'ftruncate', declared with attribute warn_unused_result
make: *** [block/vvfat.o] Error 1
CC block/vvfat.o
In file included from /usr/include/stdio.h:912,
                 from ./qemu-common.h:19,
                 from block/vvfat.c:27:
In function 'snprintf',
inlined from 'init_directories' at block/vvfat.c:871,
inlined from 'vvfat_open' at block/vvfat.c:1068:
/usr/include/bits/stdio2.h:65: error: call to __builtin___snprintf_chk will always overflow destination buffer
make: *** [block/vvfat.o] Error 1
Signed-off-by Kirill A. Shutemov<[email protected]>
Signed-off-by Juan Quintela<[email protected]>
---
  block/vvfat.c |    9 +++++++--
  1 files changed, 7 insertions(+), 2 deletions(-)
block/vvfat.c +0 -0
index 063f731..df957e5 100644
block/vvfat.c static int init_directories(BDRVVVFATState* s, · line 874
Quoted 8 lines
block/vvfat.c +2 -1
@@ -868,7 +868,8 @@ static int init_directories(BDRVVVFATState* s,
     {
        direntry_t* entry=array_get_next(&(s->directory));
        entry->attributes=0x28; /* archive | volume label */
-       snprintf((char*)entry->name,11,"QEMU VVFAT");
+       memcpy(entry->name,"QEMU VVF",8);
+       memcpy(entry->extension,"AT ",3);
     }
 
Better to use

Quoted 4 lines

memcpy(entry->name, "QEMU VVFAT", 11);
memcpy() doesn't check bounds.

Relying on such things is bad form because it isn't obvious to a casual reader what is happening. You have to know that entry->name is 8 chars long and realize that it will overflow into extension. Since that info is all the way in the structure definition, the result is difficult to read code.

Any other discussions about whether the standards allow such a thing or whether tools are "stupid" is irrelevant. It's a neat trick but it results in more difficult to maintain code.

Regards,

Anthony Liguori

Jamie Lokier

2010-01-20 16:16 UTC #23 lore raw

Re: [Qemu-devel] Re: [PATCH 07/17] block/vvfat.c: fix warnings with _FORTIFY_SOURCE

Quoted 8 lines · Kirill A. Shutemov

Quoted 4 lines · Markus Armbruster
Quoted 1 line · Kirill A. Shutemov
No, compiler can't add anything between. 'char' is always byte-aligned.
You got some reading to do then.
Do you want to say that it's not true? Could you provide an example when 'char' isn't byte-aligned?

I have no example, but that doesn't mean there is no architecture, ABI or host OS where it happens. Architectures can be surprising.

It's very easy to make assumptions which break on some architecture you've never tested on.

Take this code:

struct part1 {
	char c[5];
};

struct part2 {
	char c[3];
};

struct all {
	struct part1 p1;
	struct part2 p2;
};

What do you think the size is on ARM? Hint: not 8 when using GCC < 4, but it is 8 when using current GCC and the later ARM ABI.

This broke some data parsing application when it was ported from x86 to ARM, because of a wrong assumption about structure layout that works on nearly all architectures and passed all testing prior to the port.

-- Jamie

Anthony Liguori

Applied

2010-01-26 21:34 UTC #3 lore raw

Re: [Qemu-devel] [PATCH 01/17] Introduce qemu_write_full()