Milan Holzäpfel
2009-Aug-26 14:19 UTC
[Xen-devel] Fix for SSP error in tools/python/lowlevel/xc/xc.c
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; > > 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. 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. Please apply this patch or the change to the "h" format string (I haven''t tested it, but I believe it should work just as well). Regards, Milan Holzäpfel -- Milan Holzaepfel <mail(a)mjh(d)name> <URL:http://mjh.name/> pub 4096R/C790FC23 EB8E 5E81 81E3 53A9 9B74 B895 5179 54C0 C790 FC23 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2009-Aug-26 17:39 UTC
Re: [Xen-devel] Fix for SSP error in tools/python/lowlevel/xc/xc.c
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.> > > > 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?> > 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?> > Please apply this patch or the change to the "h" format string (I > haven''t tested it, but I believe it should work just as well). > > Regards, > Milan Holzäpfel > > > -- > Milan Holzaepfel <mail(a)mjh(d)name> <URL:http://mjh.name/> > pub 4096R/C790FC23 EB8E 5E81 81E3 53A9 9B74 B895 5179 54C0 C790 FC23> 2009-08-26 Milan Holzaepfel <mail@mjh.name> > > As documented on <URL:http://docs.python.org/c-api/arg.html>, the "i" > format string needs an integer as target. > > Error detected by gentoo Stack Smashing Protection for gcc-4.3.3. > > --- xen-3.4.1/tools/python/xen/lowlevel/xc/xc.c.orig 2009-08-26 13:43:13.000000000 +0000 > +++ xen-3.4.1/tools/python/xen/lowlevel/xc/xc.c 2009-08-26 13:43:20.000000000 +0000 > @@ -808,7 +808,7 @@ > static PyObject *pyxc_dom_set_policy_cpuid(XcObject *self, > PyObject *args) > { > - domid_t domid; > + int domid; > > if ( !PyArg_ParseTuple(args, "i", &domid) ) > return NULL;> _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Milan Holzäpfel
2009-Aug-27 08:36 UTC
Re: [Xen-devel] Fix for SSP error in tools/python/lowlevel/xc/xc.c
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): | 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.> > > 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. Should an error be raised if the domid value passed in exceeds 65535? Otherwise 65536 would be converted to 0, which is not what is expected (and might possibly be harmful?).> > Please apply this patch or the change to the "h" format string (I > > haven''t tested it, but I believe it should work just as well). > > > > Regards, > > Milan Holzäpfel > > > > > > -- > > Milan Holzaepfel <mail(a)mjh(d)name> <URL:http://mjh.name/> > > pub 4096R/C790FC23 EB8E 5E81 81E3 53A9 9B74 B895 5179 54C0 C790 FC23 > > > 2009-08-26 Milan Holzaepfel <mail@mjh.name> > > > > As documented on <URL:http://docs.python.org/c-api/arg.html>, the "i" > > format string needs an integer as target. > > > > Error detected by gentoo Stack Smashing Protection for gcc-4.3.3. > > > > --- xen-3.4.1/tools/python/xen/lowlevel/xc/xc.c.orig 2009-08-26 13:43:13.000000000 +0000 > > +++ xen-3.4.1/tools/python/xen/lowlevel/xc/xc.c 2009-08-26 13:43:20.000000000 +0000 > > @@ -808,7 +808,7 @@ > > static PyObject *pyxc_dom_set_policy_cpuid(XcObject *self, > > PyObject *args) > > { > > - domid_t domid; > > + int domid; > > > > if ( !PyArg_ParseTuple(args, "i", &domid) ) > > return NULL; > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xensource.com > > http://lists.xensource.com/xen-devel > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2009-Aug-27 12:23 UTC
Re: [Xen-devel] Fix for SSP error in tools/python/lowlevel/xc/xc.c
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Milan Holzäpfel
2009-Aug-27 16:27 UTC
Re: [Xen-devel] Fix for SSP error in tools/python/lowlevel/xc/xc.c
On Thu, 27 Aug 2009 08:23:41 -0400 Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:> 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.I agree, but use "H" instead of "h" (domid_t is unsigend, I think). On x86 and x86_64 "unsigned short int" is the same as "uint16_t", so this should be ok. (The only type known to me which behaves differntly on different platforms is "long int" -- 4 byte on x86, 8 byte on x86_64, maybe there are others? That''s why I used int initially, to make sure the types actually match.). Still, if we want to make sure the value coming from Python code is actually in the range 0...65535, we need to check for that (the Python doc says "without overflow checking" for "H"). Maybe I''ll create a patch for that later. (Will have to find out how to get arbitrary strings into exception raised from C code...)> > > > > 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.Ah, ok. I don''t know whether the Python stack recycles domain ids in case they run out. Regards, Milan -- Milan Holzaepfel <mail(a)mjh(d)name> <URL:http://mjh.name/> pub 4096R/C790FC23 EB8E 5E81 81E3 53A9 9B74 B895 5179 54C0 C790 FC23 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel