Jan Beulich
2012-Mar-07 11:01 UTC
[PATCH] libxl: don''t accept negative disk or partition indexes
When obtained via sscanf(), they were checked against an upper bound
only so far. By converting the local variables'' types to "unsigned
int"
those bounds checks become sufficient (as a consequence the helper
function''s parameter types need to be adjusted too). It''s not
strictly
necessary to also convert libxl__device_disk_dev_number()''s parameter
types - the bounds checking done (now) guarantees that the values won''t
run into the negative range of "int" values.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -260,8 +260,10 @@ int libxl__device_physdisk_major_minor(c
}
static int device_virtdisk_matches(const char *virtpath, const char *devtype,
- int *index_r, int max_index,
- int *partition_r, int max_partition) {
+ unsigned int *index_r,
+ unsigned int max_index,
+ unsigned int *partition_r,
+ unsigned int max_partition) {
const char *p;
char *ep;
int tl, c;
@@ -310,13 +312,13 @@ static int device_virtdisk_matches(const
int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
int *ppartition)
{
- int disk, partition;
+ unsigned int disk, partition;
char *ep;
unsigned long ul;
int chrused;
chrused = -1;
- if ((sscanf(virtpath, "d%ip%i%n", &disk, &partition,
&chrused) >= 2
+ if ((sscanf(virtpath, "d%up%u%n", &disk, &partition,
&chrused) >= 2
&& chrused == strlen(virtpath) && disk <
(1<<20) && partition < 256)
||
device_virtdisk_matches(virtpath, "xvd",
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Ian Campbell
2012-Mar-07 19:36 UTC
Re: [PATCH] libxl: don''t accept negative disk or partition indexes
On Wed, 2012-03-07 at 06:01 -0500, Jan Beulich wrote:> When obtained via sscanf(), they were checked against an upper bound > only so far. By converting the local variables'' types to "unsigned int" > those bounds checks become sufficient (as a consequence the helper > function''s parameter types need to be adjusted too). It''s not strictly > necessary to also convert libxl__device_disk_dev_number()''s parameter > types -Any reason not to do it though?> the bounds checking done (now) guarantees that the values won''t > run into the negative range of "int" values. > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Acked-by: Ian Campbell <ian.campbell@citrix.com>> > --- a/tools/libxl/libxl_device.c > +++ b/tools/libxl/libxl_device.c > @@ -260,8 +260,10 @@ int libxl__device_physdisk_major_minor(c > } > > static int device_virtdisk_matches(const char *virtpath, const char *devtype, > - int *index_r, int max_index, > - int *partition_r, int max_partition) { > + unsigned int *index_r, > + unsigned int max_index, > + unsigned int *partition_r, > + unsigned int max_partition) { > const char *p; > char *ep; > int tl, c; > @@ -310,13 +312,13 @@ static int device_virtdisk_matches(const > int libxl__device_disk_dev_number(const char *virtpath, int *pdisk, > int *ppartition) > { > - int disk, partition; > + unsigned int disk, partition; > char *ep; > unsigned long ul; > int chrused; > > chrused = -1; > - if ((sscanf(virtpath, "d%ip%i%n", &disk, &partition, &chrused) >= 2 > + if ((sscanf(virtpath, "d%up%u%n", &disk, &partition, &chrused) >= 2 > && chrused == strlen(virtpath) && disk < (1<<20) && partition < 256) > || > device_virtdisk_matches(virtpath, "xvd", > > >
Jan Beulich
2012-Mar-08 07:50 UTC
Re: [PATCH] libxl: don''t accept negative disk or partition indexes
>>> On 07.03.12 at 20:36, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Wed, 2012-03-07 at 06:01 -0500, Jan Beulich wrote: >> When obtained via sscanf(), they were checked against an upper bound >> only so far. By converting the local variables'' types to "unsigned int" >> those bounds checks become sufficient (as a consequence the helper >> function''s parameter types need to be adjusted too). It''s not strictly >> necessary to also convert libxl__device_disk_dev_number()''s parameter >> types - > > Any reason not to do it though?I wanted to keep the patch to the minimum. The pointed out possible cleanup I''d rather leave to those more involved with that code... Jan
Ian Jackson
2012-Mar-13 17:23 UTC
Re: [PATCH] libxl: don''t accept negative disk or partition indexes
Jan Beulich writes ("[Xen-devel] [PATCH] libxl: don''t accept
negative disk or partition indexes"):> When obtained via sscanf(), they were checked against an upper bound
> only so far. By converting the local variables'' types to
"unsigned int"
> those bounds checks become sufficient (as a consequence the helper
> function''s parameter types need to be adjusted too). It''s
not strictly
> necessary to also convert libxl__device_disk_dev_number()''s
parameter
> types - the bounds checking done (now) guarantees that the values
won''t
> run into the negative range of "int" values.
IMO "unsigned int" is not a type that should be used for things which
are like mathematical integers, even if their range happens to include
only non-negative integers. In C unsigned types have some very
surprising behaviours in comparisons and subtractions.
So I think the correct thing to do is to check that the values are
within sensible limits after sscanf returns.
Ian.
Jan Beulich
2012-Mar-14 07:41 UTC
Re: [PATCH] libxl: don''t accept negative disk or partition indexes
>>> On 13.03.12 at 18:23, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > Jan Beulich writes ("[Xen-devel] [PATCH] libxl: don''t accept negative disk or > partition indexes"): >> When obtained via sscanf(), they were checked against an upper bound >> only so far. By converting the local variables'' types to "unsigned int" >> those bounds checks become sufficient (as a consequence the helper >> function''s parameter types need to be adjusted too). It''s not strictly >> necessary to also convert libxl__device_disk_dev_number()''s parameter >> types - the bounds checking done (now) guarantees that the values won''t >> run into the negative range of "int" values. > > IMO "unsigned int" is not a type that should be used for things which > are like mathematical integers, even if their range happens to include > only non-negative integers. In C unsigned types have some very > surprising behaviours in comparisons and subtractions.I would call this surprising only if you think purely mathematically (which you shouldn''t when programming in any language that uses finite width data types). Instead I find it rather natural to actively use those properties that you call surprising, and in particular to use unsigned types for variables that can''t (or shouldn''t) have negative values (not the least because this tends to produce better code, as no sign-extension is necessary when using such variables e.g. as array indexes). Plus I''d be curious where you would see fit for unsigned types (or whether you consider them evil altogether).> So I think the correct thing to do is to check that the values are > within sensible limits after sscanf returns.As I''m disagreeing, I won''t submit a version of the patch that does so. Either you apply the patch as is (at least in this respect - if there are other problems with it I''m perfectly fine to address those), or you roll your own, or you leave the problem unfixed. Jan
Jan Beulich writes ("Re: [Xen-devel] [PATCH] libxl: don''t accept
negative disk or partition indexes"):> I would call this surprising only if you think purely mathematically
> (which you shouldn''t when programming in any language that uses
> finite width data types).
I think this is a coding style question. It isn''t addressed in
CODING_STYLE and the current code is (as ever) inconsistent, but we
should make a specific decision and stick to it for new changes at
least.
> Instead I find it rather natural to actively use those properties
> that you call surprising, and in particular to use unsigned types
> for variables that can''t (or shouldn''t) have negative
values
The problem is that the cannot-be-negative property doesn''t just apply
to the type (eg, count) in question. It also propagates to values
derived from it by arithmetic. Attempts, for example, to calculate
remaining space, or differences of any kind, go badly wrong.
I think the correct approach is to use signed types and, at
appropriately points, explicitly limit incoming values to small enough
subsets of their types that arithmetic overflow cannot occur anywhere.
> (not the least because this tends to produce better
> code, as no sign-extension is necessary when using such variables
> e.g. as array indexes).
This is not a consideration in libxl because libxl doesn''t contain hot
paths.
> Plus I''d be curious where you would see fit
> for unsigned types (or whether you consider them evil altogether).
They are useful for bitfields, values whose abstract types are
circular orders, and the like. Not things which are supposed to model
a subset of the mathematical integers.
Ian.
On Wed, Mar 14, 2012 at 11:22 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:> Jan Beulich writes ("Re: [Xen-devel] [PATCH] libxl: don''t accept negative disk or partition indexes"): >> I would call this surprising only if you think purely mathematically >> (which you shouldn''t when programming in any language that uses >> finite width data types). > > I think this is a coding style question. It isn''t addressed in > CODING_STYLE and the current code is (as ever) inconsistent, but we > should make a specific decision and stick to it for new changes at > least. > >> Instead I find it rather natural to actively use those properties >> that you call surprising, and in particular to use unsigned types >> for variables that can''t (or shouldn''t) have negative values > > The problem is that the cannot-be-negative property doesn''t just apply > to the type (eg, count) in question. It also propagates to values > derived from it by arithmetic. Attempts, for example, to calculate > remaining space, or differences of any kind, go badly wrong. > > I think the correct approach is to use signed types and, at > appropriately points, explicitly limit incoming values to small enough > subsets of their types that arithmetic overflow cannot occur anywhere. > >> (not the least because this tends to produce better >> code, as no sign-extension is necessary when using such variables >> e.g. as array indexes). > > This is not a consideration in libxl because libxl doesn''t contain hot > paths. > >> Plus I''d be curious where you would see fit >> for unsigned types (or whether you consider them evil altogether). > > They are useful for bitfields, values whose abstract types are > circular orders, and the like. Not things which are supposed to model > a subset of the mathematical integers.FWIW, overall ijc''s proposal seems sensible to me. -George
>>> On 14.03.12 at 17:02, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > On Wed, Mar 14, 2012 at 11:22 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> > wrote: >> Jan Beulich writes ("Re: [Xen-devel] [PATCH] libxl: don''t accept negative > disk or partition indexes"): >>> I would call this surprising only if you think purely mathematically >>> (which you shouldn''t when programming in any language that uses >>> finite width data types). >> >> I think this is a coding style question. It isn''t addressed in >> CODING_STYLE and the current code is (as ever) inconsistent, but we >> should make a specific decision and stick to it for new changes at >> least. >> >>> Instead I find it rather natural to actively use those properties >>> that you call surprising, and in particular to use unsigned types >>> for variables that can''t (or shouldn''t) have negative values >> >> The problem is that the cannot-be-negative property doesn''t just apply >> to the type (eg, count) in question. It also propagates to values >> derived from it by arithmetic. Attempts, for example, to calculate >> remaining space, or differences of any kind, go badly wrong. >> >> I think the correct approach is to use signed types and, at >> appropriately points, explicitly limit incoming values to small enough >> subsets of their types that arithmetic overflow cannot occur anywhere. >> >>> (not the least because this tends to produce better >>> code, as no sign-extension is necessary when using such variables >>> e.g. as array indexes). >> >> This is not a consideration in libxl because libxl doesn''t contain hot >> paths. >> >>> Plus I''d be curious where you would see fit >>> for unsigned types (or whether you consider them evil altogether). >> >> They are useful for bitfields, values whose abstract types are >> circular orders, and the like. Not things which are supposed to model >> a subset of the mathematical integers. > > FWIW, overall ijc''s proposal seems sensible to me.I hadn''t seen one, and the mail archive doesn''t show one either. Or did you mean IanJ''s reply above (in which case I continue to be of a different opinion, and hope I won''t be forbidden to use unsigned types namely in hypervisor code - if on the tools side maintainers decide to do so I guess I''ll have to try to cope with that)? Or is ijc not equivalent to IanC? Jan
On Wed, 2012-03-14 at 16:14 +0000, Jan Beulich wrote:> >>> On 14.03.12 at 17:02, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > > On Wed, Mar 14, 2012 at 11:22 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> > > wrote: > >> Jan Beulich writes ("Re: [Xen-devel] [PATCH] libxl: don''t accept negative > > disk or partition indexes"): > >>> I would call this surprising only if you think purely mathematically > >>> (which you shouldn''t when programming in any language that uses > >>> finite width data types). > >> > >> I think this is a coding style question. It isn''t addressed in > >> CODING_STYLE and the current code is (as ever) inconsistent, but we > >> should make a specific decision and stick to it for new changes at > >> least. > >> > >>> Instead I find it rather natural to actively use those properties > >>> that you call surprising, and in particular to use unsigned types > >>> for variables that can''t (or shouldn''t) have negative values > >> > >> The problem is that the cannot-be-negative property doesn''t just apply > >> to the type (eg, count) in question. It also propagates to values > >> derived from it by arithmetic. Attempts, for example, to calculate > >> remaining space, or differences of any kind, go badly wrong. > >> > >> I think the correct approach is to use signed types and, at > >> appropriately points, explicitly limit incoming values to small enough > >> subsets of their types that arithmetic overflow cannot occur anywhere. > >> > >>> (not the least because this tends to produce better > >>> code, as no sign-extension is necessary when using such variables > >>> e.g. as array indexes). > >> > >> This is not a consideration in libxl because libxl doesn''t contain hot > >> paths. > >> > >>> Plus I''d be curious where you would see fit > >>> for unsigned types (or whether you consider them evil altogether). > >> > >> They are useful for bitfields, values whose abstract types are > >> circular orders, and the like. Not things which are supposed to model > >> a subset of the mathematical integers. > > > > FWIW, overall ijc''s proposal seems sensible to me. > > I hadn''t seen one, and the mail archive doesn''t show one either. > Or did you mean IanJ''s reply above (in which case I continue to be > of a different opinion, and hope I won''t be forbidden to use unsigned > types namely in hypervisor code - if on the tools side maintainers > decide to do so I guess I''ll have to try to cope with that)? Or is ijc not > equivalent to IanC?gwd meant iwj and not ijc ;-) Ian.