[v2,4/6] block: move string allocation from stack to the heap

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(-)

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 :)