Re: Fix for SSP error in tools/python/lowlevel/xc/xc.c

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Aug 27, 2009 at 10:36:59AM +0200, Milan Holzäpfel wrote:
> On Wed, 26 Aug 2009 13:39:31 -0400
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > On Wed, Aug 26, 2009 at 04:19:54PM +0200, Milan Holzäpfel wrote:
> > > Hello, 
> > > 
> > > I compiled xen-tools with GCC-4.3.3 with Stack Smashing Protection
> > > (SSP) patches by gentoo, and found a small bug in
> > > tools/python/lowlevel/xc/xc.c.  The bug is located in
> > > pyxc_dom_set_policy_cpuid: 
> > > 
> > > (this is the change which fixes it:)
> > > 
> > > > @@ -808,7 +808,7 @@
> > > >  static PyObject *pyxc_dom_set_policy_cpuid(XcObject *self,
> > > >                                             PyObject *args)
> > > >  {
> > > > -    domid_t domid;
> > > > +    int domid;
> > 
> > I would say use uint32_t instead of int.
> 
> Why?  Quote from the Python documentation (link above):

To keep it in synch with the rest of the variables that define domid.

> 
> | i (integer) [int]
> |         Convert a Python integer to a plain C int.
> 
> So I think "int" is the best solution, as it matches what
> PyArg_ParseTuple expects, no matter what platform you're on.  There is
> also "I" for "unsigned int", used in the other places you mention. 

Aaah. So maybe all of those conversation of the domid (where it is
defined as uint32_t) should be done using 'I' instead.. Or just
maybe the 'h' and then convert all of the unint32_t to domid_t.

I would lean towards changing all of them to domid_t and changing
the 'i' to 'h'? That seems like the correct way without changing
the typedef of domid_t.

> 
> > > >      if ( !PyArg_ParseTuple(args, "i", &domid) )
> > > >          return NULL;
> > > 
> > > domid_t is defined as uint16_t (thus 2 bytes long) in xen header files,
> > > but the "i" format needs a C "int" type, which is 4 bytes long.
> > > (<URL:http://docs.python.org/c-api/arg.html>)  This error is detected
> > > by SSP as stack overflow. 
> > 
> > What about the two other cases where domid_it is used? The SSP didn't
> > detect them?
> 
> No.  Either the functions aren't called on my machine(?), or the
> overflow only overwrites other local variables (which are present
> there). 
> 
> I agree that they should be fixed, too. 
> 
> > > Attached patch fixes the error.  Maybe it would better to use "h"
> > > format instead of the "i" format, which converts the argument to an C
> > > "short int".  Then you would have to change the python wrapper if
> > > domid_t changes, though. 
> > 
> > Yeah, but running more than 64K of guests on one node?
> 
> That's unlikely, yes.  On the other hand, if you had 8 shutdowns/domain
> creations per hour, you'd limit the total uptime to ~341 days.  I admit
> that that's still unlikely. 

That is thought a Xen Python stack decision. You don't have to increment
the domid after a shutdown - you can re-use it if you would like to.