From patchwork Wed May 30 07:46:23 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jim Meyering X-Patchwork-Id: 161883 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 3C2EFB7037 for ; Wed, 30 May 2012 18:14:13 +1000 (EST) Received: from localhost ([::1]:58573 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SZddV-0001cs-4J for incoming@patchwork.ozlabs.org; Wed, 30 May 2012 03:48:05 -0400 Received: from eggs.gnu.org ([208.118.235.92]:47463) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SZdct-0008Od-PU for qemu-devel@nongnu.org; Wed, 30 May 2012 03:47:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SZdcn-0001eg-4n for qemu-devel@nongnu.org; Wed, 30 May 2012 03:47:27 -0400 Received: from mx.meyering.net ([88.168.87.75]:49296 helo=hx.meyering.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SZdcm-0001eS-O8 for qemu-devel@nongnu.org; Wed, 30 May 2012 03:47:21 -0400 Received: from hx.meyering.net (hx.meyering.net [127.0.0.1]) by hx.meyering.net (8.14.5/8.14.5) with ESMTP id q4U7lIYu013971; Wed, 30 May 2012 09:47:18 +0200 Received: (from meyering@localhost) by hx.meyering.net (8.14.5/8.14.5/Submit) id q4U7lH4l013970; Wed, 30 May 2012 09:47:17 +0200 From: Jim Meyering To: qemu-devel@nongnu.org Date: Wed, 30 May 2012 09:46:23 +0200 Message-Id: <1338364001-13892-5-git-send-email-jim@meyering.net> X-Mailer: git-send-email 1.7.10.2.605.gbefc5ed In-Reply-To: <1338364001-13892-1-git-send-email-jim@meyering.net> References: <1338364001-13892-1-git-send-email-jim@meyering.net> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 88.168.87.75 Cc: Jim Meyering Subject: [Qemu-devel] [PATCHv2 04/22] sheepdog: avoid a few buffer overruns X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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. Acked-by: Kevin Wolf Acked-by: MORITA Kazutaka Signed-off-by: Jim Meyering --- block/sheepdog.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 6d52277..6f405b5 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -857,14 +857,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 */ @@ -891,7 +891,10 @@ static int find_vdi_name(BDRVSheepdogState *s, char *filename, uint32_t snapid, return fd; } - memset(buf, 0, sizeof(buf)); + /* This pair of strncpy calls ensures that the buffer is zero-filled, + * which is desirable since we'll soon be sending those bytes, and + * don't want the send_req to read uninitialized data. + */ strncpy(buf, filename, SD_MAX_VDI_LEN); strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN); @@ -1141,7 +1144,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; @@ -1169,8 +1172,11 @@ static int do_sd_create(char *filename, int64_t vdi_size, return fd; } + /* FIXME: would it be better to fail (e.g., return -EIO) when filename + * does not fit in buf? For now, just truncate and avoid buffer overrun. + */ memset(buf, 0, sizeof(buf)); - strncpy(buf, filename, SD_MAX_VDI_LEN); + pstrcpy(buf, sizeof(buf), filename); memset(&hdr, 0, sizeof(hdr)); hdr.opcode = SD_OP_NEW_VDI; @@ -1740,6 +1746,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); @@ -1799,13 +1808,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); @@ -1934,8 +1943,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++; } }