Message ID | d8820532be77d7cd2728b9f4d4dcfc73e17af8aa.1421768887.git.jcody@redhat.com |
---|---|
State | New |
Headers | show |
On 01/20/2015 12:31 PM, Jeff Cody wrote: > Rather than allocate PATH_MAX bytes on the stack, use g_strndup() to > dynamically allocate the string, and add an exit label for cleanup. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index cbe4a32..39cd7a6 100644 > --- a/block.c > +++ b/block.c > @@ -2207,7 +2207,7 @@ int bdrv_commit(BlockDriverState *bs) > int n, ro, open_flags; > int ret = 0; > uint8_t *buf = NULL; > - char filename[PATH_MAX]; > + char *filename = NULL; > > if (!drv) > return -ENOMEDIUM; > @@ -2222,13 +2222,14 @@ int bdrv_commit(BlockDriverState *bs) > } > > ro = bs->backing_hd->read_only; > - /* Use pstrcpy (not strncpy): filename must be NUL-terminated. */ > - pstrcpy(filename, sizeof(filename), bs->backing_hd->filename); > + /* filename must be NUL-terminated. */ > + filename = g_strndup(bs->backing_hd->filename, PATH_MAX - 1); > open_flags = bs->backing_hd->open_flags; > > if (ro) { > if (bdrv_reopen(bs->backing_hd, open_flags | BDRV_O_RDWR, NULL)) { > - return -EACCES; > + ret = -EACCES; > + goto exit; > } > } > > @@ -2307,6 +2308,8 @@ ro_cleanup: > bdrv_reopen(bs->backing_hd, open_flags & ~BDRV_O_RDWR, NULL); > } > > +exit: > + g_free(filename); > return ret; > } > > Reviewed-by: John Snow <jsnow@redhat.com>
On Tue, Jan 20, 2015 at 12:31:31PM -0500, Jeff Cody wrote: > Rather than allocate PATH_MAX bytes on the stack, use g_strndup() to > dynamically allocate the string, and add an exit label for cleanup. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) Zombie alert! This is a funny: Since September 2012 in commit 0bce597d6ec34b2af802799eb53ebc863c704d05 ("block: convert bdrv_commit() to use bdrv_reopen()") the filename variable has not been used. We continued to maintain this variable faithfully, for example, to fix a buffer overflow in commit c2cba3d9314f972dfaf724d0ec2d018eb54c95f1 ("block: avoid buffer overrun by using pstrcpy, not strncpy"). Please kill the zombie filename variable instead of switching to heap allocation :-).
On Thu, Jan 22, 2015 at 11:37:54AM +0000, Stefan Hajnoczi wrote: > On Tue, Jan 20, 2015 at 12:31:31PM -0500, Jeff Cody wrote: > > Rather than allocate PATH_MAX bytes on the stack, use g_strndup() to > > dynamically allocate the string, and add an exit label for cleanup. > > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > block.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > Zombie alert! > > This is a funny: > > Since September 2012 in commit 0bce597d6ec34b2af802799eb53ebc863c704d05 > ("block: convert bdrv_commit() to use bdrv_reopen()") the filename > variable has not been used. > > We continued to maintain this variable faithfully, for example, to fix a > buffer overflow in commit c2cba3d9314f972dfaf724d0ec2d018eb54c95f1 > ("block: avoid buffer overrun by using pstrcpy, not strncpy"). > > Please kill the zombie filename variable instead of switching to heap > allocation :-). Ha! OK, will kill the zombie :)
diff --git a/block.c b/block.c index cbe4a32..39cd7a6 100644 --- a/block.c +++ b/block.c @@ -2207,7 +2207,7 @@ int bdrv_commit(BlockDriverState *bs) int n, ro, open_flags; int ret = 0; uint8_t *buf = NULL; - char filename[PATH_MAX]; + char *filename = NULL; if (!drv) return -ENOMEDIUM; @@ -2222,13 +2222,14 @@ int bdrv_commit(BlockDriverState *bs) } ro = bs->backing_hd->read_only; - /* Use pstrcpy (not strncpy): filename must be NUL-terminated. */ - pstrcpy(filename, sizeof(filename), bs->backing_hd->filename); + /* filename must be NUL-terminated. */ + filename = g_strndup(bs->backing_hd->filename, PATH_MAX - 1); open_flags = bs->backing_hd->open_flags; if (ro) { if (bdrv_reopen(bs->backing_hd, open_flags | BDRV_O_RDWR, NULL)) { - return -EACCES; + ret = -EACCES; + goto exit; } } @@ -2307,6 +2308,8 @@ ro_cleanup: bdrv_reopen(bs->backing_hd, open_flags & ~BDRV_O_RDWR, NULL); } +exit: + g_free(filename); return ret; }
Rather than allocate PATH_MAX bytes on the stack, use g_strndup() to dynamically allocate the string, and add an exit label for cleanup. Signed-off-by: Jeff Cody <jcody@redhat.com> --- block.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)