[PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()"

[PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()"

[Andrew Cooper]

c/s e316316 "xl: Rewrite trim()" uses the wrong indirection of
'output', causing memory corruption for all callers.

Introduce a new local variable, making the code more obviously
correct.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 37d4af6..0cc9f8b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -663,10 +663,12 @@ static void trim(char_predicate_t predicate, const char *input, char **output)
         ;
 
     size_t len_nonnull = after - first;
+    char *result = xmalloc(len_nonnull + 1);
 
-    *output = xmalloc(len_nonnull + 1);
-    memcpy(output, first, len_nonnull);
-    output[len_nonnull] = 0;
+    memcpy(result, first, len_nonnull);
+    result[len_nonnull] = 0;
+
+    *output = result;
 }
 
 static int split_string_into_pair(const char *str,
-- 
1.7.10.4

Re: [PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()"

[Wei Liu]

On Thu, Jul 16, 2015 at 08:18:31PM +0100, Andrew Cooper wrote:
> c/s e316316 "xl: Rewrite trim()" uses the wrong indirection of
> 'output', causing memory corruption for all callers.
> 
> Introduce a new local variable, making the code more obviously
> correct.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

One nit below.

> ---
>  tools/libxl/xl_cmdimpl.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 37d4af6..0cc9f8b 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -663,10 +663,12 @@ static void trim(char_predicate_t predicate, const char *input, char **output)
>          ;
>  
>      size_t len_nonnull = after - first;
> +    char *result = xmalloc(len_nonnull + 1);
>  

Can you move the declaration of result to the beginning of this
function?

> -    *output = xmalloc(len_nonnull + 1);
> -    memcpy(output, first, len_nonnull);
> -    output[len_nonnull] = 0;
> +    memcpy(result, first, len_nonnull);
> +    result[len_nonnull] = 0;
> +
> +    *output = result;
>  }
>  
>  static int split_string_into_pair(const char *str,
> -- 
> 1.7.10.4

Re: [PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()"

[Andrew Cooper]

On 16/07/2015 21:47, Wei Liu wrote:
> On Thu, Jul 16, 2015 at 08:18:31PM +0100, Andrew Cooper wrote:
>> c/s e316316 "xl: Rewrite trim()" uses the wrong indirection of
>> 'output', causing memory corruption for all callers.
>>
>> Introduce a new local variable, making the code more obviously
>> correct.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
>
> One nit below.
>
>> ---
>>  tools/libxl/xl_cmdimpl.c |    8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 37d4af6..0cc9f8b 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -663,10 +663,12 @@ static void trim(char_predicate_t predicate, const char *input, char **output)
>>          ;
>>  
>>      size_t len_nonnull = after - first;
>> +    char *result = xmalloc(len_nonnull + 1);
>>  
> Can you move the declaration of result to the beginning of this
> function?

I can, but why in particular?  It is adjacent to the declaration of
len_nonnull.

~Andrew

Re: [PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()"

[Wei Liu]

On Thu, Jul 16, 2015 at 10:53:02PM +0100, Andrew Cooper wrote:
> On 16/07/2015 21:47, Wei Liu wrote:
> > On Thu, Jul 16, 2015 at 08:18:31PM +0100, Andrew Cooper wrote:
> >> c/s e316316 "xl: Rewrite trim()" uses the wrong indirection of
> >> 'output', causing memory corruption for all callers.
> >>
> >> Introduce a new local variable, making the code more obviously
> >> correct.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> CC: Ian Campbell <Ian.Campbell@citrix.com>
> >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> >> CC: Wei Liu <wei.liu2@citrix.com>
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> >
> > One nit below.
> >
> >> ---
> >>  tools/libxl/xl_cmdimpl.c |    8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >> index 37d4af6..0cc9f8b 100644
> >> --- a/tools/libxl/xl_cmdimpl.c
> >> +++ b/tools/libxl/xl_cmdimpl.c
> >> @@ -663,10 +663,12 @@ static void trim(char_predicate_t predicate, const char *input, char **output)
> >>          ;
> >>  
> >>      size_t len_nonnull = after - first;
> >> +    char *result = xmalloc(len_nonnull + 1);
> >>  
> > Can you move the declaration of result to the beginning of this
> > function?
> 
> I can, but why in particular?  It is adjacent to the declaration of
> len_nonnull.
> 

C90 forbids this. Not sure how much we care about that and I'm probably
bicksheding too much.

Wei.

> ~Andrew

Re: [PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()"

[Ian Jackson]

Andrew Cooper writes ("[PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()""):
> c/s e316316 "xl: Rewrite trim()" uses the wrong indirection of
> 'output', causing memory corruption for all callers.
> 
> Introduce a new local variable, making the code more obviously
> correct.

Gah, thanks.

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

Ian.

Re: [PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()"

[Ian Jackson]

Wei Liu writes ("Re: [PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()""):
> C90 forbids this. Not sure how much we care about that and I'm probably
> bicksheding too much.

libxl/Makefile has -Wno-declaration-after-statement

Ian.

Re: [PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()"

[Ian Campbell]

On Thu, 2015-07-16 at 23:30 +0100, Wei Liu wrote:
> On Thu, Jul 16, 2015 at 10:53:02PM +0100, Andrew Cooper wrote:
> > On 16/07/2015 21:47, Wei Liu wrote:
> > > On Thu, Jul 16, 2015 at 08:18:31PM +0100, Andrew Cooper wrote:
> > >> c/s e316316 "xl: Rewrite trim()" uses the wrong indirection of
> > >> 'output', causing memory corruption for all callers.
> > >>
> > >> Introduce a new local variable, making the code more obviously
> > >> correct.
> > >>
> > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > >> CC: Ian Campbell <Ian.Campbell@citrix.com>
> > >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > >> CC: Wei Liu <wei.liu2@citrix.com>
> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> > >
> > > One nit below.
> > >
> > >> ---
> > >>  tools/libxl/xl_cmdimpl.c |    8 +++++---
> > >>  1 file changed, 5 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > >> index 37d4af6..0cc9f8b 100644
> > >> --- a/tools/libxl/xl_cmdimpl.c
> > >> +++ b/tools/libxl/xl_cmdimpl.c
> > >> @@ -663,10 +663,12 @@ static void trim(char_predicate_t predicate, const char *input, char **output)
> > >>          ;
> > >>  
> > >>      size_t len_nonnull = after - first;
> > >> +    char *result = xmalloc(len_nonnull + 1);
> > >>  
> > > Can you move the declaration of result to the beginning of this
> > > function?
> > 
> > I can, but why in particular?  It is adjacent to the declaration of
> > len_nonnull.
> > 
> 
> C90 forbids this. Not sure how much we care about that and I'm probably
> bicksheding too much.

We made an explicit exception for this in libxl, via the use of
-Wno-declaration-after-statement.

IIRC because some of the GC macros require it, but it has since become
allowed more generally (although not documented it seems).

In this particular case having result declared right after len_nonnull
makes sense.

I was about to Ack + apply but I see Ian beat me to it.

Ian.