diff mbox

vl.c: fix memleaks with g_strdup+strtok

Message ID 921079541.7300112.1456305734547.JavaMail.zimbra@ocs.online.net
State New
Headers show

Commit Message

Quentin PEREZ Feb. 24, 2016, 9:22 a.m. UTC 
From cfd35648b6ab681891fe90b2813e63a4fdbe365f Mon Sep 17 00:00:00 2001
From: Quentin Perez <qperez@ocs.online.net>
Date: Wed, 24 Feb 2016 10:10:56 +0100
Subject: [PATCH] vl.c: fix memleaks with g_strdup+strtok

Signed-off-by: Quentin Perez <qperez@ocs.online.net>
---
 vl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--
2.5.4 (Apple Git-61)

Comments

Stefan Hajnoczi March 2, 2016, 11:07 a.m. UTC | #1 
On Wed, Feb 24, 2016 at 10:22:14AM +0100, Quentin PEREZ wrote:
> diff --git a/vl.c b/vl.c
> index b87e292..9f6593a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1362,16 +1362,19 @@ static int add_semihosting_arg(void *opaque,
>  static inline void semihosting_arg_fallback(const char *file, const char *cmd)
>  {
>      char *cmd_token;
> +    char *dup_cmd;
> 
>      /* argv[0] */
>      add_semihosting_arg(&semihosting, "arg", file, NULL);
> 
>      /* split -append and initialize argv[1..n] */
> -    cmd_token = strtok(g_strdup(cmd), " ");
> +    dup_cmd = g_strdup(cmd);
> +    cmd_token = strtok(dup_cmd, " ");
>      while (cmd_token) {
>          add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
>          cmd_token = strtok(NULL, " ");
>      }
> +    g_free(dup_cmd);

add_semihosting_arg() stashes the cmd_token pointer.  semihosting.argv[]
points to freed memory if you add g_free(dup_cmd).

I suggest leaving the code as-is since the lifetime of the semihosting
global variable spans the entire run-time of the QEMU process.  It's not
pretty but the leak is harmless.

If you really want to fix it you may need to add a semihosting_cleanup()
function to free strings.
Daniel P. Berrangé March 2, 2016, 11:13 a.m. UTC | #2 
On Wed, Mar 02, 2016 at 11:07:02AM +0000, Stefan Hajnoczi wrote:
> On Wed, Feb 24, 2016 at 10:22:14AM +0100, Quentin PEREZ wrote:
> > diff --git a/vl.c b/vl.c
> > index b87e292..9f6593a 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1362,16 +1362,19 @@ static int add_semihosting_arg(void *opaque,
> >  static inline void semihosting_arg_fallback(const char *file, const char *cmd)
> >  {
> >      char *cmd_token;
> > +    char *dup_cmd;
> > 
> >      /* argv[0] */
> >      add_semihosting_arg(&semihosting, "arg", file, NULL);
> > 
> >      /* split -append and initialize argv[1..n] */
> > -    cmd_token = strtok(g_strdup(cmd), " ");
> > +    dup_cmd = g_strdup(cmd);
> > +    cmd_token = strtok(dup_cmd, " ");
> >      while (cmd_token) {
> >          add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
> >          cmd_token = strtok(NULL, " ");
> >      }
> > +    g_free(dup_cmd);
> 
> add_semihosting_arg() stashes the cmd_token pointer.  semihosting.argv[]
> points to freed memory if you add g_free(dup_cmd).
> 
> I suggest leaving the code as-is since the lifetime of the semihosting
> global variable spans the entire run-time of the QEMU process.  It's not
> pretty but the leak is harmless.
> 
> If you really want to fix it you may need to add a semihosting_cleanup()
> function to free strings.

If fixing it I'd probably suggest using g_strsplit() instead of
strdup+strtok too, as it accepts a const string & returns allocated
strings which is a nicer contract that strtok which modifies its arg

Regards,
Daniel
diff mbox

Patch

diff --git a/vl.c b/vl.c
index b87e292..9f6593a 100644
--- a/vl.c
+++ b/vl.c
@@ -1362,16 +1362,19 @@  static int add_semihosting_arg(void *opaque,
 static inline void semihosting_arg_fallback(const char *file, const char *cmd)
 {
     char *cmd_token;
+    char *dup_cmd;

     /* argv[0] */
     add_semihosting_arg(&semihosting, "arg", file, NULL);

     /* split -append and initialize argv[1..n] */
-    cmd_token = strtok(g_strdup(cmd), " ");
+    dup_cmd = g_strdup(cmd);
+    cmd_token = strtok(dup_cmd, " ");
     while (cmd_token) {
         add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
         cmd_token = strtok(NULL, " ");
     }
+    g_free(dup_cmd);
 }

 /* Now we still need this for compatibility with XEN. */