From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38809) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SS3TV-0002cN-Be for qemu-devel@nongnu.org; Wed, 09 May 2012 05:46:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SS3TO-0003iE-P9 for qemu-devel@nongnu.org; Wed, 09 May 2012 05:46:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1121) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SS3TO-0003hp-HX for qemu-devel@nongnu.org; Wed, 09 May 2012 05:46:18 -0400 Message-ID: <4FAA3CE2.8000206@redhat.com> Date: Wed, 09 May 2012 11:46:10 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1336555446-20180-1-git-send-email-jim@meyering.net> <1336555446-20180-3-git-send-email-jim@meyering.net> In-Reply-To: <1336555446-20180-3-git-send-email-jim@meyering.net> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 02/22] sheepdog: avoid a few buffer overruns List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jim Meyering Cc: Jim Meyering , qemu-devel@nongnu.org, MORITA Kazutaka Am 09.05.2012 11:23, schrieb Jim Meyering: > From: Jim Meyering > > * 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 Acked-by: Kevin Wolf 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++; > } > }