[PATCH 1/3] xl: free config_data on error in domain_create

[PATCH 1/3] xl: free config_data on error in domain_create

[Ian Campbell]

CID: 1055898

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index d5a397a..e819ee6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2795,6 +2795,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
         if (!restoring && extra_config && strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
                 fprintf(stderr, "Failed to attach extra configuration\n");
+                free(config_data);
                 return ERROR_FAIL;
             }
             /* allocate space for the extra config plus two EOLs plus \0 */
@@ -6525,6 +6526,7 @@ int main_networkattach(int argc, char **argv)
     }
 
     xlu_cfg_destroy(config);
+    config = NULL;
 
     if (argc > 0) return 1; /* Parse error above */
 
-- 
2.1.4

[PATCH 2/3] xl: use xrealloc in domain create

[Ian Campbell]

Using bare realloc risks leaking the old pointer if the realloc fails.

Since xrealloc exits on such failures, drop the error handling.

Noticed while fixing, but not related to, CID 1055898.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index e819ee6..7ba40c0 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2799,12 +2799,8 @@ static uint32_t create_domain(struct domain_create *dom_info)
                 return ERROR_FAIL;
             }
             /* allocate space for the extra config plus two EOLs plus \0 */
-            config_data = realloc(config_data, config_len
+            config_data = xrealloc(config_data, config_len
                 + strlen(extra_config) + 2 + 1);
-            if (!config_data) {
-                fprintf(stderr, "Failed to realloc config_data\n");
-                return ERROR_FAIL;
-            }
             config_len += sprintf(config_data + config_len, "\n%s\n",
                 extra_config);
         }
-- 
2.1.4

[PATCH 3/3] xl: create: close restore_fd_to_close on error

[Ian Campbell]

Currently the fd is opened and then later closed and
restore_fd_to_close set back to -1, however there are several goto out
and goto error_out paths in the interim.

Since the code resets restore_fd_to_close to -1 it is OK to check this
and close on the out path too.

CID: 1055897

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 7ba40c0..514f5a8 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3069,6 +3069,13 @@ error_out:
     }
 
 out:
+    if (restore_fd_to_close >= 0) {
+        if (close(restore_fd_to_close))
+            fprintf(stderr, "Failed to close restoring file, fd %d, errno %d\n",
+                    restore_fd_to_close, errno);
+        restore_fd_to_close = -1;
+    }
+
     if (logfile != 2)
         close(logfile);
 
-- 
2.1.4

Re: [PATCH 1/3] xl: free config_data on error in domain_create

[Ian Jackson]

Ian Campbell writes ("[PATCH 1/3] xl: free config_data on error in domain_create"):
> CID: 1055898
...
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index d5a397a..e819ee6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2795,6 +2795,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
>          if (!restoring && extra_config && strlen(extra_config)) {
>              if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
>                  fprintf(stderr, "Failed to attach extra configuration\n");
> +                free(config_data);
>                  return ERROR_FAIL;

This LGTM.

>              /* allocate space for the extra config plus two EOLs plus \0 */
> @@ -6525,6 +6526,7 @@ int main_networkattach(int argc, char **argv)
>      }
>  
>      xlu_cfg_destroy(config);
> +    config = NULL;
>  
>      if (argc > 0) return 1; /* Parse error above */

But this doesn't apply and the current code in main_networkattach
doesn't seem to have a bug that something this would fix.

Ian.

Re: [PATCH 2/3] xl: use xrealloc in domain create

[Ian Jackson]

Ian Campbell writes ("[PATCH 2/3] xl: use xrealloc in domain create"):
> Using bare realloc risks leaking the old pointer if the realloc fails.
> 
> Since xrealloc exits on such failures, drop the error handling.
> 
> Noticed while fixing, but not related to, CID 1055898.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

And queued for push.

Ian.

Re: [PATCH 3/3] xl: create: close restore_fd_to_close on error

[Ian Jackson]

Ian Campbell writes ("[PATCH 3/3] xl: create: close restore_fd_to_close on error"):
> Currently the fd is opened and then later closed and
> restore_fd_to_close set back to -1, however there are several goto out
> and goto error_out paths in the interim.
> 
> Since the code resets restore_fd_to_close to -1 it is OK to check this
> and close on the out path too.
> 
> CID: 1055897

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

And queued for push.

Ian.

Re: [PATCH 1/3] xl: free config_data on error in domain_create

[Ian Campbell]

On Fri, 2016-02-19 at 16:02 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH 1/3] xl: free config_data on error in
> domain_create"):
> > CID: 1055898
> ...
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index d5a397a..e819ee6 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -2795,6 +2795,7 @@ static uint32_t create_domain(struct
> > domain_create *dom_info)
> >          if (!restoring && extra_config && strlen(extra_config)) {
> >              if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1))
> > {
> >                  fprintf(stderr, "Failed to attach extra
> > configuration\n");
> > +                free(config_data);
> >                  return ERROR_FAIL;
> 
> This LGTM.
> 
> >              /* allocate space for the extra config plus two EOLs plus
> > \0 */
> > @@ -6525,6 +6526,7 @@ int main_networkattach(int argc, char **argv)
> >      }
> >  
> >      xlu_cfg_destroy(config);
> > +    config = NULL;
> >  
> >      if (argc > 0) return 1; /* Parse error above */
> 
> But this doesn't apply and the current code in main_networkattach
> doesn't seem to have a bug that something this would fix.

Looks like a stray line which should have been in "xl: network-attach: free
config object after use", sorry.

I'll wait for you to push the batch you have in hand and then rebase fixing
this as I go.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/3] xl: free config_data on error in domain_create

[Ian Campbell]

On Fri, 2016-02-19 at 16:08 +0000, Ian Campbell wrote:
>              /* allocate space for the extra config plus two EOLs
> > > plus
> > > \0 */
> > > @@ -6525,6 +6526,7 @@ int main_networkattach(int argc, char **argv)
> > >      }
> > >  
> > >      xlu_cfg_destroy(config);
> > > +    config = NULL;
> > >  
> > >      if (argc > 0) return 1; /* Parse error above */
> > 
> > But this doesn't apply and the current code in main_networkattach
> > doesn't seem to have a bug that something this would fix.
> 
> Looks like a stray line which should have been in "xl: network-attach: free
> config object after use", sorry.

Which I see I somehow did send out!

> I'll wait for you to push the batch you have in hand and then rebase fixing
> this as I go.

I'll still take this approach.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel