[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: |
MORITA Kazutaka |
Subject: |
Re: [Qemu-devel] [PATCH 02/22] sheepdog: avoid a few buffer overruns |
Date: |
Thu, 10 May 2012 02:29:00 +0900 |
User-agent: |
Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (Goj$(D+W(B) APEL/10.8 Emacs/23.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) |
At Wed, 09 May 2012 11:46:10 +0200,
Kevin Wolf wrote:
>
> 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.
Acked-by: MORITA Kazutaka <address@hidden>
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 22/22] doc: update HACKING wrt strncpy/pstrcpy, (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
- [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