| Message ID | 1456998223-12356-3-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
On 03/03/2016 10:43, Gonglei wrote: > CID 1352418 (#1 of 1): Out-of-bounds access (INCOMPATIBLE_CAST) > incompatible_cast: Pointer &snap_id points to an object whose effective > type is unsigned int (32 bits, unsigned) but is dereferenced as a wider > unsigned long (64 bits, unsigned). This may lead to memory corruption. > > We also need to free local_err when ret is not equals to 0. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > block/sheepdog.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/block/sheepdog.c b/block/sheepdog.c > index 8739acc..3d81bba 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -2543,7 +2543,7 @@ static int sd_snapshot_delete(BlockDriverState *bs, > const char *name, > Error **errp) > { > - uint32_t snap_id = 0; > + unsigned long snap_id = 0; > char snap_tag[SD_MAX_VDI_TAG_LEN]; > Error *local_err = NULL; > int fd, ret; > @@ -2565,20 +2565,23 @@ static int sd_snapshot_delete(BlockDriverState *bs, > memset(buf, 0, sizeof(buf)); > memset(snap_tag, 0, sizeof(snap_tag)); > pstrcpy(buf, SD_MAX_VDI_LEN, s->name); > - if (qemu_strtoul(snapshot_id, NULL, 10, (unsigned long *)&snap_id)) { > + if (qemu_strtoul(snapshot_id, NULL, 10, &snap_id)) { > return -1; > } > > if (snap_id) { > + assert(snap_id <= UINT_MAX); > + > hdr.snapid = snap_id; > } else { > pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id); > pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag); > } > > - ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true, > + ret = find_vdi_name(s, s->name, hdr.snapid, snap_tag, &vid, true, > &local_err); > if (ret) { > + error_report_err(local_err); > return ret; > } > > A patch for this has been posted yesterday by Jeff Cody. Paolo
Regards, -Gonglei > -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Thursday, March 03, 2016 7:18 PM > To: Gonglei (Arei); qemu-devel@nongnu.org > Cc: qemu-trivial@nongnu.org > Subject: Re: [PATCH 2/6] sheepdog: fix possible resouce leak and out-of-bounds > access > > > > On 03/03/2016 10:43, Gonglei wrote: > > CID 1352418 (#1 of 1): Out-of-bounds access (INCOMPATIBLE_CAST) > > incompatible_cast: Pointer &snap_id points to an object whose effective > > type is unsigned int (32 bits, unsigned) but is dereferenced as a wider > > unsigned long (64 bits, unsigned). This may lead to memory corruption. > > > > We also need to free local_err when ret is not equals to 0. > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > --- > > block/sheepdog.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/block/sheepdog.c b/block/sheepdog.c > > index 8739acc..3d81bba 100644 > > --- a/block/sheepdog.c > > +++ b/block/sheepdog.c > > @@ -2543,7 +2543,7 @@ static int sd_snapshot_delete(BlockDriverState > *bs, > > const char *name, > > Error **errp) > > { > > - uint32_t snap_id = 0; > > + unsigned long snap_id = 0; > > char snap_tag[SD_MAX_VDI_TAG_LEN]; > > Error *local_err = NULL; > > int fd, ret; > > @@ -2565,20 +2565,23 @@ static int sd_snapshot_delete(BlockDriverState > *bs, > > memset(buf, 0, sizeof(buf)); > > memset(snap_tag, 0, sizeof(snap_tag)); > > pstrcpy(buf, SD_MAX_VDI_LEN, s->name); > > - if (qemu_strtoul(snapshot_id, NULL, 10, (unsigned long *)&snap_id)) { > > + if (qemu_strtoul(snapshot_id, NULL, 10, &snap_id)) { > > return -1; > > } > > > > if (snap_id) { > > + assert(snap_id <= UINT_MAX); > > + > > hdr.snapid = snap_id; > > } else { > > pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id); > > pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, > snap_tag); > > } > > > > - ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true, > > + ret = find_vdi_name(s, s->name, hdr.snapid, snap_tag, &vid, true, > > &local_err); > > if (ret) { > > + error_report_err(local_err); > > return ret; > > } > > > > > > A patch for this has been posted yesterday by Jeff Cody. > OK, I found it. And Max's comments is right, Jef can use hdr.snapid instead of snap_tag to invoke find_vdi_name(). But, except that fix, My patch also fixed a memory leak, did you see that? Do I need post an separate patch to fix memory leak? Regards, -Gonglei
On 03/03/2016 13:00, Gonglei (Arei) wrote: >>> > > >>> > > - ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true, >>> > > + ret = find_vdi_name(s, s->name, hdr.snapid, snap_tag, &vid, true, >>> > > &local_err); >>> > > if (ret) { >>> > > + error_report_err(local_err); >>> > > return ret; >>> > > } >>> > > >>> > > >> > >> > A patch for this has been posted yesterday by Jeff Cody. >> > > OK, I found it. And Max's comments is right, Jef can use hdr.snapid instead of snap_tag > to invoke find_vdi_name(). > > But, except that fix, My patch also fixed a memory leak, did you see that? No, I didn't notice -- it's not clear that error_report_err also frees the error. > Do I need post an separate patch to fix memory leak? Yes, but the right fix in my opinion is to pass errp to find_vdi_name instead. Paolo
Regards, -Gonglei > -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Thursday, March 03, 2016 8:12 PM > To: Gonglei (Arei); qemu-devel@nongnu.org > Cc: qemu-trivial@nongnu.org > Subject: Re: [PATCH 2/6] sheepdog: fix possible resouce leak and out-of-bounds > access > > > > On 03/03/2016 13:00, Gonglei (Arei) wrote: > >>> > > > >>> > > - ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true, > >>> > > + ret = find_vdi_name(s, s->name, hdr.snapid, snap_tag, &vid, > true, > >>> > > &local_err); > >>> > > if (ret) { > >>> > > + error_report_err(local_err); > >>> > > return ret; > >>> > > } > >>> > > > >>> > > > >> > > >> > A patch for this has been posted yesterday by Jeff Cody. > >> > > > OK, I found it. And Max's comments is right, Jef can use hdr.snapid instead of > snap_tag > > to invoke find_vdi_name(). > > > > But, except that fix, My patch also fixed a memory leak, did you see that? > > No, I didn't notice -- it's not clear that error_report_err also frees > the error. > > > Do I need post an separate patch to fix memory leak? > > Yes, but the right fix in my opinion is to pass errp to find_vdi_name > instead. > You are right, we'd better drop local_err in sd_snapshot_delete(). Regards, -Gonglei
diff --git a/block/sheepdog.c b/block/sheepdog.c index 8739acc..3d81bba 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -2543,7 +2543,7 @@ static int sd_snapshot_delete(BlockDriverState *bs, const char *name, Error **errp) { - uint32_t snap_id = 0; + unsigned long snap_id = 0; char snap_tag[SD_MAX_VDI_TAG_LEN]; Error *local_err = NULL; int fd, ret; @@ -2565,20 +2565,23 @@ static int sd_snapshot_delete(BlockDriverState *bs, memset(buf, 0, sizeof(buf)); memset(snap_tag, 0, sizeof(snap_tag)); pstrcpy(buf, SD_MAX_VDI_LEN, s->name); - if (qemu_strtoul(snapshot_id, NULL, 10, (unsigned long *)&snap_id)) { + if (qemu_strtoul(snapshot_id, NULL, 10, &snap_id)) { return -1; } if (snap_id) { + assert(snap_id <= UINT_MAX); + hdr.snapid = snap_id; } else { pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id); pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag); } - ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true, + ret = find_vdi_name(s, s->name, hdr.snapid, snap_tag, &vid, true, &local_err); if (ret) { + error_report_err(local_err); return ret; }
CID 1352418 (#1 of 1): Out-of-bounds access (INCOMPATIBLE_CAST) incompatible_cast: Pointer &snap_id points to an object whose effective type is unsigned int (32 bits, unsigned) but is dereferenced as a wider unsigned long (64 bits, unsigned). This may lead to memory corruption. We also need to free local_err when ret is not equals to 0. Signed-off-by: Gonglei <arei.gonglei@huawei.com> --- block/sheepdog.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)