From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] tools/xl: Fix trim() following c/s e316316 "xl: Rewrite trim()" Date: Fri, 17 Jul 2015 11:18:46 +0100 Message-ID: <1437128326.32371.285.camel@citrix.com> References: <1437074311-25446-1-git-send-email-andrew.cooper3@citrix.com> <20150716204749.GD29094@zion.uk.xensource.com> <55A827BE.2030908@citrix.com> <20150716223032.GA1707@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150716223032.GA1707@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: Andrew Cooper , Ian Jackson , Xen-devel List-Id: xen-devel@lists.xenproject.org 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 > > >> CC: Ian Campbell > > >> CC: Ian Jackson > > >> CC: Wei Liu > > > Acked-by: Wei Liu > > > > > > 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.