[Qemu-devel] strncpy: best avoided (resend)

[Qemu-devel] strncpy: best avoided (resend)

[Jim Meyering]

[Argh.  First attempt omitted the most important address: qemu-devel.
 Sorry to all who get two copies. ]

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).

Here's a Q&D classification of these change sets:

overrun                      block.c
several overruns             block/sheepdog.c
overrun                      block/vmdk.c
2 overruns                   hw/9pfs/virtio-9p-synth.c
overrun                      hw/lm32_hwsetup.h
overrun                      os-posix.c
overrun                      target-ppc/kvm.c

2-unchecked-strdup+comment   linux-user/elfload.c
simplify/avoid strncpy       ui/vnc-auth-sasl.c
snprintf+pstrcpy fragile     hw/bt-hci.c

2 x s/strncpy/memcpy/        hw/9pfs/virtio-9p-posix-acl.c
s/strncpy/memcpy/            hw/9pfs/virtio-9p-xattr-user.c
s/strncpy/memcpy/            hw/9pfs/virtio-9p-xattr.c
s/strncpy/memcpy/            hw/spapr_vscsi.c
s/strncpy/pstrcpy/ +Makefile libcacard/vcard_emul_nss.c
s/strncpy/pstrcpy            qga/commands-posix.c
s/strncpy/pstrcpy            target-i386/cpu.c
remove strzcpy definition    hw/acpi.c

comment                      block/qcow2.c
comment                      hw/r2d.c
comment                      hw/scsi-bus.c
comment                      HACKING

qemu-trivial@nongnu.org


[0] I have mixed feelings about marking with comments the remaining,
justifiable uses of strncpy.  Note: I didn't finish that job.  On one
hand, it's good to document what I've done, presumably saving others
the work of auditing those uses.  On the other, it's ugly and not very
maintainable.

After writing the few comment-adding patches that merely say "this strncpy
use is justified...", I realized it might be better to use a different
symbol to indicate when one really does intend to use strncpy and realizes
the implications.  That would make it trivial to prohibit (mechanically)
any new direct use of "strncpy".  Then all "safe/intended" uses would
use the new symbol, say strNcpy or STRNCPY (with a comment explaining
the issues near the definition of the macro or static inline function),
and we wouldn't have to pollute the code with comments marking each use.
If there's interest (or no objection) I'll do that separately.

[PATCH 01/22] block: avoid buffer overrun by using pstrcpy, not
[PATCH 02/22] sheepdog: avoid a few buffer overruns
[PATCH 03/22] vmdk: relative_path: avoid buffer overrun
[PATCH 04/22] hw/9pfs: avoid buffer overrun
[PATCH 05/22] lm32: avoid buffer overrun
[PATCH 06/22] os-posix: avoid buffer overrun
[PATCH 07/22] ppc: avoid buffer overrun: use pstrcpy, not strncpy
[PATCH 08/22] linux-user: remove two unchecked uses of strdup
[PATCH 09/22] ui/vnc: simplify and avoid strncpy
[PATCH 10/22] bt: replace fragile snprintf use and unwarranted
[PATCH 11/22] virtio-9p: avoid unwarranted uses of strncpy
[PATCH 12/22] virtio-9p: avoid unwarranted use of strncpy
[PATCH 13/22] virtio-9p: avoid unwarranted use of strncpy
[PATCH 14/22] vscsi: avoid unwarranted strncpy
[PATCH 15/22] target-i386: use pstrcpy, not strncpy
[PATCH 16/22] qemu-ga: prefer pstrcpy: consistently NUL-terminate
[PATCH 17/22] libcacard/vcard_emul_nss: use pstrcpy in place of
[PATCH 18/22] acpi: remove strzcpy (strncpy-identical) function;
[PATCH 19/22] qcow2: mark this file's sole strncpy use as justified
[PATCH 20/22] hw/r2d: add comment: this strncpy use is ok
[PATCH 21/22] scsi: mark an strncpy use as valid
[PATCH 22/22] doc: update HACKING wrt strncpy/pstrcpy

[Qemu-devel] [PATCH 01/22] 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.

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 43c794c..ddc0f58 100644
--- a/block.c
+++ b/block.c
@@ -1162,7 +1162,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)
@@ -1182,7 +1182,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.1.487.ga3935e6

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

[Kevin Wolf]

Am 09.05.2012 11:23, schrieb 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.
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Not absolutely required because the source buffer is 1024 bytes and null
terminated as well, but it doesn't hurt either.

Acked-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 02/22] 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.

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 0ed6b19..f2579da 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -852,14 +852,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 */
@@ -886,7 +886,10 @@ static int find_vdi_name(BDRVSheepdogState *s, char *filename, uint32_t snapid,
         return -1;
     }

-    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);

@@ -1129,7 +1132,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;
@@ -1157,8 +1160,11 @@ static int do_sd_create(char *filename, int64_t vdi_size,
         return -EIO;
     }

+    /* 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;
@@ -1709,6 +1715,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);
@@ -1771,13 +1780,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);
@@ -1905,8 +1914,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.1.487.ga3935e6

Re: [Qemu-devel] [PATCH 02/22] sheepdog: avoid a few buffer overruns

[Kevin Wolf]

Am 09.05.2012 11:23, schrieb 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.
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Acked-by: Kevin Wolf <kwolf@redhat.com>

Kazutaka, can you have a look? I think you may want to send patches on
top to improve the places where Jim just put a comment.

Kevin

> ---
>  block/sheepdog.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 0ed6b19..f2579da 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -852,14 +852,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 */
> @@ -886,7 +886,10 @@ static int find_vdi_name(BDRVSheepdogState *s, char *filename, uint32_t snapid,
>          return -1;
>      }
> 
> -    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);
> 
> @@ -1129,7 +1132,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;
> @@ -1157,8 +1160,11 @@ static int do_sd_create(char *filename, int64_t vdi_size,
>          return -EIO;
>      }
> 
> +    /* 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;
> @@ -1709,6 +1715,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);
> @@ -1771,13 +1780,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);
> @@ -1905,8 +1914,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++;
>          }
>      }

Re: [Qemu-devel] [PATCH 02/22] sheepdog: avoid a few buffer overruns

[MORITA Kazutaka]

At Wed, 09 May 2012 11:46:10 +0200,
Kevin Wolf wrote:
> 
> Am 09.05.2012 11:23, schrieb 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.
> > 
> > Signed-off-by: Jim Meyering <meyering@redhat.com>
> 
> Acked-by: Kevin Wolf <kwolf@redhat.com>
> 
> Kazutaka, can you have a look? I think you may want to send patches on
> top to improve the places where Jim just put a comment.

Acked-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

I'll send a patch on top of this to address Jim's comments.

Kazutaka

> 
> Kevin
> 
> > ---
> >  block/sheepdog.c | 34 ++++++++++++++++++++++------------
> >  1 file changed, 22 insertions(+), 12 deletions(-)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index 0ed6b19..f2579da 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -852,14 +852,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 */
> > @@ -886,7 +886,10 @@ static int find_vdi_name(BDRVSheepdogState *s, char *filename, uint32_t snapid,
> >          return -1;
> >      }
> > 
> > -    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);
> > 
> > @@ -1129,7 +1132,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;
> > @@ -1157,8 +1160,11 @@ static int do_sd_create(char *filename, int64_t vdi_size,
> >          return -EIO;
> >      }
> > 
> > +    /* 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;
> > @@ -1709,6 +1715,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);
> > @@ -1771,13 +1780,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);
> > @@ -1905,8 +1914,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++;
> >          }
> >      }
> 
> 

[Qemu-devel] [PATCH 03/22] vmdk: relative_path: avoid buffer overrun

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

strncpy does not guarantee NUL-termination.
Setting dest[n-1] = '\0' *before* calling strncpy(dest, src, n-1)
is a no-op.  Use pstrcpy to ensure NUL-termination, not strncpy.

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 18e9b4c..045e279 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 - 1, target);
         return 0;
     }
     while (base[i] == target[i]) {
-- 
1.7.10.1.487.ga3935e6

Re: [Qemu-devel] [PATCH 03/22] vmdk: relative_path: avoid buffer overrun

[Kevin Wolf]

Am 09.05.2012 11:23, schrieb Jim Meyering:
> From: Jim Meyering <meyering@redhat.com>
> 
> strncpy does not guarantee NUL-termination.
> Setting dest[n-1] = '\0' *before* calling strncpy(dest, src, n-1)
> is a no-op.  Use pstrcpy to ensure NUL-termination, not strncpy.

It's not, it would only be a no-op before strncpy(dest, src, n). But
pstrcpy() is definitely nicer.

> 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 18e9b4c..045e279 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 - 1, target);

I think you mean pstrcpy(dest, dest_size, target).

>          return 0;
>      }
>      while (base[i] == target[i]) {

Kevin

Re: [Qemu-devel] [PATCH 03/22] vmdk: relative_path: avoid buffer overrun

[Jim Meyering]

Kevin Wolf wrote:
> Am 09.05.2012 11:23, schrieb Jim Meyering:
>> From: Jim Meyering <meyering@redhat.com>
>>
>> strncpy does not guarantee NUL-termination.
>> Setting dest[n-1] = '\0' *before* calling strncpy(dest, src, n-1)
>> is a no-op.  Use pstrcpy to ensure NUL-termination, not strncpy.
>
> It's not, it would only be a no-op before strncpy(dest, src, n). But
> pstrcpy() is definitely nicer.
>
>> 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 18e9b4c..045e279 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 - 1, target);
>
> I think you mean pstrcpy(dest, dest_size, target).

Good points.  Thanks for the review.
Here's the corrected commit:

>From 78a367d846c95e84fddc971cbae544f24bc3455f Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Mon, 7 May 2012 12:46:02 +0200
Subject: [PATCH] vmdk: relative_path: use pstrcpy in place of strncpy

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

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 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.1.487.ga3935e6

Re: [Qemu-devel] [PATCH 03/22] vmdk: relative_path: avoid buffer overrun

[Kevin Wolf]

Am 09.05.2012 14:09, schrieb Jim Meyering:
> Kevin Wolf wrote:
>> Am 09.05.2012 11:23, schrieb Jim Meyering:
>>> From: Jim Meyering <meyering@redhat.com>
>>>
>>> strncpy does not guarantee NUL-termination.
>>> Setting dest[n-1] = '\0' *before* calling strncpy(dest, src, n-1)
>>> is a no-op.  Use pstrcpy to ensure NUL-termination, not strncpy.
>>
>> It's not, it would only be a no-op before strncpy(dest, src, n). But
>> pstrcpy() is definitely nicer.
>>
>>> 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 18e9b4c..045e279 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 - 1, target);
>>
>> I think you mean pstrcpy(dest, dest_size, target).
> 
> Good points.  Thanks for the review.
> Here's the corrected commit:
> 
> From 78a367d846c95e84fddc971cbae544f24bc3455f Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering@redhat.com>
> Date: Mon, 7 May 2012 12:46:02 +0200
> Subject: [PATCH] vmdk: relative_path: use pstrcpy in place of strncpy
> 
> Avoid strncpy+manual-NUL-terminate.  Use pstrcpy instead.
> 
> 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 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.1.487.ga3935e6

Acked-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 04/22] 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.

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.7.10.1.487.ga3935e6

Re: [Qemu-devel] [PATCH 04/22] hw/9pfs: avoid buffer overrun

[Aneesh Kumar K.V]

Jim Meyering <jim@meyering.net> writes:

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

Acked-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.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.1.487.ga3935e6

[Qemu-devel] [PATCH 05/22] lm32: avoid buffer overrun

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

Actually do what the comment says, using pstrcpy to 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.7.10.1.487.ga3935e6

[Qemu-devel] [PATCH 06/22] 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 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.1.487.ga3935e6

[PATCH 07/22] 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 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.1.487.ga3935e6

[Qemu-devel] [PATCH 07/22] 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 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.1.487.ga3935e6

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

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

Remove unnecessary and unchecked uses of strdup,
and add a comment that this strncpy use is ok.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 linux-user/elfload.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index f3b1552..25175cc 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2338,12 +2338,14 @@ 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));
+    filename = ts->bprm->filename;
+    base_filename = basename(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);

     bswap_psinfo(psinfo);
     return (0);
-- 
1.7.10.1.487.ga3935e6

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

[Peter Maydell]

On 9 May 2012 10:23, Jim Meyering <jim@meyering.net> wrote:
> From: Jim Meyering <meyering@redhat.com>
>
> Remove unnecessary and unchecked uses of strdup,
> and add a comment that this strncpy use is ok.
>
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
>  linux-user/elfload.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index f3b1552..25175cc 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2338,12 +2338,14 @@ 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));
> +    filename = ts->bprm->filename;
> +    base_filename = basename(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);

This doesn't look right -- basename can modify
the string it's passed, which is why we create a copy
for it.

-- PMM

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

[Jim Meyering]

Peter Maydell wrote:
> On 9 May 2012 10:23, Jim Meyering <jim@meyering.net> wrote:
>> From: Jim Meyering <meyering@redhat.com>
>>
>> Remove unnecessary and unchecked uses of strdup,
>> and add a comment that this strncpy use is ok.
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>> ---
>>  linux-user/elfload.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index f3b1552..25175cc 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -2338,12 +2338,14 @@ 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));
>> +    filename = ts->bprm->filename;
>> +    base_filename = basename(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);
>
> This doesn't look right -- basename can modify
> the string it's passed, which is why we create a copy
> for it.

Hi Peter,

Thank you for the review and the correction.
I'm spoiled from using gnulib with which basename never modifies its argument.

Here's an adjusted patch:

>From 5dce6a0222252cdc2a45ada3e3e96a8c3ef4e90f Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@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 Meyering <meyering@redhat.com>
---
 linux-user/elfload.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index f3b1552..977283e 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2338,13 +2338,16 @@ 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));
+    filename = ts->bprm->filename;
+    base_filename = g_path_get_basename(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);

+    free(base_filename);
     bswap_psinfo(psinfo);
     return (0);
 }
--
1.7.10.1.487.ga3935e6

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

[Peter Maydell]

On 9 May 2012 14:42, Jim Meyering <jim@meyering.net> wrote:
> From 5dce6a0222252cdc2a45ada3e3e96a8c3ef4e90f Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering@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 Meyering <meyering@redhat.com>
> ---
>  linux-user/elfload.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index f3b1552..977283e 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2338,13 +2338,16 @@ 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));
> +    filename = ts->bprm->filename;
> +    base_filename = g_path_get_basename(filename);

'filename' is now pointless and you might as well
just drop it and use
 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);
>
> +    free(base_filename);

You need to g_free() this, not free() it, right?

>     bswap_psinfo(psinfo);
>     return (0);
>  }
> --
> 1.7.10.1.487.ga3935e6


-- PMM

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

[Jim Meyering]

Peter Maydell wrote:
> On 9 May 2012 14:42, Jim Meyering <jim@meyering.net> wrote:
>> From 5dce6a0222252cdc2a45ada3e3e96a8c3ef4e90f Mon Sep 17 00:00:00 2001
>> From: Jim Meyering <meyering@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 Meyering <meyering@redhat.com>
>> ---
>>  linux-user/elfload.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index f3b1552..977283e 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -2338,13 +2338,16 @@ 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));
>> +    filename = ts->bprm->filename;
>> +    base_filename = g_path_get_basename(filename);
>
> 'filename' is now pointless and you might as well
> just drop it and use
>  base_filename = g_path_get_basename(ts->bprm->filename);

I agree.

>> +    /*
>> +     * 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);
>>
>> +    free(base_filename);
>
> You need to g_free() this, not free() it, right?

Even if free would work, g_free looks better.
Thanks.

>From 402100deb7e27b1d7ac619ebac963f861fae91b0 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@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 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 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.1.487.ga3935e6

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

[Peter Maydell]

On 9 May 2012 15:01, Jim Meyering <jim@meyering.net> wrote:
> From 402100deb7e27b1d7ac619ebac963f861fae91b0 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering@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 Meyering <meyering@redhat.com>

This version
Reviewed-by: Peter Maydell <peter.maydell@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
-- PMM

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

[Jim Meyering]

Peter Maydell wrote:
> On 9 May 2012 15:01, Jim Meyering <jim@meyering.net> wrote:
>> From 402100deb7e27b1d7ac619ebac963f861fae91b0 Mon Sep 17 00:00:00 2001
>> From: Jim Meyering <meyering@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 Meyering <meyering@redhat.com>
>
> This version
> Reviewed-by: Peter Maydell <peter.maydell@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.

[Qemu-devel] [PATCH 09/22] 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.7.10.1.487.ga3935e6

[Qemu-devel] [PATCH 10/22] 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.7.10.1.487.ga3935e6

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

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

In both mp_pacl_listxattr and mp_dacl_listxattr, 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.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 hw/9pfs/virtio-9p-posix-acl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 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;
 }

-- 
1.7.10.1.487.ga3935e6

Re: [Qemu-devel] [PATCH 11/22] virtio-9p: avoid unwarranted uses of strncpy

[Aneesh Kumar K.V]

Jim Meyering <jim@meyering.net> writes:

> From: Jim Meyering <meyering@redhat.com>
>
> In both mp_pacl_listxattr and mp_dacl_listxattr, 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.
>
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Acked-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>


> ---
>  hw/9pfs/virtio-9p-posix-acl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 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;
>  }
>
> -- 
> 1.7.10.1.487.ga3935e6

[Qemu-devel] [PATCH 12/22] virtio-9p: avoid unwarranted use of strncpy

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

The use of strncpy in mp_user_listxattr is unnecessary, since we
know that the NUL-terminated source bytes fit in the destination
buffer.  Use memcpy in place of strncpy.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 hw/9pfs/virtio-9p-xattr-user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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;
 }

-- 
1.7.10.1.487.ga3935e6

Re: [Qemu-devel] [PATCH 12/22] virtio-9p: avoid unwarranted use of strncpy

[Aneesh Kumar K.V]

Jim Meyering <jim@meyering.net> writes:

> From: Jim Meyering <meyering@redhat.com>
>
> The use of strncpy in mp_user_listxattr is unnecessary, since we
> know that the NUL-terminated source bytes fit in the destination
> buffer.  Use memcpy in place of strncpy.
>
> Signed-off-by: Jim Meyering <meyering@redhat.com>


May be this can be folded into the earlier patch ?

Reviewed-by: Aneeseh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>


> ---
>  hw/9pfs/virtio-9p-xattr-user.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> 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;
>  }
>
> -- 
> 1.7.10.1.487.ga3935e6

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

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

The use of strncpy in pt_listxattr is unnecessary, since we
know that the NUL-terminated source bytes fit in the destination
buffer.  Use memcpy in place of strncpy.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 hw/9pfs/virtio-9p-xattr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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.1.487.ga3935e6

Re: [Qemu-devel] [PATCH 13/22] virtio-9p: avoid unwarranted use of strncpy

[Aneesh Kumar K.V]

Jim Meyering <jim@meyering.net> writes:

> From: Jim Meyering <meyering@redhat.com>
>
> The use of strncpy in pt_listxattr is unnecessary, since we
> know that the NUL-terminated source bytes fit in the destination
> buffer.  Use memcpy in place of strncpy.
>
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Fold this with Patch 11 ?

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>


> ---
>  hw/9pfs/virtio-9p-xattr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> 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.1.487.ga3935e6

Re: [Qemu-devel] [PATCH 13/22] virtio-9p: avoid unwarranted use of strncpy

[Jim Meyering]

Aneesh Kumar K.V wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> From: Jim Meyering <meyering@redhat.com>
>>
>> The use of strncpy in pt_listxattr is unnecessary, since we
>> know that the NUL-terminated source bytes fit in the destination
>> buffer.  Use memcpy in place of strncpy.
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>
> Fold this with Patch 11 ?
>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

Thanks for the reviews.
I've noted the Reviewed-by: and merged those three into one.

[Qemu-devel] [PATCH 14/22] 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.

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 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.1.487.ga3935e6

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

[David Gibson]

On Wed, May 09, 2012 at 11:23:58AM +0200, Jim Meyering wrote:
> 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.
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

\-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

[Jim Meyering]

From: Jim Meyering <meyering@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 <meyering@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 65d9af6..f810389 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1266,7 +1266,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.1.487.ga3935e6

[Qemu-devel] [PATCH 16/22] 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.

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 d58730a..7112984 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -786,7 +786,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 addres of %s: %s",
-- 
1.7.10.1.487.ga3935e6

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

[Luiz Capitulino]

On Wed,  9 May 2012 11:24:00 +0200
Jim Meyering <jim@meyering.net> wrote:

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

Reviewed-by: Luiz Capitulino <lcapitulino@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 d58730a..7112984 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -786,7 +786,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 addres of %s: %s",

[Qemu-devel] [PATCH 17/22] 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.

Signed-off-by: Jim Meyering <meyering@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.1.487.ga3935e6

[Qemu-devel] [PATCH 18/22] 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 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.1.487.ga3935e6

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

[Peter Maydell]

On 9 May 2012 10:24, Jim Meyering <jim@meyering.net> wrote:
> From: Jim Meyering <meyering@redhat.com>
>
> Adjust all uses s/strzcpy/strncpy/ and mark these uses
> of strncpy as "ok".

Note that this will conflict with Paolo's patch
 http://patchwork.ozlabs.org/patch/151895/
"convert -acpitable to QemuOpts" which also gets rid
of the pointless strzcpy().

-- PMM

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

[Jim Meyering]

Peter Maydell wrote:
> On 9 May 2012 10:24, Jim Meyering <jim@meyering.net> wrote:
>> From: Jim Meyering <meyering@redhat.com>
>>
>> Adjust all uses s/strzcpy/strncpy/ and mark these uses
>> of strncpy as "ok".
>
> Note that this will conflict with Paolo's patch
>  http://patchwork.ozlabs.org/patch/151895/
> "convert -acpitable to QemuOpts" which also gets rid
> of the pointless strzcpy().

Good point.
I suggest to drop mine, since it's purely a clean-up change.

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

[Paolo Bonzini]

Il 09/05/2012 16:15, Jim Meyering ha scritto:
>> > Note that this will conflict with Paolo's patch
>> >  http://patchwork.ozlabs.org/patch/151895/
>> > "convert -acpitable to QemuOpts" which also gets rid
>> > of the pointless strzcpy().
> Good point.
> I suggest to drop mine, since it's purely a clean-up change.

No big deal, if I come later I'll fix the conflict.

Paolo

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

[Jim Meyering]

From: Jim Meyering <meyering@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 8c60a6f..abc985e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -990,6 +990,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.1.487.ga3935e6

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

[Kevin Wolf]

Am 09.05.2012 11:24, schrieb Jim Meyering:
> From: Jim Meyering <meyering@redhat.com>
> 
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Acked-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 20/22] 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 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.1.487.ga3935e6

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

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>


Signed-off-by: Jim Meyering <meyering@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 dbdb99c..916f425 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -402,6 +402,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.1.487.ga3935e6

[Qemu-devel] [PATCH 22/22] 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.7.10.1.487.ga3935e6

Re: [Qemu-devel] strncpy: best avoided (resend)

[Kevin Wolf]

Am 09.05.2012 11:23, schrieb Jim Meyering:
> [Argh.  First attempt omitted the most important address: qemu-devel.
>  Sorry to all who get two copies. ]
> 
> 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.

Anthony, I think it's best if you (or any other committer) can take the
whole series at once, so I didn't pick up the block patches in it but
just added my Acked-by. Hope that's okay with you.

Kevin

Re: [Qemu-devel] strncpy: best avoided (resend)

[Jim Meyering]

Kevin Wolf wrote:
> Am 09.05.2012 11:23, schrieb Jim Meyering:
>> [Argh.  First attempt omitted the most important address: qemu-devel.
>>  Sorry to all who get two copies. ]
>>
>> 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.
>
> Anthony, I think it's best if you (or any other committer) can take the
> whole series at once, so I didn't pick up the block patches in it but
> just added my Acked-by. Hope that's okay with you.

I can post a V2 series (with ACK and Reviewed-by comments already added)
or push to a publicly accessible git server if that helps.

Re: [Qemu-devel] strncpy: best avoided (resend)

[Anthony Liguori]

On 05/10/2012 11:02 AM, Jim Meyering wrote:
> Kevin Wolf wrote:
>> Am 09.05.2012 11:23, schrieb Jim Meyering:
>>> [Argh.  First attempt omitted the most important address: qemu-devel.
>>>   Sorry to all who get two copies. ]
>>>
>>> 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.
>>
>> Anthony, I think it's best if you (or any other committer) can take the
>> whole series at once, so I didn't pick up the block patches in it but
>> just added my Acked-by. Hope that's okay with you.
>
> I can post a V2 series (with ACK and Reviewed-by comments already added)

My scripts will add them so no worries.  I'll queue them up shortly.

Thanks,

Anthony Liguori

> or push to a publicly accessible git server if that helps.

Re: [Qemu-devel] strncpy: best avoided (resend)

[Anthony Liguori]

On 05/10/2012 11:02 AM, Jim Meyering wrote:
> Kevin Wolf wrote:
>> Am 09.05.2012 11:23, schrieb Jim Meyering:
>>> [Argh.  First attempt omitted the most important address: qemu-devel.
>>>   Sorry to all who get two copies. ]
>>>
>>> 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.
>>
>> Anthony, I think it's best if you (or any other committer) can take the
>> whole series at once, so I didn't pick up the block patches in it but
>> just added my Acked-by. Hope that's okay with you.
>
> I can post a V2 series (with ACK and Reviewed-by comments already added)
> or push to a publicly accessible git server if that helps.

Sorry, I didn't realize that you had updated patches in responses.  Yes, please 
do post a v2.

Regards,

Anthony Liguori