From patchwork Thu Oct 4 11:09:47 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jim Meyering X-Patchwork-Id: 189310 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 658102C03F1 for ; Fri, 5 Oct 2012 06:35:07 +1000 (EST) Received: from localhost ([::1]:49722 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJpdg-0006FS-Bl for incoming@patchwork.ozlabs.org; Thu, 04 Oct 2012 13:55:12 -0400 Received: from eggs.gnu.org ([208.118.235.92]:59318) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJjKd-0002K2-MI for qemu-devel@nongnu.org; Thu, 04 Oct 2012 07:11:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TJjKX-0003HR-FS for qemu-devel@nongnu.org; Thu, 04 Oct 2012 07:11:07 -0400 Received: from mx.meyering.net ([88.168.87.75]:57004 helo=hx.meyering.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJjKX-0003HE-1N for qemu-devel@nongnu.org; Thu, 04 Oct 2012 07:11:01 -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 q94BAxNx015758; Thu, 4 Oct 2012 13:10:59 +0200 Received: (from meyering@localhost) by hx.meyering.net (8.14.5/8.14.5/Submit) id q94BAwrq015757; Thu, 4 Oct 2012 13:10:58 +0200 From: Jim Meyering To: qemu-devel@nongnu.org Date: Thu, 4 Oct 2012 13:09:47 +0200 Message-Id: <1349349003-15672-5-git-send-email-jim@meyering.net> X-Mailer: git-send-email 1.8.0.rc0.18.gf84667d In-Reply-To: <1349349003-15672-1-git-send-email-jim@meyering.net> References: <1349349003-15672-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 X-Mailman-Approved-At: Thu, 04 Oct 2012 13:54:20 -0400 Cc: Jim Meyering Subject: [Qemu-devel] [PATCHv3 04/20] 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 4742f8a..f35ff5b 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -866,14 +866,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 */ @@ -900,7 +900,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); @@ -1149,7 +1152,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; @@ -1177,8 +1180,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; @@ -1752,6 +1758,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); @@ -1811,13 +1820,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); @@ -1946,8 +1955,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++; } }