From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: Fix for SSP error in tools/python/lowlevel/xc/xc.c Date: Thu, 27 Aug 2009 08:23:41 -0400 Message-ID: <20090827122341.GA30794@phenom.dumpdata.com> References: <20090826161954.4ce96275.listen@mjh.name> <20090826173931.GA15189@phenom.dumpdata.com> <20090827103659.a2abbb6d.listen@mjh.name> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <20090827103659.a2abbb6d.listen@mjh.name> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Milan =?iso-8859-1?Q?Holz=E4pfel?= Cc: mail@mjh.name, xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org On Thu, Aug 27, 2009 at 10:36:59AM +0200, Milan Holz=E4pfel wrote: > On Wed, 26 Aug 2009 13:39:31 -0400 > Konrad Rzeszutek Wilk wrote: >=20 > > On Wed, Aug 26, 2009 at 04:19:54PM +0200, Milan Holz=E4pfel wrote: > > > Hello,=20 > > >=20 > > > 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:=20 > > >=20 > > > (this is the change which fixes it:) > > >=20 > > > > @@ -808,7 +808,7 @@ > > > > static PyObject *pyxc_dom_set_policy_cpuid(XcObject *self, > > > > PyObject *args) > > > > { > > > > - domid_t domid; > > > > + int domid; > >=20 > > I would say use uint32_t instead of int. >=20 > Why? Quote from the Python documentation (link above): To keep it in synch with the rest of the variables that define domid. >=20 > | i (integer) [int] > | Convert a Python integer to a plain C int. >=20 > 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.=20 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. >=20 > > > > if ( !PyArg_ParseTuple(args, "i", &domid) ) > > > > return NULL; > > >=20 > > > domid_t is defined as uint16_t (thus 2 bytes long) in xen header fi= les, > > > but the "i" format needs a C "int" type, which is 4 bytes long. > > > () This error is detect= ed > > > by SSP as stack overflow.=20 > >=20 > > What about the two other cases where domid_it is used? The SSP didn't > > detect them? >=20 > No. Either the functions aren't called on my machine(?), or the > overflow only overwrites other local variables (which are present > there).=20 >=20 > I agree that they should be fixed, too.=20 >=20 > > > 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.=20 > >=20 > > Yeah, but running more than 64K of guests on one node? >=20 > 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.=20 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.