[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 02/22] sheepdog: avoid a few buffer overruns
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 02/22] sheepdog: avoid a few buffer overruns |
Date: |
Wed, 09 May 2012 11:46:10 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1 |
Am 09.05.2012 11:23, schrieb Jim Meyering:
> From: Jim Meyering <address@hidden>
>
> * 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 <address@hidden>
Acked-by: Kevin Wolf <address@hidden>
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 18/22] acpi: remove strzcpy (strncpy-identical) function; just use strncpy, (continued)
- [Qemu-devel] [PATCH 22/22] doc: update HACKING wrt strncpy/pstrcpy, Jim Meyering, 2012/05/09
- [Qemu-devel] [PATCH 11/22] virtio-9p: avoid unwarranted uses of strncpy, Jim Meyering, 2012/05/09
- [Qemu-devel] [PATCH 17/22] libcacard/vcard_emul_nss: use pstrcpy in place of strncpy, Jim Meyering, 2012/05/09
- [Qemu-devel] [PATCH 16/22] qemu-ga: prefer pstrcpy: consistently NUL-terminate ifreq.ifr_name, Jim Meyering, 2012/05/09
- [Qemu-devel] [PATCH 15/22] target-i386: use pstrcpy, not strncpy, Jim Meyering, 2012/05/09
- [Qemu-devel] [PATCH 07/22] ppc: avoid buffer overrun: use pstrcpy, not strncpy, Jim Meyering, 2012/05/09
- [Qemu-devel] [PATCH 02/22] sheepdog: avoid a few buffer overruns, Jim Meyering, 2012/05/09
- Re: [Qemu-devel] [PATCH 02/22] sheepdog: avoid a few buffer overruns,
Kevin Wolf <=
- [Qemu-devel] [PATCH 14/22] vscsi: avoid unwarranted strncpy, Jim Meyering, 2012/05/09
- [Qemu-devel] [PATCH 20/22] hw/r2d: add comment: this strncpy use is ok, Jim Meyering, 2012/05/09
- [Qemu-devel] [PATCH 04/22] hw/9pfs: avoid buffer overrun, Jim Meyering, 2012/05/09
- [Qemu-devel] [PATCH 08/22] linux-user: remove two unchecked uses of strdup, Jim Meyering, 2012/05/09