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

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

[Jim Meyering]

From: Jim Meyering <meyering@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 01/20] scsi, pci, qdev, isa-bus, sysbus: don't let *_get_fw_dev_path return NULL

[Jim Meyering]

From: Jim Meyering <meyering@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 <meyering@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 5ea9b8f..f2e4ea4 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -60,7 +60,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 f9b2373..47c93d3 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -236,7 +236,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 f855cf3..de4b448 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1962,7 +1962,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 b5a52ac..3b5ce33 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -520,7 +520,7 @@ char* qdev_get_fw_dev_path(DeviceState *dev)

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

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

 char *qdev_get_dev_path(DeviceState *dev)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 058d3b2..dfb2631 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1723,12 +1723,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 9d8b1ea..c173840 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -211,7 +211,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.8.0.rc0.18.gf84667d

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

[Anthony Liguori]

Jim Meyering <jim@meyering.net> writes:

> From: Jim Meyering <meyering@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 <meyering@redhat.com>

Applied all. Thanks.

Regards,

Anthony Liguori

> ---
>  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 5ea9b8f..f2e4ea4 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -60,7 +60,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 f9b2373..47c93d3 100644
> --- a/hw/isa-bus.c
> +++ b/hw/isa-bus.c
> @@ -236,7 +236,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 f855cf3..de4b448 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1962,7 +1962,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 b5a52ac..3b5ce33 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -520,7 +520,7 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
>
>      path[l-1] = '\0';
>
> -    return strdup(path);
> +    return g_strdup(path);
>  }
>
>  char *qdev_get_dev_path(DeviceState *dev)
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index 058d3b2..dfb2631 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -1723,12 +1723,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 9d8b1ea..c173840 100644
> --- a/hw/sysbus.c
> +++ b/hw/sysbus.c
> @@ -211,7 +211,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.8.0.rc0.18.gf84667d

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

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

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

Signed-off-by: Jim Meyering <meyering@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 03/20] block: avoid buffer overrun by using pstrcpy, not strncpy

[Jim Meyering]

From: Jim Meyering <meyering@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 <kwolf@redhat.com>
Signed-off-by: Jim Meyering <meyering@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 04/20] sheepdog: avoid a few buffer overruns

[Jim Meyering]

From: Jim Meyering <meyering@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 <kwolf@redhat.com>
Acked-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Jim Meyering <meyering@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 05/20] vmdk: relative_path: use pstrcpy in place of strncpy

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

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

Acked-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Jim Meyering <meyering@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 06/20] hw/9pfs: avoid buffer overrun

[Jim Meyering]

From: Jim Meyering <meyering@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.kumar@linux.vnet.ibm.com>
Signed-off-by: Jim Meyering <meyering@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 07/20] lm32: avoid buffer overrun

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

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

Signed-off-by: Jim Meyering <meyering@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 08/20] os-posix: avoid buffer overrun

[Jim Meyering]

From: Jim Meyering <meyering@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 <meyering@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 09/20] ppc: avoid buffer overrun: use pstrcpy, not strncpy

[Jim Meyering]

From: Jim Meyering <meyering@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 <meyering@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 10/20] linux-user: remove two unchecked uses of strdup

[Jim Meyering]

From: Jim Meyering <meyering@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.maydell@linaro.org>
Signed-off-by: Jim Meyering <meyering@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 11/20] ui/vnc: simplify and avoid strncpy

[Jim Meyering]

From: Jim Meyering <meyering@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 <meyering@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

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

[Peter Maydell]

On 4 October 2012 12:09, Jim Meyering <jim@meyering.net> wrote:
> From: Jim Meyering <meyering@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 <meyering@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?)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

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

[Jim Meyering]

Peter Maydell wrote:

> On 4 October 2012 12:09, Jim Meyering <jim@meyering.net> wrote:
>> From: Jim Meyering <meyering@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 <meyering@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.maydell@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

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

[Peter Maydell]

On 4 October 2012 19:56, Jim Meyering <jim@meyering.net> wrote:
> Peter Maydell wrote:
>> (Does the compiler really insist on that cast?)
>
> Yes.  Without it, I get a warning/failure when char is signed.
> See below.

Ah yes, signed vs unsigned char. Thanks for the clarification.

-- PMM

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

[Jim Meyering]

From: Jim Meyering <meyering@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 <meyering@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 13/20] virtio-9p: avoid unwarranted uses of strncpy

[Jim Meyering]

From: Jim Meyering <meyering@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.kumar@linux.vnet.ibm.com>
Signed-off-by: Jim Meyering <meyering@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

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

[Jim Meyering]

From: Jim Meyering <meyering@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 <david@gibson.dropbear.id.au>
Signed-off-by: Jim Meyering <meyering@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 15/20] qemu-ga: prefer pstrcpy: consistently NUL-terminate ifreq.ifr_name

[Jim Meyering]

From: Jim Meyering <meyering@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 <lcapitulino@redhat.com>
Signed-off-by: Jim Meyering <meyering@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 16/20] libcacard/vcard_emul_nss: use pstrcpy in place of strncpy

[Jim Meyering]

From: Jim Meyering <meyering@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 <alevy@redhat.com>
Signed-off-by: Jim Meyering <meyering@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 17/20] acpi: remove strzcpy (strncpy-identical) function; just use strncpy

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

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

Signed-off-by: Jim Meyering <meyering@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 18/20] qcow2: mark this file's sole strncpy use as justified

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

Acked-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Jim Meyering <meyering@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 19/20] hw/r2d: add comment: this strncpy use is ok

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>


Signed-off-by: Jim Meyering <meyering@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 20/20] doc: update HACKING wrt strncpy/pstrcpy

[Jim Meyering]

From: Jim Meyering <meyering@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 <meyering@redhat.com>
---
 HACKING | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/HACKING b/HACKING
index 471cf1d..dddd617 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