| Message ID | 20210706173629.22032-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] libsepol: avoid unsigned integer overflow | expand |
On Tue, Jul 6, 2021 at 7:36 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > Unsigned integer overflow is well-defined and not undefined behavior. > It is commonly used for hashing or pseudo random number generation. > But it is still useful to enable undefined behavior sanitizer checks on > unsigned arithmetic to detect possible issues on counters or variables > with similar purpose or missed overflow checks on user input. > > Use a spaceship operator like comparison instead of subtraction. > > policydb.c:851:24: runtime error: unsigned integer overflow: 801 - 929 cannot be represented in type 'unsigned int' > > Follow-up of: 1537ea8412e4 ("libsepol: avoid unsigned integer overflow") > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org> Thanks! Nicolas > --- > libsepol/src/policydb.c | 10 +++++----- > libsepol/src/private.h | 2 ++ > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > index ef2217c2..0398ceed 100644 > --- a/libsepol/src/policydb.c > +++ b/libsepol/src/policydb.c > @@ -817,11 +817,11 @@ static int filenametr_cmp(hashtab_t h __attribute__ ((unused)), > const filename_trans_key_t *ft2 = (const filename_trans_key_t *)k2; > int v; > > - v = (ft1->ttype > ft2->ttype) - (ft1->ttype < ft2->ttype); > + v = spaceship_cmp(ft1->ttype, ft2->ttype); > if (v) > return v; > > - v = (ft1->tclass > ft2->tclass) - (ft1->tclass < ft2->tclass); > + v = spaceship_cmp(ft1->tclass, ft2->tclass); > if (v) > return v; > > @@ -843,15 +843,15 @@ static int rangetr_cmp(hashtab_t h __attribute__ ((unused)), > const struct range_trans *key2 = (const struct range_trans *)k2; > int v; > > - v = key1->source_type - key2->source_type; > + v = spaceship_cmp(key1->source_type, key2->source_type); > if (v) > return v; > > - v = key1->target_type - key2->target_type; > + v = spaceship_cmp(key1->target_type, key2->target_type); > if (v) > return v; > > - v = key1->target_class - key2->target_class; > + v = spaceship_cmp(key1->target_class, key2->target_class); > > return v; > } > diff --git a/libsepol/src/private.h b/libsepol/src/private.h > index 72f21262..c63238ab 100644 > --- a/libsepol/src/private.h > +++ b/libsepol/src/private.h > @@ -47,6 +47,8 @@ > #define is_saturated(x) (x == (typeof(x))-1) > #define zero_or_saturated(x) ((x == 0) || is_saturated(x)) > > +#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b))) > + > /* Policy compatibility information. */ > struct policydb_compat_info { > unsigned int type; > -- > 2.32.0 >
On Mon, Jul 12, 2021 at 9:33 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > On Tue, Jul 6, 2021 at 7:36 PM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Unsigned integer overflow is well-defined and not undefined behavior. > > It is commonly used for hashing or pseudo random number generation. > > But it is still useful to enable undefined behavior sanitizer checks on > > unsigned arithmetic to detect possible issues on counters or variables > > with similar purpose or missed overflow checks on user input. > > > > Use a spaceship operator like comparison instead of subtraction. > > > > policydb.c:851:24: runtime error: unsigned integer overflow: 801 - 929 cannot be represented in type 'unsigned int' > > > > Follow-up of: 1537ea8412e4 ("libsepol: avoid unsigned integer overflow") > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org> > > Thanks! > Nicolas Merged. Thanks! Nicolas > > --- > > libsepol/src/policydb.c | 10 +++++----- > > libsepol/src/private.h | 2 ++ > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > > index ef2217c2..0398ceed 100644 > > --- a/libsepol/src/policydb.c > > +++ b/libsepol/src/policydb.c > > @@ -817,11 +817,11 @@ static int filenametr_cmp(hashtab_t h __attribute__ ((unused)), > > const filename_trans_key_t *ft2 = (const filename_trans_key_t *)k2; > > int v; > > > > - v = (ft1->ttype > ft2->ttype) - (ft1->ttype < ft2->ttype); > > + v = spaceship_cmp(ft1->ttype, ft2->ttype); > > if (v) > > return v; > > > > - v = (ft1->tclass > ft2->tclass) - (ft1->tclass < ft2->tclass); > > + v = spaceship_cmp(ft1->tclass, ft2->tclass); > > if (v) > > return v; > > > > @@ -843,15 +843,15 @@ static int rangetr_cmp(hashtab_t h __attribute__ ((unused)), > > const struct range_trans *key2 = (const struct range_trans *)k2; > > int v; > > > > - v = key1->source_type - key2->source_type; > > + v = spaceship_cmp(key1->source_type, key2->source_type); > > if (v) > > return v; > > > > - v = key1->target_type - key2->target_type; > > + v = spaceship_cmp(key1->target_type, key2->target_type); > > if (v) > > return v; > > > > - v = key1->target_class - key2->target_class; > > + v = spaceship_cmp(key1->target_class, key2->target_class); > > > > return v; > > } > > diff --git a/libsepol/src/private.h b/libsepol/src/private.h > > index 72f21262..c63238ab 100644 > > --- a/libsepol/src/private.h > > +++ b/libsepol/src/private.h > > @@ -47,6 +47,8 @@ > > #define is_saturated(x) (x == (typeof(x))-1) > > #define zero_or_saturated(x) ((x == 0) || is_saturated(x)) > > > > +#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b))) > > + > > /* Policy compatibility information. */ > > struct policydb_compat_info { > > unsigned int type; > > -- > > 2.32.0 > >
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c index ef2217c2..0398ceed 100644 --- a/libsepol/src/policydb.c +++ b/libsepol/src/policydb.c @@ -817,11 +817,11 @@ static int filenametr_cmp(hashtab_t h __attribute__ ((unused)), const filename_trans_key_t *ft2 = (const filename_trans_key_t *)k2; int v; - v = (ft1->ttype > ft2->ttype) - (ft1->ttype < ft2->ttype); + v = spaceship_cmp(ft1->ttype, ft2->ttype); if (v) return v; - v = (ft1->tclass > ft2->tclass) - (ft1->tclass < ft2->tclass); + v = spaceship_cmp(ft1->tclass, ft2->tclass); if (v) return v; @@ -843,15 +843,15 @@ static int rangetr_cmp(hashtab_t h __attribute__ ((unused)), const struct range_trans *key2 = (const struct range_trans *)k2; int v; - v = key1->source_type - key2->source_type; + v = spaceship_cmp(key1->source_type, key2->source_type); if (v) return v; - v = key1->target_type - key2->target_type; + v = spaceship_cmp(key1->target_type, key2->target_type); if (v) return v; - v = key1->target_class - key2->target_class; + v = spaceship_cmp(key1->target_class, key2->target_class); return v; } diff --git a/libsepol/src/private.h b/libsepol/src/private.h index 72f21262..c63238ab 100644 --- a/libsepol/src/private.h +++ b/libsepol/src/private.h @@ -47,6 +47,8 @@ #define is_saturated(x) (x == (typeof(x))-1) #define zero_or_saturated(x) ((x == 0) || is_saturated(x)) +#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b))) + /* Policy compatibility information. */ struct policydb_compat_info { unsigned int type;
Unsigned integer overflow is well-defined and not undefined behavior. It is commonly used for hashing or pseudo random number generation. But it is still useful to enable undefined behavior sanitizer checks on unsigned arithmetic to detect possible issues on counters or variables with similar purpose or missed overflow checks on user input. Use a spaceship operator like comparison instead of subtraction. policydb.c:851:24: runtime error: unsigned integer overflow: 801 - 929 cannot be represented in type 'unsigned int' Follow-up of: 1537ea8412e4 ("libsepol: avoid unsigned integer overflow") Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- libsepol/src/policydb.c | 10 +++++----- libsepol/src/private.h | 2 ++ 2 files changed, 7 insertions(+), 5 deletions(-)