from:"Jim Meyering"

[Qemu-devel] [PATCHv3 07/20] lm32: avoid buffer overrun

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Actually do what the comment says, using pstrcpy NUL-terminate:
strncpy does not always do that.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/lm32_hwsetup.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/lm32_hwsetup.h b/hw/lm32_hwsetup.h
index 8fc285e..70dc61f 100644
--- a/hw/lm32_hwsetup.h
+++ b/hw/lm32_hwsetup.h
@@ -96,7 +96,7 @@ static inline void hwsetup_add_tag(HWSetup *hw, enum 
hwsetup_tag t)

 static inline void hwsetup_add_str(HWSetup *hw, const char *str)
 {
-strncpy(hw-ptr, str, 31); /* make sure last byte is zero */
+pstrcpy(hw-ptr, 32, str);
 hw-ptr += 32;
 }

-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 03/20] block: avoid buffer overrun by using pstrcpy, not strncpy

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Also, use PATH_MAX, rather than the arbitrary 1024.
Using PATH_MAX is more consistent with other filename-related
variables in this file, like backing_filename and tmp_filename.

Acked-by: Kevin Wolf kw...@redhat.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 block.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 751ebdc..b62432c 100644
--- a/block.c
+++ b/block.c
@@ -1503,7 +1503,7 @@ int bdrv_commit(BlockDriverState *bs)
 int n, ro, open_flags;
 int ret = 0;
 uint8_t *buf;
-char filename[1024];
+char filename[PATH_MAX];

 if (!drv)
 return -ENOMEDIUM;
@@ -1517,7 +1517,8 @@ int bdrv_commit(BlockDriverState *bs)
 }

 ro = bs-backing_hd-read_only;
-strncpy(filename, bs-backing_hd-filename, sizeof(filename));
+/* Use pstrcpy (not strncpy): filename must be NUL-terminated. */
+pstrcpy(filename, sizeof(filename), bs-backing_hd-filename);
 open_flags =  bs-backing_hd-open_flags;

 if (ro) {
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 11/20] ui/vnc: simplify and avoid strncpy

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Don't bother with strncpy.  There's no need for its zero-fill.
Use g_strndup in place of g_malloc+strncpy+NUL-terminate.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 ui/vnc-auth-sasl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 8fba770..bfdcb46 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -432,9 +432,7 @@ static int protocol_client_auth_sasl_start_len(VncState 
*vs, uint8_t *data, size

 static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, 
size_t len)
 {
-char *mechname = g_malloc(len + 1);
-strncpy(mechname, (char*)data, len);
-mechname[len] = '\0';
+char *mechname = g_strndup((const char *) data, len);
 VNC_DEBUG(Got client mechname '%s' check against '%s'\n,
   mechname, vs-sasl.mechlist);

-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 06/20] hw/9pfs: avoid buffer overrun

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

v9fs_add_dir_node and qemu_v9fs_synth_add_file used strncpy
to form node-name, which requires NUL-termination, but
strncpy does not ensure NUL-termination.
Use pstrcpy, which does.

Acked-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/9pfs/virtio-9p-synth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/virtio-9p-synth.c b/hw/9pfs/virtio-9p-synth.c
index 92e0b09..e95a856 100644
--- a/hw/9pfs/virtio-9p-synth.c
+++ b/hw/9pfs/virtio-9p-synth.c
@@ -58,7 +58,7 @@ static V9fsSynthNode *v9fs_add_dir_node(V9fsSynthNode 
*parent, int mode,
 node-attr-read  = NULL;
 }
 node-private = node;
-strncpy(node-name, name, sizeof(node-name));
+pstrcpy(node-name, sizeof(node-name), name);
 QLIST_INSERT_HEAD_RCU(parent-child, node, sibling);
 return node;
 }
@@ -132,7 +132,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int 
mode,
 node-attr-write  = write;
 node-attr-mode   = mode;
 node-private  = arg;
-strncpy(node-name, name, sizeof(node-name));
+pstrcpy(node-name, sizeof(node-name), name);
 QLIST_INSERT_HEAD_RCU(parent-child, node, sibling);
 ret = 0;
 err_out:
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 18/20] qcow2: mark this file's sole strncpy use as justified

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Acked-by: Kevin Wolf kw...@redhat.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index aa5e603..c1ff31f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1096,6 +1096,7 @@ int qcow2_update_header(BlockDriverState *bs)
 goto fail;
 }

+/* Using strncpy is ok here, since buf is not NUL-terminated. */
 strncpy(buf, bs-backing_file, buflen);

 header-backing_file_offset = cpu_to_be64(buf - ((char*) header));
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 15/20] qemu-ga: prefer pstrcpy: consistently NUL-terminate ifreq.ifr_name

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

NUL-termination of the .ifr_name field is not required, but is fine
(and preferable to using strncpy and leaving the reader to wonder),
since the first thing the linux kernel does is to clear the last byte.
Besides, using pstrcpy here makes this setting of ifr_name consistent
with the other code (e.g., net/tap-linux.c) that does the same thing.

Reviewed-by: Luiz Capitulino lcapitul...@redhat.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 qga/commands-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index ce90421..b9f357c 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -828,7 +828,7 @@ GuestNetworkInterfaceList 
*qmp_guest_network_get_interfaces(Error **errp)
 }

 memset(ifr, 0, sizeof(ifr));
-strncpy(ifr.ifr_name,  info-value-name, IF_NAMESIZE);
+pstrcpy(ifr.ifr_name, IF_NAMESIZE, info-value-name);
 if (ioctl(sock, SIOCGIFHWADDR, ifr) == -1) {
 snprintf(err_msg, sizeof(err_msg),
  failed to get MAC address of %s: %s,
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 14/20] vscsi: avoid unwarranted strncpy

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Don't use strncpy when the source string is known to fit
in the destination buffer.  Use equivalent memcpy.
We could even use strcpy, here, but some static analyzers
warn about that, so don't add new uses.

Acked-by: David Gibson da...@gibson.dropbear.id.au
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/spapr_vscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 3cf5844..e3d4b23 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -737,7 +737,7 @@ static int vscsi_send_adapter_info(VSCSIState *s, vscsi_req 
*req)
 #endif
 memset(info, 0, sizeof(info));
 strcpy(info.srp_version, SRP_VERSION);
-strncpy(info.partition_name, qemu, sizeof(qemu));
+memcpy(info.partition_name, qemu, sizeof(qemu));
 info.partition_number = cpu_to_be32(0);
 info.mad_version = cpu_to_be32(1);
 info.os_type = cpu_to_be32(2);
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 17/20] acpi: remove strzcpy (strncpy-identical) function; just use strncpy

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Adjust all uses s/strzcpy/strncpy/ and mark these uses
of strncpy as ok.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/acpi.c | 24 
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/hw/acpi.c b/hw/acpi.c
index f7950be..f4aca49 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -61,18 +61,6 @@ static int acpi_checksum(const uint8_t *data, int len)
 return (-sum)  0xff;
 }

-/* like strncpy() but zero-fills the tail of destination */
-static void strzcpy(char *dst, const char *src, size_t size)
-{
-size_t len = strlen(src);
-if (len = size) {
-len = size;
-} else {
-  memset(dst + len, 0, size - len);
-}
-memcpy(dst, src, len);
-}
-
 /* XXX fixme: this function uses obsolete argument parsing interface */
 int acpi_table_add(const char *t)
 {
@@ -157,7 +145,8 @@ int acpi_table_add(const char *t)
 hdr._length = cpu_to_le16(len);

 if (get_param_value(buf, sizeof(buf), sig, t)) {
-strzcpy(hdr.sig, buf, sizeof(hdr.sig));
+/* strncpy is justified: the field need not be NUL-terminated. */
+strncpy(hdr.sig, buf, sizeof(hdr.sig));
 ++changed;
 }

@@ -187,12 +176,14 @@ int acpi_table_add(const char *t)
 }

 if (get_param_value(buf, sizeof(buf), oem_id, t)) {
-strzcpy(hdr.oem_id, buf, sizeof(hdr.oem_id));
+/* strncpy is justified: the field need not be NUL-terminated. */
+strncpy(hdr.oem_id, buf, sizeof(hdr.oem_id));
 ++changed;
 }

 if (get_param_value(buf, sizeof(buf), oem_table_id, t)) {
-strzcpy(hdr.oem_table_id, buf, sizeof(hdr.oem_table_id));
+/* strncpy is justified: the field need not be NUL-terminated. */
+strncpy(hdr.oem_table_id, buf, sizeof(hdr.oem_table_id));
 ++changed;
 }

@@ -207,7 +198,8 @@ int acpi_table_add(const char *t)
 }

 if (get_param_value(buf, sizeof(buf), asl_compiler_id, t)) {
-strzcpy(hdr.asl_compiler_id, buf, sizeof(hdr.asl_compiler_id));
+/* strncpy is justified: the field need not be NUL-terminated. */
+strncpy(hdr.asl_compiler_id, buf, sizeof(hdr.asl_compiler_id));
 ++changed;
 }

-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 13/20] virtio-9p: avoid unwarranted uses of strncpy

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

In all of these cases, the uses of strncpy were unnecessary, since
at each point of use we know that the NUL-terminated source bytes
fit in the destination buffer.  Use memcpy in place of strncpy.

Acked-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/9pfs/virtio-9p-posix-acl.c  | 6 --
 hw/9pfs/virtio-9p-xattr-user.c | 3 ++-
 hw/9pfs/virtio-9p-xattr.c  | 3 ++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/virtio-9p-posix-acl.c b/hw/9pfs/virtio-9p-posix-acl.c
index a1948e3..c064017 100644
--- a/hw/9pfs/virtio-9p-posix-acl.c
+++ b/hw/9pfs/virtio-9p-posix-acl.c
@@ -44,7 +44,8 @@ static ssize_t mp_pacl_listxattr(FsContext *ctx, const char 
*path,
 return -1;
 }

-strncpy(value, ACL_ACCESS, len);
+/* len includes the trailing NUL */
+memcpy(value, ACL_ACCESS, len);
 return 0;
 }

@@ -95,7 +96,8 @@ static ssize_t mp_dacl_listxattr(FsContext *ctx, const char 
*path,
 return -1;
 }

-strncpy(value, ACL_DEFAULT, len);
+/* len includes the trailing NUL */
+memcpy(value, ACL_ACCESS, len);
 return 0;
 }

diff --git a/hw/9pfs/virtio-9p-xattr-user.c b/hw/9pfs/virtio-9p-xattr-user.c
index 5044a3e..5bb6020 100644
--- a/hw/9pfs/virtio-9p-xattr-user.c
+++ b/hw/9pfs/virtio-9p-xattr-user.c
@@ -61,7 +61,8 @@ static ssize_t mp_user_listxattr(FsContext *ctx, const char 
*path,
 return -1;
 }

-strncpy(value, name, name_size);
+/* name_size includes the trailing NUL. */
+memcpy(value, name, name_size);
 return name_size;
 }

diff --git a/hw/9pfs/virtio-9p-xattr.c b/hw/9pfs/virtio-9p-xattr.c
index 7f08f6e..a839606 100644
--- a/hw/9pfs/virtio-9p-xattr.c
+++ b/hw/9pfs/virtio-9p-xattr.c
@@ -53,7 +53,8 @@ ssize_t pt_listxattr(FsContext *ctx, const char *path,
 return -1;
 }

-strncpy(value, name, name_size);
+/* no need for strncpy: name_size is strlen(name)+1 */
+memcpy(value, name, name_size);
 return name_size;
 }

-- 
1.8.0.rc0.18.gf84667d

Re: [Qemu-devel] [PATCHv3 11/20] ui/vnc: simplify and avoid strncpy

2012-10-04 Thread Jim Meyering

Peter Maydell wrote:

 On 4 October 2012 12:09, Jim Meyering j...@meyering.net wrote:
 From: Jim Meyering meyer...@redhat.com

 Don't bother with strncpy.  There's no need for its zero-fill.
 Use g_strndup in place of g_malloc+strncpy+NUL-terminate.

 Signed-off-by: Jim Meyering meyer...@redhat.com
 ---
  ui/vnc-auth-sasl.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

 diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
 index 8fba770..bfdcb46 100644
 --- a/ui/vnc-auth-sasl.c
 +++ b/ui/vnc-auth-sasl.c
 @@ -432,9 +432,7 @@ static int protocol_client_auth_sasl_start_len(VncState 
 *vs, uint8_t *data, size

  static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, 
 size_t len)
  {
 -char *mechname = g_malloc(len + 1);
 -strncpy(mechname, (char*)data, len);
 -mechname[len] = '\0';
 +char *mechname = g_strndup((const char *) data, len);
  VNC_DEBUG(Got client mechname '%s' check against '%s'\n,
mechname, vs-sasl.mechlist);


 (Does the compiler really insist on that cast?)

Yes.  Without it, I get a warning/failure when char is signed.
See below.

 Reviewed-by: Peter Maydell peter.mayd...@linaro.org

Thanks for the review.

ui/vnc-auth-sasl.c: In function 'protocol_client_auth_sasl_mechname':
ui/vnc-auth-sasl.c:435:5: error: pointer targets in passing argument 1 of 
'g_strndup' differ in signedness [-Werror=pointer-sign]
 char *mechname = g_strndup(data, len);
 ^
In file included from /usr/include/glib-2.0/glib.h:81:0,
 from ./qemu-common.h:41,
 from ui/vnc.h:30,
 from ui/vnc-auth-sasl.c:25:
/usr/include/glib-2.0/glib/gstrfuncs.h:192:8: note: expected 'const gchar 
*' but argument is of type 'uint8_t *'
 gchar*   g_strndup(const gchar *str,
^
cc1: all warnings being treated as errors
make: *** [ui/vnc-auth-sasl.o] Error 1

[Qemu-devel] [PATCHv3 16/20] libcacard/vcard_emul_nss: use pstrcpy in place of strncpy

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Replace strncpy+NUL-terminate use with use of pstrcpy.
This requires linking with cutils.o (or else vssclient doesn't link),
so add that in the Makefile.

Acked-by: Alon Levy al...@redhat.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 libcacard/Makefile | 3 +++
 libcacard/vcard_emul_nss.c | 3 +--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/libcacard/Makefile b/libcacard/Makefile
index 63990b7..487f434 100644
--- a/libcacard/Makefile
+++ b/libcacard/Makefile
@@ -14,6 +14,9 @@ QEMU_CFLAGS+=-I../

 libcacard.lib-y=$(patsubst %.o,%.lo,$(libcacard-y))

+vscclient: $(libcacard-y) $(QEMU_OBJS) vscclient.o cutils.o
+   $(call quiet-command,$(CC) -o $@ $^ $(libcacard_libs) $(LIBS),  LINK  
$@)
+
 clean:
rm -f *.o */*.o *.d */*.d *.a */*.a *~ */*~ vscclient *.lo */*.lo 
.libs/* */.libs/* *.la */*.la *.pc
rm -Rf .libs */.libs
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 802cae3..e1cae5b 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -1169,8 +1169,7 @@ vcard_emul_options(const char *args)
 NEXT_TOKEN(vname)
 NEXT_TOKEN(type_params)
 type_params_length = MIN(type_params_length, sizeof(type_str)-1);
-strncpy(type_str, type_params, type_params_length);
-type_str[type_params_length] = 0;
+pstrcpy(type_str, type_params_length, type_params);
 type = vcard_emul_type_from_string(type_str);

 NEXT_TOKEN(type_params)
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 12/20] bt: replace fragile snprintf use and unwarranted strncpy

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

In bt_hci_name_req a failed snprintf could return len larger than
sizeof(params.name), which means the following memset call would
have a length value of (size_t)-1, -2, etc...  Sounds scary.
But currently, one can deduce that there is no problem:
strlen(slave-lmp_name) is guaranteed to be smaller than
CHANGE_LOCAL_NAME_CP_SIZE, which is the same as sizeof(params.name),
so this cannot happen.  Regardless, there is no justification for
using snprintf+memset.  Use pstrcpy instead.

Also, in bt_hci_event_complete_read_local_name, use pstrcpy in place
of unwarranted strncpy.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/bt-hci.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/bt-hci.c b/hw/bt-hci.c
index a3a7fb4..47f9a4e 100644
--- a/hw/bt-hci.c
+++ b/hw/bt-hci.c
@@ -943,7 +943,6 @@ static int bt_hci_name_req(struct bt_hci_s *hci, bdaddr_t 
*bdaddr)
 {
 struct bt_device_s *slave;
 evt_remote_name_req_complete params;
-int len;

 for (slave = hci-device.net-slave; slave; slave = slave-next)
 if (slave-page_scan  !bacmp(slave-bd_addr, bdaddr))
@@ -955,9 +954,7 @@ static int bt_hci_name_req(struct bt_hci_s *hci, bdaddr_t 
*bdaddr)

 params.status   = HCI_SUCCESS;
 bacpy(params.bdaddr, slave-bd_addr);
-len = snprintf(params.name, sizeof(params.name),
-%s, slave-lmp_name ?: );
-memset(params.name + len, 0, sizeof(params.name) - len);
+pstrcpy(params.name, sizeof(params.name), slave-lmp_name ?: );
 bt_hci_event(hci, EVT_REMOTE_NAME_REQ_COMPLETE,
 params, EVT_REMOTE_NAME_REQ_COMPLETE_SIZE);

@@ -1388,7 +1385,7 @@ static inline void 
bt_hci_event_complete_read_local_name(struct bt_hci_s *hci)
 params.status = HCI_SUCCESS;
 memset(params.name, 0, sizeof(params.name));
 if (hci-device.lmp_name)
-strncpy(params.name, hci-device.lmp_name, sizeof(params.name));
+pstrcpy(params.name, sizeof(params.name), hci-device.lmp_name);

 bt_hci_event_complete(hci, params, READ_LOCAL_NAME_RP_SIZE);
 }
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 02/20] sparc: use g_strdup in place of unchecked strdup

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

This avoids a NULL-deref upon strdup failure.
Also update matching free to g_free.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 target-sparc/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index f7c004c..eb9f0e7 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -643,7 +643,7 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, 
const char *cpu_model)
 {
 unsigned int i;
 const sparc_def_t *def = NULL;
-char *s = strdup(cpu_model);
+char *s = g_strdup(cpu_model);
 char *featurestr, *name = strtok(s, ,);
 uint32_t plus_features = 0;
 uint32_t minus_features = 0;
@@ -735,7 +735,7 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, 
const char *cpu_model)
 #ifdef DEBUG_FEATURES
 print_features(stderr, fprintf, cpu_def-features, NULL);
 #endif
-free(s);
+g_free(s);
 return 0;

  error:
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 10/20] linux-user: remove two unchecked uses of strdup

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Remove two uses of strdup (use g_path_get_basename instead),
and add a comment that this strncpy use is ok.

Reviewed-by: Peter Maydell peter.mayd...@linaro.org
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 linux-user/elfload.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 819fdd5..1d8bcb4 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2442,7 +2442,7 @@ static void fill_prstatus(struct target_elf_prstatus 
*prstatus,

 static int fill_psinfo(struct target_elf_prpsinfo *psinfo, const TaskState *ts)
 {
-char *filename, *base_filename;
+char *base_filename;
 unsigned int i, len;

 (void) memset(psinfo, 0, sizeof (*psinfo));
@@ -2464,13 +2464,15 @@ static int fill_psinfo(struct target_elf_prpsinfo 
*psinfo, const TaskState *ts)
 psinfo-pr_uid = getuid();
 psinfo-pr_gid = getgid();

-filename = strdup(ts-bprm-filename);
-base_filename = strdup(basename(filename));
+base_filename = g_path_get_basename(ts-bprm-filename);
+/*
+ * Using strncpy here is fine: at max-length,
+ * this field is not NUL-terminated.
+ */
 (void) strncpy(psinfo-pr_fname, base_filename,
sizeof(psinfo-pr_fname));
-free(base_filename);
-free(filename);

+g_free(base_filename);
 bswap_psinfo(psinfo);
 return (0);
 }
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 00/20] strncpy: best avoided

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

I included a quick classification of these change sets for the original series,
(see https://lists.gnu.org/archive/html/qemu-devel/2012-05/msg01151.html)
rebased in late May but perhaps the release-related timing was off.

I've rebased one last time to prepare this v3 series.
Other than the rebase, which ended up obviating 1 or 2 commits,
there have has been no change since v2.

Jim Meyering (20):
  scsi,pci,qdev,isa-bus,sysbus: don't let *_get_fw_dev_path return NULL
  sparc: use g_strdup in place of unchecked strdup
  block: avoid buffer overrun by using pstrcpy, not strncpy
  sheepdog: avoid a few buffer overruns
  vmdk: relative_path: use pstrcpy in place of strncpy
  hw/9pfs: avoid buffer overrun
  lm32: avoid buffer overrun
  os-posix: avoid buffer overrun
  ppc: avoid buffer overrun: use pstrcpy, not strncpy
  linux-user: remove two unchecked uses of strdup
  ui/vnc: simplify and avoid strncpy
  bt: replace fragile snprintf use and unwarranted strncpy
  virtio-9p: avoid unwarranted uses of strncpy
  vscsi: avoid unwarranted strncpy
  qemu-ga: prefer pstrcpy: consistently NUL-terminate ifreq.ifr_name
  libcacard/vcard_emul_nss: use pstrcpy in place of strncpy
  acpi: remove strzcpy (strncpy-identical) function; just use strncpy
  qcow2: mark this file's sole strncpy use as justified
  hw/r2d: add comment: this strncpy use is ok
  doc: update HACKING wrt strncpy/pstrcpy

 HACKING|  9 +
 block.c|  5 +++--
 block/qcow2.c  |  1 +
 block/sheepdog.c   | 34 ++
 block/vmdk.c   |  3 +--
 hw/9pfs/virtio-9p-posix-acl.c  |  6 --
 hw/9pfs/virtio-9p-synth.c  |  4 ++--
 hw/9pfs/virtio-9p-xattr-user.c |  3 ++-
 hw/9pfs/virtio-9p-xattr.c  |  3 ++-
 hw/acpi.c  | 24 
 hw/bt-hci.c|  7 ++-
 hw/ide/qdev.c  |  2 +-
 hw/isa-bus.c   |  2 +-
 hw/lm32_hwsetup.h  |  2 +-
 hw/pci.c   |  2 +-
 hw/qdev.c  |  2 +-
 hw/r2d.c   |  2 ++
 hw/scsi-bus.c  |  8 ++--
 hw/spapr_vscsi.c   |  2 +-
 hw/sysbus.c|  2 +-
 libcacard/Makefile |  3 +++
 libcacard/vcard_emul_nss.c |  3 +--
 linux-user/elfload.c   | 12 +++-
 os-posix.c |  3 +--
 qga/commands-posix.c   |  2 +-
 target-ppc/kvm.c   |  2 +-
 target-sparc/cpu.c |  4 ++--
 ui/vnc-auth-sasl.c |  4 +---
 28 files changed, 80 insertions(+), 76 deletions(-)

--
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 09/20] ppc: avoid buffer overrun: use pstrcpy, not strncpy

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

A terminal NUL is required by caller's use of strchr.
It's better not to use strncpy at all, since there is no need
to zero out hundreds of trailing bytes for each iteration.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 target-ppc/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index a31d278..7f6e4e0 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -795,7 +795,7 @@ static int read_cpuinfo(const char *field, char *value, int 
len)
 break;
 }
 if (!strncmp(line, field, field_len)) {
-strncpy(value, line, len);
+pstrcpy(value, len, line);
 ret = 0;
 break;
 }
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 05/20] vmdk: relative_path: use pstrcpy in place of strncpy

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Avoid strncpy+manual-NUL-terminate.  Use pstrcpy instead.

Acked-by: Kevin Wolf kw...@redhat.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 block/vmdk.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index f2e861b..1a80e5a 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1408,8 +1408,7 @@ static int relative_path(char *dest, int dest_size,
 return -1;
 }
 if (path_is_absolute(target)) {
-dest[dest_size - 1] = '\0';
-strncpy(dest, target, dest_size - 1);
+pstrcpy(dest, dest_size, target);
 return 0;
 }
 while (base[i] == target[i]) {
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 20/20] doc: update HACKING wrt strncpy/pstrcpy

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Reword the section on strncpy: its NUL-filling is important
in some cases.  Mention that pstrcpy's signature is different.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 HACKING | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/HACKING b/HACKING
index 471cf1d..617 100644
--- a/HACKING
+++ b/HACKING
@@ -91,10 +91,11 @@ emulators.

 4. String manipulation

-Do not use the strncpy function.  According to the man page, it does
-*not* guarantee a NULL-terminated buffer, which makes it extremely dangerous
-to use.  Instead, use functionally equivalent function:
-void pstrcpy(char *buf, int buf_size, const char *str)
+Do not use the strncpy function.  As mentioned in the man page, it does *not*
+guarantee a NULL-terminated buffer, which makes it extremely dangerous to use.
+It also zeros trailing destination bytes out to the specified length.  Instead,
+use this similar function when possible, but note its different signature:
+void pstrcpy(char *dest, int dest_buf_size, const char *src)

 Don't use strcat because it can't check for buffer overflows, but:
 char *pstrcat(char *buf, int buf_size, const char *s)
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 08/20] os-posix: avoid buffer overrun

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

os_set_proc_name: Use pstrcpy, in place of strncpy and the
ineffectual preceding assignment: name[sizeof(name) - 1] = 0;

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 os-posix.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index eabccb8..f855abb 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -148,8 +148,7 @@ void os_set_proc_name(const char *s)
 char name[16];
 if (!s)
 return;
-name[sizeof(name) - 1] = 0;
-strncpy(name, s, sizeof(name));
+pstrcpy(name, sizeof(name), s);
 /* Could rewrite argv[0] too, but that's a bit more complicated.
This simple way is enough for `top'. */
 if (prctl(PR_SET_NAME, name)) {
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 04/20] sheepdog: avoid a few buffer overruns

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

* parse_vdiname: Use pstrcpy, not strncpy, when the destination
buffer must be NUL-terminated.
* sd_open: Likewise, avoid buffer overrun.
* do_sd_create: Likewise.  Leave the preceding memset, since
pstrcpy does not NUL-fill, and filename needs that.
* sd_snapshot_create: Add a comment/question.
* find_vdi_name: Remove a useless memset.
* sd_snapshot_goto: Remove a useless memset.
Use pstrcpy to NUL-terminate, because find_vdi_name requires
that its vdi arg (filename parameter) be NUL-terminated.
It seems ok not to NUL-fill the buffer.
Do the same for snapid: remove useless memset-0 (instead,
zero tag[0]).  Use pstrcpy, not strncpy.
* sd_snapshot_list: Use pstrcpy, not strncpy to write
into the -name member.  Each must be NUL-terminated.

Acked-by: Kevin Wolf kw...@redhat.com
Acked-by: MORITA Kazutaka morita.kazut...@lab.ntt.co.jp
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 block/sheepdog.c | 34 ++
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 4742f8a..f35ff5b 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -866,14 +866,14 @@ static int parse_vdiname(BDRVSheepdogState *s, const char 
*filename,
 s-port = 0;
 }

-strncpy(vdi, p, SD_MAX_VDI_LEN);
+pstrcpy(vdi, SD_MAX_VDI_LEN, p);

 p = strchr(vdi, ':');
 if (p) {
 *p++ = '\0';
 *snapid = strtoul(p, NULL, 10);
 if (*snapid == 0) {
-strncpy(tag, p, SD_MAX_VDI_TAG_LEN);
+pstrcpy(tag, SD_MAX_VDI_TAG_LEN, p);
 }
 } else {
 *snapid = CURRENT_VDI_ID; /* search current vdi */
@@ -900,7 +900,10 @@ static int find_vdi_name(BDRVSheepdogState *s, char 
*filename, uint32_t snapid,
 return fd;
 }

-memset(buf, 0, sizeof(buf));
+/* This pair of strncpy calls ensures that the buffer is zero-filled,
+ * which is desirable since we'll soon be sending those bytes, and
+ * don't want the send_req to read uninitialized data.
+ */
 strncpy(buf, filename, SD_MAX_VDI_LEN);
 strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);

@@ -1149,7 +1152,7 @@ static int sd_open(BlockDriverState *bs, const char 
*filename, int flags)
 s-max_dirty_data_idx = 0;

 bs-total_sectors = s-inode.vdi_size / SECTOR_SIZE;
-strncpy(s-name, vdi, sizeof(s-name));
+pstrcpy(s-name, sizeof(s-name), vdi);
 qemu_co_mutex_init(s-lock);
 g_free(buf);
 return 0;
@@ -1177,8 +1180,11 @@ static int do_sd_create(char *filename, int64_t vdi_size,
 return fd;
 }

+/* FIXME: would it be better to fail (e.g., return -EIO) when filename
+ * does not fit in buf?  For now, just truncate and avoid buffer overrun.
+ */
 memset(buf, 0, sizeof(buf));
-strncpy(buf, filename, SD_MAX_VDI_LEN);
+pstrcpy(buf, sizeof(buf), filename);

 memset(hdr, 0, sizeof(hdr));
 hdr.opcode = SD_OP_NEW_VDI;
@@ -1752,6 +1758,9 @@ static int sd_snapshot_create(BlockDriverState *bs, 
QEMUSnapshotInfo *sn_info)

 s-inode.vm_state_size = sn_info-vm_state_size;
 s-inode.vm_clock_nsec = sn_info-vm_clock_nsec;
+/* It appears that inode.tag does not require a NUL terminator,
+ * which means this use of strncpy is ok.
+ */
 strncpy(s-inode.tag, sn_info-name, sizeof(s-inode.tag));
 /* we don't need to update entire object */
 datalen = SD_INODE_SIZE - sizeof(s-inode.data_vdi_id);
@@ -1811,13 +1820,13 @@ static int sd_snapshot_goto(BlockDriverState *bs, const 
char *snapshot_id)

 memcpy(old_s, s, sizeof(BDRVSheepdogState));

-memset(vdi, 0, sizeof(vdi));
-strncpy(vdi, s-name, sizeof(vdi));
+pstrcpy(vdi, sizeof(vdi), s-name);

-memset(tag, 0, sizeof(tag));
 snapid = strtoul(snapshot_id, NULL, 10);
-if (!snapid) {
-strncpy(tag, s-name, sizeof(tag));
+if (snapid) {
+tag[0] = 0;
+} else {
+pstrcpy(tag, sizeof(tag), s-name);
 }

 ret = find_vdi_name(s, vdi, snapid, tag, vid, 1);
@@ -1946,8 +1955,9 @@ static int sd_snapshot_list(BlockDriverState *bs, 
QEMUSnapshotInfo **psn_tab)

 snprintf(sn_tab[found].id_str, sizeof(sn_tab[found].id_str), %u,
  inode.snap_id);
-strncpy(sn_tab[found].name, inode.tag,
-MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)));
+pstrcpy(sn_tab[found].name,
+MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)),
+inode.tag);
 found++;
 }
 }
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 19/20] hw/r2d: add comment: this strncpy use is ok

2012-10-04 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/r2d.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/r2d.c b/hw/r2d.c
index 0f16e81..1bc191f 100644
--- a/hw/r2d.c
+++ b/hw/r2d.c
@@ -332,6 +332,8 @@ static void r2d_init(ram_addr_t ram_size,
 }

 if (kernel_cmdline) {
+/* I see no evidence that this .kernel_cmdline buffer requires
+   NUL-termination, so using strncpy should be ok. */
 strncpy(boot_params.kernel_cmdline, kernel_cmdline,
 sizeof(boot_params.kernel_cmdline));
 }
-- 
1.8.0.rc0.18.gf84667d

[Qemu-devel] [PATCHv3 1/5] qemu-ga: don't leak a file descriptor upon failed lockf

2012-08-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 qemu-ga.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qemu-ga.c b/qemu-ga.c
index 8f87621..26671fe 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -247,6 +247,9 @@ static bool ga_open_pidfile(const char *pidfile)
 pidfd = open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
 if (pidfd == -1 || lockf(pidfd, F_TLOCK, 0)) {
 g_critical(Cannot lock pid file, %s, strerror(errno));
+if (pidfd != -1) {
+close(pidfd);
+}
 return false;
 }

-- 
1.7.12

[Qemu-devel] [PATCHv3 2/5] linux-user: do_msgrcv: don't leak host_mb upon TARGET_EFAULT failure

2012-08-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Also, use g_malloc to avoid NULL-deref upon OOM.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 linux-user/syscall.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 41c869b..1174306 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2848,7 +2848,7 @@ static inline abi_long do_msgrcv(int msqid, abi_long msgp,
 if (!lock_user_struct(VERIFY_WRITE, target_mb, msgp, 0))
 return -TARGET_EFAULT;

-host_mb = malloc(msgsz+sizeof(long));
+host_mb = g_malloc(msgsz+sizeof(long));
 ret = get_errno(msgrcv(msqid, host_mb, msgsz, tswapal(msgtyp), msgflg));

 if (ret  0) {
@@ -2863,11 +2863,11 @@ static inline abi_long do_msgrcv(int msqid, abi_long 
msgp,
 }

 target_mb-mtype = tswapal(host_mb-mtype);
-free(host_mb);

 end:
 if (target_mb)
 unlock_user_struct(target_mb, msgp, 1);
+g_free(host_mb);
 return ret;
 }

-- 
1.7.12

[Qemu-devel] [PATCHv3 0/5] plug memory and file-descriptor leaks

2012-08-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Hi Anthony,

I posted this series back in May, got some good feedback leading to a
pair of v2 patches.  Since then one of the 6 patches was applied.
I'm calling this v3, but it is merely a trivial rebase of the v1 and v2
patches.  Hoping it's not too late for 1.2, here are the remaining five:

Jim Meyering (5):
  qemu-ga: don't leak a file descriptor upon failed lockf
  linux-user: do_msgrcv: don't leak host_mb upon TARGET_EFAULT failure
  sheepdog: don't leak socket file descriptor upon connection failure
  arm-semi: don't leak 1KB user string lock buffer upon TARGET_SYS_OPEN
  softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

 block/sheepdog.c  |  1 +
 linux-user/syscall.c  |  4 ++--
 qemu-ga.c |  3 +++
 softmmu-semi.h|  5 -
 target-arm/arm-semi.c | 13 +++--
 5 files changed, 17 insertions(+), 9 deletions(-)

--
1.7.12

[Qemu-devel] [PATCHv3 4/5] arm-semi: don't leak 1KB user string lock buffer upon TARGET_SYS_OPEN

2012-08-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Always call unlock_user before returning.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 target-arm/arm-semi.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
index 2495206..73bde58 100644
--- a/target-arm/arm-semi.c
+++ b/target-arm/arm-semi.c
@@ -194,18 +194,19 @@ uint32_t do_arm_semihosting(CPUARMState *env)
 if (!(s = lock_user_string(ARG(0
 /* FIXME - should this error code be -TARGET_EFAULT ? */
 return (uint32_t)-1;
-if (ARG(1) = 12)
+if (ARG(1) = 12) {
+unlock_user(s, ARG(0), 0);
 return (uint32_t)-1;
+}
 if (strcmp(s, :tt) == 0) {
-if (ARG(1)  4)
-return STDIN_FILENO;
-else
-return STDOUT_FILENO;
+int result_fileno = ARG(1)  4 ? STDIN_FILENO : STDOUT_FILENO;
+unlock_user(s, ARG(0), 0);
+return result_fileno;
 }
 if (use_gdb_syscalls()) {
 gdb_do_syscall(arm_semi_cb, open,%s,%x,1a4, ARG(0),
   (int)ARG(2)+1, gdb_open_modeflags[ARG(1)]);
-return env-regs[0];
+ret = env-regs[0];
 } else {
 ret = set_swi_errno(ts, open(s, open_modeflags[ARG(1)], 0644));
 }
-- 
1.7.12

[Qemu-devel] [PATCHv3 3/5] sheepdog: don't leak socket file descriptor upon connection failure

2012-08-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 block/sheepdog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index a04ad99..df4f441 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -485,6 +485,7 @@ static int connect_to_sdog(const char *addr, const char 
*port)
 if (errno == EINTR) {
 goto reconnect;
 }
+close(fd);
 break;
 }

-- 
1.7.12

[Qemu-devel] [PATCHv3 5/5] softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

2012-08-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Return NULL upon malloc failure.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 softmmu-semi.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/softmmu-semi.h b/softmmu-semi.h
index 648cb95..bcb979a 100644
--- a/softmmu-semi.h
+++ b/softmmu-semi.h
@@ -40,7 +40,7 @@ static void *softmmu_lock_user(CPUArchState *env, uint32_t 
addr, uint32_t len,
 uint8_t *p;
 /* TODO: Make this something that isn't fixed size.  */
 p = malloc(len);
-if (copy)
+if (p  copy)
 cpu_memory_rw_debug(env, addr, p, len, 0);
 return p;
 }
@@ -52,6 +52,9 @@ static char *softmmu_lock_user_string(CPUArchState *env, 
uint32_t addr)
 uint8_t c;
 /* TODO: Make this something that isn't fixed size.  */
 s = p = malloc(1024);
+if (!s) {
+return NULL;
+}
 do {
 cpu_memory_rw_debug(env, addr, c, 1, 0);
 addr++;
-- 
1.7.12

Re: [Qemu-devel] [PATCH 4/6] sheepdog: don't leak socket file descriptor upon connection failure

2012-08-17 Thread Jim Meyering

Kevin Wolf wrote:
 Am 16.05.2012 15:07, schrieb Jim Meyering:
 From: Jim Meyering meyer...@redhat.com

 Signed-off-by: Jim Meyering meyer...@redhat.com

 Acked-by: Kevin Wolf kw...@redhat.com

Hi Kevin,

AFAICS, only one of these 6 patches has been applied.
From what I recall (it's been nearly 3mo), there was good
feedback and I posted at least one V2 patch.
For reference, here's the start of the series:

http://marc.info/?l=qemu-develm=133717388221635w=2

Let me know if there's anything I can do to help.

Jim Meyering (5):
  qemu-ga: don't leak a file descriptor upon failed lockf
  linux-user: do_msgrcv: don't leak host_mb upon TARGET_EFAULT failure
  sheepdog: don't leak socket file descriptor upon connection failure
  arm-semi: don't leak 1KB user string lock buffer upon TARGET_SYS_OPEN
  softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

 block/sheepdog.c  |1 +
 linux-user/syscall.c  |4 ++--
 qemu-ga.c |3 +++
 softmmu-semi.h|5 -
 target-arm/arm-semi.c |   13 +++--
 5 files changed, 17 insertions(+), 9 deletions(-)

Re: [Qemu-devel] [PATCHv2 2/2] envlist.c: handle strdup failure

2012-08-17 Thread Jim Meyering

Jim Meyering wrote:
 From: Jim Meyering meyer...@redhat.com

 Without this, envlist_to_environ may silently fail to copy all
 strings into the destination buffer, and both callers would leak
 any env strings allocated after a failing strdup, because the
 freeing code stops at the first NULL pointer.

 Signed-off-by: Jim Meyering meyer...@redhat.com
 ---
  envlist.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

 diff --git a/envlist.c b/envlist.c
 index be0addb..7532091 100644
 --- a/envlist.c
 +++ b/envlist.c
 @@ -234,8 +234,16 @@ envlist_to_environ(const envlist_t *envlist, size_t 
 *count)
  return (NULL);

  for (entry = envlist-el_entries.lh_first; entry != NULL;
 -entry = entry-ev_link.le_next) {
 -*(penv++) = strdup(entry-ev_var);
 + entry = entry-ev_link.le_next, penv++) {
 +*penv = strdup(entry-ev_var);
 +if (*penv == NULL) {
 +char **e = env;
 +while (e = penv) {
 +free(*e++);
 +}
 +free(env);
 +return NULL;
 +}
  }
  *penv = NULL; /* NULL terminate the list */

It seems this has been lost in this list's high volume of patches.
Anyone interested?  Repost desired?

 b/envlist.c |  256 ++--
 envlist.c   |   12 ++
 2 files changed, 138 insertions(+), 130 deletions(-)

Re: [Qemu-devel] [PATCH 0/9] convert many more globals to static

2012-08-17 Thread Jim Meyering

Jim Meyering wrote:
 From: Jim Meyering meyer...@redhat.com

 Following up on discussion here,

   http://marc.info/?t=13375948768r=1w=2

 here are patches to limit the scope of the remaining global variables.
 Most changes simply added a preceding static.  However, in some cases,
 I've made minor additional changes, e.g., to make a static
 table const as well (which sometimes required making an iterator
 pointer const, too), and to move or remove declarations of variables
 that the compiler then was able to identify as unused.

 Initially I put the changes to each file in a separate commit, but that
 got old quickly, and I lumped all of the remaining changes into the
 9th commit.  If that's a problem, let me know and I'll separate it.


 Jim Meyering (9):
   ccid: declare DEFAULT_ATR table to be static const
   tcg: declare __jit_debug_descriptor to be static
   alpha-dis: remove unused global; declare others to be static
   linux-user: arg_table need not have global scope
   ccid: make backend_enum_table static const and adjust users
   sheepdog: declare bdrv_sheepdog to be static
   mips-dis: declare four globals to be static
   bonito: declare bonito_state to be static
   convert many more globals to static

  alpha-dis.c   | 26 ++--
  arm-dis.c |  8 ++---
  block/sheepdog.c  |  2 +-
  cpus.c|  4 +--
  cris-dis.c|  2 +-
  hw/9pfs/virtio-9p-synth.c |  2 +-
  hw/bonito.c   |  2 +-
  hw/ccid-card-emulated.c   |  6 ++--
  hw/ccid-card-passthru.c   |  2 +-
  hw/ide/pci.c  |  2 +-
  hw/leon3.c|  2 +-
  hw/mips_fulong2e.c|  2 +-
  hw/s390-virtio-bus.c  |  2 +-
  hw/spapr_rtas.c   |  2 +-
  hw/xen_platform.c |  2 +-
  hw/xgmac.c|  2 +-
  linux-user/main.c |  6 ++--
  m68k-dis.c| 79 
 ---
  memory.c  |  2 +-
  microblaze-dis.c  |  6 ++--
  mips-dis.c| 15 +
  ppc-dis.c | 26 
  sh4-dis.c |  2 +-
  target-cris/translate.c   |  2 +-
  target-i386/cpu.c |  4 +--
  target-i386/kvm.c |  2 +-
  tcg/tcg.c |  2 +-
  tests/fdc-test.c  |  2 +-
  vl.c  | 12 ---
  29 files changed, 110 insertions(+), 118 deletions(-)

I've just rebased my local branch with these patches and
see that they are still pending.

Let me know if you're interested.

Re: [Qemu-devel] [PATCH 4/6] sheepdog: don't leak socket file descriptor upon connection failure

2012-08-17 Thread Jim Meyering

Kevin Wolf wrote:

 Am 17.08.2012 15:30, schrieb Jim Meyering:
 Kevin Wolf wrote:
 Am 16.05.2012 15:07, schrieb Jim Meyering:
 From: Jim Meyering meyer...@redhat.com

 Signed-off-by: Jim Meyering meyer...@redhat.com

 Acked-by: Kevin Wolf kw...@redhat.com

 Hi Kevin,

 AFAICS, only one of these 6 patches has been applied.
 From what I recall (it's been nearly 3mo), there was good
 feedback and I posted at least one V2 patch.
 For reference, here's the start of the series:

 http://marc.info/?l=qemu-develm=133717388221635w=2

 Let me know if there's anything I can do to help.

 Oh, that's bad. This series is spreads across several subsystems, so by
 acking the sheepdog patch (the only block layer one) I was intending to
 signal that I'm okay with merging it, but that I expect a global
 maintainer to actually commit it.

 Did all your other series get merged? There were a lot more patches with
 small fixes and I can't see them in git master at all. I seem to
 remember that they got delayed because you posted them late during the
 last freeze, but obviously they should have been long committed now.

I'm going through them now.
So far, it looks like most have been deferred.

Re: [Qemu-devel] [PATCHv2 2/2] envlist.c: handle strdup failure

2012-08-17 Thread Jim Meyering

Andreas Färber wrote:

 Am 17.08.2012 15:35, schrieb Jim Meyering:
 Jim Meyering wrote:
 From: Jim Meyering meyer...@redhat.com

 Without this, envlist_to_environ may silently fail to copy all
 strings into the destination buffer, and both callers would leak
 any env strings allocated after a failing strdup, because the
 freeing code stops at the first NULL pointer.

 Signed-off-by: Jim Meyering meyer...@redhat.com
 ---
  envlist.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

 diff --git a/envlist.c b/envlist.c
 index be0addb..7532091 100644
 --- a/envlist.c
 +++ b/envlist.c
 @@ -234,8 +234,16 @@ envlist_to_environ(const envlist_t *envlist, size_t 
 *count)
  return (NULL);

  for (entry = envlist-el_entries.lh_first; entry != NULL;
 -entry = entry-ev_link.le_next) {
 -*(penv++) = strdup(entry-ev_var);
 + entry = entry-ev_link.le_next, penv++) {
 +*penv = strdup(entry-ev_var);
 +if (*penv == NULL) {
 +char **e = env;
 +while (e = penv) {
 +free(*e++);
 +}
 +free(env);
 +return NULL;
 +}
  }
  *penv = NULL; /* NULL terminate the list */

 It seems this has been lost in this list's high volume of patches.
 Anyone interested?  Repost desired?

 You announced a v3 but replied to v2?

Here are links to v3:

  [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces
  https://lists.gnu.org/archive/html/qemu-devel/2012-05/msg02967.html

  [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure
  https://lists.gnu.org/archive/html/qemu-devel/2012-05/msg02966.html

 Indentation looks odd in 2/2, too.

That was fixed in v3.

[Qemu-devel] [PATCHv4 0/2] envlist.c: handle strdup failure

2012-08-17 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Differences from v3 (no semantic change):
  - change 1/2 so this file conforms more closely to QEMU's coding style,
by adding braces around each one-line if body (there was no one-line
else- or while-block).
  - move an indentation correction from 2/2 into 1/2

Jim Meyering (2):
  envlist.c: conform to QEMU's coding style
  envlist.c: handle strdup failure

 envlist.c | 284 +-
 1 file changed, 152 insertions(+), 132 deletions(-)

--
1.7.12.rc2

[Qemu-devel] [PATCHv4 1/2] envlist.c: conform to QEMU's coding style

2012-08-17 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Convert each TAB(width-4) to equivalent spaces.
Put braces around each one-line if-body.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 envlist.c | 268 --
 1 file changed, 140 insertions(+), 128 deletions(-)

diff --git a/envlist.c b/envlist.c
index f2303cd..230596f 100644
--- a/envlist.c
+++ b/envlist.c
@@ -8,13 +8,13 @@
 #include envlist.h

 struct envlist_entry {
-   const char *ev_var; /* actual env value */
-   QLIST_ENTRY(envlist_entry) ev_link;
+const char *ev_var; /* actual env value */
+QLIST_ENTRY(envlist_entry) ev_link;
 };

 struct envlist {
-   QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */
-   size_t el_count;/* number of entries */
+QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */
+size_t el_count;/* number of entries */
 };

 static int envlist_parse(envlist_t *envlist,
@@ -27,15 +27,16 @@ static int envlist_parse(envlist_t *envlist,
 envlist_t *
 envlist_create(void)
 {
-   envlist_t *envlist;
+envlist_t *envlist;

-   if ((envlist = malloc(sizeof (*envlist))) == NULL)
-   return (NULL);
+if ((envlist = malloc(sizeof(*envlist))) == NULL) {
+return NULL;
+}

-   QLIST_INIT(envlist-el_entries);
-   envlist-el_count = 0;
+QLIST_INIT(envlist-el_entries);
+envlist-el_count = 0;

-   return (envlist);
+return envlist;
 }

 /*
@@ -44,18 +45,18 @@ envlist_create(void)
 void
 envlist_free(envlist_t *envlist)
 {
-   struct envlist_entry *entry;
+struct envlist_entry *entry;

-   assert(envlist != NULL);
+assert(envlist != NULL);

-   while (envlist-el_entries.lh_first != NULL) {
-   entry = envlist-el_entries.lh_first;
-   QLIST_REMOVE(entry, ev_link);
+while (envlist-el_entries.lh_first != NULL) {
+entry = envlist-el_entries.lh_first;
+QLIST_REMOVE(entry, ev_link);

-   free((char *)entry-ev_var);
-   free(entry);
-   }
-   free(envlist);
+free((char *)entry-ev_var);
+free(entry);
+}
+free(envlist);
 }

 /*
@@ -72,7 +73,7 @@ envlist_free(envlist_t *envlist)
 int
 envlist_parse_set(envlist_t *envlist, const char *env)
 {
-   return (envlist_parse(envlist, env, envlist_setenv));
+return envlist_parse(envlist, env, envlist_setenv);
 }

 /*
@@ -84,7 +85,7 @@ envlist_parse_set(envlist_t *envlist, const char *env)
 int
 envlist_parse_unset(envlist_t *envlist, const char *env)
 {
-   return (envlist_parse(envlist, env, envlist_unsetenv));
+return envlist_parse(envlist, env, envlist_unsetenv);
 }

 /*
@@ -97,32 +98,34 @@ static int
 envlist_parse(envlist_t *envlist, const char *env,
 int (*callback)(envlist_t *, const char *))
 {
-   char *tmpenv, *envvar;
-   char *envsave = NULL;
-
-   assert(callback != NULL);
-
-   if ((envlist == NULL) || (env == NULL))
-   return (EINVAL);
-
-   /*
-* We need to make temporary copy of the env string
-* as strtok_r(3) modifies it while it tokenizes.
-*/
-   if ((tmpenv = strdup(env)) == NULL)
-   return (errno);
-
-   envvar = strtok_r(tmpenv, ,, envsave);
-   while (envvar != NULL) {
-   if ((*callback)(envlist, envvar) != 0) {
-   free(tmpenv);
-   return (errno);
-   }
-   envvar = strtok_r(NULL, ,, envsave);
-   }
-
-   free(tmpenv);
-   return (0);
+char *tmpenv, *envvar;
+char *envsave = NULL;
+
+assert(callback != NULL);
+
+if ((envlist == NULL) || (env == NULL)) {
+return EINVAL;
+}
+
+/*
+ * We need to make temporary copy of the env string
+ * as strtok_r(3) modifies it while it tokenizes.
+ */
+if ((tmpenv = strdup(env)) == NULL) {
+return errno;
+}
+
+envvar = strtok_r(tmpenv, ,, envsave);
+while (envvar != NULL) {
+if ((*callback)(envlist, envvar) != 0) {
+free(tmpenv);
+return errno;
+}
+envvar = strtok_r(NULL, ,, envsave);
+}
+
+free(tmpenv);
+return 0;
 }

 /*
@@ -134,46 +137,50 @@ envlist_parse(envlist_t *envlist, const char *env,
 int
 envlist_setenv(envlist_t *envlist, const char *env)
 {
-   struct envlist_entry *entry = NULL;
-   const char *eq_sign;
-   size_t envname_len;
-
-   if ((envlist == NULL) || (env == NULL))
-   return (EINVAL);
-
-   /* find out first equals sign in given env */
-   if ((eq_sign = strchr(env, '=')) == NULL)
-   return (EINVAL);
-   envname_len = eq_sign - env + 1;
-
-   /*
-* If there already exists variable with given name
-* we remove and release it before allocating a whole
-* new entry.
-*/
-   for (entry

[Qemu-devel] [PATCHv4 2/2] envlist.c: handle strdup failure

2012-08-17 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Without this, envlist_to_environ may silently fail to copy all
strings into the destination buffer, and both callers would leak
any env strings allocated after a failing strdup, because the
freeing code stops at the first NULL pointer.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 envlist.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/envlist.c b/envlist.c
index 230596f..cf3f2d8 100644
--- a/envlist.c
+++ b/envlist.c
@@ -245,8 +245,16 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)
 }

 for (entry = envlist-el_entries.lh_first; entry != NULL;
- entry = entry-ev_link.le_next) {
-*(penv++) = strdup(entry-ev_var);
+ entry = entry-ev_link.le_next, penv++) {
+*penv = strdup(entry-ev_var);
+if (*penv == NULL) {
+char **e = env;
+while (e = penv) {
+free(*e++);
+}
+free(env);
+return NULL;
+}
 }
 *penv = NULL; /* NULL terminate the list */

-- 
1.7.12.rc2

Re: [Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure

2012-08-17 Thread Jim Meyering

Andreas Färber wrote:
 Am 22.05.2012 12:16, schrieb Jim Meyering:
 From: Jim Meyering meyer...@redhat.com

 Without this, envlist_to_environ may silently fail to copy all
 strings into the destination buffer, and both callers would leak
 any env strings allocated after a failing strdup, because the
 freeing code stops at the first NULL pointer.

 Signed-off-by: Jim Meyering meyer...@redhat.com
 ---
  envlist.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

 diff --git a/envlist.c b/envlist.c
 index e44889b..df5c723 100644
 --- a/envlist.c
 +++ b/envlist.c
 @@ -234,8 +234,16 @@ envlist_to_environ(const envlist_t *envlist, size_t 
 *count)
  return (NULL);

  for (entry = envlist-el_entries.lh_first; entry != NULL;
 -entry = entry-ev_link.le_next) {
 -*(penv++) = strdup(entry-ev_var);
 + entry = entry-ev_link.le_next, penv++) {

 Scratch my comment on 1/2, there's an added penv++ that I overlooked.
 Not changing the indentation twice would still be nice.

I posted a V4 that does that, and removes parens around return arguments
as well.  However, I did not remove assignments from conditionals.  IMHO,
that would require an inordinate amount of change for the marginal gain
of making legacy code conform.

Re: [Qemu-devel] [PATCH 08/22] linux-user: remove two unchecked uses of strdup

2012-05-30 Thread Jim Meyering

Anthony Liguori wrote:
 On 05/09/2012 10:12 PM, Jim Meyering wrote:
 Peter Maydell wrote:
 On 9 May 2012 15:01, Jim Meyeringj...@meyering.net  wrote:
  From 402100deb7e27b1d7ac619ebac963f861fae91b0 Mon Sep 17 00:00:00 2001
 From: Jim Meyeringmeyer...@redhat.com
 Date: Mon, 7 May 2012 18:34:26 +0200
 Subject: [PATCH] linux-user: remove two unchecked uses of strdup

 Remove two uses of strdup (use g_path_get_basename instead),
 and add a comment that this strncpy use is ok.

 Signed-off-by: Jim Meyeringmeyer...@redhat.com

 This version
 Reviewed-by: Peter Maydellpeter.mayd...@linaro.org

 but you'll need to retransmit it (presumably as part of a v2
 of this series) so it is in the proper format for a patch email
 (otherwise all the conversational chatter ends up in the git
 commit message).

 Thanks again.
 I'm amending to reflect ACKs and Reviewed-by: comments as they come in,
 as well as corrections like yours.

 I expect to repost the whole series (or any subset) when appropriate.
 Or I can push it to a publicly-accessible repository somewhere.

 Has this gotten reposted?  I don't see a new version on the list.

Hi Anthony,
I've reposted (as V2, V3, etc.) any individual patch that required revision,
but have not reposted the series.  I would have reposted, but I recall
someone saying that their tools managed quite well with the one-off
reposts.

Would you like me to repost just this one or the whole 22-patch series?

[Qemu-devel] [PATCHv2 03/22] block: avoid buffer overrun by using pstrcpy, not strncpy

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Also, use PATH_MAX, rather than the arbitrary 1024.
Using PATH_MAX is more consistent with other filename-related
variables in this file, like backing_filename and tmp_filename.

Acked-by: Kevin Wolf kw...@redhat.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 block.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index af2ab4f..efc7071 100644
--- a/block.c
+++ b/block.c
@@ -1232,7 +1232,7 @@ int bdrv_commit(BlockDriverState *bs)
 int n, ro, open_flags;
 int ret = 0, rw_ret = 0;
 uint8_t *buf;
-char filename[1024];
+char filename[PATH_MAX];
 BlockDriverState *bs_rw, *bs_ro;

 if (!drv)
@@ -1252,7 +1252,8 @@ int bdrv_commit(BlockDriverState *bs)

 backing_drv = bs-backing_hd-drv;
 ro = bs-backing_hd-read_only;
-strncpy(filename, bs-backing_hd-filename, sizeof(filename));
+/* Use pstrcpy (not strncpy): filename must be NUL-terminated. */
+pstrcpy(filename, sizeof(filename), bs-backing_hd-filename);
 open_flags =  bs-backing_hd-open_flags;

 if (ro) {
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 20/22] hw/r2d: add comment: this strncpy use is ok

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/r2d.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/r2d.c b/hw/r2d.c
index c55de01..ed841c5 100644
--- a/hw/r2d.c
+++ b/hw/r2d.c
@@ -328,6 +328,8 @@ static void r2d_init(ram_addr_t ram_size,
 }

 if (kernel_cmdline) {
+/* I see no evidence that this .kernel_cmdline buffer requires
+   NUL-termination, so using strncpy should be ok. */
 strncpy(boot_params.kernel_cmdline, kernel_cmdline,
 sizeof(boot_params.kernel_cmdline));
 }
-- 
1.7.10.2.605.gbefc5ed

Re: [Qemu-devel] [PATCH 08/22] linux-user: remove two unchecked uses of strdup

2012-05-30 Thread Jim Meyering

Anthony Liguori wrote:
 On 05/30/2012 03:12 PM, Jim Meyering wrote:
 Anthony Liguori wrote:
 On 05/09/2012 10:12 PM, Jim Meyering wrote:
 Peter Maydell wrote:
 On 9 May 2012 15:01, Jim Meyeringj...@meyering.net   wrote:
From 402100deb7e27b1d7ac619ebac963f861fae91b0 Mon Sep 17 00:00:00 2001
 From: Jim Meyeringmeyer...@redhat.com
 Date: Mon, 7 May 2012 18:34:26 +0200
 Subject: [PATCH] linux-user: remove two unchecked uses of strdup

 Remove two uses of strdup (use g_path_get_basename instead),
 and add a comment that this strncpy use is ok.

 Signed-off-by: Jim Meyeringmeyer...@redhat.com

 This version
 Reviewed-by: Peter Maydellpeter.mayd...@linaro.org

 but you'll need to retransmit it (presumably as part of a v2
 of this series) so it is in the proper format for a patch email
 (otherwise all the conversational chatter ends up in the git
 commit message).

 Thanks again.
 I'm amending to reflect ACKs and Reviewed-by: comments as they come in,
 as well as corrections like yours.

 I expect to repost the whole series (or any subset) when appropriate.
 Or I can push it to a publicly-accessible repository somewhere.

 Has this gotten reposted?  I don't see a new version on the list.

 Hi Anthony,
 I've reposted (as V2, V3, etc.) any individual patch that required revision,
 but have not reposted the series.  I would have reposted, but I recall
 someone saying that their tools managed quite well with the one-off
 reposts.

 I corrected myself later as I misunderstood your question.  It handles
 the acked-bys but not partial updates to a patch series.

 Would you like me to repost just this one or the whole 22-patch series?

 Could you please repost the full series?

Sure.  Rebased, confirmed it still compiles and passes make check
and reposted as v2.

FYI, this time I sent only to qemu-devel, rather than adding per-patch Cc's.

[Qemu-devel] [PATCHv2 09/22] ppc: avoid buffer overrun: use pstrcpy, not strncpy

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

A terminal NUL is required by caller's use of strchr.
It's better not to use strncpy at all, since there is no need
to zero out hundreds of trailing bytes for each iteration.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 target-ppc/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index c09cc39..fb79e9f 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -587,7 +587,7 @@ static int read_cpuinfo(const char *field, char *value, int 
len)
 break;
 }
 if (!strncmp(line, field, field_len)) {
-strncpy(value, line, len);
+pstrcpy(value, len, line);
 ret = 0;
 break;
 }
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 02/22] sparc: use g_strdup in place of unchecked strdup

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

This avoids a NULL-deref upon strdup failure.
Also update matching free to g_free.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 target-sparc/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index 7ac6bdb..1e31318 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -648,7 +648,7 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, 
const char *cpu_model)
 {
 unsigned int i;
 const sparc_def_t *def = NULL;
-char *s = strdup(cpu_model);
+char *s = g_strdup(cpu_model);
 char *featurestr, *name = strtok(s, ,);
 uint32_t plus_features = 0;
 uint32_t minus_features = 0;
@@ -740,7 +740,7 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, 
const char *cpu_model)
 #ifdef DEBUG_FEATURES
 print_features(stderr, fprintf, cpu_def-features, NULL);
 #endif
-free(s);
+g_free(s);
 return 0;

  error:
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 12/22] bt: replace fragile snprintf use and unwarranted strncpy

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

In bt_hci_name_req a failed snprintf could return len larger than
sizeof(params.name), which means the following memset call would
have a length value of (size_t)-1, -2, etc...  Sounds scary.
But currently, one can deduce that there is no problem:
strlen(slave-lmp_name) is guaranteed to be smaller than
CHANGE_LOCAL_NAME_CP_SIZE, which is the same as sizeof(params.name),
so this cannot happen.  Regardless, there is no justification for
using snprintf+memset.  Use pstrcpy instead.

Also, in bt_hci_event_complete_read_local_name, use pstrcpy in place
of unwarranted strncpy.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/bt-hci.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/bt-hci.c b/hw/bt-hci.c
index a3a7fb4..47f9a4e 100644
--- a/hw/bt-hci.c
+++ b/hw/bt-hci.c
@@ -943,7 +943,6 @@ static int bt_hci_name_req(struct bt_hci_s *hci, bdaddr_t 
*bdaddr)
 {
 struct bt_device_s *slave;
 evt_remote_name_req_complete params;
-int len;

 for (slave = hci-device.net-slave; slave; slave = slave-next)
 if (slave-page_scan  !bacmp(slave-bd_addr, bdaddr))
@@ -955,9 +954,7 @@ static int bt_hci_name_req(struct bt_hci_s *hci, bdaddr_t 
*bdaddr)

 params.status   = HCI_SUCCESS;
 bacpy(params.bdaddr, slave-bd_addr);
-len = snprintf(params.name, sizeof(params.name),
-%s, slave-lmp_name ?: );
-memset(params.name + len, 0, sizeof(params.name) - len);
+pstrcpy(params.name, sizeof(params.name), slave-lmp_name ?: );
 bt_hci_event(hci, EVT_REMOTE_NAME_REQ_COMPLETE,
 params, EVT_REMOTE_NAME_REQ_COMPLETE_SIZE);

@@ -1388,7 +1385,7 @@ static inline void 
bt_hci_event_complete_read_local_name(struct bt_hci_s *hci)
 params.status = HCI_SUCCESS;
 memset(params.name, 0, sizeof(params.name));
 if (hci-device.lmp_name)
-strncpy(params.name, hci-device.lmp_name, sizeof(params.name));
+pstrcpy(params.name, sizeof(params.name), hci-device.lmp_name);

 bt_hci_event_complete(hci, params, READ_LOCAL_NAME_RP_SIZE);
 }
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 14/22] vscsi: avoid unwarranted strncpy

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Don't use strncpy when the source string is known to fit
in the destination buffer.  Use equivalent memcpy.
We could even use strcpy, here, but some static analyzers
warn about that, so don't add new uses.

Acked-by: David Gibson da...@gibson.dropbear.id.au
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/spapr_vscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 037867a..f4fc898 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -736,7 +736,7 @@ static int vscsi_send_adapter_info(VSCSIState *s, vscsi_req 
*req)
 #endif
 memset(info, 0, sizeof(info));
 strcpy(info.srp_version, SRP_VERSION);
-strncpy(info.partition_name, qemu, sizeof(qemu));
+memcpy(info.partition_name, qemu, sizeof(qemu));
 info.partition_number = cpu_to_be32(0);
 info.mad_version = cpu_to_be32(1);
 info.os_type = cpu_to_be32(2);
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 13/22] virtio-9p: avoid unwarranted uses of strncpy

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

In all of these cases, the uses of strncpy were unnecessary, since
at each point of use we know that the NUL-terminated source bytes
fit in the destination buffer.  Use memcpy in place of strncpy.

Acked-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/9pfs/virtio-9p-posix-acl.c  | 6 --
 hw/9pfs/virtio-9p-xattr-user.c | 3 ++-
 hw/9pfs/virtio-9p-xattr.c  | 3 ++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/virtio-9p-posix-acl.c b/hw/9pfs/virtio-9p-posix-acl.c
index a1948e3..c064017 100644
--- a/hw/9pfs/virtio-9p-posix-acl.c
+++ b/hw/9pfs/virtio-9p-posix-acl.c
@@ -44,7 +44,8 @@ static ssize_t mp_pacl_listxattr(FsContext *ctx, const char 
*path,
 return -1;
 }

-strncpy(value, ACL_ACCESS, len);
+/* len includes the trailing NUL */
+memcpy(value, ACL_ACCESS, len);
 return 0;
 }

@@ -95,7 +96,8 @@ static ssize_t mp_dacl_listxattr(FsContext *ctx, const char 
*path,
 return -1;
 }

-strncpy(value, ACL_DEFAULT, len);
+/* len includes the trailing NUL */
+memcpy(value, ACL_ACCESS, len);
 return 0;
 }

diff --git a/hw/9pfs/virtio-9p-xattr-user.c b/hw/9pfs/virtio-9p-xattr-user.c
index 5044a3e..5bb6020 100644
--- a/hw/9pfs/virtio-9p-xattr-user.c
+++ b/hw/9pfs/virtio-9p-xattr-user.c
@@ -61,7 +61,8 @@ static ssize_t mp_user_listxattr(FsContext *ctx, const char 
*path,
 return -1;
 }

-strncpy(value, name, name_size);
+/* name_size includes the trailing NUL. */
+memcpy(value, name, name_size);
 return name_size;
 }

diff --git a/hw/9pfs/virtio-9p-xattr.c b/hw/9pfs/virtio-9p-xattr.c
index 7f08f6e..a839606 100644
--- a/hw/9pfs/virtio-9p-xattr.c
+++ b/hw/9pfs/virtio-9p-xattr.c
@@ -53,7 +53,8 @@ ssize_t pt_listxattr(FsContext *ctx, const char *path,
 return -1;
 }

-strncpy(value, name, name_size);
+/* no need for strncpy: name_size is strlen(name)+1 */
+memcpy(value, name, name_size);
 return name_size;
 }

-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 16/22] qemu-ga: prefer pstrcpy: consistently NUL-terminate ifreq.ifr_name

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

NUL-termination of the .ifr_name field is not required, but is fine
(and preferable to using strncpy and leaving the reader to wonder),
since the first thing the linux kernel does is to clear the last byte.
Besides, using pstrcpy here makes this setting of ifr_name consistent
with the other code (e.g., net/tap-linux.c) that does the same thing.

Reviewed-by: Luiz Capitulino lcapitul...@redhat.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 qga/commands-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index dab3bf9..607aad7 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -759,7 +759,7 @@ GuestNetworkInterfaceList 
*qmp_guest_network_get_interfaces(Error **errp)
 }

 memset(ifr, 0, sizeof(ifr));
-strncpy(ifr.ifr_name,  info-value-name, IF_NAMESIZE);
+pstrcpy(ifr.ifr_name, IF_NAMESIZE, info-value-name);
 if (ioctl(sock, SIOCGIFHWADDR, ifr) == -1) {
 snprintf(err_msg, sizeof(err_msg),
  failed to get MAC address of %s: %s,
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 04/22] sheepdog: avoid a few buffer overruns

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

* parse_vdiname: Use pstrcpy, not strncpy, when the destination
buffer must be NUL-terminated.
* sd_open: Likewise, avoid buffer overrun.
* do_sd_create: Likewise.  Leave the preceding memset, since
pstrcpy does not NUL-fill, and filename needs that.
* sd_snapshot_create: Add a comment/question.
* find_vdi_name: Remove a useless memset.
* sd_snapshot_goto: Remove a useless memset.
Use pstrcpy to NUL-terminate, because find_vdi_name requires
that its vdi arg (filename parameter) be NUL-terminated.
It seems ok not to NUL-fill the buffer.
Do the same for snapid: remove useless memset-0 (instead,
zero tag[0]).  Use pstrcpy, not strncpy.
* sd_snapshot_list: Use pstrcpy, not strncpy to write
into the -name member.  Each must be NUL-terminated.

Acked-by: Kevin Wolf kw...@redhat.com
Acked-by: MORITA Kazutaka morita.kazut...@lab.ntt.co.jp
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 block/sheepdog.c | 34 ++
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 6d52277..6f405b5 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -857,14 +857,14 @@ static int parse_vdiname(BDRVSheepdogState *s, const char 
*filename,
 s-port = 0;
 }

-strncpy(vdi, p, SD_MAX_VDI_LEN);
+pstrcpy(vdi, SD_MAX_VDI_LEN, p);

 p = strchr(vdi, ':');
 if (p) {
 *p++ = '\0';
 *snapid = strtoul(p, NULL, 10);
 if (*snapid == 0) {
-strncpy(tag, p, SD_MAX_VDI_TAG_LEN);
+pstrcpy(tag, SD_MAX_VDI_TAG_LEN, p);
 }
 } else {
 *snapid = CURRENT_VDI_ID; /* search current vdi */
@@ -891,7 +891,10 @@ static int find_vdi_name(BDRVSheepdogState *s, char 
*filename, uint32_t snapid,
 return fd;
 }

-memset(buf, 0, sizeof(buf));
+/* This pair of strncpy calls ensures that the buffer is zero-filled,
+ * which is desirable since we'll soon be sending those bytes, and
+ * don't want the send_req to read uninitialized data.
+ */
 strncpy(buf, filename, SD_MAX_VDI_LEN);
 strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);

@@ -1141,7 +1144,7 @@ static int sd_open(BlockDriverState *bs, const char 
*filename, int flags)
 s-max_dirty_data_idx = 0;

 bs-total_sectors = s-inode.vdi_size / SECTOR_SIZE;
-strncpy(s-name, vdi, sizeof(s-name));
+pstrcpy(s-name, sizeof(s-name), vdi);
 qemu_co_mutex_init(s-lock);
 g_free(buf);
 return 0;
@@ -1169,8 +1172,11 @@ static int do_sd_create(char *filename, int64_t vdi_size,
 return fd;
 }

+/* FIXME: would it be better to fail (e.g., return -EIO) when filename
+ * does not fit in buf?  For now, just truncate and avoid buffer overrun.
+ */
 memset(buf, 0, sizeof(buf));
-strncpy(buf, filename, SD_MAX_VDI_LEN);
+pstrcpy(buf, sizeof(buf), filename);

 memset(hdr, 0, sizeof(hdr));
 hdr.opcode = SD_OP_NEW_VDI;
@@ -1740,6 +1746,9 @@ static int sd_snapshot_create(BlockDriverState *bs, 
QEMUSnapshotInfo *sn_info)

 s-inode.vm_state_size = sn_info-vm_state_size;
 s-inode.vm_clock_nsec = sn_info-vm_clock_nsec;
+/* It appears that inode.tag does not require a NUL terminator,
+ * which means this use of strncpy is ok.
+ */
 strncpy(s-inode.tag, sn_info-name, sizeof(s-inode.tag));
 /* we don't need to update entire object */
 datalen = SD_INODE_SIZE - sizeof(s-inode.data_vdi_id);
@@ -1799,13 +1808,13 @@ static int sd_snapshot_goto(BlockDriverState *bs, const 
char *snapshot_id)

 memcpy(old_s, s, sizeof(BDRVSheepdogState));

-memset(vdi, 0, sizeof(vdi));
-strncpy(vdi, s-name, sizeof(vdi));
+pstrcpy(vdi, sizeof(vdi), s-name);

-memset(tag, 0, sizeof(tag));
 snapid = strtoul(snapshot_id, NULL, 10);
-if (!snapid) {
-strncpy(tag, s-name, sizeof(tag));
+if (snapid) {
+tag[0] = 0;
+} else {
+pstrcpy(tag, sizeof(tag), s-name);
 }

 ret = find_vdi_name(s, vdi, snapid, tag, vid, 1);
@@ -1934,8 +1943,9 @@ static int sd_snapshot_list(BlockDriverState *bs, 
QEMUSnapshotInfo **psn_tab)

 snprintf(sn_tab[found].id_str, sizeof(sn_tab[found].id_str), %u,
  inode.snap_id);
-strncpy(sn_tab[found].name, inode.tag,
-MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)));
+pstrcpy(sn_tab[found].name,
+MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)),
+inode.tag);
 found++;
 }
 }
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 01/22] scsi, pci, qdev, isa-bus, sysbus: don't let *_get_fw_dev_path return NULL

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Use g_strdup rather than strdup, because the sole caller
(qdev_get_fw_dev_path_helper) assumes it gets non-NULL, and dereferences
it.  Besides, in that caller, the allocated buffer is already freed with
g_free, so it's better to allocate with a matching g_strdup.

In one case, (scsi-bus.c) it was trivial, so I replaced an snprintf+
g_strdup combination with an equivalent g_strdup_printf use.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/ide/qdev.c | 2 +-
 hw/isa-bus.c  | 2 +-
 hw/pci.c  | 2 +-
 hw/qdev.c | 2 +-
 hw/scsi-bus.c | 8 ++--
 hw/sysbus.c   | 2 +-
 6 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index a46578d..9edfd8b 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -50,7 +50,7 @@ static char *idebus_get_fw_dev_path(DeviceState *dev)
 snprintf(path, sizeof(path), %s@%d, qdev_fw_name(dev),
  ((IDEBus*)dev-parent_bus)-bus_id);

-return strdup(path);
+return g_strdup(path);
 }

 static int ide_qdev_init(DeviceState *qdev)
diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 5a43f03..822b040 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -227,7 +227,7 @@ static char *isabus_get_fw_dev_path(DeviceState *dev)
 snprintf(path + off, sizeof(path) - off, @%04x, d-ioport_id);
 }

-return strdup(path);
+return g_strdup(path);
 }

 MemoryRegion *isa_address_space(ISADevice *dev)
diff --git a/hw/pci.c b/hw/pci.c
index c1ebdde..79f46e6 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1897,7 +1897,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev)
PCI_SLOT(d-devfn));
 if (PCI_FUNC(d-devfn))
 snprintf(path + off, sizeof(path) + off, ,%x, PCI_FUNC(d-devfn));
-return strdup(path);
+return g_strdup(path);
 }

 static char *pcibus_get_dev_path(DeviceState *dev)
diff --git a/hw/qdev.c b/hw/qdev.c
index 6a8f6bd..ab299cf 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -512,7 +512,7 @@ char* qdev_get_fw_dev_path(DeviceState *dev)

 path[l-1] = '\0';

-return strdup(path);
+return g_strdup(path);
 }

 static char *qdev_get_type(Object *obj, Error **errp)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index f10f3ec..3edda28 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1470,12 +1470,8 @@ static char *scsibus_get_dev_path(DeviceState *dev)
 static char *scsibus_get_fw_dev_path(DeviceState *dev)
 {
 SCSIDevice *d = SCSI_DEVICE(dev);
-char path[100];
-
-snprintf(path, sizeof(path), channel@%x/%s@%x,%x, d-channel,
- qdev_fw_name(dev), d-id, d-lun);
-
-return strdup(path);
+return g_strdup_printf(channel@%x/%s@%x,%x, d-channel,
+   qdev_fw_name(dev), d-id, d-lun);
 }

 SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
diff --git a/hw/sysbus.c b/hw/sysbus.c
index db4efcc..8a9b85d 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -203,7 +203,7 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
 snprintf(path + off, sizeof(path) - off, @i%04x, s-pio[0]);
 }

-return strdup(path);
+return g_strdup(path);
 }

 void sysbus_add_memory(SysBusDevice *dev, target_phys_addr_t addr,
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 19/22] qcow2: mark this file's sole strncpy use as justified

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Acked-by: Kevin Wolf kw...@redhat.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index c2e49cd..6d34f1a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -994,6 +994,7 @@ int qcow2_update_header(BlockDriverState *bs)
 goto fail;
 }

+/* Using strncpy is ok here, since buf is not NUL-terminated. */
 strncpy(buf, bs-backing_file, buflen);

 header-backing_file_offset = cpu_to_be64(buf - ((char*) header));
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 06/22] hw/9pfs: avoid buffer overrun

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

v9fs_add_dir_node and qemu_v9fs_synth_add_file used strncpy
to form node-name, which requires NUL-termination, but
strncpy does not ensure NUL-termination.
Use pstrcpy, which does.

Acked-by: Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/9pfs/virtio-9p-synth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/virtio-9p-synth.c b/hw/9pfs/virtio-9p-synth.c
index 92e0b09..e95a856 100644
--- a/hw/9pfs/virtio-9p-synth.c
+++ b/hw/9pfs/virtio-9p-synth.c
@@ -58,7 +58,7 @@ static V9fsSynthNode *v9fs_add_dir_node(V9fsSynthNode 
*parent, int mode,
 node-attr-read  = NULL;
 }
 node-private = node;
-strncpy(node-name, name, sizeof(node-name));
+pstrcpy(node-name, sizeof(node-name), name);
 QLIST_INSERT_HEAD_RCU(parent-child, node, sibling);
 return node;
 }
@@ -132,7 +132,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int 
mode,
 node-attr-write  = write;
 node-attr-mode   = mode;
 node-private  = arg;
-strncpy(node-name, name, sizeof(node-name));
+pstrcpy(node-name, sizeof(node-name), name);
 QLIST_INSERT_HEAD_RCU(parent-child, node, sibling);
 ret = 0;
 err_out:
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 22/22] doc: update HACKING wrt strncpy/pstrcpy

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Reword the section on strncpy: its NUL-filling is important
in some cases.  Mention that pstrcpy's signature is different.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 HACKING | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/HACKING b/HACKING
index 471cf1d..617 100644
--- a/HACKING
+++ b/HACKING
@@ -91,10 +91,11 @@ emulators.

 4. String manipulation

-Do not use the strncpy function.  According to the man page, it does
-*not* guarantee a NULL-terminated buffer, which makes it extremely dangerous
-to use.  Instead, use functionally equivalent function:
-void pstrcpy(char *buf, int buf_size, const char *str)
+Do not use the strncpy function.  As mentioned in the man page, it does *not*
+guarantee a NULL-terminated buffer, which makes it extremely dangerous to use.
+It also zeros trailing destination bytes out to the specified length.  Instead,
+use this similar function when possible, but note its different signature:
+void pstrcpy(char *dest, int dest_buf_size, const char *src)

 Don't use strcat because it can't check for buffer overflows, but:
 char *pstrcat(char *buf, int buf_size, const char *s)
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 08/22] os-posix: avoid buffer overrun

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

os_set_proc_name: Use pstrcpy, in place of strncpy and the
ineffectual preceding assignment: name[sizeof(name) - 1] = 0;

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 os-posix.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index daf3d6f..2acfce0 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -148,8 +148,7 @@ void os_set_proc_name(const char *s)
 char name[16];
 if (!s)
 return;
-name[sizeof(name) - 1] = 0;
-strncpy(name, s, sizeof(name));
+pstrcpy(name, sizeof(name), s);
 /* Could rewrite argv[0] too, but that's a bit more complicated.
This simple way is enough for `top'. */
 if (prctl(PR_SET_NAME, name)) {
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 15/22] target-i386: use pstrcpy, not strncpy

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Use pstrcpy rather than strncpy in one more case
(in cpudef_setfield). This makes our handling of -model_id
consistent with another pstrcpy-vs-model_id use below.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 target-i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 388bc5c..722e597 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1275,7 +1275,7 @@ static int cpudef_setfield(const char *name, const char 
*str, void *opaque)
 g_free((void *)def-name);
 def-name = g_strdup(str);
 } else if (!strcmp(name, model_id)) {
-strncpy(def-model_id, str, sizeof (def-model_id));
+pstrcpy(def-model_id, sizeof(def-model_id), str);
 } else if (!strcmp(name, level)) {
 setscalar(def-level, str, err)
 } else if (!strcmp(name, vendor)) {
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 11/22] ui/vnc: simplify and avoid strncpy

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Don't bother with strncpy.  There's no need for its zero-fill.
Use g_strndup in place of g_malloc+strncpy+NUL-terminate.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 ui/vnc-auth-sasl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 8fba770..bfdcb46 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -432,9 +432,7 @@ static int protocol_client_auth_sasl_start_len(VncState 
*vs, uint8_t *data, size

 static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, 
size_t len)
 {
-char *mechname = g_malloc(len + 1);
-strncpy(mechname, (char*)data, len);
-mechname[len] = '\0';
+char *mechname = g_strndup((const char *) data, len);
 VNC_DEBUG(Got client mechname '%s' check against '%s'\n,
   mechname, vs-sasl.mechlist);

-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 21/22] scsi: mark an strncpy use as valid

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/scsi-bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 3edda28..98170c3 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -406,6 +406,7 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
 r-buf[7] = 0x10 | (r-req.bus-info-tcq ? 0x02 : 0); /* Sync, TCQ.  
*/
 memcpy(r-buf[8], QEMU, 8);
 memcpy(r-buf[16], QEMU TARGET , 16);
+/* This use of strncpy is ok. */
 strncpy((char *) r-buf[32], QEMU_VERSION, 4);
 }
 return true;
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 18/22] acpi: remove strzcpy (strncpy-identical) function; just use strncpy

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Adjust all uses s/strzcpy/strncpy/ and mark these uses
of strncpy as ok.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/acpi.c | 24 
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/hw/acpi.c b/hw/acpi.c
index 5d521e5..45ab345 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -60,18 +60,6 @@ static int acpi_checksum(const uint8_t *data, int len)
 return (-sum)  0xff;
 }

-/* like strncpy() but zero-fills the tail of destination */
-static void strzcpy(char *dst, const char *src, size_t size)
-{
-size_t len = strlen(src);
-if (len = size) {
-len = size;
-} else {
-  memset(dst + len, 0, size - len);
-}
-memcpy(dst, src, len);
-}
-
 /* XXX fixme: this function uses obsolete argument parsing interface */
 int acpi_table_add(const char *t)
 {
@@ -156,7 +144,8 @@ int acpi_table_add(const char *t)
 hdr._length = cpu_to_le16(len);

 if (get_param_value(buf, sizeof(buf), sig, t)) {
-strzcpy(hdr.sig, buf, sizeof(hdr.sig));
+/* strncpy is justified: the field need not be NUL-terminated. */
+strncpy(hdr.sig, buf, sizeof(hdr.sig));
 ++changed;
 }

@@ -186,12 +175,14 @@ int acpi_table_add(const char *t)
 }

 if (get_param_value(buf, sizeof(buf), oem_id, t)) {
-strzcpy(hdr.oem_id, buf, sizeof(hdr.oem_id));
+/* strncpy is justified: the field need not be NUL-terminated. */
+strncpy(hdr.oem_id, buf, sizeof(hdr.oem_id));
 ++changed;
 }

 if (get_param_value(buf, sizeof(buf), oem_table_id, t)) {
-strzcpy(hdr.oem_table_id, buf, sizeof(hdr.oem_table_id));
+/* strncpy is justified: the field need not be NUL-terminated. */
+strncpy(hdr.oem_table_id, buf, sizeof(hdr.oem_table_id));
 ++changed;
 }

@@ -206,7 +197,8 @@ int acpi_table_add(const char *t)
 }

 if (get_param_value(buf, sizeof(buf), asl_compiler_id, t)) {
-strzcpy(hdr.asl_compiler_id, buf, sizeof(hdr.asl_compiler_id));
+/* strncpy is justified: the field need not be NUL-terminated. */
+strncpy(hdr.asl_compiler_id, buf, sizeof(hdr.asl_compiler_id));
 ++changed;
 }

-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 10/22] linux-user: remove two unchecked uses of strdup

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Remove two uses of strdup (use g_path_get_basename instead),
and add a comment that this strncpy use is ok.

Reviewed-by: Peter Maydell peter.mayd...@linaro.org
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 linux-user/elfload.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index f3b1552..8807684 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2316,7 +2316,7 @@ static void fill_prstatus(struct target_elf_prstatus 
*prstatus,

 static int fill_psinfo(struct target_elf_prpsinfo *psinfo, const TaskState *ts)
 {
-char *filename, *base_filename;
+char *base_filename;
 unsigned int i, len;

 (void) memset(psinfo, 0, sizeof (*psinfo));
@@ -2338,13 +2338,15 @@ static int fill_psinfo(struct target_elf_prpsinfo 
*psinfo, const TaskState *ts)
 psinfo-pr_uid = getuid();
 psinfo-pr_gid = getgid();

-filename = strdup(ts-bprm-filename);
-base_filename = strdup(basename(filename));
+base_filename = g_path_get_basename(ts-bprm-filename);
+/*
+ * Using strncpy here is fine: at max-length,
+ * this field is not NUL-terminated.
+ */
 (void) strncpy(psinfo-pr_fname, base_filename,
sizeof(psinfo-pr_fname));
-free(base_filename);
-free(filename);

+g_free(base_filename);
 bswap_psinfo(psinfo);
 return (0);
 }
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 17/22] libcacard/vcard_emul_nss: use pstrcpy in place of strncpy

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Replace strncpy+NUL-terminate use with use of pstrcpy.
This requires linking with cutils.o (or else vssclient doesn't link),
so add that in the Makefile.

Acked-by: Alon Levy al...@redhat.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 libcacard/Makefile | 2 +-
 libcacard/vcard_emul_nss.c | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/libcacard/Makefile b/libcacard/Makefile
index c6a896a..2a5a42d 100644
--- a/libcacard/Makefile
+++ b/libcacard/Makefile
@@ -17,7 +17,7 @@ QEMU_CFLAGS+=-I../

 libcacard.lib-y=$(addsuffix .lo,$(basename $(libcacard-y)))

-vscclient: $(libcacard-y) $(QEMU_OBJS) vscclient.o
+vscclient: $(libcacard-y) $(QEMU_OBJS) vscclient.o cutils.o
$(call quiet-command,$(CC) -o $@ $^ $(libcacard_libs) $(LIBS),  LINK  
$@)

 clean:
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 802cae3..e1cae5b 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -1169,8 +1169,7 @@ vcard_emul_options(const char *args)
 NEXT_TOKEN(vname)
 NEXT_TOKEN(type_params)
 type_params_length = MIN(type_params_length, sizeof(type_str)-1);
-strncpy(type_str, type_params, type_params_length);
-type_str[type_params_length] = 0;
+pstrcpy(type_str, type_params_length, type_params);
 type = vcard_emul_type_from_string(type_str);

 NEXT_TOKEN(type_params)
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 05/22] vmdk: relative_path: use pstrcpy in place of strncpy

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Avoid strncpy+manual-NUL-terminate.  Use pstrcpy instead.

Acked-by: Kevin Wolf kw...@redhat.com
Signed-off-by: Jim Meyering meyer...@redhat.com
---
 block/vmdk.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 18e9b4c..bfd7357 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1319,8 +1319,7 @@ static int relative_path(char *dest, int dest_size,
 return -1;
 }
 if (path_is_absolute(target)) {
-dest[dest_size - 1] = '\0';
-strncpy(dest, target, dest_size - 1);
+pstrcpy(dest, dest_size, target);
 return 0;
 }
 while (base[i] == target[i]) {
-- 
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 00/22] strncpy: best avoided

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Given qemu's HACKING comments, I'm sure many here have read man strncpy,
where it indicates it is often not the best function to use.

However, many of the uses of strncpy in qemu mistakenly fail to ensure
that the destination buffer is NUL-terminated.  The first 7 c-sets fix
a dozen or so buffer overrun errors due to the lack of NUL-termination
in buffers that are later used in a context that requires the terminating
NUL.

I audited all of the strndup uses in qemu and have replaced many with
uses of qemu's pstrcpy function (it guarantees NUL-termination and does
not zero-fill).  A few are easily/cleanly replaced by uses of memcpy,
and for the few remaining uses that are justified, I added comments
marking the use as justified, explaining that it's ok because uses of
the destination buffer (currently) do not require NUL-termination.
But see the note[0] below.

Some of these changes definitely count as trivial, while others look
trivial but required that I look into kernel sources to confirm that
NUL-termination is ok, but not required (e.g., for the SIOCGIFHWADDR
ioctl's ifr.ifr_name input: linux clears its last byte, up front).

I included a quick classification of these change sets for the original series,
(see https://lists.gnu.org/archive/html/qemu-devel/2012-05/msg01151.html)
but note that a few have changed as the result of review feedback.

Jim Meyering (22):
  scsi,pci,qdev,isa-bus,sysbus: don't let *_get_fw_dev_path return NULL
  sparc: use g_strdup in place of unchecked strdup
  block: avoid buffer overrun by using pstrcpy, not strncpy
  sheepdog: avoid a few buffer overruns
  vmdk: relative_path: use pstrcpy in place of strncpy
  hw/9pfs: avoid buffer overrun
  lm32: avoid buffer overrun
  os-posix: avoid buffer overrun
  ppc: avoid buffer overrun: use pstrcpy, not strncpy
  linux-user: remove two unchecked uses of strdup
  ui/vnc: simplify and avoid strncpy
  bt: replace fragile snprintf use and unwarranted strncpy
  virtio-9p: avoid unwarranted uses of strncpy
  vscsi: avoid unwarranted strncpy
  target-i386: use pstrcpy, not strncpy
  qemu-ga: prefer pstrcpy: consistently NUL-terminate ifreq.ifr_name
  libcacard/vcard_emul_nss: use pstrcpy in place of strncpy
  acpi: remove strzcpy (strncpy-identical) function; just use strncpy
  qcow2: mark this file's sole strncpy use as justified
  hw/r2d: add comment: this strncpy use is ok
  scsi: mark an strncpy use as valid
  doc: update HACKING wrt strncpy/pstrcpy

 HACKING|  9 +
 block.c|  5 +++--
 block/qcow2.c  |  1 +
 block/sheepdog.c   | 34 ++
 block/vmdk.c   |  3 +--
 hw/9pfs/virtio-9p-posix-acl.c  |  6 --
 hw/9pfs/virtio-9p-synth.c  |  4 ++--
 hw/9pfs/virtio-9p-xattr-user.c |  3 ++-
 hw/9pfs/virtio-9p-xattr.c  |  3 ++-
 hw/acpi.c  | 24 
 hw/bt-hci.c|  7 ++-
 hw/ide/qdev.c  |  2 +-
 hw/isa-bus.c   |  2 +-
 hw/lm32_hwsetup.h  |  2 +-
 hw/pci.c   |  2 +-
 hw/qdev.c  |  2 +-
 hw/r2d.c   |  2 ++
 hw/scsi-bus.c  |  9 +++--
 hw/spapr_vscsi.c   |  2 +-
 hw/sysbus.c|  2 +-
 libcacard/Makefile |  2 +-
 libcacard/vcard_emul_nss.c |  3 +--
 linux-user/elfload.c   | 12 +++-
 os-posix.c |  3 +--
 qga/commands-posix.c   |  2 +-
 target-i386/cpu.c  |  2 +-
 target-ppc/kvm.c   |  2 +-
 target-sparc/cpu.c |  4 ++--
 ui/vnc-auth-sasl.c |  4 +---
 29 files changed, 80 insertions(+), 78 deletions(-)

--
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCHv2 07/22] lm32: avoid buffer overrun

2012-05-30 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Actually do what the comment says, using pstrcpy NUL-terminate:
strncpy does not always do that.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/lm32_hwsetup.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/lm32_hwsetup.h b/hw/lm32_hwsetup.h
index 8fc285e..70dc61f 100644
--- a/hw/lm32_hwsetup.h
+++ b/hw/lm32_hwsetup.h
@@ -96,7 +96,7 @@ static inline void hwsetup_add_tag(HWSetup *hw, enum 
hwsetup_tag t)

 static inline void hwsetup_add_str(HWSetup *hw, const char *str)
 {
-strncpy(hw-ptr, str, 31); /* make sure last byte is zero */
+pstrcpy(hw-ptr, 32, str);
 hw-ptr += 32;
 }

-- 
1.7.10.2.605.gbefc5ed

Re: [Qemu-devel] [PATCHv2 03/22] block: avoid buffer overrun by using pstrcpy, not strncpy

2012-05-30 Thread Jim Meyering

Stefan Weil wrote:
 Am 30.05.2012 09:46, schrieb Jim Meyering:
 From: Jim Meyeringmeyer...@redhat.com

 Also, use PATH_MAX, rather than the arbitrary 1024.
 Using PATH_MAX is more consistent with other filename-related
 variables in this file, like backing_filename and tmp_filename.

 Acked-by: Kevin Wolfkw...@redhat.com
 Signed-off-by: Jim Meyeringmeyer...@redhat.com
 ---
   block.c | 5 +++--
   1 file changed, 3 insertions(+), 2 deletions(-)

 diff --git a/block.c b/block.c
 index af2ab4f..efc7071 100644
 --- a/block.c
 +++ b/block.c
 @@ -1232,7 +1232,7 @@ int bdrv_commit(BlockDriverState *bs)
   int n, ro, open_flags;
   int ret = 0, rw_ret = 0;
   uint8_t *buf;
 -char filename[1024];
 +char filename[PATH_MAX];
   BlockDriverState *bs_rw, *bs_ro;

   if (!drv)
 @@ -1252,7 +1252,8 @@ int bdrv_commit(BlockDriverState *bs)

   backing_drv = bs-backing_hd-drv;
   ro = bs-backing_hd-read_only;
 -strncpy(filename, bs-backing_hd-filename, sizeof(filename));
 +/* Use pstrcpy (not strncpy): filename must be NUL-terminated. */
 +pstrcpy(filename, sizeof(filename), bs-backing_hd-filename);
   open_flags =  bs-backing_hd-open_flags;

   if (ro) {

 PATH_MAX can have any value, from 259 or 512 on Windows to
 4096 on Linux or even more.

We've seen PATH_MAX values of 32KiB, and even INT_MAX.
On the Hurd it wasn't defined at all for many years --
that may still be the case.  It's enough of a portability
rats nest that we've removed all non-advisory uses from the
100+ programs in the coreutils package.

I'm 100% with you that PATH_MAX is best avoided, but it's certainly
an improvement over a hard-coded constant like 1024 -- who knows
if/when it may mistakenly used as an array dimension supposedly
adequate to hold a PATH_MAX=4096-length buffer.

As the commit log comment suggests, I'd prefer consistency first
(at least here), with a larger scope clean-up effort to do something
about all of these:

$ git grep -E '\[(4096|2048|1024|512)\]'|wc -l
135

But if you'd prefer, I'm happy to revert that part of the above change.
It certainly shouldn't hold up the buffer overrun fix.

 I usually avoid arrays with more than some hundred bytes on the stack.
 Even if the stack is large enough, it's not good for caching.

 Of course, avoiding arbitrary limits like PATH_MAX would be best.

 Using a QEMU_PATH_MAX which is the same for all hosts might be
 the second best solution.

[Qemu-devel] [PATCH] block: prevent snapshot mode $TMPDIR symlink attack

2012-05-28 Thread Jim Meyering


In snapshot mode, bdrv_open creates an empty temporary file without
checking for mkstemp or close failure, and ignoring the possibility
of a buffer overrun given a surprisingly long $TMPDIR.
Change the get_tmp_filename function to return int (not void),
so that it can inform its two callers of those failures.
Also avoid the risk of buffer overrun and do not ignore mkstemp
or close failure.
Update both callers (in block.c and vvfat.c) to propagate
temp-file-creation failure to their callers.

get_tmp_filename creates and closes an empty file, while its
callers later open that presumed-existing file with O_CREAT.
The problem was that a malicious user could provoke mkstemp failure
and race to create a symlink with the selected temporary file name,
thus causing the qemu process (usually root owned) to open through
the symlink, overwriting an attacker-chosen file.

This addresses CVE-2012-2652.
http://bugzilla.redhat.com/CVE-2012-2652

Signed-off-by: Jim Meyering meyer...@redhat.com
---
Note that I haven't tried to see if the _WIN32 -GetLastError() return
value is properly diagnosed as it is propagated up the call stack.

 block.c   | 37 -
 block/vvfat.c |  7 ++-
 block_int.h   |  2 +-
 3 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index af2ab4f..7547051 100644
--- a/block.c
+++ b/block.c
@@ -409,28 +409,36 @@ int bdrv_create_file(const char* filename, 
QEMUOptionParameter *options)
 return bdrv_create(drv, filename, options);
 }

-#ifdef _WIN32
-void get_tmp_filename(char *filename, int size)
+/*
+ * Create a uniquely-named empty temporary file.
+ * Return 0 upon success, otherwise a negative errno value.
+ */
+int get_tmp_filename(char *filename, int size)
 {
+#ifdef _WIN32
 char temp_dir[MAX_PATH];
-
-GetTempPath(MAX_PATH, temp_dir);
-GetTempFileName(temp_dir, qem, 0, filename);
-}
+/* GetTempFileName requires that its output buffer (4th param)
+   have length MAX_PATH or greater.  */
+assert(size = MAX_PATH);
+return (GetTempPath(MAX_PATH, temp_dir)
+ GetTempFileName(temp_dir, qem, 0, filename)
+? 0 : -GetLastError());
 #else
-void get_tmp_filename(char *filename, int size)
-{
 int fd;
 const char *tmpdir;
-/* XXX: race condition possible */
 tmpdir = getenv(TMPDIR);
 if (!tmpdir)
 tmpdir = /tmp;
-snprintf(filename, size, %s/vl.XX, tmpdir);
+if (snprintf(filename, size, %s/vl.XX, tmpdir) = size) {
+return -EOVERFLOW;
+}
 fd = mkstemp(filename);
-close(fd);
-}
+if (fd  0 || close(fd)) {
+return -errno;
+}
+return 0;
 #endif
+}

 /*
  * Detect host devices. By convention, /dev/cdrom[N] is always
@@ -753,7 +761,10 @@ int bdrv_open(BlockDriverState *bs, const char *filename, 
int flags,

 bdrv_delete(bs1);

-get_tmp_filename(tmp_filename, sizeof(tmp_filename));
+ret = get_tmp_filename(tmp_filename, sizeof(tmp_filename));
+if (ret  0) {
+return ret;
+}

 /* Real path is meaningless for protocols */
 if (is_protocol)
diff --git a/block/vvfat.c b/block/vvfat.c
index 2dc9d50..0fd3367 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2808,7 +2808,12 @@ static int enable_write_target(BDRVVVFATState *s)
 array_init((s-commits), sizeof(commit_t));

 s-qcow_filename = g_malloc(1024);
-get_tmp_filename(s-qcow_filename, 1024);
+ret = get_tmp_filename(s-qcow_filename, 1024);
+if (ret  0) {
+g_free(s-qcow_filename);
+s-qcow_filename = NULL;
+return ret;
+}

 bdrv_qcow = bdrv_find_format(qcow);
 options = parse_option_parameters(, bdrv_qcow-create_options, NULL);
diff --git a/block_int.h b/block_int.h
index b80e66d..3d4abc6 100644
--- a/block_int.h
+++ b/block_int.h
@@ -335,7 +335,7 @@ struct BlockDriverState {
 BlockJob *job;
 };

-void get_tmp_filename(char *filename, int size);
+int get_tmp_filename(char *filename, int size);

 void bdrv_set_io_limits(BlockDriverState *bs,
 BlockIOLimit *io_limits);
--
1.7.10.2.605.gbefc5ed

[Qemu-devel] [PATCH v2 6/6] softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

2012-05-24 Thread Jim Meyering


Return NULL upon malloc failure.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
Improved based on suggestion from Peter Maydell:
Handle malloc failure rather than relying on g_malloc, since we
can't afford to let guest-provided len induce g_malloc's abort.

 softmmu-semi.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/softmmu-semi.h b/softmmu-semi.h
index 648cb95..bcb979a 100644
--- a/softmmu-semi.h
+++ b/softmmu-semi.h
@@ -40,7 +40,7 @@ static void *softmmu_lock_user(CPUArchState *env, uint32_t 
addr, uint32_t len,
 uint8_t *p;
 /* TODO: Make this something that isn't fixed size.  */
 p = malloc(len);
-if (copy)
+if (p  copy)
 cpu_memory_rw_debug(env, addr, p, len, 0);
 return p;
 }
@@ -52,6 +52,9 @@ static char *softmmu_lock_user_string(CPUArchState *env, 
uint32_t addr)
 uint8_t c;
 /* TODO: Make this something that isn't fixed size.  */
 s = p = malloc(1024);
+if (!s) {
+return NULL;
+}
 do {
 cpu_memory_rw_debug(env, addr, c, 1, 0);
 addr++;
--
1.7.10.2.565.gbd578b5

Re: [Qemu-devel] [PATCH 6/6] softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

2012-05-24 Thread Jim Meyering

Peter Maydell wrote:

 On 16 May 2012 14:08, Jim Meyering j...@meyering.net wrote:
 From: Jim Meyering meyer...@redhat.com

 Use g_malloc/g_free in place of malloc/free.

 Signed-off-by: Jim Meyering meyer...@redhat.com
 ---
  softmmu-semi.h | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

 diff --git a/softmmu-semi.h b/softmmu-semi.h
 index 648cb95..996e0f7 100644
 --- a/softmmu-semi.h
 +++ b/softmmu-semi.h
 @@ -39,7 +39,7 @@ static void *softmmu_lock_user(CPUArchState *env,
 uint32_t addr, uint32_t len,
  {
     uint8_t *p;
     /* TODO: Make this something that isn't fixed size.  */
 -    p = malloc(len);
 +    p = g_malloc(len);
     if (copy)
         cpu_memory_rw_debug(env, addr, p, len, 0);
     return p;

 Nak. This function is called with a length passed from the guest, so
 killing qemu if the length is too large is a bad idea. The callers
 should handle it returning NULL on failure. (Most of them do already,
 if any do not that's a bug.) The bug in this function is passing
 NULL to cpu_memory_rw_debug().

That makes sense.
I've adjusted as you suggest and posted a V2.

[Qemu-devel] [PATCH 0/2] envlist.c: handle strdup failure

2012-05-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Per discussion, let's switch envlist.c to indent with spaces,
and then make the fix:

Jim Meyering (2):
  envlist.c: convert many leading TABs to spaces via expand -i
  envlist.c: handle strdup failure

 envlist.c | 272 --
 1 file changed, 140 insertions(+), 132 deletions(-)

--
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 1/2] envlist.c: convert many leading TABs to spaces via expand -i

2012-05-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 envlist.c | 256 +++---
 1 file changed, 128 insertions(+), 128 deletions(-)

diff --git a/envlist.c b/envlist.c
index f2303cd..1d98108 100644
--- a/envlist.c
+++ b/envlist.c
@@ -8,13 +8,13 @@
 #include envlist.h

 struct envlist_entry {
-   const char *ev_var; /* actual env value */
-   QLIST_ENTRY(envlist_entry) ev_link;
+const char *ev_var;/* actual env value */
+QLIST_ENTRY(envlist_entry) ev_link;
 };

 struct envlist {
-   QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */
-   size_t el_count;/* number of entries */
+QLIST_HEAD(, envlist_entry) el_entries;/* actual entries */
+size_t el_count;   /* number of entries */
 };

 static int envlist_parse(envlist_t *envlist,
@@ -27,15 +27,15 @@ static int envlist_parse(envlist_t *envlist,
 envlist_t *
 envlist_create(void)
 {
-   envlist_t *envlist;
+envlist_t *envlist;

-   if ((envlist = malloc(sizeof (*envlist))) == NULL)
-   return (NULL);
+if ((envlist = malloc(sizeof (*envlist))) == NULL)
+return (NULL);

-   QLIST_INIT(envlist-el_entries);
-   envlist-el_count = 0;
+QLIST_INIT(envlist-el_entries);
+envlist-el_count = 0;

-   return (envlist);
+return (envlist);
 }

 /*
@@ -44,18 +44,18 @@ envlist_create(void)
 void
 envlist_free(envlist_t *envlist)
 {
-   struct envlist_entry *entry;
+struct envlist_entry *entry;

-   assert(envlist != NULL);
+assert(envlist != NULL);

-   while (envlist-el_entries.lh_first != NULL) {
-   entry = envlist-el_entries.lh_first;
-   QLIST_REMOVE(entry, ev_link);
+while (envlist-el_entries.lh_first != NULL) {
+entry = envlist-el_entries.lh_first;
+QLIST_REMOVE(entry, ev_link);

-   free((char *)entry-ev_var);
-   free(entry);
-   }
-   free(envlist);
+free((char *)entry-ev_var);
+free(entry);
+}
+free(envlist);
 }

 /*
@@ -72,7 +72,7 @@ envlist_free(envlist_t *envlist)
 int
 envlist_parse_set(envlist_t *envlist, const char *env)
 {
-   return (envlist_parse(envlist, env, envlist_setenv));
+return (envlist_parse(envlist, env, envlist_setenv));
 }

 /*
@@ -84,7 +84,7 @@ envlist_parse_set(envlist_t *envlist, const char *env)
 int
 envlist_parse_unset(envlist_t *envlist, const char *env)
 {
-   return (envlist_parse(envlist, env, envlist_unsetenv));
+return (envlist_parse(envlist, env, envlist_unsetenv));
 }

 /*
@@ -97,32 +97,32 @@ static int
 envlist_parse(envlist_t *envlist, const char *env,
 int (*callback)(envlist_t *, const char *))
 {
-   char *tmpenv, *envvar;
-   char *envsave = NULL;
-
-   assert(callback != NULL);
-
-   if ((envlist == NULL) || (env == NULL))
-   return (EINVAL);
-
-   /*
-* We need to make temporary copy of the env string
-* as strtok_r(3) modifies it while it tokenizes.
-*/
-   if ((tmpenv = strdup(env)) == NULL)
-   return (errno);
-
-   envvar = strtok_r(tmpenv, ,, envsave);
-   while (envvar != NULL) {
-   if ((*callback)(envlist, envvar) != 0) {
-   free(tmpenv);
-   return (errno);
-   }
-   envvar = strtok_r(NULL, ,, envsave);
-   }
-
-   free(tmpenv);
-   return (0);
+char *tmpenv, *envvar;
+char *envsave = NULL;
+
+assert(callback != NULL);
+
+if ((envlist == NULL) || (env == NULL))
+return (EINVAL);
+
+/*
+ * We need to make temporary copy of the env string
+ * as strtok_r(3) modifies it while it tokenizes.
+ */
+if ((tmpenv = strdup(env)) == NULL)
+return (errno);
+
+envvar = strtok_r(tmpenv, ,, envsave);
+while (envvar != NULL) {
+if ((*callback)(envlist, envvar) != 0) {
+free(tmpenv);
+return (errno);
+}
+envvar = strtok_r(NULL, ,, envsave);
+}
+
+free(tmpenv);
+return (0);
 }

 /*
@@ -134,46 +134,46 @@ envlist_parse(envlist_t *envlist, const char *env,
 int
 envlist_setenv(envlist_t *envlist, const char *env)
 {
-   struct envlist_entry *entry = NULL;
-   const char *eq_sign;
-   size_t envname_len;
-
-   if ((envlist == NULL) || (env == NULL))
-   return (EINVAL);
-
-   /* find out first equals sign in given env */
-   if ((eq_sign = strchr(env, '=')) == NULL)
-   return (EINVAL);
-   envname_len = eq_sign - env + 1

[Qemu-devel] [PATCH 2/2] envlist.c: handle strdup failure

2012-05-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Without this, envlist_to_environ may silently fail to copy all
strings into the destination buffer, and both callers would leak
any env strings allocated after a failing strdup, because the
freeing code stops at the first NULL pointer.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 envlist.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/envlist.c b/envlist.c
index 1d98108..0dfb56c 100644
--- a/envlist.c
+++ b/envlist.c
@@ -234,8 +234,16 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)
 return (NULL);

 for (entry = envlist-el_entries.lh_first; entry != NULL;
-entry = entry-ev_link.le_next) {
-*(penv++) = strdup(entry-ev_var);
+ entry = entry-ev_link.le_next, penv++) {
+*penv = strdup(entry-ev_var);
+if (*penv == NULL) {
+char **e = env;
+while (e = penv) {
+free(*e++);
+}
+free(env);
+return NULL;
+}
 }
 *penv = NULL; /* NULL terminate the list */

-- 
1.7.10.2.552.gaa3bb87

Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

2012-05-22 Thread Jim Meyering

Kevin Wolf wrote:
 A patch replacing tabs by spaces isn't really the kind of patches that
 we would want to avoid during freeze. It's easy enough to check with git
 diff -w that it doesn't change anything semantically.

That makes sense, so I've posted two patches:

1) two patches: one replacing all leading TABs with equivalent spaces,
  then the above patch

Note however, that envlist.c must predate checkpatch.pl, since
that patch provokes numerous warnings about style issues:
(hmm... I've just noticed these first two, which seem to suggest
that even for comments I should remove the TABs.  Is it worth
a V2 to fix those two lines?  Or are they false positives, since
they're not really code indentation?  )

ERROR: code indent should never use tabs
#23: FILE: envlist.c:11:
+const char *ev_var;^I^I^I/* actual env value */$

ERROR: code indent should never use tabs
#30: FILE: envlist.c:16:
+QLIST_HEAD(, envlist_entry) el_entries;^I/* actual entries */$

ERROR: code indent should never use tabs
#31: FILE: envlist.c:17:
+size_t el_count;^I^I^I/* number of entries */$

WARNING: space prohibited between function name and open parenthesis '('
#44: FILE: envlist.c:32:
+if ((envlist = malloc(sizeof (*envlist))) == NULL)

ERROR: do not use assignment in if condition
#44: FILE: envlist.c:32:
+if ((envlist = malloc(sizeof (*envlist))) == NULL)

WARNING: braces {} are necessary for all arms of this statement
#44: FILE: envlist.c:32:
+if ((envlist = malloc(sizeof (*envlist))) == NULL)
[...]

ERROR: return is not a function, parentheses are not required
#45: FILE: envlist.c:33:
+return (NULL);

ERROR: return is not a function, parentheses are not required
#53: FILE: envlist.c:38:
+return (envlist);

ERROR: return is not a function, parentheses are not required
#90: FILE: envlist.c:75:
+return (envlist_parse(envlist, env, envlist_setenv));

ERROR: return is not a function, parentheses are not required
#99: FILE: envlist.c:87:
+return (envlist_parse(envlist, env, envlist_unsetenv));

WARNING: braces {} are necessary for all arms of this statement
#138: FILE: envlist.c:105:
+if ((envlist == NULL) || (env == NULL))
[...]

ERROR: return is not a function, parentheses are not required
#139: FILE: envlist.c:106:
+return (EINVAL);

ERROR: do not use assignment in if condition
#145: FILE: envlist.c:112:
+if ((tmpenv = strdup(env)) == NULL)

WARNING: braces {} are necessary for all arms of this statement
#145: FILE: envlist.c:112:
+if ((tmpenv = strdup(env)) == NULL)
[...]

ERROR: return is not a function, parentheses are not required
#146: FILE: envlist.c:113:
+return (errno);

ERROR: return is not a function, parentheses are not required
#152: FILE: envlist.c:119:
+return (errno);

ERROR: return is not a function, parentheses are not required
#158: FILE: envlist.c:125:
+return (0);

WARNING: braces {} are necessary for all arms of this statement
#210: FILE: envlist.c:141:
+if ((envlist == NULL) || (env == NULL))
[...]

ERROR: return is not a function, parentheses are not required
#211: FILE: envlist.c:142:
+return (EINVAL);

ERROR: do not use assignment in if condition
#214: FILE: envlist.c:145:
+if ((eq_sign = strchr(env, '=')) == NULL)

WARNING: braces {} are necessary for all arms of this statement
#214: FILE: envlist.c:145:
+if ((eq_sign = strchr(env, '=')) == NULL)
[...]

ERROR: return is not a function, parentheses are not required
#215: FILE: envlist.c:146:
+return (EINVAL);

WARNING: braces {} are necessary for all arms of this statement
#225: FILE: envlist.c:156:
+if (strncmp(entry-ev_var, env, envname_len) == 0)
[...]

WARNING: space prohibited between function name and open parenthesis '('
#237: FILE: envlist.c:168:
+if ((entry = malloc(sizeof (*entry))) == NULL)

ERROR: do not use assignment in if condition
#237: FILE: envlist.c:168:
+if ((entry = malloc(sizeof (*entry))) == NULL)

WARNING: braces {} are necessary for all arms of this statement
#237: FILE: envlist.c:168:
+if ((entry = malloc(sizeof (*entry))) == NULL)
[...]

ERROR: return is not a function, parentheses are not required
#238: FILE: envlist.c:169:
+return (errno);

ERROR: do not use assignment in if condition
#239: FILE: envlist.c:170:
+if ((entry-ev_var = strdup(env)) == NULL) {

ERROR: return is not a function, parentheses are not required
#241: FILE: envlist.c:172:
+return (errno);

ERROR: return is not a function, parentheses are not required
#245: FILE: envlist.c:176:
+return (0);

WARNING: braces {} are necessary for all arms of this statement
#284: FILE: envlist.c:189:
+if ((envlist == NULL) || (env == NULL))
[...]

ERROR: return is not a function, parentheses are not required
#285: FILE: envlist.c:190:
+

[Qemu-devel] [PATCHv2 0/2] envlist.c: handle strdup failure

2012-05-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

This is the same as v1, except that two lines of non-leading TABs
in envlist.c (indenting comments after code) have also been converted
to use equivalent spaces instead of TABs.

Jim Meyering (2):
  envlist.c: convert all TABs to equivalent spaces
  envlist.c: handle strdup failure

 envlist.c | 272 --
 1 file changed, 140 insertions(+), 132 deletions(-)

--
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCHv2 2/2] envlist.c: handle strdup failure

2012-05-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Without this, envlist_to_environ may silently fail to copy all
strings into the destination buffer, and both callers would leak
any env strings allocated after a failing strdup, because the
freeing code stops at the first NULL pointer.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 envlist.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/envlist.c b/envlist.c
index be0addb..7532091 100644
--- a/envlist.c
+++ b/envlist.c
@@ -234,8 +234,16 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)
 return (NULL);

 for (entry = envlist-el_entries.lh_first; entry != NULL;
-entry = entry-ev_link.le_next) {
-*(penv++) = strdup(entry-ev_var);
+ entry = entry-ev_link.le_next, penv++) {
+*penv = strdup(entry-ev_var);
+if (*penv == NULL) {
+char **e = env;
+while (e = penv) {
+free(*e++);
+}
+free(env);
+return NULL;
+}
 }
 *penv = NULL; /* NULL terminate the list */

-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCHv2 1/2] envlist.c: convert all TABs to equivalent spaces

2012-05-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 envlist.c | 256 +++---
 1 file changed, 128 insertions(+), 128 deletions(-)

diff --git a/envlist.c b/envlist.c
index f2303cd..be0addb 100644
--- a/envlist.c
+++ b/envlist.c
@@ -8,13 +8,13 @@
 #include envlist.h

 struct envlist_entry {
-   const char *ev_var; /* actual env value */
-   QLIST_ENTRY(envlist_entry) ev_link;
+const char *ev_var; /* actual env value */
+QLIST_ENTRY(envlist_entry) ev_link;
 };

 struct envlist {
-   QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */
-   size_t el_count;/* number of entries */
+QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */
+size_t el_count;/* number of entries */
 };

 static int envlist_parse(envlist_t *envlist,
@@ -27,15 +27,15 @@ static int envlist_parse(envlist_t *envlist,
 envlist_t *
 envlist_create(void)
 {
-   envlist_t *envlist;
+envlist_t *envlist;

-   if ((envlist = malloc(sizeof (*envlist))) == NULL)
-   return (NULL);
+if ((envlist = malloc(sizeof (*envlist))) == NULL)
+return (NULL);

-   QLIST_INIT(envlist-el_entries);
-   envlist-el_count = 0;
+QLIST_INIT(envlist-el_entries);
+envlist-el_count = 0;

-   return (envlist);
+return (envlist);
 }

 /*
@@ -44,18 +44,18 @@ envlist_create(void)
 void
 envlist_free(envlist_t *envlist)
 {
-   struct envlist_entry *entry;
+struct envlist_entry *entry;

-   assert(envlist != NULL);
+assert(envlist != NULL);

-   while (envlist-el_entries.lh_first != NULL) {
-   entry = envlist-el_entries.lh_first;
-   QLIST_REMOVE(entry, ev_link);
+while (envlist-el_entries.lh_first != NULL) {
+entry = envlist-el_entries.lh_first;
+QLIST_REMOVE(entry, ev_link);

-   free((char *)entry-ev_var);
-   free(entry);
-   }
-   free(envlist);
+free((char *)entry-ev_var);
+free(entry);
+}
+free(envlist);
 }

 /*
@@ -72,7 +72,7 @@ envlist_free(envlist_t *envlist)
 int
 envlist_parse_set(envlist_t *envlist, const char *env)
 {
-   return (envlist_parse(envlist, env, envlist_setenv));
+return (envlist_parse(envlist, env, envlist_setenv));
 }

 /*
@@ -84,7 +84,7 @@ envlist_parse_set(envlist_t *envlist, const char *env)
 int
 envlist_parse_unset(envlist_t *envlist, const char *env)
 {
-   return (envlist_parse(envlist, env, envlist_unsetenv));
+return (envlist_parse(envlist, env, envlist_unsetenv));
 }

 /*
@@ -97,32 +97,32 @@ static int
 envlist_parse(envlist_t *envlist, const char *env,
 int (*callback)(envlist_t *, const char *))
 {
-   char *tmpenv, *envvar;
-   char *envsave = NULL;
-
-   assert(callback != NULL);
-
-   if ((envlist == NULL) || (env == NULL))
-   return (EINVAL);
-
-   /*
-* We need to make temporary copy of the env string
-* as strtok_r(3) modifies it while it tokenizes.
-*/
-   if ((tmpenv = strdup(env)) == NULL)
-   return (errno);
-
-   envvar = strtok_r(tmpenv, ,, envsave);
-   while (envvar != NULL) {
-   if ((*callback)(envlist, envvar) != 0) {
-   free(tmpenv);
-   return (errno);
-   }
-   envvar = strtok_r(NULL, ,, envsave);
-   }
-
-   free(tmpenv);
-   return (0);
+char *tmpenv, *envvar;
+char *envsave = NULL;
+
+assert(callback != NULL);
+
+if ((envlist == NULL) || (env == NULL))
+return (EINVAL);
+
+/*
+ * We need to make temporary copy of the env string
+ * as strtok_r(3) modifies it while it tokenizes.
+ */
+if ((tmpenv = strdup(env)) == NULL)
+return (errno);
+
+envvar = strtok_r(tmpenv, ,, envsave);
+while (envvar != NULL) {
+if ((*callback)(envlist, envvar) != 0) {
+free(tmpenv);
+return (errno);
+}
+envvar = strtok_r(NULL, ,, envsave);
+}
+
+free(tmpenv);
+return (0);
 }

 /*
@@ -134,46 +134,46 @@ envlist_parse(envlist_t *envlist, const char *env,
 int
 envlist_setenv(envlist_t *envlist, const char *env)
 {
-   struct envlist_entry *entry = NULL;
-   const char *eq_sign;
-   size_t envname_len;
-
-   if ((envlist == NULL) || (env == NULL))
-   return (EINVAL);
-
-   /* find out first equals sign in given env */
-   if ((eq_sign = strchr(env, '=')) == NULL)
-   return (EINVAL);
-   envname_len = eq_sign - env + 1;
-
-   /*
-* If there already

Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

2012-05-22 Thread Jim Meyering

Kevin Wolf wrote:

 Am 22.05.2012 11:05, schrieb Jim Meyering:
 Kevin Wolf wrote:
 A patch replacing tabs by spaces isn't really the kind of patches that
 we would want to avoid during freeze. It's easy enough to check with git
 diff -w that it doesn't change anything semantically.

 That makes sense, so I've posted two patches:

 1) two patches: one replacing all leading TABs with equivalent spaces,
   then the above patch

 Note however, that envlist.c must predate checkpatch.pl, since
 that patch provokes numerous warnings about style issues:

 Quite possible. Most files in qemu will still contain some violations.

 (hmm... I've just noticed these first two, which seem to suggest
 that even for comments I should remove the TABs.  Is it worth
 a V2 to fix those two lines?  Or are they false positives, since
 they're not really code indentation?  )

 We don't use tabs at all where possible. There shouldn't be much more
 exceptions than the Makefiles.

Sounds like a v2 would be welcome.
Here you go...

[Qemu-devel] [PATCHv3 0/2] envlist.c: handle strdup failure

2012-05-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Same as v2, but now with TABs converted using expand --tabs=4.

Jim Meyering (2):
  envlist.c: convert each TAB(width-4) to equivalent spaces
  envlist.c: handle strdup failure

 envlist.c | 272 --
 1 file changed, 140 insertions(+), 132 deletions(-)

--
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCHv3 1/2] envlist.c: convert each TAB(width-4) to equivalent spaces

2012-05-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 envlist.c | 256 +++---
 1 file changed, 128 insertions(+), 128 deletions(-)

diff --git a/envlist.c b/envlist.c
index f2303cd..e44889b 100644
--- a/envlist.c
+++ b/envlist.c
@@ -8,13 +8,13 @@
 #include envlist.h

 struct envlist_entry {
-   const char *ev_var; /* actual env value */
-   QLIST_ENTRY(envlist_entry) ev_link;
+const char *ev_var; /* actual env value */
+QLIST_ENTRY(envlist_entry) ev_link;
 };

 struct envlist {
-   QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */
-   size_t el_count;/* number of entries */
+QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */
+size_t el_count;/* number of entries */
 };

 static int envlist_parse(envlist_t *envlist,
@@ -27,15 +27,15 @@ static int envlist_parse(envlist_t *envlist,
 envlist_t *
 envlist_create(void)
 {
-   envlist_t *envlist;
+envlist_t *envlist;

-   if ((envlist = malloc(sizeof (*envlist))) == NULL)
-   return (NULL);
+if ((envlist = malloc(sizeof (*envlist))) == NULL)
+return (NULL);

-   QLIST_INIT(envlist-el_entries);
-   envlist-el_count = 0;
+QLIST_INIT(envlist-el_entries);
+envlist-el_count = 0;

-   return (envlist);
+return (envlist);
 }

 /*
@@ -44,18 +44,18 @@ envlist_create(void)
 void
 envlist_free(envlist_t *envlist)
 {
-   struct envlist_entry *entry;
+struct envlist_entry *entry;

-   assert(envlist != NULL);
+assert(envlist != NULL);

-   while (envlist-el_entries.lh_first != NULL) {
-   entry = envlist-el_entries.lh_first;
-   QLIST_REMOVE(entry, ev_link);
+while (envlist-el_entries.lh_first != NULL) {
+entry = envlist-el_entries.lh_first;
+QLIST_REMOVE(entry, ev_link);

-   free((char *)entry-ev_var);
-   free(entry);
-   }
-   free(envlist);
+free((char *)entry-ev_var);
+free(entry);
+}
+free(envlist);
 }

 /*
@@ -72,7 +72,7 @@ envlist_free(envlist_t *envlist)
 int
 envlist_parse_set(envlist_t *envlist, const char *env)
 {
-   return (envlist_parse(envlist, env, envlist_setenv));
+return (envlist_parse(envlist, env, envlist_setenv));
 }

 /*
@@ -84,7 +84,7 @@ envlist_parse_set(envlist_t *envlist, const char *env)
 int
 envlist_parse_unset(envlist_t *envlist, const char *env)
 {
-   return (envlist_parse(envlist, env, envlist_unsetenv));
+return (envlist_parse(envlist, env, envlist_unsetenv));
 }

 /*
@@ -97,32 +97,32 @@ static int
 envlist_parse(envlist_t *envlist, const char *env,
 int (*callback)(envlist_t *, const char *))
 {
-   char *tmpenv, *envvar;
-   char *envsave = NULL;
-
-   assert(callback != NULL);
-
-   if ((envlist == NULL) || (env == NULL))
-   return (EINVAL);
-
-   /*
-* We need to make temporary copy of the env string
-* as strtok_r(3) modifies it while it tokenizes.
-*/
-   if ((tmpenv = strdup(env)) == NULL)
-   return (errno);
-
-   envvar = strtok_r(tmpenv, ,, envsave);
-   while (envvar != NULL) {
-   if ((*callback)(envlist, envvar) != 0) {
-   free(tmpenv);
-   return (errno);
-   }
-   envvar = strtok_r(NULL, ,, envsave);
-   }
-
-   free(tmpenv);
-   return (0);
+char *tmpenv, *envvar;
+char *envsave = NULL;
+
+assert(callback != NULL);
+
+if ((envlist == NULL) || (env == NULL))
+return (EINVAL);
+
+/*
+ * We need to make temporary copy of the env string
+ * as strtok_r(3) modifies it while it tokenizes.
+ */
+if ((tmpenv = strdup(env)) == NULL)
+return (errno);
+
+envvar = strtok_r(tmpenv, ,, envsave);
+while (envvar != NULL) {
+if ((*callback)(envlist, envvar) != 0) {
+free(tmpenv);
+return (errno);
+}
+envvar = strtok_r(NULL, ,, envsave);
+}
+
+free(tmpenv);
+return (0);
 }

 /*
@@ -134,46 +134,46 @@ envlist_parse(envlist_t *envlist, const char *env,
 int
 envlist_setenv(envlist_t *envlist, const char *env)
 {
-   struct envlist_entry *entry = NULL;
-   const char *eq_sign;
-   size_t envname_len;
-
-   if ((envlist == NULL) || (env == NULL))
-   return (EINVAL);
-
-   /* find out first equals sign in given env */
-   if ((eq_sign = strchr(env, '=')) == NULL)
-   return (EINVAL);
-   envname_len = eq_sign - env + 1;
-
-   /*
-* If there already exists variable with given name
-* we remove and release it before allocating a whole
-* new entry.
-*/
-   for (entry = envlist-el_entries.lh_first; entry != NULL;
-   entry = entry-ev_link.le_next

[Qemu-devel] [PATCHv2 2/9] tcg: __jit_debug_descriptor must not be static

2012-05-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Add comments so no one else will be tempted to reduce the scope
of this global variable.

Signed-off-by: Jim Meyering meyer...@redhat.com
---

 tcg/tcg.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index ab589c7..2793fa6 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2264,7 +2264,9 @@ void tcg_dump_info(FILE *f, fprintf_function cpu_fprintf)
(3) Call tcg_register_jit_int, with the constructed .debug_frame.
 */

-/* Begin GDB interface.  THE FOLLOWING MUST MATCH GDB DOCS.  */
+/* Begin GDB interface.  THE FOLLOWING MUST MATCH GDB DOCS:
+   http://sourceware.org/gdb/onlinedocs/gdb/Declarations.html
+*/
 typedef enum {
 JIT_NOACTION = 0,
 JIT_REGISTER_FN,
@@ -2291,8 +2293,10 @@ void __jit_debug_register_code(void)
 asm();
 }

-/* Must statically initialize the version, because GDB may check
-   the version before we can set it.  */
+/* We must initialize the version this way, because GDB may check
+   the version before we can set it.  This declaration must have
+   external scope.  If it were static, an aggressive compiler might
+   notice that we never read this symbol and remove it altogether. */
 struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };

 /* End GDB interface.  */
--
1.7.10.2.552.gaa3bb87

Re: [Qemu-devel] [PATCH 2/9] tcg: declare __jit_debug_descriptor to be static

2012-05-22 Thread Jim Meyering

Peter Maydell wrote:
 On 21 May 2012 21:10, Jim Meyering j...@meyering.net wrote:
 Peter Maydell wrote:
 On 21 May 2012 20:51, Jim Meyering j...@meyering.net wrote:
 From: Jim Meyering meyer...@redhat.com


 Signed-off-by: Jim Meyering meyer...@redhat.com
 ---
  tcg/tcg.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/tcg/tcg.c b/tcg/tcg.c
 index ab589c7..350fdad 100644
 --- a/tcg/tcg.c
 +++ b/tcg/tcg.c
 @@ -2293,7 +2293,7 @@ void __jit_debug_register_code(void)

  /* Must statically initialize the version, because GDB may check
    the version before we can set it.  */
 -struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };
 +static struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };

  /* End GDB interface.  */

 Nak. This symbol is global so that gdb can find it by fishing around
 in the executable's symbol table.

 Thanks for the quick feedback.

 How does the scope of the symbol affect whether gdb can find it?

 If you mark it 'static' the compiler can throw it away or completely
 rearrange it if it's feeling clever enough, I think.

 Anyway, we're following a prescribed interface here:
 http://sourceware.org/gdb/onlinedocs/gdb/Declarations.html

 and I don't think we should deviate from it. As the comment says,
 THE FOLLOWING MUST MATCH GDB DOCS..

 If declaring this variable static is not appropriate,
 then the comment saying that static initialization is desired
 should be changed.

 The comment means statically initialize this variable rather than
 doing it dynamically in some function at startup.

Thanks.  I've clarified the comments and posted a V2.

Re: [Qemu-devel] [PATCHv2 1/2] envlist.c: convert all TABs to equivalent spaces

2012-05-22 Thread Jim Meyering

Peter Maydell wrote:
 On 22 May 2012 10:50, Jim Meyering j...@meyering.net wrote:
 From: Jim Meyering meyer...@redhat.com

 If we're going to go to the effort of a complete reindent
 patch we should actually reindent to the QEMU coding style
 standard, which is four-space, not eight.

Good point.  V3 is on its way.

[Qemu-devel] [PATCHv3 2/2] envlist.c: handle strdup failure

2012-05-22 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Without this, envlist_to_environ may silently fail to copy all
strings into the destination buffer, and both callers would leak
any env strings allocated after a failing strdup, because the
freeing code stops at the first NULL pointer.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 envlist.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/envlist.c b/envlist.c
index e44889b..df5c723 100644
--- a/envlist.c
+++ b/envlist.c
@@ -234,8 +234,16 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)
 return (NULL);

 for (entry = envlist-el_entries.lh_first; entry != NULL;
-entry = entry-ev_link.le_next) {
-*(penv++) = strdup(entry-ev_var);
+ entry = entry-ev_link.le_next, penv++) {
+*penv = strdup(entry-ev_var);
+if (*penv == NULL) {
+char **e = env;
+while (e = penv) {
+free(*e++);
+}
+free(env);
+return NULL;
+}
 }
 *penv = NULL; /* NULL terminate the list */

-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 0/3] tighten scope of accidentally global variables

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

I noticed this commit,

virtio-pci: add missing 'static'

which made this change:

 -const MemoryRegionPortio virtio_portio[] = {
 +static const MemoryRegionPortio virtio_portio[] = {

and wondered if there were other variables like that.
The following command shows that there are:
[note that there are probably more: this finds only those
 for which the variable name appears in only one source file. ]

$ for i in $(nm -e *.o|sed -n 's/.* [BCDGRS] //p'); do \
test $(git grep -lw $i|wc -l) = 1  echo $i;done
BlockDeviceIoStatus_lookup
SpiceQueryMouseMode_lookup
qemu_boot_opts
qemu_option_rom_opts
vmstate_info_scsi_requests
xen_xcg

The *_lookup names are false positives, since the symbols are actually
used from two or more .o files.  Here are patches for the others:

Jim Meyering (3):
  xen: remove unused global, xen_xcg
  scsi: declare vmstate_info_scsi_requests to be static
  qemu-config: qemu_option_rom_opts, qemu_boot_opts: declare static

 hw/scsi-bus.c| 2 +-
 hw/xen_backend.c | 1 -
 qemu-config.c| 4 ++--
 3 files changed, 3 insertions(+), 4 deletions(-)

--
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 1/3] xen: remove unused global, xen_xcg

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/xen_backend.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/xen_backend.c b/hw/xen_backend.c
index 66cb144..e44ced0 100644
--- a/hw/xen_backend.c
+++ b/hw/xen_backend.c
@@ -47,7 +47,6 @@

 /* public */
 XenXC xen_xc = XC_HANDLER_INITIAL_VALUE;
-XenGnttab xen_xcg = XC_HANDLER_INITIAL_VALUE;
 struct xs_handle *xenstore = NULL;
 const char *xen_protocol;

-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 2/3] scsi: declare vmstate_info_scsi_requests to be static

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/scsi-bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 8ab9bcd..f10f3ec 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1561,7 +1561,7 @@ static int get_scsi_requests(QEMUFile *f, void *pv, 
size_t size)
 return 0;
 }

-const VMStateInfo vmstate_info_scsi_requests = {
+static const VMStateInfo vmstate_info_scsi_requests = {
 .name = scsi-requests,
 .get  = get_scsi_requests,
 .put  = put_scsi_requests,
-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 3/3] qemu-config: qemu_option_rom_opts, qemu_boot_opts: declare static

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 qemu-config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index be84a03..c03e52b 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -528,7 +528,7 @@ QemuOptsList qemu_spice_opts = {
 },
 };

-QemuOptsList qemu_option_rom_opts = {
+static QemuOptsList qemu_option_rom_opts = {
 .name = option-rom,
 .implied_opt_name = romfile,
 .head = QTAILQ_HEAD_INITIALIZER(qemu_option_rom_opts.head),
@@ -587,7 +587,7 @@ static QemuOptsList qemu_machine_opts = {
 },
 };

-QemuOptsList qemu_boot_opts = {
+static QemuOptsList qemu_boot_opts = {
 .name = boot-opts,
 .head = QTAILQ_HEAD_INITIALIZER(qemu_boot_opts.head),
 .desc = {
-- 
1.7.10.2.552.gaa3bb87

Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

2012-05-21 Thread Jim Meyering

Blue Swirl wrote:
 On Tue, May 15, 2012 at 1:04 PM,  j...@meyering.net wrote:
 From: Jim Meyering meyer...@redhat.com

 Without this, envlist_to_environ may silently fail to copy all
 strings into the destination buffer, and both callers would leak
 any env strings allocated after a failing strdup, because the
 freeing code stops at the first NULL pointer.

 Signed-off-by: Jim Meyering meyer...@redhat.com
 ---
  envlist.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

 diff --git a/envlist.c b/envlist.c
 index f2303cd..2bbd99c 100644
 --- a/envlist.c
 +++ b/envlist.c
 @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t 
 *count)

        for (entry = envlist-el_entries.lh_first; entry != NULL;
            entry = entry-ev_link.le_next) {
 -               *(penv++) = strdup(entry-ev_var);
 +               if ((*(penv++) = strdup(entry-ev_var)) == NULL) {
 +                       char **e = env;
 +                       while (e != penv) {
 +                               free(*e++);
 +                       }
 +                       free(env);
 +                       return NULL;
 +               }

 ERROR: code indent should never use tabs
 #82: FILE: envlist.c:238:
 +^I^Iif ((*(penv++) = strdup(entry-ev_var)) == NULL) {$

That entire file is indented solely with TABs, so adding these new
lines using spaces for indentation seems unjustified: the mix tends
to make the code unreadable in some contexts (email quoting, for one).
How about two patches: one to convert all leading TABs in envlist.c to
spaces, and the next to make the above change, but indenting with spaces?

 ERROR: do not use assignment in if condition
 #82: FILE: envlist.c:238:
 +   if ((*(penv++) = strdup(entry-ev_var)) == NULL) {

I agree with the sentiment, but found that the alternative was less
readable and less maintainable, since I'd have to increment penv in
two places (both in the if-block and after it) rather than in just one.
However, I've just realized I can hoist the penv++ increment into the
for-statement, in which case it's ok:

for (entry = envlist-el_entries.lh_first; entry != NULL;
 entry = entry-ev_link.le_next, penv++) {
*penv = strdup(entry-ev_var);
if (*penv == NULL) {
char **e = env;
while (e = penv) {
free(*e++);
}
free(env);
return NULL;
}
}

Your move.  Which would you prefer?
  1) two patches: one replacing all leading TABs with equivalent spaces,
  then the above patch
  2) one patch, indented using TABs, in spite of the checkpatch failure
  3) one patch, indented using spaces, in spite of the consistency issue

...
 total: 9 errors, 0 warnings, 15 lines checked

 Please fix.

[Qemu-devel] [PATCHv2 1/6] qcow2: don't leak buffer for unexpected qcow_version in header

2012-05-21 Thread Jim Meyering


Signed-off-by: Jim Meyering meyer...@redhat.com
---
Thanks to Kevin Wolf for the improvement.

 block/qcow2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 655799c..c2e49cd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -919,7 +919,8 @@ int qcow2_update_header(BlockDriverState *bs)
 ret = sizeof(*header);
 break;
 default:
-return -EINVAL;
+ret = -EINVAL;
+goto fail;
 }

 buf += ret;
--
1.7.10.2.552.gaa3bb87

Re: [Qemu-devel] [PATCH 1/6] qcow2: don't leak buffer for unexpected qcow_version in header

2012-05-21 Thread Jim Meyering

Kevin Wolf wrote:
 Am 16.05.2012 15:07, schrieb Jim Meyering:
 From: Jim Meyering meyer...@redhat.com


 Signed-off-by: Jim Meyering meyer...@redhat.com
 ---
  block/qcow2.c | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/block/qcow2.c b/block/qcow2.c
 index 655799c..f3388bf 100644
 --- a/block/qcow2.c
 +++ b/block/qcow2.c
 @@ -919,6 +919,7 @@ int qcow2_update_header(BlockDriverState *bs)
  ret = sizeof(*header);
  break;
  default:
 +free(buf);
  return -EINVAL;
  }


 buf was allocated with qemu_blockalign(), so it must be freed with
 qemu_vfree(). But instead of open-coding it here, this place should work
 like all other places in the same function:

 ret = -EINVAL;
 goto fail;

Hi Kevin,
Thanks.  That is obviously better.
I've just posted a v2.

Re: [Qemu-devel] [PATCH 0/3] tighten scope of accidentally global variables

2012-05-21 Thread Jim Meyering

Blue Swirl wrote:
 On Mon, May 21, 2012 at 10:03 AM, Jim Meyering j...@meyering.net wrote:
 From: Jim Meyering meyer...@redhat.com

 I noticed this commit,

    virtio-pci: add missing 'static'

 which made this change:

     -const MemoryRegionPortio virtio_portio[] = {
     +static const MemoryRegionPortio virtio_portio[] = {

 and wondered if there were other variables like that.
 The following command shows that there are:
 [note that there are probably more: this finds only those
  for which the variable name appears in only one source file. ]

 Also, only for files at the top level.

Thanks.  There were so many .o files, I assumed that all were at the top.
Searching all .o files, I found many more:

  $ for i in $(nm -e $(find . -name '*.o')|sed -n 's/.* [BCDGRS] //p'|sort -u);\
do test $(git grep -lw $i|wc -l) = 1  echo $i;done
  BlockDeviceIoStatus_lookup
  DEFAULT_ATR
  SpiceQueryMouseMode_lookup
  __jit_debug_descriptor
  alpha_num_opcodes
  alpha_num_operands
  alpha_opcodes
  alpha_operands
  arg_table
  backend_enum_table
  bdrv_sheepdog
  bfd_mips_num_builtin_opcodes
  bfd_mips_num_opcodes
  bonito_state
  check_cpuid
  cris_cond15s
  cris_cores
  device_configs
  display_remote
  enforce_cpuid
  floatformat_arm_ext_big
  floatformat_arm_ext_littlebyte_bigword
  floatformat_i387_ext
  floatformat_i960_ext
  floatformat_ia64_quad_big
  floatformat_ia64_quad_little
  floatformat_ia64_spill_big
  floatformat_ia64_spill_little
  floatformat_ieee_double_big
  floatformat_ieee_double_little
  floatformat_ieee_double_littlebyte_bigword
  floatformat_ieee_single_big
  floatformat_ieee_single_little
  floatformat_m68881_ext
  floatformat_m88110_ext
  floatformat_m88110_harris_ext
  fsl_register_prefix
  fw_boot_order
  last_mapping_addr
  last_mapping_sym
  last_type
  leon3_generic_machine
  m68k_numaliases
  m68k_numopcodes
  m68k_opcode_aliases
  m68k_opcodes
  memory_region_transaction_depth
  mips_builtin_opcodes
  mips_fulong2e_machine
  mips_opcodes
  no_reboot
  num_powerpc_operands
  para_features
  powerpc_macros
  powerpc_num_macros
  powerpc_num_opcodes
  powerpc_operands
  pvr_register_prefix
  qemu_boot_opts
  qemu_global_mutex
  qemu_option_rom_opts
  register_prefix
  rtas_next
  s390_virtio_bus_info
  sh_table
  special_register_prefix
  test_image
  timers_state
  v9fs_synth_root
  virtcon_hds
  vmstate_bmdma_status
  vmstate_info_scsi_requests
  vmstate_rxtx_stats
  xen_platform_ioport
  xen_xcg

 What about functions? ;-)

I planned to check them separately, as I do in gnulib's
sc_tight_scope syntax-check rule:

  http://git.sv.gnu.org/cgit/gnulib.git/tree/top/maint.mk#n1443

Re: [Qemu-devel] [PATCH 0/3] tighten scope of accidentally global variables

2012-05-21 Thread Jim Meyering

Blue Swirl wrote:
 On Mon, May 21, 2012 at 6:10 PM, Jim Meyering j...@meyering.net wrote:
 Blue Swirl wrote:
 On Mon, May 21, 2012 at 10:03 AM, Jim Meyering j...@meyering.net wrote:
 From: Jim Meyering meyer...@redhat.com

 I noticed this commit,

    virtio-pci: add missing 'static'

 which made this change:

     -const MemoryRegionPortio virtio_portio[] = {
     +static const MemoryRegionPortio virtio_portio[] = {

 and wondered if there were other variables like that.
 The following command shows that there are:
 [note that there are probably more: this finds only those
  for which the variable name appears in only one source file. ]

 Also, only for files at the top level.

 Thanks.  There were so many .o files, I assumed that all were at the top.
 Searching all .o files, I found many more:

  $ for i in $(nm -e $(find . -name '*.o')|sed -n 's/.* [BCDGRS] //p'|sort 
 -u);\
    do test $(git grep -lw $i|wc -l) = 1  echo $i;done

 How about just checking the executables (*-softmmu/qemu-system-xyz and
 linux-user/qemu-xyz)?

If I understood what you suggest, that would seem to miss only test_image:
(README-scope-vars is the result of the one above using find and *.o)

$ for i in $(nm -e *-softmmu/qemu-system-* linux-user/qemu-*|sed -n 's/.* 
[BCDGRS] //p'|sort -u); do test $(git grep -lw $i|wc -l) = 1  echo $i;done  k
nm: linux-user/qemu-types.h: File format not recognized
[Exit 1]
$ diff -u k README-scope-vars
--- k   2012-05-21 20:38:15.912149444 +0200
+++ README-scope-vars   2012-05-21 20:12:13.270023293 +0200
@@ -53,6 +62,7 @@ rtas_next
 s390_virtio_bus_info
 sh_table
 special_register_prefix
+test_image
 timers_state
 v9fs_synth_root
 virtcon_hds

[Qemu-devel] [PATCH 0/9] convert many more globals to static

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Following up on discussion here,

  http://marc.info/?t=13375948768r=1w=2

here are patches to limit the scope of the remaining global variables.
Most changes simply added a preceding static.  However, in some cases,
I've made minor additional changes, e.g., to make a static
table const as well (which sometimes required making an iterator
pointer const, too), and to move or remove declarations of variables
that the compiler then was able to identify as unused.

Initially I put the changes to each file in a separate commit, but that
got old quickly, and I lumped all of the remaining changes into the
9th commit.  If that's a problem, let me know and I'll separate it.


Jim Meyering (9):
  ccid: declare DEFAULT_ATR table to be static const
  tcg: declare __jit_debug_descriptor to be static
  alpha-dis: remove unused global; declare others to be static
  linux-user: arg_table need not have global scope
  ccid: make backend_enum_table static const and adjust users
  sheepdog: declare bdrv_sheepdog to be static
  mips-dis: declare four globals to be static
  bonito: declare bonito_state to be static
  convert many more globals to static

 alpha-dis.c   | 26 ++--
 arm-dis.c |  8 ++---
 block/sheepdog.c  |  2 +-
 cpus.c|  4 +--
 cris-dis.c|  2 +-
 hw/9pfs/virtio-9p-synth.c |  2 +-
 hw/bonito.c   |  2 +-
 hw/ccid-card-emulated.c   |  6 ++--
 hw/ccid-card-passthru.c   |  2 +-
 hw/ide/pci.c  |  2 +-
 hw/leon3.c|  2 +-
 hw/mips_fulong2e.c|  2 +-
 hw/s390-virtio-bus.c  |  2 +-
 hw/spapr_rtas.c   |  2 +-
 hw/xen_platform.c |  2 +-
 hw/xgmac.c|  2 +-
 linux-user/main.c |  6 ++--
 m68k-dis.c| 79 ---
 memory.c  |  2 +-
 microblaze-dis.c  |  6 ++--
 mips-dis.c| 15 +
 ppc-dis.c | 26 
 sh4-dis.c |  2 +-
 target-cris/translate.c   |  2 +-
 target-i386/cpu.c |  4 +--
 target-i386/kvm.c |  2 +-
 tcg/tcg.c |  2 +-
 tests/fdc-test.c  |  2 +-
 vl.c  | 12 ---
 29 files changed, 110 insertions(+), 118 deletions(-)

--
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 1/9] ccid: declare DEFAULT_ATR table to be static const

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/ccid-card-passthru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
index bd6c777..1caaa45 100644
--- a/hw/ccid-card-passthru.c
+++ b/hw/ccid-card-passthru.c
@@ -27,7 +27,7 @@ do {\
 #define D_VERBOSE 4

 /* TODO: do we still need this? */
-uint8_t DEFAULT_ATR[] = {
+static const uint8_t DEFAULT_ATR[] = {
 /*
  * From some example somewhere
  * 0x3B, 0xB0, 0x18, 0x00, 0xD1, 0x81, 0x05, 0xB1, 0x40, 0x38, 0x1F, 0x03, 0x28
-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 1/9] ccid: declare DEFAULT_ATR table to be static const

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/ccid-card-passthru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
index bd6c777..1caaa45 100644
--- a/hw/ccid-card-passthru.c
+++ b/hw/ccid-card-passthru.c
@@ -27,7 +27,7 @@ do {\
 #define D_VERBOSE 4

 /* TODO: do we still need this? */
-uint8_t DEFAULT_ATR[] = {
+static const uint8_t DEFAULT_ATR[] = {
 /*
  * From some example somewhere
  * 0x3B, 0xB0, 0x18, 0x00, 0xD1, 0x81, 0x05, 0xB1, 0x40, 0x38, 0x1F, 0x03, 0x28
-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 2/9] tcg: declare __jit_debug_descriptor to be static

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 tcg/tcg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index ab589c7..350fdad 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2293,7 +2293,7 @@ void __jit_debug_register_code(void)

 /* Must statically initialize the version, because GDB may check
the version before we can set it.  */
-struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };
+static struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };

 /* End GDB interface.  */

-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 2/9] tcg: declare __jit_debug_descriptor to be static

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 tcg/tcg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index ab589c7..350fdad 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2293,7 +2293,7 @@ void __jit_debug_register_code(void)

 /* Must statically initialize the version, because GDB may check
the version before we can set it.  */
-struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };
+static struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };

 /* End GDB interface.  */

-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 3/9] alpha-dis: remove unused global; declare others to be static

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

alpha_num_operands: Remove both declarations of this unused global.
alpha_opcodes: Declare static to limit scope. Remove duplicate decl.
alpha_num_opcodes: Likewise.
alpha_operands: Likewise.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 alpha-dis.c | 26 +-
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/alpha-dis.c b/alpha-dis.c
index ae331b3..f39b56d 100644
--- a/alpha-dis.c
+++ b/alpha-dis.c
@@ -53,12 +53,6 @@ struct alpha_opcode
   unsigned char operands[4];
 };

-/* The table itself is sorted by major opcode number, and is otherwise
-   in the order in which the disassembler should consider
-   instructions.  */
-extern const struct alpha_opcode alpha_opcodes[];
-extern const unsigned alpha_num_opcodes;
-
 /* Values defined for the flags field of a struct alpha_opcode.  */

 /* CPU Availability */
@@ -134,12 +128,6 @@ struct alpha_operand
   int (*extract) (unsigned instruction, int *invalid);
 };

-/* Elements in the table are retrieved by indexing with values from
-   the operands field of the alpha_opcodes table.  */
-
-extern const struct alpha_operand alpha_operands[];
-extern const unsigned alpha_num_operands;
-
 /* Values defined for the flags field of a struct alpha_operand.  */

 /* Mask for selecting the type for typecheck purposes */
@@ -292,8 +280,10 @@ static int extract_ev6hwjhint (unsigned, int *);

 
 /* The operands table  */
+/* Elements are retrieved by indexing with values from
+   the operands field of the alpha_opcodes table.  */

-const struct alpha_operand alpha_operands[] =
+static const struct alpha_operand alpha_operands[] =
 {
   /* The fields are bits, shift, insert, extract, flags */
   /* The zero index is used to indicate end-of-list */
@@ -424,8 +414,6 @@ const struct alpha_operand alpha_operands[] =
 insert_ev6hwjhint, extract_ev6hwjhint }
 };

-const unsigned alpha_num_operands = 
sizeof(alpha_operands)/sizeof(*alpha_operands);
-
 /* The RB field when it is the same as the RA field in the same insn.
This operand is marked fake.  The insertion function just copies
the RA field into the RB field, and the extraction function just
@@ -662,6 +650,9 @@ extract_ev6hwjhint(unsigned insn, int *invalid 
ATTRIBUTE_UNUSED)
 #define ARG_EV6HWMEM   { RA, EV6HWDISP, PRB }
 
 /* The opcode table.
+   The table itself is sorted by major opcode number, and is otherwise
+   in the order in which the disassembler should consider
+   instructions.

The format of the opcode table is:

@@ -706,7 +697,7 @@ extract_ev6hwjhint(unsigned insn, int *invalid 
ATTRIBUTE_UNUSED)
that were not assigned to a particular extension.
 */

-const struct alpha_opcode alpha_opcodes[] = {
+static const struct alpha_opcode alpha_opcodes[] = {
   { halt,SPCD(0x00,0x), BASE, ARG_NONE },
   { draina,  SPCD(0x00,0x0002), BASE, ARG_NONE },
   { bpt, SPCD(0x00,0x0080), BASE, ARG_NONE },
@@ -1732,7 +1723,8 @@ const struct alpha_opcode alpha_opcodes[] = {
   { bgt, BRA(0x3F), BASE, ARG_BRA },
 };

-const unsigned alpha_num_opcodes = 
sizeof(alpha_opcodes)/sizeof(*alpha_opcodes);
+static const unsigned alpha_num_opcodes =
+  sizeof(alpha_opcodes)/sizeof(*alpha_opcodes);

 /* OSF register names.  */

-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 3/9] alpha-dis: remove unused global; declare others to be static

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

alpha_num_operands: Remove both declarations of this unused global.
alpha_opcodes: Declare static to limit scope. Remove duplicate decl.
alpha_num_opcodes: Likewise.
alpha_operands: Likewise.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 alpha-dis.c | 26 +-
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/alpha-dis.c b/alpha-dis.c
index ae331b3..f39b56d 100644
--- a/alpha-dis.c
+++ b/alpha-dis.c
@@ -53,12 +53,6 @@ struct alpha_opcode
   unsigned char operands[4];
 };

-/* The table itself is sorted by major opcode number, and is otherwise
-   in the order in which the disassembler should consider
-   instructions.  */
-extern const struct alpha_opcode alpha_opcodes[];
-extern const unsigned alpha_num_opcodes;
-
 /* Values defined for the flags field of a struct alpha_opcode.  */

 /* CPU Availability */
@@ -134,12 +128,6 @@ struct alpha_operand
   int (*extract) (unsigned instruction, int *invalid);
 };

-/* Elements in the table are retrieved by indexing with values from
-   the operands field of the alpha_opcodes table.  */
-
-extern const struct alpha_operand alpha_operands[];
-extern const unsigned alpha_num_operands;
-
 /* Values defined for the flags field of a struct alpha_operand.  */

 /* Mask for selecting the type for typecheck purposes */
@@ -292,8 +280,10 @@ static int extract_ev6hwjhint (unsigned, int *);

 
 /* The operands table  */
+/* Elements are retrieved by indexing with values from
+   the operands field of the alpha_opcodes table.  */

-const struct alpha_operand alpha_operands[] =
+static const struct alpha_operand alpha_operands[] =
 {
   /* The fields are bits, shift, insert, extract, flags */
   /* The zero index is used to indicate end-of-list */
@@ -424,8 +414,6 @@ const struct alpha_operand alpha_operands[] =
 insert_ev6hwjhint, extract_ev6hwjhint }
 };

-const unsigned alpha_num_operands = 
sizeof(alpha_operands)/sizeof(*alpha_operands);
-
 /* The RB field when it is the same as the RA field in the same insn.
This operand is marked fake.  The insertion function just copies
the RA field into the RB field, and the extraction function just
@@ -662,6 +650,9 @@ extract_ev6hwjhint(unsigned insn, int *invalid 
ATTRIBUTE_UNUSED)
 #define ARG_EV6HWMEM   { RA, EV6HWDISP, PRB }
 
 /* The opcode table.
+   The table itself is sorted by major opcode number, and is otherwise
+   in the order in which the disassembler should consider
+   instructions.

The format of the opcode table is:

@@ -706,7 +697,7 @@ extract_ev6hwjhint(unsigned insn, int *invalid 
ATTRIBUTE_UNUSED)
that were not assigned to a particular extension.
 */

-const struct alpha_opcode alpha_opcodes[] = {
+static const struct alpha_opcode alpha_opcodes[] = {
   { halt,SPCD(0x00,0x), BASE, ARG_NONE },
   { draina,  SPCD(0x00,0x0002), BASE, ARG_NONE },
   { bpt, SPCD(0x00,0x0080), BASE, ARG_NONE },
@@ -1732,7 +1723,8 @@ const struct alpha_opcode alpha_opcodes[] = {
   { bgt, BRA(0x3F), BASE, ARG_BRA },
 };

-const unsigned alpha_num_opcodes = 
sizeof(alpha_opcodes)/sizeof(*alpha_opcodes);
+static const unsigned alpha_num_opcodes =
+  sizeof(alpha_opcodes)/sizeof(*alpha_opcodes);

 /* OSF register names.  */

-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 6/9] sheepdog: declare bdrv_sheepdog to be static

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 block/sheepdog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index e01d371..fdb3eca 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2014,7 +2014,7 @@ static QEMUOptionParameter sd_create_options[] = {
 { NULL }
 };

-BlockDriver bdrv_sheepdog = {
+static BlockDriver bdrv_sheepdog = {
 .format_name= sheepdog,
 .protocol_name  = sheepdog,
 .instance_size  = sizeof(BDRVSheepdogState),
-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 4/9] linux-user: arg_table need not have global scope

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com

Declare arg_table to be static const, and adjust the two users
to also be const.

Signed-off-by: Jim Meyering meyer...@redhat.com
---
 linux-user/main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 191b750..cc5b54b 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3133,7 +3133,7 @@ struct qemu_argument {
 const char *help;
 };

-struct qemu_argument arg_table[] = {
+static const struct qemu_argument arg_table[] = {
 {h,  , false, handle_arg_help,
  ,   print this help},
 {g,  QEMU_GDB, true,  handle_arg_gdb,
@@ -3175,7 +3175,7 @@ struct qemu_argument arg_table[] = {

 static void usage(void)
 {
-struct qemu_argument *arginfo;
+const struct qemu_argument *arginfo;
 int maxarglen;
 int maxenvlen;

@@ -3241,7 +3241,7 @@ static int parse_args(int argc, char **argv)
 {
 const char *r;
 int optind;
-struct qemu_argument *arginfo;
+const struct qemu_argument *arginfo;

 for (arginfo = arg_table; arginfo-handle_opt != NULL; arginfo++) {
 if (arginfo-env == NULL) {
-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 8/9] bonito: declare bonito_state to be static

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/bonito.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/bonito.c b/hw/bonito.c
index 77786f8..6bd0242 100644
--- a/hw/bonito.c
+++ b/hw/bonito.c
@@ -218,7 +218,7 @@ typedef struct PCIBonitoState

 } PCIBonitoState;

-PCIBonitoState * bonito_state;
+static PCIBonitoState *bonito_state;

 static void bonito_writel(void *opaque, target_phys_addr_t addr,
   uint64_t val, unsigned size)
-- 
1.7.10.2.552.gaa3bb87

[Qemu-devel] [PATCH 5/9] ccid: make backend_enum_table static const and adjust users

2012-05-21 Thread Jim Meyering

From: Jim Meyering meyer...@redhat.com


Signed-off-by: Jim Meyering meyer...@redhat.com
---
 hw/ccid-card-emulated.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ccid-card-emulated.c b/hw/ccid-card-emulated.c
index f4a6da4..440f050 100644
--- a/hw/ccid-card-emulated.c
+++ b/hw/ccid-card-emulated.c
@@ -462,14 +462,14 @@ typedef struct EnumTable {
 uint32_t value;
 } EnumTable;

-EnumTable backend_enum_table[] = {
+static const EnumTable backend_enum_table[] = {
 {BACKEND_NSS_EMULATED_NAME, BACKEND_NSS_EMULATED},
 {BACKEND_CERTIFICATES_NAME, BACKEND_CERTIFICATES},
 {NULL, 0},
 };

 static uint32_t parse_enumeration(char *str,
-EnumTable *table, uint32_t not_found_value)
+const EnumTable *table, uint32_t not_found_value)
 {
 uint32_t ret = not_found_value;

@@ -487,7 +487,7 @@ static int emulated_initfn(CCIDCardState *base)
 {
 EmulatedState *card = DO_UPCAST(EmulatedState, base, base);
 VCardEmulError ret;
-EnumTable *ptable;
+const EnumTable *ptable;

 QSIMPLEQ_INIT(card-event_list);
 QSIMPLEQ_INIT(card-guest_apdu_list);
-- 
1.7.10.2.552.gaa3bb87

Re: [Qemu-devel] [PATCH 2/9] tcg: declare __jit_debug_descriptor to be static

2012-05-21 Thread Jim Meyering

Peter Maydell wrote:
 On 21 May 2012 20:51, Jim Meyering j...@meyering.net wrote:
 From: Jim Meyering meyer...@redhat.com


 Signed-off-by: Jim Meyering meyer...@redhat.com
 ---
  tcg/tcg.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/tcg/tcg.c b/tcg/tcg.c
 index ab589c7..350fdad 100644
 --- a/tcg/tcg.c
 +++ b/tcg/tcg.c
 @@ -2293,7 +2293,7 @@ void __jit_debug_register_code(void)

  /* Must statically initialize the version, because GDB may check
    the version before we can set it.  */
 -struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };
 +static struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };

  /* End GDB interface.  */

 Nak. This symbol is global so that gdb can find it by fishing around
 in the executable's symbol table.

Thanks for the quick feedback.

How does the scope of the symbol affect whether gdb can find it?
With it declared static, it seems to be no less visible than before:

$ gdb --eval 'p __jit_debug_descriptor' x86_64-softmmu/qemu-system-x86_64
Reading symbols from 
/h/j/w/co/qemu/x86_64-softmmu/qemu-system-x86_64...done.
$1 = {
  version = 1,
  action_flag = 0,
  relevant_entry = 0x0,
  first_entry = 0x0
}

If declaring this variable static is not appropriate,
then the comment saying that static initialization is desired
should be changed.

1 2 >

1 - 100 of 157 matches

Mail list logo