Xen devel - Nov 2011 - [PATCH] tools: misc: xen-tmem-list-parse: fix output ugliness

(This should apply cleanly to 4.0, 4.1, and unstable.  It would
be nice to apply to the next dot release of 4.0 and 4.1, but
please definitely apply at least to unstable.)

Fix ugly parse output for xen-tmem-list-parse

This program parses the output of xm/xl tmem-list into
human-readable format.  A missing NULL terminator sometimes
causes garbage to be spewed where the two-letter pool type
should be printed.

Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>

diff -r 54a5e994a241 tools/misc/xen-tmem-list-parse.c
--- a/tools/misc/xen-tmem-list-parse.c	Wed Nov 02 17:09:09 2011 +0000
+++ b/tools/misc/xen-tmem-list-parse.c	Wed Nov 09 14:28:40 2011 -0700
@@ -64,6 +64,7 @@
         return;
     for ( i = 0; i < len; i++ )
         *buf++ = *s1++;
+    *buf = ''\0'';
 }
 
 void parse_sharers(char *s, char *match, char *buf, int len)

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

Dan Magenheimer writes ("[Xen-devel] [PATCH] tools: misc:
xen-tmem-list-parse: fix output ugliness"):
> (This should apply cleanly to 4.0, 4.1, and unstable.  It would
> be nice to apply to the next dot release of 4.0 and 4.1, but
> please definitely apply at least to unstable.)
> 
> Fix ugly parse output for xen-tmem-list-parse
> 
> This program parses the output of xm/xl tmem-list into
> human-readable format.  A missing NULL terminator sometimes
> causes garbage to be spewed where the two-letter pool type
> should be printed.
> 
> Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>
> 
> diff -r 54a5e994a241 tools/misc/xen-tmem-list-parse.c
> --- a/tools/misc/xen-tmem-list-parse.c	Wed Nov 02 17:09:09 2011 +0000
> +++ b/tools/misc/xen-tmem-list-parse.c	Wed Nov 09 14:28:40 2011 -0700
> @@ -64,6 +64,7 @@
>          return;
>      for ( i = 0; i < len; i++ )
>          *buf++ = *s1++;
> +    *buf = ''\0'';
>  }
This has a buffer overrun AFAICT.

Ian.

On Wed, 2011-11-09 at 21:40 +0000, Dan Magenheimer
wrote:
> (This should apply cleanly to 4.0, 4.1, and unstable.  It would
> be nice to apply to the next dot release of 4.0 and 4.1, but
> please definitely apply at least to unstable.)
> 
> Fix ugly parse output for xen-tmem-list-parse
> 
> This program parses the output of xm/xl tmem-list into
> human-readable format.
Why does xm/xl not just have a human readable output option (or more
likely, default)?
>   A missing NULL terminator sometimes
> causes garbage to be spewed where the two-letter pool type
> should be printed.
> 
> Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>
> 
> diff -r 54a5e994a241 tools/misc/xen-tmem-list-parse.c
> --- a/tools/misc/xen-tmem-list-parse.c	Wed Nov 02 17:09:09 2011 +0000
> +++ b/tools/misc/xen-tmem-list-parse.c	Wed Nov 09 14:28:40 2011 -0700
> @@ -64,6 +64,7 @@
>          return;
>      for ( i = 0; i < len; i++ )
>          *buf++ = *s1++;
> +    *buf = ''\0'';
>  }
>  
>  void parse_sharers(char *s, char *match, char *buf, int len)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Tuesday, November 22, 2011 10:22 AM
> To: Dan Magenheimer
> Cc: stefano.stabellini@eu.citrix.com; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] tools: misc: xen-tmem-list-parse: fix
output ugliness
> 
> Dan Magenheimer writes ("[Xen-devel] [PATCH] tools: misc:
xen-tmem-list-parse: fix output ugliness"):
> > (This should apply cleanly to 4.0, 4.1, and unstable.  It would
> > be nice to apply to the next dot release of 4.0 and 4.1, but
> > please definitely apply at least to unstable.)
> >
> > Fix ugly parse output for xen-tmem-list-parse
> >
> > This program parses the output of xm/xl tmem-list into
> > human-readable format.  A missing NULL terminator sometimes
> > causes garbage to be spewed where the two-letter pool type
> > should be printed.
> >
> > Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>
> >
> > diff -r 54a5e994a241 tools/misc/xen-tmem-list-parse.c
> > --- a/tools/misc/xen-tmem-list-parse.c	Wed Nov 02 17:09:09 2011 +0000
> > +++ b/tools/misc/xen-tmem-list-parse.c	Wed Nov 09 14:28:40 2011 -0700
> > @@ -64,6 +64,7 @@
> >          return;
> >      for ( i = 0; i < len; i++ )
> >          *buf++ = *s1++;
> > +    *buf = ''\0'';
> >  }
> 
> This has a buffer overrun AFAICT.
> 
> Ian.
No, it doesn''t.  I agree it *could* if parse_string is
used/called differently.  The caller simply needs to
ensure that the declared buffer is at least one larger
than the data to be matched which is true for both
callers.

P.S. Please note that I am still not receiving email
from the xen-devel reflector (and am on vacation this
week so probably won''t be looking into it... my best
guess is that the Oracle spam filter isn''t happy with
the new source of the xen-devel messages, as some
other Oracle folk are having problems too).

> From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> Sent: Tuesday, November 22, 2011 10:34 AM
> To: Dan Magenheimer
> Cc: Ian Jackson; Stefano Stabellini; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] tools: misc: xen-tmem-list-parse: fix
output ugliness
> 
> On Wed, 2011-11-09 at 21:40 +0000, Dan Magenheimer wrote:
> > (This should apply cleanly to 4.0, 4.1, and unstable.  It would
> > be nice to apply to the next dot release of 4.0 and 4.1, but
> > please definitely apply at least to unstable.)
> >
> > Fix ugly parse output for xen-tmem-list-parse
> >
> > This program parses the output of xm/xl tmem-list into
> > human-readable format.
> 
> Why does xm/xl not just have a human readable output option (or more
> likely, default)?
The raw data originates in the hypervisor in an ASCII format
that was designed to be forward/backward compatible because it
was unclear how much it would need to change over time.
The end consumer is a higher-level management tool; xm/xl
simply pass the data through.  xen-tmem-list-parse acts
as "documentation" for the format and is useful for those
experimenting with tmem.  It didn''t seem to make
sense to create an additional dependency for parsing
the format in xm/xl.

Dan

Dan Magenheimer writes ("Re: [Xen-devel] [PATCH] tools: misc:
xen-tmem-list-pars> No, it doesn''t.  I agree it *could* if
parse_string is
> used/called differently.  The caller simply needs to
> ensure that the declared buffer is at least one larger
> than the data to be matched which is true for both
> callers.
Urgh.  So in the previous code the buffer was a byte too big ?

I think functions with prototypes like
     f(int len, char *buf)
should take buf[len], not buf[len+1].
> P.S. Please note that I am still not receiving email
> >from the xen-devel reflector (and am on vacation this
> week so probably won''t be looking into it... my best
> guess is that the Oracle spam filter isn''t happy with
> the new source of the xen-devel messages, as some
> other Oracle folk are having problems too).
Hrmm.  If this is still a problem when you get back please let me know.

ian.

> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Thursday, November 24, 2011 11:42 AM
> To: Dan Magenheimer
> Cc: xen-devel@lists.xensource.com; stefano.stabellini@eu.citrix.com
> Subject: Re: [Xen-devel] [PATCH] tools: misc: xen-tmem-list-parse: fix
output ugliness
> 
> Dan Magenheimer writes ("Re: [Xen-devel] [PATCH] tools: misc:
xen-tmem-list-pars> No, it doesn''t.  I
> agree it *could* if parse_string is
> > used/called differently.  The caller simply needs to
> > ensure that the declared buffer is at least one larger
> > than the data to be matched which is true for both
> > callers.
> 
> Urgh.  So in the previous code the buffer was a byte too big ?
> 
> I think functions with prototypes like
>      f(int len, char *buf)
> should take buf[len], not buf[len+1].
FWIW, I agree completely.  I didn''t write this code from
scratch, I heavily leveraged it from elsewhere (don''t
recall exactly where, maybe xentop?).

If necessary, I will rewrite the callers and API but
I was just trying to fix an annoying bug expeditiously.
Let me know if you won''t accept it as the one-line
ugly fix.
 
> > P.S. Please note that I am still not receiving email
> > >from the xen-devel reflector (and am on vacation this
> > week so probably won''t be looking into it... my best
> > guess is that the Oracle spam filter isn''t happy with
> > the new source of the xen-devel messages, as some
> > other Oracle folk are having problems too).
> 
> Hrmm.  If this is still a problem when you get back please let me know.
Still a problem.  Konrad filed an internal bug report
(Oracle has an Exchange-like mail server that is eat-
your-own-dog-food) but the US holiday weekend, which
many turn into a whole week, means it''s probably not
even getting looked at yet by Oracle''s IT group.

It may be unrelated to the xen.org server change but
the coincidence is suspicious.

Interestingly, I got precisely one xen-devel email in
the last week which is what led me to the spam filter
theory... though I have gotten every xen-changelog email.

Dan

> > > P.S. Please note that I am still not receiving email
> > > >from the xen-devel reflector (and am on vacation this
> > > week so probably won''t be looking into it... my best
> > > guess is that the Oracle spam filter isn''t happy with
> > > the new source of the xen-devel messages, as some
> > > other Oracle folk are having problems too).
> >
> > Hrmm.  If this is still a problem when you get back please let me
know.
> 
> Still a problem.  Konrad filed an internal bug report
> (Oracle has an Exchange-like mail server that is eat-
> your-own-dog-food) but the US holiday weekend, which
> many turn into a whole week, means it''s probably not
> even getting looked at yet by Oracle''s IT group.
> 
> It may be unrelated to the xen.org server change but
> the coincidence is suspicious.
> 
> Interestingly, I got precisely one xen-devel email in
> the last week which is what led me to the spam filter
> theory... though I have gotten every xen-changelog email.
AFAICT, the problem is now resolved, as of sometime
early Sunday AM GMT.  I haven''t heard from Konrad
or other Oracle xen-devel subscribers but will assume
theirs is fixed as well.

Dan

Dan Magenheimer writes ("Re: [Xen-devel] [PATCH] tools: misc:
xen-tmem-list-parse: fix output ugliness"):
> Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > I think functions with prototypes like
> >      f(int len, char *buf)
> > should take buf[len], not buf[len+1].
> 
> FWIW, I agree completely.  I didn''t write this code from
> scratch, I heavily leveraged it from elsewhere (don''t
> recall exactly where, maybe xentop?).
>
> If necessary, I will rewrite the callers and API but
> I was just trying to fix an annoying bug expeditiously.
> Let me know if you won''t accept it as the one-line
> ugly fix.
I really don''t want to make this worse.  Previously the function would
not write past len.  So sorry, would you mind fixing the callers ?

Also if you "leveraged" this code perhaps it should be combined or the
original one fixed or something ?

Thanks,
Ian.