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@xxxxxxxxxx> 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home