| Message ID | 20241216164055.96267-5-cgoettsche@seltendoof.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RFC,v2,01/22] selinux: supply missing field initializers | expand |
On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote: > > Avoid using nontransitive comparison to prevent unexpected sorting > results due to (well-defined) overflows. > See https://www.qualys.com/2024/01/30/qsort.txt for a related issue in > glibc's qsort(3). > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > security/selinux/ss/policydb.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 3ba5506a3fff..eb944582d7a6 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -37,6 +37,8 @@ > #include "mls.h" > #include "services.h" > > +#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b))) I'll admit that it took me a while to figure out why you decided to name this macro "spaceship_cmp", and then I had a little laugh when I realized why it was called the "spaceship" operator :) Anyway, while the spaceship operator is likely familiar to people who have a Perl background, the kernel is still mostly a C project so I don't think we can expect a base understanding of Perl, especially these days as Perl isn't as popular as in the past. Can we rename this to something else that makes more sense in the context of C? -- paul-moore.com
On Wed, 8 Jan 2025 at 04:00, Paul Moore <paul@paul-moore.com> wrote: > > On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote: > > > > Avoid using nontransitive comparison to prevent unexpected sorting > > results due to (well-defined) overflows. > > See https://www.qualys.com/2024/01/30/qsort.txt for a related issue in > > glibc's qsort(3). > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > security/selinux/ss/policydb.c | 18 ++++++++++-------- > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > > index 3ba5506a3fff..eb944582d7a6 100644 > > --- a/security/selinux/ss/policydb.c > > +++ b/security/selinux/ss/policydb.c > > @@ -37,6 +37,8 @@ > > #include "mls.h" > > #include "services.h" > > > > +#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b))) > > I'll admit that it took me a while to figure out why you decided to > name this macro "spaceship_cmp", and then I had a little laugh when > I realized why it was called the "spaceship" operator :) > > Anyway, while the spaceship operator is likely familiar to people who > have a Perl background, the kernel is still mostly a C project so I > don't think we can expect a base understanding of Perl, especially > these days as Perl isn't as popular as in the past. Can we rename > this to something else that makes more sense in the context of C? There seem to be 4 similar macros already in the kernel, 3 named cmp_int() and one instance named cmp_ptr(). So I am going with cmp_int() in v3 for now, but I am open for other suggestions. > > -- > paul-moore.com
On Wed, 8 Jan 2025 at 14:17, Christian Göttsche <cgzones@googlemail.com> wrote: > > On Wed, 8 Jan 2025 at 04:00, Paul Moore <paul@paul-moore.com> wrote: > > > > On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote: > > > > > > Avoid using nontransitive comparison to prevent unexpected sorting > > > results due to (well-defined) overflows. > > > See https://www.qualys.com/2024/01/30/qsort.txt for a related issue in > > > glibc's qsort(3). > > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > --- > > > security/selinux/ss/policydb.c | 18 ++++++++++-------- > > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > > > index 3ba5506a3fff..eb944582d7a6 100644 > > > --- a/security/selinux/ss/policydb.c > > > +++ b/security/selinux/ss/policydb.c > > > @@ -37,6 +37,8 @@ > > > #include "mls.h" > > > #include "services.h" > > > > > > +#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b))) > > > > I'll admit that it took me a while to figure out why you decided to > > name this macro "spaceship_cmp", and then I had a little laugh when > > I realized why it was called the "spaceship" operator :) > > > > Anyway, while the spaceship operator is likely familiar to people who > > have a Perl background, the kernel is still mostly a C project so I > > don't think we can expect a base understanding of Perl, especially > > these days as Perl isn't as popular as in the past. Can we rename > > this to something else that makes more sense in the context of C? > > There seem to be 4 similar macros already in the kernel, 3 named > cmp_int() and one instance named cmp_ptr(). > So I am going with cmp_int() in v3 for now, but I am open for other suggestions. p.s.: C++ also had takeoff: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0515r3.pdf > > > > -- > > paul-moore.com
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 3ba5506a3fff..eb944582d7a6 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -37,6 +37,8 @@ #include "mls.h" #include "services.h" +#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b))) + #ifdef CONFIG_SECURITY_SELINUX_DEBUG /* clang-format off */ static const char *const symtab_name[SYM_NUM] = { @@ -426,11 +428,11 @@ static int filenametr_cmp(const void *k1, const void *k2) const struct filename_trans_key *ft2 = k2; int v; - v = ft1->ttype - ft2->ttype; + v = spaceship_cmp(ft1->ttype, ft2->ttype); if (v) return v; - v = ft1->tclass - ft2->tclass; + v = spaceship_cmp(ft1->tclass, ft2->tclass); if (v) return v; @@ -461,15 +463,15 @@ static int rangetr_cmp(const void *k1, const void *k2) const struct range_trans *key1 = k1, *key2 = 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; } @@ -498,15 +500,15 @@ static int role_trans_cmp(const void *k1, const void *k2) const struct role_trans_key *key1 = k1, *key2 = k2; int v; - v = key1->role - key2->role; + v = spaceship_cmp(key1->role, key2->role); if (v) return v; - v = key1->type - key2->type; + v = spaceship_cmp(key1->type, key2->type); if (v) return v; - return key1->tclass - key2->tclass; + return spaceship_cmp(key1->tclass, key2->tclass); } static const struct hashtab_key_params roletr_key_params = {